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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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