| Message ID | 1425980538-5508-3-git-send-email-tiejun.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2015-03-10 at 17:42 +0800, Tiejun Chen wrote: > Although we already have 'gfx_passthru' in b_info, this doesn' 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 false > 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 "-gfx_passthru" is just introduced to work for qemu-xen-traditional > so we should get this away from libxl__build_device_model_args_new() in > the case of qemu upstream. > > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> > --- > tools/libxl/libxl_dm.c | 15 ++++++++++++--- > tools/libxl/libxl_pci.c | 4 ++-- > tools/libxl/libxl_types.idl | 6 ++++++ > tools/libxl/xl_cmdimpl.c | 22 ++++++++++++++++++++-- > 4 files changed, 40 insertions(+), 7 deletions(-) > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 8599a6a..2d06038 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,18 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > machinearg, max_ram_below_4g); > } > } > + > + 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? 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. > + > 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). > } > > /* > 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. > ("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. Ian.
>> + >> + 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
On Thu, 2015-03-12 at 11:18 +0800, Chen, Tiejun wrote: > >> + > >> + 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, In libxl an abort is only warranted if something _cannot_ happen and is not under user control. I think think that applies here unless some earlier code is guaranteed to have validated the value before it reaches this point. > 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(); I don't think you can abort here, since a user can set b_info->u.hvm.gfx_passthru_kind to default. You would need to return an error. > @@ -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. Almost, please also explicitly name the type field as other similar comments do for clarity. > + */ > +#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 No ifdef needed here (or in libxl_internal.h) > +bool libxl__is_igd_vga_passthru(libxl__gc *gc, > + const libxl_domain_config *d_config); and this should be in libxl_internal.h not here... > But looks libxl__gc{} is defined in the libxl_internal.h file... I guess > we need a conversion, or other idea? ... which answers this question. > 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); > + } This is exactly the same as: libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); Ian.
> I don't think you can abort here, since a user can set > b_info->u.hvm.gfx_passthru_kind to default. You would need to return an > error. Then, looks I should do this, LOG(ERROR, "No supported IGD to passthru," " or please force set gfx_passthru=\"igd\".\n"); return NULL; > >> @@ -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. > > Almost, please also explicitly name the type field as other similar > comments do for clarity. Okay, maybe something is like this, +/* + * LIBXL_HAVE_IGD_GFX_PASSTHRU + * + * If this is defined, the IGD Graphic Device Passthrough is supported. + * + * LIBXL_HAVE_IGD_GFX_PASSTHRU indicates that the + * libxl_device_pci field in the hvm is present in the pci_info structure + * fixup_ids[] which contains all supported IGD devices. So wwe use + * "igd-passthru=on" specify on the qemu command-line. + */ +#define LIBXL_HAVE_IGD_GFX_PASSTHRU 1 + >> + */ [snip] > and this should be in libxl_internal.h not here... Okay. I mistakenly understand we always have to expose this in libxl.h... > >> But looks libxl__gc{} is defined in the libxl_internal.h file... I guess [snip] >> + 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); >> + } > > This is exactly the same as: > libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l); > Sure. Thanks Tiejun
On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote: > > I don't think you can abort here, since a user can set > > b_info->u.hvm.gfx_passthru_kind to default. You would need to return an > > error. > > Then, looks I should do this, > > LOG(ERROR, "No supported IGD to passthru," > " or please force set gfx_passthru=\"igd\".\n"); > return NULL; If I remember the context correctly this is in the autodetect case, so I think shouldn't mention IGD. Something like "Unable to detect graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5) for more information." > > > >> @@ -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. > > > > Almost, please also explicitly name the type field as other similar > > comments do for clarity. > > Okay, maybe something is like this, > > +/* > + * LIBXL_HAVE_IGD_GFX_PASSTHRU > + * > + * If this is defined, the IGD Graphic Device Passthrough is supported. > + * > + * LIBXL_HAVE_IGD_GFX_PASSTHRU indicates that the > + * libxl_device_pci field in the hvm is present in the pci_info structure > + * fixup_ids[] which contains all supported IGD devices. So wwe use > + * "igd-passthru=on" specify on the qemu command-line. This: /* * 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 Users don't care about the internal details, just about the existence of the support. Ian.
On 2015/3/13 18:11, Ian Campbell wrote: > On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote: >>> I don't think you can abort here, since a user can set >>> b_info->u.hvm.gfx_passthru_kind to default. You would need to >>> return an error. >> >> Then, looks I should do this, >> >> LOG(ERROR, "No supported IGD to passthru," " or please force set >> gfx_passthru=\"igd\".\n"); return NULL; > > If I remember the context correctly this is in the autodetect case, > so I think shouldn't mention IGD. Something like "Unable to detect > graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5) > for more s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get 'gfx_passthru_kind' from 'gfx_passthru'. > information." > >>> >>>> @@ -720,6 +720,13 @@ void libxl_mac_copy(libxl_ctx *ctx, >>>> libxl_mac *dst, libxl_mac *src); [snip] > /* * 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 > > Users don't care about the internal details, just about the existence > of the support. > Updated. Thanks Tiejun
On Mon, 2015-03-16 at 09:07 +0800, Chen, Tiejun wrote: > On 2015/3/13 18:11, Ian Campbell wrote: > > On Fri, 2015-03-13 at 09:39 +0800, Chen, Tiejun wrote: > >>> I don't think you can abort here, since a user can set > >>> b_info->u.hvm.gfx_passthru_kind to default. You would need to > >>> return an error. > >> > >> Then, looks I should do this, > >> > >> LOG(ERROR, "No supported IGD to passthru," " or please force set > >> gfx_passthru=\"igd\".\n"); return NULL; > > > > If I remember the context correctly this is in the autodetect case, > > so I think shouldn't mention IGD. Something like "Unable to detect > > graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5) > > for more > > s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get > 'gfx_passthru_kind' from 'gfx_passthru'. I think you have it backwards. In the case here gfx_passthru=1 has been set by the user, but gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has failed. So if the user wants to make progress they should set gfx_passthru_kind to whatever type of passthrough they were trying to do. Alternatively I suppose you could recommend removing gfx_passthru=1 (or changing to=0), but given they've set =1 that doesn't seem to be the most productive suggestion. Ian.
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 8599a6a..2d06038 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,18 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, machinearg, max_ram_below_4g); } } + + 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"); + } + 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; } /* 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), ("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); + } + } 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); + } switch (xlu_cfg_get_list_as_string_list(config, "serial", &b_info->u.hvm.serial_list, 1))
Although we already have 'gfx_passthru' in b_info, this doesn' 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 false 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 "-gfx_passthru" is just introduced to work for qemu-xen-traditional so we should get this away from libxl__build_device_model_args_new() in the case of qemu upstream. Signed-off-by: Tiejun Chen <tiejun.chen@intel.com> --- tools/libxl/libxl_dm.c | 15 ++++++++++++--- tools/libxl/libxl_pci.c | 4 ++-- tools/libxl/libxl_types.idl | 6 ++++++ tools/libxl/xl_cmdimpl.c | 22 ++++++++++++++++++++-- 4 files changed, 40 insertions(+), 7 deletions(-)