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.d...@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);