From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50922) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVetq-0006O6-Qa for qemu-devel@nongnu.org; Wed, 11 Mar 2015 07:34:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVetn-0001v3-JB for qemu-devel@nongnu.org; Wed, 11 Mar 2015 07:34:06 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:56995) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVetn-0001uN-Bs for qemu-devel@nongnu.org; Wed, 11 Mar 2015 07:34:03 -0400 Message-ID: <1426073641.21353.192.camel@citrix.com> From: Ian Campbell Date: Wed, 11 Mar 2015 11:34:01 +0000 In-Reply-To: <1425980538-5508-3-git-send-email-tiejun.chen@intel.com> References: <1425980538-5508-1-git-send-email-tiejun.chen@intel.com> <1425980538-5508-3-git-send-email-tiejun.chen@intel.com> Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Tiejun Chen Cc: Ian.Jackson@eu.citrix.com, wei.liu2@citrix.com, qemu-devel@nongnu.org, stefano.stabellini@citrix.com, xen-devel@lists.xen.org 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 > --- > 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 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. Ian.