From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind Date: Thu, 12 Mar 2015 11:18:46 +0800 Message-ID: <55010596.2030106__43704.7065388926$1426130452$gmane$org@intel.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1426073641.21353.192.camel@citrix.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: Ian Campbell 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 >> + >> + 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 > interpreted as C (C<0>) or C (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