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

[Jan Kiszka]

On 2012-08-24 10:11, Michael S. Tsirkin wrote:
> 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?

In fact, it's not as easy. ivshmem already tries to restore the usage
flag but fails due to reset handler ordering. I do not see ATM where
there is some "enable device" during re-initialization, at least for
ivshmem. Haven't checked megasas yet.

I tend to think removing is simpler and less risky. Please have a look
at the patch I sent earlier.

Jan

signature.asc
Description: OpenPGP digital signature