Re: [Xen-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Chen, Tiejun]

This duplicates the code from above. I think this would be best done as:
static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
{
     if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
         return 0;

     if (libxl__is_igd_vga_passthru(gc, guest_config)) {
         b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
         return 0;
     }

     LOG(ERROR, "Unable to detect graphics passthru kind");
     return 1;
}

Then for the code in libxl__build_device_model_args_new:

          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
              if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
                   return NULL
              switch (b_info->u.hvm.gfx_passthru_kind) {
              case LIBXL_GFX_PASSTHRU_KIND_IGD:
                  machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
                  break;
              default:
                  LOG(ERROR, "unknown gfx_passthru_kind\n");
                 return NULL;
              }
         }

That is, a helper to try and autodetect kind if it is default and then a
single switch entry for each kind.

+            default:
+                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");

Please return an error here, as I've shown above.

Looks good and thanks, but here 'guest_config' is a const so we 
shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,

b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;

So I tried to refactor a little bit to follow up yours,

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..605b17c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
     return opt;
 }

+static int
+libxl__detect_gfx_passthru_kind(libxl__gc *gc,
+                                const libxl_domain_config *guest_config)
+{
+    const libxl_domain_build_info *b_info = &guest_config->b_info;
+
+    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
+        return b_info->u.hvm.gfx_passthru_kind;
+
+    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+        return LIBXL_GFX_PASSTHRU_KIND_IGD;
+    }
+
+    LOG(ERROR, "Unable to detect graphics passthru kind");
+    return -1;
+}
+
 static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         const libxl_domain_config 
*guest_config,
@@ -427,7 +444,7 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
     const char *keymap = dm_keymap(guest_config);
     char *machinearg;
     flexarray_t *dm_args;
-    int i, connection, devid;
+    int i, connection, devid, gfx_passthru_kind;
     uint64_t ram_size;
     const char *path, *chardev;

@@ -710,9 +727,6 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, "-net");
             flexarray_append(dm_args, "none");
         }
-        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-            flexarray_append(dm_args, "-gfx_passthru");
-        }
     } else {
         if (!sdl && !vnc) {
             flexarray_append(dm_args, "-nographic");
@@ -757,6 +771,20 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                             machinearg, max_ram_below_4g);
             }
         }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
+ 
guest_config);
+            switch (gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(ERROR, "unknown gfx_passthru_kind\n");
+                return NULL;
+            }
+        }
+
         flexarray_append(dm_args, machinearg);
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);


diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c97c62d..8912421 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3632,6 +3632,11 @@ static inline void
libxl__update_config_vtpm(libxl__gc *gc,
    */
   void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
                                       const libxl_bitmap *sptr);
+
+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND

No need for this ifdef.

Removed.

Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel