Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Markus Armbruster <armbru@redhat.com>]

Ladi Prosek <lprosek@redhat.com> writes:

> On Fri, Dec 8, 2017 at 6:28 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Ladi Prosek <lprosek@redhat.com> writes:
>>
>>> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Ladi Prosek <lprosek@redhat.com> writes:
>>>>
>>>>> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>>>>>
>>>>> This manifested as:
>>>>> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>>>>>
>>>>> when irqfd was enabled before reset and then enabled again after reset, making
>>>>> ivshmem_enable_irqfd() run for the second time.
>>>>>
>>>>> To reproduce, run:
>>>>>
>>>>>   ivshmem-server
>>>>>
>>>>> and QEMU with:
>>>>>
>>>>>   -device ivshmem-doorbell,chardev=iv
>>>>>   -chardev socket,path=/tmp/ivshmem_socket,id=iv
>>>>>
>>>>> then install the Windows driver, at the time of writing available at:
>>>>>
>>>>> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>>>>>
>>>>> and crash-reboot the guest by inducing a BSOD.
>>>>>
>>>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>>>> ---
>>>>>  hw/misc/ivshmem.c | 24 +++++++++++++-----------
>>>>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>>>> index d1bb246d12..4be0d2627b 100644
>>>>> --- a/hw/misc/ivshmem.c
>>>>> +++ b/hw/misc/ivshmem.c
>>>>> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>>>>>      }
>>>>>  }
>>>>>
>>>>> -static void ivshmem_reset(DeviceState *d)
>>>>> -{
>>>>> -    IVShmemState *s = IVSHMEM_COMMON(d);
>>>>> -
>>>>> -    s->intrstatus = 0;
>>>>> -    s->intrmask = 0;
>>>>> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>>>> -        ivshmem_msix_vector_use(s);
>>>>> -    }
>>>>> -}
>>>>> -
>>>>>  static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>>>>  {
>>>>>      /* allocate QEMU callback data for receiving interrupts */
>>>>> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>>>>>
>>>>>  }
>>>>>
>>>>> +static void ivshmem_reset(DeviceState *d)
>>>>> +{
>>>>> +    IVShmemState *s = IVSHMEM_COMMON(d);
>>>>> +
>>>>> +    ivshmem_disable_irqfd(s);
>>>>> +
>>>>> +    s->intrstatus = 0;
>>>>> +    s->intrmask = 0;
>>>>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>>>> +        ivshmem_msix_vector_use(s);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>>>>                                   uint32_t val, int len)
>>>>>  {
>>>>
>>>> Why are you moving ivshmem_reset()?  Makes the actual change harder to
>>>> see than necessary.
>>>
>>> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
>>> source file. I generally prefer to order static functions
>>> topologically if possible. If you'd prefer adding a forward decl
>>> instead (fewer lines touched, easier to bisect?) I can certainly do
>>> that. Thanks!
>>
>> Well, it compiles before your patch, your patch doesn't add any calls,
>> so I can't quite see why a forward declaration would be needed.
>
> It adds a call to ivshmem_disable_irqfd().

Note to self: patch review on Friday late afternoon is a bad idea.

I'd prefer the forward declaration.  Thanks!