Message ID | 55092A24.8010409@intel.com |
---|---|
State | New |
Headers | show |
On Wed, 2015-03-18 at 15:32 +0800, Chen, Tiejun wrote: > > 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. Bah, yes. I don't think it is reasonable to ask you to rework the error handling here completely for this. Lets leave this as a potential future enhancement. > 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 _info->u.hvm.gfx_passthru_kind: > + 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"); Please change this to "Unable to detect graphics passthru kind" to avoid talking xl-isms in libxl. > + return NULL; > + } > + break; > + case LIBXL_GFX_PASSTHRU_KIND_IGD: > + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); > + break; 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. > 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. Ian.
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); + }