[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind
From: |
Ian Campbell |
Subject: |
Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind |
Date: |
Wed, 11 Mar 2015 11:34:01 +0000 |
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 <address@hidden>
> ---
> 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<NUMBER>
interpreted as C<False> (C<0>) or C<True> (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.
- [Qemu-devel] [v2][PATCH 0/2] libxl: try to support IGD passthrough for qemu upstream, Tiejun Chen, 2015/03/10
- [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Tiejun Chen, 2015/03/10
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind,
Ian Campbell <=
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/11
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/12
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/12
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/13
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/15
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/16
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/17
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/17
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/18
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/18