Re: [RFC PATCH 1/1] hw/display/xenfb: always register vfb and allocate console early

On 2026/08/24 16:11, Dario Faggioli wrote:
> This commit addresses a black console issues for Xen PV and PVH guests.
> 
> In fact, commit 6ece1df966 ("hw/xen: Register framebuffer backend via
> xen_backend_init()") introduced a check before registering the vfb
> backend. Problem is that the '-vga' agrument may not be present (e.g.,
> for PV/PVH guests started with 'xl') and this causes the backend to be
> silently ignored.
> 
> This commit restores the unconditional registration of the vfb backend.

This part looks correct.

> 
> Furthermore, even with the backend always being registered, the fact
> that xenfb allocates the QemuConsole asynchronously in fb_initialise()
> looks problematic. In fact, when the UI initializes, it finds 0 active
> consoles and it permanently allocates a dummy surface showing the
> message "This VM has no graphic display device". And since the removal
> of console_select() there's no way to dynamically switch to the xenfb
> console, when it is finally up and running.
> 
> This commit works around the issue by moving console creation to
> fb_init(), so that VNC attaches to it immediately. The surface is then
> updated normally via qemu_console_set_surface() once the guest framebuffer
> is mapped.

Moving console creation to fb_init() does not look sufficient. fb_init() 
is driven by the Xenstore backend state machine, so it is not guaranteed 
to run before display initialization. If the vfb backend instance is 
discovered later, fb_init() will run later in response to a Xenstore event.

fb_init() is also a per-connection hook. If the frontend closes and 
reconnects while the backend object remains, fb_init() runs again on the 
same XenFB object. The unconditional assignment then creates another 
QemuConsole and overwrites fb->con, leaving the previous console registered.

There is also no matching teardown. When the backend Xenstore node 
disappears, xen_pv_del_xendev() invokes ops->free before unplugging the 
XenFB object, but xen_framebuffer_ops has no .free callback. The 
QemuConsole can therefore retain a pointer to the freed XenFB as its 
opaque value.

Could the QemuConsole instead be created by xen_framebuffer_ops.alloc 
and closed with qemu_graphic_console_close() from 
xen_framebuffer_ops.free? The .alloc hook runs once per XenLegacyDevice, 
so the console would remain stable across frontend reconnects, while 
.free would close it when the backend object is removed.

The .alloc hook is the earliest per-device point. However, it still
cannot make the console visible before display initialization if the vfb
backend instance itself is created later.

> 
> Fixes: 6ece1df966 ("hw/xen: Register framebuffer backend via xen_backend_init()")

The unconditional-registration hunk fixes 6ece1df96629. The console 
creation hunk addresses a separate regression introduced by 
e99441a3793b. Please split the changes and give each patch its 
corresponding Fixes tag, using at least 12 hexadecimal digits:

Fixes: 6ece1df96629 ("hw/xen: Register framebuffer backend via 
xen_backend_init()")
Fixes: e99441a3793b ("ui/curses: Do not use console_select()")

These commits first appeared in v9.1 and v9.0, respectively. The 
registration change is not needed in v9.0, so combining the fixes 
complicates backporting the console fix to that release.

Regards,
Akihiko Odaki

> Signed-off-by: Dario Faggioli <[email protected]>
> ---
>   hw/display/xenfb.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index ae302b217f..3a0cdc0578 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -851,9 +851,14 @@ static void xenfb_handle_events(struct XenFB *xenfb)
>   
>   static int fb_init(struct XenLegacyDevice *xendev)
>   {
> +    struct XenFB *fb = container_of(xendev, struct XenFB, c.xendev);
> +
>   #ifdef XENFB_TYPE_RESIZE
>       xenstore_write_be_int(xendev, "feature-resize", 1);
>   #endif
> +
> +    fb->con = qemu_graphic_console_create(NULL, 0, &xenfb_ops, fb);
> +
>       return 0;
>   }
>   
> @@ -882,8 +887,6 @@ static int fb_initialise(struct XenLegacyDevice *xendev)
>       if (rc != 0)
>           return rc;
>   
> -    fb->con = qemu_graphic_console_create(NULL, 0, &xenfb_ops, fb);
> -
>       if (xenstore_read_fe_int(xendev, "feature-update", &fb->feature_update) == -1)
>           fb->feature_update = 0;
>       if (fb->feature_update)
> @@ -973,9 +976,6 @@ static const GraphicHwOps xenfb_ops = {
>   static void xen_ui_register_backend(void)
>   {
>       xen_be_register("vkbd", &xen_kbdmouse_ops);
> -
> -    if (vga_interface_type == VGA_XENFB) {
> -        xen_be_register("vfb", &xen_framebuffer_ops);
> -    }
> +    xen_be_register("vfb", &xen_framebuffer_ops);
>   }
>   xen_backend_init(xen_ui_register_backend);