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

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

On Thu, 2015-03-12 at 11:18 +0800, Chen, Tiejun wrote:
> >> +
> >> +        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,

In libxl an abort is only warranted if something _cannot_ happen and is
not under user control. I think think that applies here unless some
earlier code is guaranteed to have validated the value before it reaches
this point.

> 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();

I don't think you can abort here, since a user can set
b_info->u.hvm.gfx_passthru_kind to default. You would need to return an
error.

> @@ -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.

Almost, please also explicitly name the type field as other similar
comments do for clarity.

> + */
> +#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

No ifdef needed here (or in libxl_internal.h)


> +bool libxl__is_igd_vga_passthru(libxl__gc *gc,
> +                                const libxl_domain_config *d_config);

and this should be in libxl_internal.h not here...

> But looks libxl__gc{} is defined in the libxl_internal.h file... I guess 
> we need a conversion, or other idea?

... which answers this question.

> 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);
> +            }

This is exactly the same as:
        libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);

Ian.