Re: [Xen-devel] [PATCH v5 11/11] vpci/msix: add MSI-X handlers

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Sep 15, 2017 at 05:43:15AM -0600, Jan Beulich wrote:
> >>> On 15.09.17 at 12:44, <roger.pau@xxxxxxxxxx> wrote:
> > On Thu, Sep 07, 2017 at 10:12:59AM -0600, Jan Beulich wrote:
> >> >>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> >> > +    for ( i = 0; i < ARRAY_SIZE(bar->msix); i++ )
> >> > +    {
> >> > +        struct vpci_msix_mem *msix = bar->msix[i];
> >> > +
> >> > +        if ( !msix || msix->addr == INVALID_PADDR )
> >> > +            continue;
> >> > +
> >> > +        if ( map )
> >> > +        {
> >> > +            /*
> >> > +             * Make sure the MSI-X regions of the BAR are not mapped 
> >> > into the
> >> > +             * domain p2m, or else the MSI-X handlers are useless. Only 
> >> > do this
> >> > +             * when mapping, since that's when the memory decoding on 
> >> > the
> >> > +             * device is enabled.
> >> > +             *
> >> > +             * This is required because iommu_inclusive_mapping might 
> >> > have
> >> > +             * mapped MSI-X regions into the guest p2m.
> >> > +             */
> >> > +            rc = vpci_unmap_msix(d, msix);
> >> > +            if ( rc )
> >> > +            {
> >> > +                rangeset_destroy(mem);
> >> > +                return rc;
> >> > +            }
> >> > +        }
> >> > +
> >> > +        rc = rangeset_remove_range(mem, PFN_DOWN(msix->addr),
> >> > +                                   PFN_DOWN(msix->addr + msix->size));
> >> > +        if ( rc )
> >> > +        {
> >> > +            rangeset_destroy(mem);
> >> > +            return rc;
> >> > +        }
> >> > +
> >> > +    }
> >> 
> >> Why do you do this for the PBA regardless of whether it's shared
> >> with a table page?
> > 
> > Writes to the PBA area are described as undefined by the spec:
> > 
> > "If software writes to Pending Bits, the result is undefined."
> > 
> > I think it's better to simply not allow the guest to perform such
> > writes, and hence we need to trap this area unconditionally IMHO.
> 
> For DomU I would agree, but for Dom0 I think we better allow a
> driver anything it would also be able to do without Xen.

OK. IMHO it's simpler to always trap access to the PBA, regardless of
whether is sharing a page with the MSIX table or not. I will add a
is_hardware_domain check here in order to forward the writes.

> >> > +    if ( MSIX_ADDR_IN_RANGE(addr, &msix->pba) )
> >> > +    {
> >> > +        /* Access to PBA. */
> >> > +        switch ( len )
> >> > +        {
> >> > +        case 4:
> >> > +            *data = readl(addr);
> >> > +            break;
> >> > +        case 8:
> >> > +            *data = readq(addr);
> >> > +            break;
> >> 
> >> This is strictly only valid for Dom0, so perhaps worth a comment.
> > 
> > I'm not sure I follow, why should Xen disallow accesses to the PBA for
> > a DomU?
> 
> That's not the point, but instead the readl() / readq() using an
> untranslated address.

Right, ATM we identity map the PBA into the guest address space. If
that changes then this is certainly going to be wrong. I will add a
TODO item here so that I don't forget later.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel