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

[Michael S. Tsirkin]

On Fri, Aug 24, 2012 at 07:59:06AM +0200, Jan Kiszka wrote:
> 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 <address@hidden>
> > 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?

Second option seems more prudent to me. Can you send a patch pls?

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