From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind Date: Wed, 18 Mar 2015 10:25:10 +0000 Message-ID: <1426674310.18247.318.camel__37788.5362403051$1426674439$gmane$org@citrix.com> References: <1425980538-5508-1-git-send-email-tiejun.chen@intel.com> <1425980538-5508-3-git-send-email-tiejun.chen@intel.com> <1426073641.21353.192.camel@citrix.com> <55010596.2030106@intel.com> <1426163186.21353.409.camel@citrix.com> <55023FDA.7030204@intel.com> <1426241479.32572.94.camel@citrix.com> <55062CD2.8050305@intel.com> <1426508400.18247.33.camel@citrix.com> <5507DBD6.8030401@intel.com> <1426584396.18247.136.camel@citrix.com> <55092A24.8010409@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55092A24.8010409@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Chen, Tiejun" Cc: Ian.Jackson@eu.citrix.com, wei.liu2@citrix.com, qemu-devel@nongnu.org, stefano.stabellini@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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.