Re: [PATCH v1] tools: make "opengl" generic

[Anthony PERARD <anthony@xxxxxxxxxxxxxx>]

On Mon, Jul 28, 2025 at 12:40:07PM +0800, Penny Zheng wrote:
> Display option, like vnc, sdl, etc, will be checked against in latest QEMU
> whether it is compatile with opengl context. And vnc is incompatible with GL
> context.
> Now, when running hvm domain with gl context on, such as
> "device_model_args_hvm = ["-display", "sdl,gl=on"]", we will fail with
> the error of "qemu-system-i386: -vnc 127.0.0.1:0,to=99: Display vnc is
> incompatible with the GL context", as vnc is set enabled on default
> for HVM domain.
> 
> We shall move "opengl" option out of specifc sdl display, to make it

Unfurtunatly, we can't remove field from a struct in the public API of
libxl. You'll find some explanation about the "libxl API compatibility"
in "libxl.h". It's not well explain but "libxl_types.idl" is part of the
public API.

> generic. Then when users explicitly set "opengl = 1", default values for
> vnc shall be changed to disabled and libxl__dm_vnc() needs to return NULL
> indicating vnc being disabled.

> If users select both vnc and opengl in xl configuration, creation
> will fail and error out incompatible info.
> To keep consistency, we also make "opengl" generic for vfb[] options
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>

This patch description is a bit confusing to read to me. Right now, there's no
mention of "opengl" in the docs, or in `man xl.cfg`, even if "opengl=1"
seems to be a thing for `xl`.

In libxl code, it seems "opengl" is only an option for the PV display
interface, or VFB. So what I'm guessing is you want to add support to
HVM guest to have the opengl option working.

The patch description mention "-display sdl,gl=on", yet the patch itself
doesn't modify "-display sdl" to enable the gl context. Why?

The xl.cfg man page isn't forthcoming about it but the use of
"device_model_args*" is mainly useful for debugging, to allow to add
extra option without having to change libxl. If an option added there is
incompatible with the command line generated by libxl, then it would be
better to teach libxl to generate that option.


> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 28cdbf07c2..9e9adcec77 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2791,14 +2790,16 @@ skip_usbdev:
>  
>  #undef parse_extra_args
>  
> +    if (!xlu_cfg_get_long (config, "vnc", &l, 0))
> +        vnc_enabled = l;
> +    xlu_cfg_get_defbool(config, "opengl", &b_info->u.hvm.opengl, 0);
> +    if (vnc_enabled && libxl_defbool_val(b_info->u.hvm.opengl)) {
> +        fprintf(stderr, "vnc is incompatible with opengl\n");
> +        exit(1);

This sort of incompatibility need to at least live in libxl
(tools/libs/light). `xl` is only one of several users of `libxl`.


Thanks,

-- 
Anthony PERARD