Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

["Chen, Tiejun" <tiejun.chen@intel.com>]

> I think the _libxl_ message needs to be just "Unable to detect graphics
> passthru kind". i.e. it can't/shouldn't reference anything to do with xl
> config options etc (which would make no sense if libvirt was being
> used).
>
> That's not very user friendly though, so you may want to consider adding
> a new specific error code for this case and returning it here, such that
> xl or libvirt can then give a more comprehensible error message.

But,
     args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
                                           guest_config, d_state, NULL);
	|
	+ return libxl__build_device_model_args_new(gc, dm,guest_domid, 
guest_config, state, dm_state_fd);
		|
		+  Currently we always return "NULL" inside.

     if (!args) {
         ret = ERROR_FAIL;
         goto out;
     }

So I'm not very sure how to pass this new specific error code to xl/libvirt.

>
>> So looks the whole policy should be something like this,
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 5c40e84..5518759 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1953,8 +1953,27 @@ skip_vfb:
>>            xlu_cfg_replace_string (config, "spice_streaming_video",
>>                                    &b_info->u.hvm.spice.streaming_video, 0);
>>            xlu_cfg_get_defbool(config, "nographic",
>> &b_info->u.hvm.nographic, 0);
>> -        xlu_cfg_get_defbool(config, "gfx_passthru",
>> -                            &b_info->u.hvm.gfx_passthru, 0);
>> +        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
>> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
>> +        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
>> +            if (libxl_gfx_passthru_kind_from_string(buf,
>> +
>> &b_info->u.hvm.gfx_passthru_kind)) {
>> +                fprintf(stderr,
>> +                        "ERROR: invalid value \"%s\" for
>> \"gfx_passthru\"\n",
>> +                        buf);
>> +                exit (1);
>> +            }
>> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
>> +        }
>
> Up to here is fine.

Thanks but I guess I'd better to paste this whole patch to avoid I'm 
still missing something :)

---
  tools/libxl/libxl.h          |  6 ++++++
  tools/libxl/libxl_dm.c       | 25 ++++++++++++++++++++++---
  tools/libxl/libxl_internal.h |  5 +++++
  tools/libxl/libxl_types.idl  |  6 ++++++
  tools/libxl/xl_cmdimpl.c     | 14 ++++++++++++--
  5 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bbc52d..62b3ae5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
  #define LIBXL_HAVE_PSR_MBM 1
  #endif

+/*
+ * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
+ * the libxl_gfx_passthru_kind enumeration is defined.
+*/
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND
+
  typedef char **libxl_string_list;
  void libxl_string_list_dispose(libxl_string_list *sl);
  int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..045d48a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -710,9 +710,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 +754,28 @@ 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)) {
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
+                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
machinearg);
+                } else {
+                    LOG(ERROR, "Unable to detect graphics passthru kind,"
+                        " please set gfx_passthru_kind. See xl.cfg(5) 
for more"
+                        " information.\n");
+                    return NULL;
+                }
+                break;
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
+                break;
+            default:
+                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
+                break;
+            }
+        }
+
          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
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+#endif
  #endif

  /*
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 47af340..76642a8 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -140,6 +140,11 @@ libxl_tsc_mode = Enumeration("tsc_mode", [
      (3, "native_paravirt"),
      ])

+libxl_gfx_passthru_kind = Enumeration("gfx_passthru_kind", [
+    (0, "default"),
+    (1, "igd"),
+    ])
+
  # Consistent with the values defined for HVM_PARAM_TIMER_MODE.
  libxl_timer_mode = Enumeration("timer_mode", [
      (-1, "unknown"),
@@ -430,6 +435,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                         ("spice", 
libxl_spice_info),

                                         ("gfx_passthru", 
libxl_defbool),
+                                       ("gfx_passthru_kind", 
libxl_gfx_passthru_kind),

                                         ("serial",           string),
                                         ("boot",             string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c40e84..3ea3833 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1953,8 +1953,18 @@ skip_vfb:
          xlu_cfg_replace_string (config, "spice_streaming_video",
                                  &b_info->u.hvm.spice.streaming_video, 0);
          xlu_cfg_get_defbool(config, "nographic", 
&b_info->u.hvm.nographic, 0);
-        xlu_cfg_get_defbool(config, "gfx_passthru",
-                            &b_info->u.hvm.gfx_passthru, 0);
+        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
+        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
+            if (libxl_gfx_passthru_kind_from_string(buf,
+ 
&b_info->u.hvm.gfx_passthru_kind)) {
+                fprintf(stderr,
+                        "ERROR: invalid value \"%s\" for 
\"gfx_passthru\"\n",
+                        buf);
+                exit (1);
+            }
+            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+        }
          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))


Thanks
Tiejun