Re: [Xen-devel] [PATCH v4 9/9] vpci/msix: add MSI-X handlers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.08.17 at 19:04, <roger.pau@xxxxxxxxxx> wrote:
> On Wed, Aug 02, 2017 at 09:07:54AM -0600, Jan Beulich wrote:
>> >>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 06/30/17 5:01 PM >>>
>> >@@ -113,6 +148,35 @@ static int vpci_modify_bar(struct domain *d, const 
>> >struct vpci_bar *bar,
>> >if ( IS_ERR(mem) )
>> >return -PTR_ERR(mem);
>>  >
>> >+    /*
>> >+     * 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.
>> >+     */
>> >+    for ( i = 0; map && i < ARRAY_SIZE(bar->msix); i++ )
>> >+    {
>> >+        struct vpci_msix_mem *msix = bar->msix[i];
>> >+
>> >+        if ( !msix || msix->addr == INVALID_PADDR )
>> >+            continue;
>> >+
>> >+        rc = vpci_unmap_msix(d, msix);
>> 
>> Why do you need this, instead of being able to simply rely on the rangeset
>> based (un)mapping?
> 
> This is because the series that I've sent called: "x86/pvh: implement
> iommu_inclusive_mapping for PVH Dom0" will map the MSI-X memory areas
> into the guest, and thus we need to make sure they are not mapped
> here for the emulation path to work.
> 
> https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg02849.html 

Oh, okay. The patch description doesn't mention any such
dependency though.

>> >+        break;
>> >+    case PCI_MSIX_ENTRY_DATA_OFFSET:
>> >+        /*
>> >+         * 8 byte writes to the msg data and vector control fields are
>> >+         * only allowed if the entry is masked.
>> >+         */
>> >+        if ( len == 8 && !entry->masked && !msix->masked && msix->enabled )
>> >+        {
>> >+            vpci_unlock(d);
>> >+            return X86EMUL_OKAY;
>> >+        }
>> 
>> I don't think this is correct - iirc such writes simply don't take effect 
>> immediately
>> (but I then seem to recall this to apply to the address field and 32-bit 
>> writes to
>> the data field as well). They'd instead take effect the next time the entry 
>> is being
>> unmasked (or some such). A while ago I did fix the qemu code to behave in 
>> this
>> way.
> 
> There's an Implementation Note called "Special Considerations for QWORD
> Accesses" in the MSI-X section of the PCI 3.0 spec that states:
> 
> If a given entry is currently masked (via its Mask bit or the Function
> Mask bit), software is permitted to fill in the Message Data and
> Vector Control fields with a single QWORD write, taking advantage of
> the fact the Message Data field is guaranteed to become visible to
> hardware no later than the Vector Control field.
> 
> So I think the above chunk is correct. The specification also states
> that:
> 
> Software must not modify the Address or Data fields of an entry while
> it is unmasked. Refer to Section 6.8.3.5 for details.
> 
> AFAICT this is not enforced by QEMU, and you can write to the
> address/data fields while the message is not masked. The update will
> only take effect once the message is masked and unmasked.

The spec also says:

"For MSI-X, a function is permitted to cache Address and Data values
 from unmasked MSIX Table entries. However, anytime software
 unmasks a currently masked MSI-X Table entry either by clearing its
 Mask bit or by clearing the Function Mask bit, the function must
 update any Address or Data values that it cached from that entry. If
 software changes the Address or Data value of an entry while the
 entry is unmasked, the result is undefined."

>> >+    for_each_domain ( d )
>> >+    {
>> >+        if ( !has_vpci(d) )
>> >+            continue;
>> >+
>> >+        printk("vPCI MSI-X information for guest %u\n", d->domain_id);
>> 
>> Wouldn't it be better (more useful) to dump the MSI and MSI-X data for a
>> domain next to each other?
> 
> Possibly yes, and printing the MSI and MSI-X data of each device
> together would be even better IMHO.

Not sure about that last aspect - devices aren't permitted to enable
both at the same time, so only one of them can really be relevant.

Jan

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