On 6/11/26 11:03, Marc-André Lureau wrote:
> The QemuGraphicConsole holds a strong QOM link back to the device via
> its "device" property (OBJ_PROP_LINK_STRONG). When graphic_console_close()
> is only called from vfio_display_finalize() during object finalize, this
> creates a ref-cycle deadlock: the device can't reach refcount 0 because
> the console holds a strong ref, but the console's ref is only dropped by
> graphic_console_close() which runs inside finalize.
>
> Split the display teardown into two phases:
> - vfio_display_exit(): called during unrealize (vfio_exitfn), closes
> the graphic console to break the ref cycle, and removes display
> region subregions while the parent memory regions are still alive.
> - vfio_display_finalize(): remains in finalize (vfio_pci_put_device),
> frees display region memory, dmabuf, and edid resources. The region
> memory contains QOM child objects (MemoryRegions) that must stay
> alive until QOM finalization has processed them.
>
> Acked-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> MAINTAINERS | 1 +
> hw/vfio/pci.h | 1 +
> hw/vfio/display.c | 29 +++++++++++++++++------------
> hw/vfio/pci.c | 1 +
> 4 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 748ec77beb4..8320b37fc32 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2347,6 +2347,7 @@ S: Supported
> F: hw/vfio/*
> F: util/vfio-helpers.c
> F: include/hw/vfio/
> +F: docs/devel/vfio-mdpy.rst
> F: docs/devel/migration/vfio.rst
> F: qapi/vfio.json
> F: migration/vfio-stub.c
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index c3a1f53d350..cf567115870 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -270,6 +270,7 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
>
> void vfio_display_reset(VFIOPCIDevice *vdev);
> bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
> +void vfio_display_exit(VFIOPCIDevice *vdev);
> void vfio_display_finalize(VFIOPCIDevice *vdev);
>
> extern const VMStateDescription vfio_display_vmstate;
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 8f91e83da88..cb83d98e9af 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -505,15 +505,6 @@ static bool vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
> return true;
> }
>
> -static void vfio_display_region_exit(VFIODisplay *dpy)
> -{
> - if (!dpy->region.buffer.size) {
> - return;
> - }
> -
> - vfio_region_exit(&dpy->region.buffer);
> - vfio_region_finalize(&dpy->region.buffer);
> -}
>
> /* ---------------------------------------------------------------------- */
>
> @@ -547,17 +538,31 @@ bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
> return false;
> }
>
> -void vfio_display_finalize(VFIOPCIDevice *vdev)
> +void vfio_display_exit(VFIOPCIDevice *vdev)
> {
> if (!vdev->dpy) {
> return;
> }
>
> - qemu_graphic_console_close(vdev->dpy->con);
> vfio_display_dmabuf_exit(vdev->dpy);
> - vfio_display_region_exit(vdev->dpy);
> + qemu_graphic_console_close(vdev->dpy->con);
> + if (vdev->dpy->region.buffer.size) {
> + vfio_region_exit(&vdev->dpy->region.buffer);
> + }
> +}
> +
> +void vfio_display_finalize(VFIOPCIDevice *vdev)
> +{
> + if (!vdev->dpy) {
> + return;
> + }
> +
> + if (vdev->dpy->region.buffer.size) {
> + vfio_region_finalize(&vdev->dpy->region.buffer);
> + }
> vfio_display_edid_exit(vdev->dpy);
> g_free(vdev->dpy);
> + vdev->dpy = NULL;
> }
>
> static bool migrate_needed(void *opaque)
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 9c06b25e637..78beacd24e1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3624,6 +3624,7 @@ static void vfio_exitfn(PCIDevice *pdev)
> VFIOPCIDevice *vdev = VFIO_PCI_DEVICE(pdev);
> VFIODevice *vbasedev = &vdev->vbasedev;
>
> + vfio_display_exit(vdev);
In vfio_pci_realize(), if there is a failure after vfio_display_probe(),
should we cleanup explicitly the display resources too ?
Thanks,
C.
> vfio_unregister_req_notifier(vdev);
> vfio_unregister_err_notifier(vdev);
> pci_device_set_intx_routing_notifier(pdev, NULL);
>