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

["Cédric Le Goater" <clg@redhat.com>]

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);
>