xl/libxl only applies vkb=[] to PV & PVH guests.  HVM gets only a single
vkb by default, but that can be disabled by the vkb_device boolean.
Notably the HVM vkb cannot be configured, so feature-abs-pointer or the
backend-type cannot be specified.

Re-arrange the logic so that vkb=[] is handled regardless of domain
type.  If vkb is empty or unspecified, follow the vkb_device boolean for
HVMs.  Nothing changes for PVH & PV.  HVMs can now get a configured vkb
instead of just the default one.

The chance for regression is an HVM config with
vkb=["$something"]
vkb_device=false

Which would now get a vkb.

This is useful for vGlass which provides a VKB to HVMs.  vGlass wants to
specify feature-abs-pointer, but that is racily written by vGlass
instead of coming through the xl.cfg.  Unhelpfully, Linux xen-kbdfront
reads the backend nodes without checking that the backend is in
InitWait.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libs/light/libxl_create.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index ce1d431103..39c50b3711 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1771,24 +1771,31 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         libxl__device_add(gc, domid, &libxl__virtio_devtype,
                           &d_config->virtios[i]);
 
+    if (d_config->num_vkbs) {
+        for (i = 0; i < d_config->num_vkbs; i++) {
+            libxl__device_add(gc, domid, &libxl__vkb_devtype,
+                              &d_config->vkbs[i]);
+        }
+    } else if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
+               libxl_defbool_val(d_config->b_info.u.hvm.vkb_device)) {
+        libxl_device_vkb vkb;
+
+        libxl_device_vkb_init(&vkb);
+        libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
+        libxl_device_vkb_dispose(&vkb);
+    }
+
     switch (d_config->c_info.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
     {
         libxl__device_console console;
         libxl__device device;
-        libxl_device_vkb vkb;
 
         init_console_info(gc, &console, 0);
         console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
-        if (libxl_defbool_val(d_config->b_info.u.hvm.vkb_device)) {
-            libxl_device_vkb_init(&vkb);
-            libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
-            libxl_device_vkb_dispose(&vkb);
-        }
-
         dcs->sdss.dm.guest_domid = domid;
         if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
             libxl__spawn_stub_dm(egc, &dcs->sdss);
@@ -1816,11 +1823,6 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
                               &d_config->vfbs[i]);
         }
 
-        for (i = 0; i < d_config->num_vkbs; i++) {
-            libxl__device_add(gc, domid, &libxl__vkb_devtype,
-                              &d_config->vkbs[i]);
-        }
-
         if (d_config->b_info.arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
             init_console_info(gc, &vuart, 0);
             vuart.backend_domid = state->console_domid;
-- 
2.43.0

On Tue, Jan 09, 2024 at 12:16:31PM -0500, Jason Andryuk wrote:
> xl/libxl only applies vkb=[] to PV & PVH guests.  HVM gets only a single
> vkb by default, but that can be disabled by the vkb_device boolean.
> Notably the HVM vkb cannot be configured, so feature-abs-pointer or the
> backend-type cannot be specified.
> 
> Re-arrange the logic so that vkb=[] is handled regardless of domain
> type.  If vkb is empty or unspecified, follow the vkb_device boolean for
> HVMs.  Nothing changes for PVH & PV.  HVMs can now get a configured vkb
> instead of just the default one.
> 
> The chance for regression is an HVM config with
> vkb=["$something"]
> vkb_device=false
> 
> Which would now get a vkb.
> 
> This is useful for vGlass which provides a VKB to HVMs.  vGlass wants to
> specify feature-abs-pointer, but that is racily written by vGlass
> instead of coming through the xl.cfg.  Unhelpfully, Linux xen-kbdfront
> reads the backend nodes without checking that the backend is in
> InitWait.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

I guess we would want an entry in the CHANGELOG about "vkb=[]" been
available on HVM guests.


> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index ce1d431103..39c50b3711 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -1771,24 +1771,31 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>          libxl__device_add(gc, domid, &libxl__virtio_devtype,
>                            &d_config->virtios[i]);
>  
> +    if (d_config->num_vkbs) {
> +        for (i = 0; i < d_config->num_vkbs; i++) {
> +            libxl__device_add(gc, domid, &libxl__vkb_devtype,
> +                              &d_config->vkbs[i]);

While there, could you check the return value of that function? That
would be having:
    ret = libxl__device_add();
    if (ret) goto error_out;

With that: Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD