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

[Markus Armbruster]

Ladi Prosek  writes:

> On Fri, Dec 8, 2017 at 6:28 PM, Markus Armbruster  wrote:
>> Ladi Prosek  writes:
>>
>>> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster  wrote:
 Ladi Prosek  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 
> ---
>  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!

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

[Ladi Prosek]

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

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

[Markus Armbruster]

Ladi Prosek  writes:

> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster  wrote:
>> Ladi Prosek  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 
>>> ---
>>>  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.

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

[Eric Blake]

On 12/08/2017 07:44 AM, Ladi Prosek wrote:

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

That, or split it into two patches (one doing just the code motion, the
other making the semantic change).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

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

[Ladi Prosek]

On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster  wrote:
> Ladi Prosek  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 
>> ---
>>  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!

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

[Markus Armbruster]

Ladi Prosek  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 
> ---
>  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.