1. Patchew
  2. QEMU
  3. [PATCH v4 0/5] VFIO migration related refactor and bug fix
  4. View patch

[PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path

Zhenzhong Duan posted 5 patches 2 years, 7 months ago
There is a newer version of this series
[PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
Posted by Zhenzhong Duan 2 years, 7 months ago 
When vfio realize fails, INTx isn't disabled if it has been enabled.
This may confuse host side with unhandled interrupt report.

Add a new label to be used for vfio_intx_enable() failed case.

Fixes: a9994687cb9b ("vfio/display: core & wireup")
Fixes: b290659fc3dd ("hw/vfio/display: add ramfb support")
Fixes: c62a0c7ce34e ("vfio/display: add xres + yres properties")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ab6645ba60af..54a8179d1c64 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3167,7 +3167,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
         ret = vfio_intx_enable(vdev, errp);
         if (ret) {
-            goto out_deregister;
+            goto out_intx_disable;
         }
     }
 
@@ -3220,6 +3220,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     return;
 
 out_deregister:
+    vfio_disable_interrupts(vdev);
+out_intx_disable:
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     if (vdev->irqchip_change_notifier.notify) {
         kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
-- 
2.34.1
Re: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
Posted by Joao Martins 2 years, 7 months ago 
On 29/06/2023 09:40, Zhenzhong Duan wrote:
> When vfio realize fails, INTx isn't disabled if it has been enabled.
> This may confuse host side with unhandled interrupt report.
> 
> Add a new label to be used for vfio_intx_enable() failed case.
> 
> Fixes: a9994687cb9b ("vfio/display: core & wireup")
> Fixes: b290659fc3dd ("hw/vfio/display: add ramfb support")
> Fixes: c62a0c7ce34e ("vfio/display: add xres + yres properties")

Sounds to me the correct Fixes tag is the same as first patch i.e.:

Fixes: c5478fea27ac ("vfio/pci: Respond to KVM irqchip change notifier")

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Looks good, but see some clarifications below.

> ---
>  hw/vfio/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ab6645ba60af..54a8179d1c64 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3167,7 +3167,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
>          ret = vfio_intx_enable(vdev, errp);
>          if (ret) {
> -            goto out_deregister;
> +            goto out_intx_disable;
>          }
>      }
>  
> @@ -3220,6 +3220,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      return;
>  
>  out_deregister:
> +    vfio_disable_interrupts(vdev);

You are calling vfio_disable_interrupts() when what you want is
vfio_intx_disable() ? But I guess your thinking was to call
vfio_disable_interrupt() which eventually calls vfio_intx_disable() in case INTx
was really setup, thus saving the duplicated check. The MSIx/MSI in realize() I
don't think they will be enabled at this point. Let me know if I misunderstood.

> +out_intx_disable:

Maybe 'out_intx_teardown' or 'out_intx_deregister' because you are not really
disabling INTx.

>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      if (vdev->irqchip_change_notifier.notify) {
>          kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
Re: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
Posted by Cédric Le Goater 2 years, 7 months ago 
On 6/29/23 13:24, Joao Martins wrote:
> On 29/06/2023 09:40, Zhenzhong Duan wrote:
>> When vfio realize fails, INTx isn't disabled if it has been enabled.
>> This may confuse host side with unhandled interrupt report.
>>
>> Add a new label to be used for vfio_intx_enable() failed case.
>>
>> Fixes: a9994687cb9b ("vfio/display: core & wireup")
>> Fixes: b290659fc3dd ("hw/vfio/display: add ramfb support")
>> Fixes: c62a0c7ce34e ("vfio/display: add xres + yres properties")
> 
> Sounds to me the correct Fixes tag is the same as first patch i.e.:
> 
> Fixes: c5478fea27ac ("vfio/pci: Respond to KVM irqchip change notifier")
> 
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> 
> Looks good, but see some clarifications below.
> 
>> ---
>>   hw/vfio/pci.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ab6645ba60af..54a8179d1c64 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3167,7 +3167,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>           kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
>>           ret = vfio_intx_enable(vdev, errp);
>>           if (ret) {
>> -            goto out_deregister;
>> +            goto out_intx_disable;
>>           }
>>       }
>>   
>> @@ -3220,6 +3220,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>       return;
>>   
>>   out_deregister:
>> +    vfio_disable_interrupts(vdev);
> 
> You are calling vfio_disable_interrupts() when what you want is
> vfio_intx_disable() ? But I guess your thinking was to call
> vfio_disable_interrupt() which eventually calls vfio_intx_disable() in case INTx
> was really setup, thus saving the duplicated check. The MSIx/MSI in realize() I
> don't think they will be enabled at this point. Let me know if I misunderstood.
> 
>> +out_intx_disable:
> 
> Maybe 'out_intx_teardown' or 'out_intx_deregister' because you are not really
> disabling INTx.

or simply extract from vfio_disable_interrupts() :
  
     if (vdev->interrupt == VFIO_INT_INTx) {
         vfio_intx_disable(vdev);
     }

and add the above code before cleaning up the intx routing
notifier without any new goto labels.

Thanks,

C.


> 
>>       pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>       if (vdev->irqchip_change_notifier.notify) {
>>           kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
>
Re: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
Posted by Joao Martins 2 years, 7 months ago 

On 29/06/2023 16:13, Cédric Le Goater wrote:
> On 6/29/23 13:24, Joao Martins wrote:
>> On 29/06/2023 09:40, Zhenzhong Duan wrote:
>>> When vfio realize fails, INTx isn't disabled if it has been enabled.
>>> This may confuse host side with unhandled interrupt report.
>>>
>>> Add a new label to be used for vfio_intx_enable() failed case.
>>>
>>> Fixes: a9994687cb9b ("vfio/display: core & wireup")
>>> Fixes: b290659fc3dd ("hw/vfio/display: add ramfb support")
>>> Fixes: c62a0c7ce34e ("vfio/display: add xres + yres properties")
>>
>> Sounds to me the correct Fixes tag is the same as first patch i.e.:
>>
>> Fixes: c5478fea27ac ("vfio/pci: Respond to KVM irqchip change notifier")
>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>
>> Looks good, but see some clarifications below.
>>
>>> ---
>>>   hw/vfio/pci.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index ab6645ba60af..54a8179d1c64 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3167,7 +3167,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>           kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
>>>           ret = vfio_intx_enable(vdev, errp);
>>>           if (ret) {
>>> -            goto out_deregister;
>>> +            goto out_intx_disable;
>>>           }
>>>       }
>>>   @@ -3220,6 +3220,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>       return;
>>>     out_deregister:
>>> +    vfio_disable_interrupts(vdev);
>>
>> You are calling vfio_disable_interrupts() when what you want is
>> vfio_intx_disable() ? But I guess your thinking was to call
>> vfio_disable_interrupt() which eventually calls vfio_intx_disable() in case INTx
>> was really setup, thus saving the duplicated check. The MSIx/MSI in realize() I
>> don't think they will be enabled at this point. Let me know if I misunderstood.
>>
>>> +out_intx_disable:
>>
>> Maybe 'out_intx_teardown' or 'out_intx_deregister' because you are not really
>> disabling INTx.
> 
> or simply extract from vfio_disable_interrupts() :
>  
>     if (vdev->interrupt == VFIO_INT_INTx) {
>         vfio_intx_disable(vdev);
>     }
> 
> and add the above code before cleaning up the intx routing
> notifier without any new goto labels.
> 
An even better option indeed.
RE: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
Posted by Duan, Zhenzhong 2 years, 7 months ago 
>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: Re: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
>
>
>
>On 29/06/2023 16:13, Cédric Le Goater wrote:
>> On 6/29/23 13:24, Joao Martins wrote:
>>> On 29/06/2023 09:40, Zhenzhong Duan wrote:
>>>> When vfio realize fails, INTx isn't disabled if it has been enabled.
>>>> This may confuse host side with unhandled interrupt report.
>>>>
>>>> Add a new label to be used for vfio_intx_enable() failed case.
>>>>
>>>> Fixes: a9994687cb9b ("vfio/display: core & wireup")
>>>> Fixes: b290659fc3dd ("hw/vfio/display: add ramfb support")
>>>> Fixes: c62a0c7ce34e ("vfio/display: add xres + yres properties")
>>>
>>> Sounds to me the correct Fixes tag is the same as first patch i.e.:
>>>
>>> Fixes: c5478fea27ac ("vfio/pci: Respond to KVM irqchip change
>>> notifier")

OK, will use it.
Previously I thought I should pick commit a9994687cb9b which firstly
introduced the timer leak with a jump label out_teardown, then
b290659fc3dd and c62a0c7ce34e which used out_teardown.

>>>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>
>>> Looks good, but see some clarifications below.
>>>
>>>> ---
>>>>   hw/vfio/pci.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
>>>> ab6645ba60af..54a8179d1c64 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -3167,7 +3167,7 @@ static void vfio_realize(PCIDevice *pdev,
>>>> Error **errp)
>>>>
>>>> kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
>>>>           ret = vfio_intx_enable(vdev, errp);
>>>>           if (ret) {
>>>> -            goto out_deregister;
>>>> +            goto out_intx_disable;
>>>>           }
>>>>       }
>>>>   @@ -3220,6 +3220,8 @@ static void vfio_realize(PCIDevice *pdev,
>>>> Error **errp)
>>>>       return;
>>>>     out_deregister:
>>>> +    vfio_disable_interrupts(vdev);
>>>
>>> You are calling vfio_disable_interrupts() when what you want is
>>> vfio_intx_disable() ? But I guess your thinking was to call
>>> vfio_disable_interrupt() which eventually calls vfio_intx_disable()
>>> in case INTx was really setup, thus saving the duplicated check. The
>>> MSIx/MSI in realize() I don't think they will be enabled at this point.
Yes.

>>> Let me know if I misunderstood.
>>>
>>>> +out_intx_disable:
>>>
>>> Maybe 'out_intx_teardown' or 'out_intx_deregister' because you are
>>> not really disabling INTx.
>>
>> or simply extract from vfio_disable_interrupts() :
>>
>>     if (vdev->interrupt == VFIO_INT_INTx) {
>>         vfio_intx_disable(vdev);
>>     }
>>
>> and add the above code before cleaning up the intx routing notifier
>> without any new goto labels.
>>
>An even better option indeed.
Will do.

Thanks
Zhenzhong

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.