>-----Original Message----- >From: Joao Martins <[email protected]> >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 <[email protected]> >>> >>> 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
