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: Thu, 19 Mar 2015 10:44:20 +0000 Message-ID: <1426761860.610.22.camel__12053.4128779129$1426761913$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> <1426674310.18247.318.camel@citrix.com> <550A2F6A.4060508@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <550A2F6A.4060508@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 Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote: > > 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. > > Looks good and thanks, but here 'guest_config' is a const so we > shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this, > > b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD; > > So I tried to refactor a little bit to follow up yours, > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 8599a6a..605b17c 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc, > return opt; > } > > +static int > +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; > + } > + > + LOG(ERROR, "Unable to detect graphics passthru kind"); > + return -1; I think you can make this function return enum libxl_gfx_passthrough_kind and then return LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case. > +} > + > static char ** libxl__build_device_model_args_new(libxl__gc *gc, > const char *dm, int guest_domid, > const libxl_domain_config > *guest_config, > @@ -427,7 +444,7 @@ static char ** > libxl__build_device_model_args_new(libxl__gc *gc, > const char *keymap = dm_keymap(guest_config); > char *machinearg; > flexarray_t *dm_args; > - int i, connection, devid; > + int i, connection, devid, gfx_passthru_kind; Please declare this in the smallest necessary scope... [...] > + if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { > + gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc, i.e. here. > + > guest_config); > + switch (gfx_passthru_kind) { > + case LIBXL_GFX_PASSTHRU_KIND_IGD: > + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); > + break; > + default: With the suggestion to return KIND_DEFAULT if detection fails then I think an extra case should be added: case LIBXL_GFX_PASSTHRU_KIND_DEFAULT: LOG(ERROR, "unable to detect required gfx_passthru_kind"); return NULL; > + LOG(ERROR, "unknown gfx_passthru_kind\n"); I think LOG is supposed to not include the final \n. Ian.