Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell <ian.campbell@citrix.com>]

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 <tiejun.chen@intel.com>
> ---
>  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.