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

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

> > +
> > +        if (b_info->u.hvm.gfx_passthru_kind ==
> > +                LIBXL_GFX_PASSTHRU_KIND_DEFAULT) {
> > +            if (libxl__is_igd_vga_passthru(gc, guest_config))
> > +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> > +        } else if (b_info->u.hvm.gfx_passthru_kind ==
> > +                        LIBXL_GFX_PASSTHRU_KIND_IGD) {
> > +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> > +        } else {
> > +            LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
> > +        }
> I think this whole block should be inside an:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)
> the semantics should be that kind is ignored unless passthru is enabled.
>
> and then the if/else chain could become a switch perhaps?
Okay.

> I'm unsure what we should do if kind==DEFAULT but
> libxl__is_igd_vga_passthru fails. At a minimum a warning seems
> desirable. I'm not sure if it should warrant a failure or not.
I think a warning message plus abort() should be enough, and then user 
can determine what's next,
Please take a look at this,

+
+        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(WARN, "No supported IGD to passthru,"
+                        " or please force set gfx_passthru=\"igd\".\n");
+                    abort();
+                }
+                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_pci.c b/tools/libxl/libxl_pci.c
> > index fc060c6..9a534cc 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -608,11 +608,11 @@ bool libxl__is_igd_vga_passthru(libxl__gc *gc,
> >               device = fixup_ids[j].device;
> >
> >               if (pt_vendor == vendor &&  pt_device == device)
> > -                return 1;
> > +                return true;
> >           }
> >       }
> >
> > -    return 0;
> > +    return false;
> Please fold this into the previous patch. (You may retain the ack I
> gave).
Thanks for your catch.

> >   }
> >
> >   /*
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 02be466..d64ad10 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),
> Please add a #define LIBXL_HAVE_... to libxl.h to indicate that this
> functionality is now available. There is a comment near the top
> explaining why etc and a bunch of examples.
>
> Note that the #define is for 3rd party code only, there is no need to
> actually use it in either libxl or xl code.
Like this,

@@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
libxl_mac *src);
 #define LIBXL_HAVE_PSR_MBM 1
 #endif

+/*
+ * LIBXL_HAVE_GFX_PASSTHRU_KIND
+ *
+ * If this is defined, the Graphic Device Passthrough Override is 
supported.
+ */
+#define LIBXL_HAVE_GFX_PASSTHRU_KIND 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -1501,6 +1508,11 @@ int libxl_psr_cmt_get_sample(libxl_ctx *ctx,
                              uint64_t *tsc_r);
 #endif

+#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND
+bool libxl__is_igd_vga_passthru(libxl__gc *gc,
+                                const libxl_domain_config *d_config);
+#endif
+
 /* misc */

 /* Each of these sets or clears the flag according to whether the

But looks libxl__gc{} is defined in the libxl_internal.h file... I guess 
we need a conversion, or other idea?
> >                                          ("serial",           string),
> >                                          ("boot",             string),
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 440db78..d0d6ce3 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1953,8 +1953,26 @@ 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)) {
> > +            if (!l) {
> > +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> > +            } else if (i == 1) {
> > +                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
> > +            } else {
> > +                fprintf(stderr,
> > +                        "ERROR: invalid value %ld for \"gfx_passthru\"\n", l);
> > +                exit (1);
> > +            }
> The docs (xl.cfg man page) say, regarding booleans "A C<NUMBER>
> interpreted as C<False> (C<0>) or C<True> (any other value)."
>
> So just libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); should be
> fine here.
>
> > +        } 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, false);
> Should be true, I see you wrote false in the commit message so I assume
> this was my mistake from earlier as well, sorry about that.
Here I update this chunk of code based on your two comments:

@@ -1953,8 +1953,22 @@ 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)) {
+            if (l) {
+                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
+            } else {
+                libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
+            }
+        } 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

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