Re: [Qemu-devel] [PATCH v2 22/42] ivshmem: Leave INTx alone when using MSI-X

[Marc-André Lureau]

Hi

On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster <address@hidden> wrote:
> The ivshmem device can either use MSI-X or legacy INTx for interrupts.
>
> With MSI-X enabled, peer interrupt events trigger an MSI as they
> should.  But software can still raise INTx via interrupt status and
> mask register in BAR 0.  This is explicitly prohibited by PCI Local
> Bus Specification Revision 3.0, section 6.8.3.3:
>
>     While enabled for MSI or MSI-X operation, a function is prohibited
>     from using its INTx# pin (if implemented) to request service (MSI,
>     MSI-X, and INTx# are mutually exclusive).
>
> Fix the device model to leave INTx alone when using MSI-X.
>
> Document that we claim to use INTx in config space even when we don't.
> Unlike other devices, ivshmem does *not* use INTx when configured for
> MSI-X and MSI-X isn't enabled by software.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hw/misc/ivshmem.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index cfea151..fc37feb 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -126,6 +126,11 @@ static void ivshmem_update_irq(IVShmemState *s)
>      PCIDevice *d = PCI_DEVICE(s);
>      uint32_t isr = s->intrstatus & s->intrmask;
>
> +    /* No INTx with msi=off, whether the guest enabled MSI-X or not */
> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +        return;

So you probably mean msi=on

with that
Reviewed-by: Marc-André Lureau <address@hidden>



> +    }
> +
>      /* don't print ISR resets */
>      if (isr) {
>          IVSHMEM_DPRINTF("Set IRQ to %d (%04x %04x)\n",
> @@ -874,6 +879,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>      pci_conf = dev->config;
>      pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
>
> +    /*
> +     * 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);
>
>      memory_region_init_io(&s->ivshmem_mmio, OBJECT(s), &ivshmem_mmio_ops, s,
> --
> 2.4.3
>
>



-- 
Marc-André Lureau