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

On 2026/08/24 22:59, Akihiko Odaki wrote:
> 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.

Also probably it is a good idea to have Cc: [email protected]

> 
> 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);
>