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

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

On Fri, Aug 11, 2017 at 04:01:05AM -0600, Jan Beulich wrote:
> >>> 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.

Will make that clearer on the next version, in fact I'm going to send
this series rebased on top of the iommu_inclusive_mapping one. AFAICT
that one is closer to being committed, and in any case changing the
order is trivial, there are not conflicts.

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

I'm not sure it's clear to me what to do if the guest writes to an
entry while unmasked. For once it says that it must not modify it, but
OTHO it says result is undefined when doing so.

Would you be fine with ignoring writes to the address and data fields
if the entry is unmasked?

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

I think (for debugging purposes) it's useful to print both together
in order to spot if the guest is doing something wrong.

Roger.

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