| Message ID | 20230629084042.86502-4-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFIO migration related refactor and bug fix | expand |
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);
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); >
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.
>-----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
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);
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(-)