On Fri, Dec 8, 2017 at 6:28 PM, Markus Armbruster <arm...@redhat.com> wrote: > Ladi Prosek <lpro...@redhat.com> writes: > >> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster <arm...@redhat.com> wrote: >>> Ladi Prosek <lpro...@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 <lpro...@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().