Re: [Qemu-devel] [PATCH 18/38] ivshmem: Leave INTx alone when using MSI-X

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 01/03/2016 18:14, Marc-André Lureau wrote:
>> > +    /*
>> > +     * Note: we don't use INTx with IVSHMEM_MSI at all, so this is a
>> > +     * bald-faced lie then.  But it's a backwards compatible lie.
>> > +     */
>> >      pci_config_set_interrupt_pin(pci_conf, 1);
>> 
>> I am not sure how much of a problem this is. Apparently, other devices
>> claim interrupt and msi (ich, hda, pvscsi)
>> 
>> Better ask someone more familiar with PCI details.
>
> The interrupt pin is read-only and just helps the OS figure out which
> interrupt is routed to intx.  If you return early from
> ivshmem_update_irq if IVSHMEM_MSI, you should skip this line too.
>
> I think it's better to leave this line in and check
>
>     if (msix_enabled(pci_dev)) {
>         return;
>     }
>
> in ivshmem_update_irq instead.  This matches what xhci does, for example.

Yes, but it's not what ivshmem has ever done.  In other words, it's a
backward-incompatible change.

A PCI function declares whether it can do MSI or MSI-X with
capabilities.

Use of MSI and MSI-X is optional.  Software can enable either MSI or
MSI-X, both not both.  When MSI-X is enabled, the function must signal
interrupts via MSI-X.  When MSI is enabled, it must signal interrupts
via MSI.  When neither is enabled, it signals interrupts via INTx *if*
it has the pin wired up.  PCI Local Bus Specification Revision 3.0,
section 6.8 Message Signaled Interrupts:

    It is recommended that devices implement interrupt pins to provide
    compatibility in systems that do not support MSI (devices default to
    interrupt pins).  However, it is expected that the need for
    interrupt pins will diminish over time.  Devices that do not support
    interrupt pins due to pin constraints (rely on polling for device
    service) may implement messages to increase performance without
    adding additional pins.  Therefore, system configuration software
    must not assume that a message capable device has an interrupt pin.

The xhci device *does* implement this fallback to INTx.

For better or worse, fallback to INTx has never been implemented in
ivshmem.  You can either ask for an INTx-only device (msi=off), or for
an MSI-X-only device (msi=on).  The latter *cannot* do interrupts until
you enable MSI-X.

Similarly, the ivshmem-doorbell device introduced later in this series
can only do MSI-X, and the ivshmem-plain device cannot do interrupts at
all.

We could of course implement the fallback in ivshmem, too.  It's not
quite as simple as making ivshmem_update_irq() do nothing when
msix_enabled(), we also have to adapt ivshmem_vector_notify(), update
ivshmem-spec.txt, and cover the fallback in the tests.  Also limit the
change to revision 1 of the device for compatibility.  I very much doubt
this is worth the trouble.

A PCI function declares its INTx use in config space register Interrupt
Pin.  Ibid., section 6.2.4. Miscellaneous Registers:

    The Interrupt Pin register tells which interrupt pin the device (or
    device function) uses.  A value of 1 corresponds to INTA#.  A value
    of 2 corresponds to INTB#.  A value of 3 corresponds to INTC#.  A
    value of 4 corresponds to INTD#.  Devices (or device functions) that
    do not use an interrupt pin must put a 0 in this register.

ivshmem with msi=on should therefore put 0 in this register.  It
doesn't, but I feel it's better to let it remain bug-compatible.
ivshmem-doorbell and ivshmem-plain get it right.

Aside: xhci falls back to INTx, and should therefore declare its use of
INTx in the Interrupt Pin register, but I can't see where it does.