diff mbox

[v3,2/2] libxl: introduce gfx_passthru_kind

Message ID 551B4445.9070007@intel.com
State New
Headers show

Commit Message

Tiejun Chen April 1, 2015, 1:05 a.m. UTC
On 2015/3/30 17:19, Ian Campbell wrote:
> On Mon, 2015-03-30 at 09:28 +0800, Chen, Tiejun wrote:
>> Sounds it should be a legacy fix to qemu-xen-tranditional :) So lets do
>> it now,
>>
>> @@ -326,6 +326,10 @@ static char **
>> libxl__build_device_model_args_old(libxl__gc *gc,
>>            }
>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>                flexarray_append(dm_args, "-gfx_passthru");
>> +            if (b_info->u.hvm.gfx_passthru_kind >
>> +                                            LIBXL_GFX_PASSTHRU_KIND_IGD)
>> +                LOG(ERROR, "unsupported device type for
>> \"gfx_passthru\".\n");
>> +                return NULL;
>
> I'd rather not encode any ordering constraints if we don't have to. I
> think this is preferable:
>
>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>                 switch (b_info->u.hvm.gfx_passthru_kind) {
>                 case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>                 case LIBXL_GFX_PASSTHRU_KIND_IGD:
>                     flexarray_append(dm_args, "-gfx_passthru");
>                     break;
>                 default:
>                     LOG(ERROR, "unsupported gfx_passthru_kind.\n");
>                     return NULL;
>                 }
>           }
>
> (notice that the error message above doesn't refer to the xl specific
> option naming).
>

Sorry for this delay response.

This looks reasonable and I regenerate this patch based on this comment:

  libxl: introduce gfx_passthru_kind

Although we already have 'gfx_passthru' in b_info, this doesn't suffice
after we want to handle IGD specifically. Now we define a new field of
type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
this means we can benefit this to support other specific devices just
by extending gfx_passthru_kind. And then we can cooperate with
gfx_passthru to address IGD cases as follows:

     gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
     gfx_passthru = 1    => sets build_info.u.gfx_passthru to true and
                            build_info.u.gfx_passthru_kind to DEFAULT
     gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to true
                                and build_info.u.gfx_passthru_kind to IGD

Here if gfx_passthru_kind = DEFAULT, we will call
libxl__is_igd_vga_passthru() to check if we're hitting that table to need
to pass that option to qemu. But if gfx_passthru_kind = "igd" we always
force to pass that.

And now "gfx_passthru" is supported both with the qemu-xen-traditional
device-model and upstream qemu-xen device-model. But when given as a
string this option describes the type of device to enable. Note this
behavior is only supported with the upstream qemu-xen device-model.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
  docs/man/xl.cfg.pod.5       | 34 +++++++++++++++++++++++++++++----
  tools/libxl/libxl.h         |  6 ++++++
  tools/libxl/libxl_dm.c      | 46 
+++++++++++++++++++++++++++++++++++++++++----
  tools/libxl/libxl_types.idl |  6 ++++++
  tools/libxl/xl_cmdimpl.c    | 14 ++++++++++++--
  5 files changed, 96 insertions(+), 10 deletions(-)

          switch (xlu_cfg_get_list_as_string_list(config, "serial",
 
&b_info->u.hvm.serial_list,
                                                  1))

Thanks
Tiejun

Comments

Ian Campbell April 1, 2015, 8:45 a.m. UTC | #1
On Wed, 2015-04-01 at 09:05 +0800, Chen, Tiejun wrote:
> @@ -699,9 +699,35 @@ working graphics passthrough. See the 
> XenVGAPassthroughTestedAdapters
>   L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>   for currently supported graphics cards for gfx_passthru.
> 
> -gfx_passthru is currently only supported with the qemu-xen-traditional
> -device-model. Upstream qemu-xen device-model currently does not have
> -support for gfx_passthru.
> +gfx_passthru is currently supported both with the qemu-xen-traditional
> +device-model and upstream qemu-xen device-model.
> +
> +When given as a boolean the B<gfx_passthru> option either disables gfx
> +passthru or enables autodetection.
> +
> +But when given as a string the B<gfx_passthru> option describes the type
> +of device to enable. Note this behavior is only supported with the upstream
> +qemu-xen device-model.

Perhaps add "With qemu-xen-traditional IGD is always assumed and other
options than autodetect or explicit IGD will result in an error"?

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index a8b08f2..4fd6310 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -325,7 +325,15 @@ static char ** 
> libxl__build_device_model_args_old(libxl__gc *gc,
>               flexarray_vappend(dm_args, "-net", "none", NULL);
>           }
>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> -            flexarray_append(dm_args, "-gfx_passthru");
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                flexarray_append(dm_args, "-gfx_passthru");
> +                break;
> +            default:
> +                LOG(ERROR, "unsupported gfx_passthru_kind.\n");

Sorry, LOG should not get a \n like my example had, my fault.

With that if you resend the series with git send-email (so it doesn't
get whitespace mangled) I think we are good to go!

Ian.
Tiejun Chen April 1, 2015, 9:18 a.m. UTC | #2
> Perhaps add "With qemu-xen-traditional IGD is always assumed and other
> options than autodetect or explicit IGD will result in an error"?

Will do.

>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index a8b08f2..4fd6310 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -325,7 +325,15 @@ static char **
>> libxl__build_device_model_args_old(libxl__gc *gc,
>>                flexarray_vappend(dm_args, "-net", "none", NULL);
>>            }
>>            if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> -            flexarray_append(dm_args, "-gfx_passthru");
>> +            switch (b_info->u.hvm.gfx_passthru_kind) {
>> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
>> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
>> +                flexarray_append(dm_args, "-gfx_passthru");
>> +                break;
>> +            default:
>> +                LOG(ERROR, "unsupported gfx_passthru_kind.\n");
>
> Sorry, LOG should not get a \n like my example had, my fault.

Actually myself really should double check this.

>
> With that if you resend the series with git send-email (so it doesn't
> get whitespace mangled) I think we are good to go!
>

Currently Qemu maintainers are busy finalizing qemu 2.3, they don't 
complete to review all associated qemu patch set. Although that don't 
bring any change to our two patches on Xen side, I think we'd better 
merge these patches until qemu patches are really applied into qemu 
tree. So I will send this series again until we can really consume this 
with qemu upstream, right?

BTW, I really appreciate your all comments in this thread.

Thanks
Tiejun
Ian Campbell April 1, 2015, 9:53 a.m. UTC | #3
On Wed, 2015-04-01 at 17:18 +0800, Chen, Tiejun wrote:
> Currently Qemu maintainers are busy finalizing qemu 2.3, they don't 
> complete to review all associated qemu patch set. Although that don't 
> bring any change to our two patches on Xen side, I think we'd better 
> merge these patches until qemu patches are really applied into qemu 
> tree. So I will send this series again until we can really consume this 
> with qemu upstream, right?

IOW I should put this to one side until you tell me the qemu side is in
place (by resending the series)? Fine by me, thanks.

> BTW, I really appreciate your all comments in this thread.

No problem, thanks for sticking with my nit picking.

Ian.
diff mbox

Patch

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 408653f..dfde92d 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -671,7 +671,7 @@  through to this VM. See L<seize|/"seize_boolean"> above.
  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
  above.

-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru=BOOLEAN|"STRING">

  Enable graphics device PCI passthrough. This option makes an assigned
  PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@  working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters
  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
  for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B<gfx_passthru> option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B<gfx_passthru> option describes the type
+of device to enable. Note this behavior is only supported with the upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B<gfx_passthru=0>
+
+Disables graphics device PCI passthrough.
+
+=item B<gfx_passthru=1>, B<gfx_passthru="default">
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but forcing the type
+of device to Intel Graphics Device.
+
+=back

  Note that some graphics adapters (AMD/ATI cards, for example) do not
  necessarily require gfx_passthru option, so you can use the normal Xen
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5eec092..1144c5e 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 a8b08f2..4fd6310 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -325,7 +325,15 @@  static char ** 
libxl__build_device_model_args_old(libxl__gc *gc,
              flexarray_vappend(dm_args, "-net", "none", NULL);
          }
          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
-            flexarray_append(dm_args, "-gfx_passthru");
+            switch (b_info->u.hvm.gfx_passthru_kind) {
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+            case LIBXL_GFX_PASSTHRU_KIND_IGD:
+                flexarray_append(dm_args, "-gfx_passthru");
+                break;
+            default:
+                LOG(ERROR, "unsupported gfx_passthru_kind.\n");
+                return NULL;
+            }
          }
      } else {
          if (!sdl && !vnc)
@@ -416,6 +424,22 @@  static char *dm_spice_options(libxl__gc *gc,
      return opt;
  }

+static enum libxl_gfx_passthru_kind
+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;
+    }
+
+    return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
+}
+
  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                          const char *dm, int guest_domid,
                                          const libxl_domain_config 
*guest_config,
@@ -727,9 +751,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");
@@ -774,6 +795,23 @@  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)) {
+            enum libxl_gfx_passthru_kind 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;
+            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
+                LOG(ERROR, "unable to detect required gfx_passthru_kind");
+                return NULL;
+            default:
+                LOG(ERROR, "invalid value for gfx_passthru_kind");
+                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_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 04faf98..b78b4ec 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1956,8 +1956,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);
+        }