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 <jan.kis...@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?
>
> 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
