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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.08.17 at 12:11, <roger.pau@xxxxxxxxxx> wrote:
> 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 >>>
>> >> >+    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?

No, not really. I've intentionally pointed you to the qemu code,
as there I've implemented the caching behavior described by the
quote above. I'd expect vPCI to behave similarly.

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

For Dom0 maybe. For DomU we'd have to refuse guest attempts
to do anything possibly resulting in undefined behavior.

Jan


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