From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUrRh-0004wp-Pk for qemu-devel@nongnu.org; Mon, 09 Mar 2015 02:45:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YUrRc-00053G-LN for qemu-devel@nongnu.org; Mon, 09 Mar 2015 02:45:45 -0400 Received: from mga09.intel.com ([134.134.136.24]:28003) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YUrRc-0004xS-AO for qemu-devel@nongnu.org; Mon, 09 Mar 2015 02:45:40 -0400 Message-ID: <54FD4190.80606@intel.com> Date: Mon, 09 Mar 2015 14:45:36 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1425632903-5502-1-git-send-email-tiejun.chen@intel.com> <1425632903-5502-3-git-send-email-tiejun.chen@intel.com> <20150306125545.GP12103@zion.uk.xensource.com> In-Reply-To: <20150306125545.GP12103@zion.uk.xensource.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] libxl: introduce gfx_passthru_kind List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wei Liu Cc: qemu-devel@nongnu.org, stefano.stabellini@citrix.com, Ian.Jackson@eu.citrix.com, ian.campbell@citrix.com, xen-devel@lists.xen.org On 2015/3/6 20:55, Wei Liu wrote: > On Fri, Mar 06, 2015 at 05:08:23PM +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 false and > ^^^^^ > true? I just simply pick up this from Campbell's comment but I agree you. > >> 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. >> >> Signed-off-by: Tiejun Chen >> --- >> tools/libxl/libxl_dm.c | 13 +++++++++++++ >> tools/libxl/libxl_types.idl | 6 ++++++ >> tools/libxl/xl_cmdimpl.c | 19 +++++++++++++++++-- >> 3 files changed, 36 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c >> index 8599a6a..780dd2a 100644 >> --- a/tools/libxl/libxl_dm.c >> +++ b/tools/libxl/libxl_dm.c >> @@ -757,6 +757,19 @@ 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); >> + } > > You can remove that {} around machinearg -- it's only one line after > `if'. Okay. > >> + } 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"); >> + } >> + >> 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_types.idl b/tools/libxl/libxl_types.idl >> index 02be466..e209460 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"), >> + ], init_val = "LIBXL_GFX_PASSTHRU_KIND_DEFAULT") >> + > > You don't need to set init_val if the default value is 0. Got it. > >> # 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), > ^^^^ > One space is enough, I think. Okay. > >> >> ("serial", string), >> ("boot", string), >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> index 440db78..3f7fede 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -1953,8 +1953,23 @@ 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); > > Please wrap this line to be < 80 column. > >> + exit (1); >> + } >> + } 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)) { > > Ditto. > >> + fprintf(stderr, "ERROR: invalid value \"%s\" for \"gfx_passthru\"\n", > > Ditto. So, @@ -1959,13 +1959,15 @@ skip_vfb: } 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); + fprintf(stderr, "ERROR: invalid value %ld for" + " \"gfx_passthru\"\n", l); exit (1); } } 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); + 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); Thanks Tiejun