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

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

On Thu, Sep 07, 2017 at 10:12:59AM -0600, Jan Beulich wrote:
> >>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> > +void vpci_msix_arch_mask(struct vpci_arch_msix_entry *arch,
> > +                         struct pci_dev *pdev, bool mask)
> > +{
> > +    if ( arch->pirq == INVALID_PIRQ )
> > +        return;
> 
> How come no similar guard is needed in vpci_msi_arch_mask()?

That's right, this should be an ASSERT instead.

> > +    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.

> > +    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?

Thanks, Roger.

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