Re: [Qemu-devel] MSI-X bug with ivshmem since msix_reset moved to PCI

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

On 2012-08-24 01:13, Cam Macdonell wrote:
> Hi Jan,
> 
> I've bisected a bug in which MSI interrupts are not being delivered to
> the following patch, where msix_reset was moved in tot he PCI core.
> 
> commit cbd2d4342b3d42ab33baa99f5b7a23491b5692f2
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date:   Tue May 15 20:09:56 2012 -0300
> 
>     msi: Invoke msi/msix_reset from PCI core
> 
>     There is no point in pushing this burden to the devices, they tend to
>     forget to call them (like intel-hda, ahci, xhci did). Instead, reset
>     functions are now called from pci_device_reset. They do nothing if
>     MSI/MSI-X is not in use.
> 
> I've been debugging and it seems that when msix_notify() is triggered
> the second test in the "if" fails
> 
> /* Send an MSI-X message */
> void msix_notify(PCIDevice *dev, unsigned vector)
> {
>     MSIMessage msg;
> 
>     if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
>         return;
> 
>    …
> }
> 
> here is some MSI-X debugging statements
> 
> msix_init
> IVSHMEM: msix initialized (1 vectors)
> IVSHMEM: using vector 0
> IVSHMEM: ivshmem_reset
> IVSHMEM: using vector 0
> msix_reset
> msix_free_irq_entries 0x7fd52d1cea20
> 
> msix_free_irq_entries() sets dev->msix_entries_nr to 0, so I think it
> may be the cause.

I suppose you mean it sets the msix_entry_used array to 0.

> 
> Shouldn't ivshmem's reset (which reenables the vectors) be triggered
> by the msix_reset?

Actually, the whole msix vector usage tracking is useless today, this
just shows its downsides (in the absence of benefits). Megasas is
affected by this problem as well, virtio not as it calls msix_vector_use
during the configuration process the guest driver triggers.

Two options:
 - I can send my removal patch for msix_vector_use/unuse that I was
   only planning for 1.3 so far, and we kill this pitfall earlier.
 - We re-add msix_vector_use calls to the affected device models for
   1.2 and drop them later again for 1.3 when removing usage tracking.
[The third option to keep the usage tracking is a non-option for me. ;)]

Michael?

> 
> Thanks,
> Cam
> 
> p.s. And apologies, I should've caught this bug closer to that patch
> being merged.

No problem. I should have seen this issue while changing the code.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]