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

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

On Wed, 2015-03-18 at 15:32 +0800, Chen, Tiejun wrote:
> > I think the _libxl_ message needs to be just "Unable to detect graphics
> > passthru kind". i.e. it can't/shouldn't reference anything to do with xl
> > config options etc (which would make no sense if libvirt was being
> > used).
> >
> > That's not very user friendly though, so you may want to consider adding
> > a new specific error code for this case and returning it here, such that
> > xl or libvirt can then give a more comprehensible error message.
> 
> But,
>      args = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
>                                            guest_config, d_state, NULL);
> 	|
> 	+ return libxl__build_device_model_args_new(gc, dm,guest_domid, 
> guest_config, state, dm_state_fd);
> 		|
> 		+  Currently we always return "NULL" inside.
> 
>      if (!args) {
>          ret = ERROR_FAIL;
>          goto out;
>      }
> 
> So I'm not very sure how to pass this new specific error code to xl/libvirt.

Bah, yes. I don't think it is reasonable to ask you to rework the error
handling here completely for this. Lets leave this as a potential future
enhancement.

> Thanks but I guess I'd better to paste this whole patch to avoid I'm 
> still missing something :)
> 
> ---
>   tools/libxl/libxl.h          |  6 ++++++
>   tools/libxl/libxl_dm.c       | 25 ++++++++++++++++++++++---
>   tools/libxl/libxl_internal.h |  5 +++++
>   tools/libxl/libxl_types.idl  |  6 ++++++
>   tools/libxl/xl_cmdimpl.c     | 14 ++++++++++++--
>   5 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6bbc52d..62b3ae5 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -720,6 +720,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>   #define LIBXL_HAVE_PSR_MBM 1
>   #endif
> 
> +/*
> + * libxl_domain_build_info has the u.hvm.gfx_passthru_kind field and
> + * the libxl_gfx_passthru_kind enumeration is defined.
> +*/
> +#define LIBXL_HAVE_GFX_PASSTHRU_KIND
> +
>   typedef char **libxl_string_list;
>   void libxl_string_list_dispose(libxl_string_list *sl);
>   int libxl_string_list_length(const libxl_string_list *sl);
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..045d48a 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,28 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                               machinearg, max_ram_below_4g);
>               }
>           }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            switch (b_info->u.hvm.gfx_passthru_kind) {
> +            case _info->u.hvm.gfx_passthru_kind:
> +                if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> +                    machinearg = GCSPRINTF("%s,igd-passthru=on", 
> machinearg);
> +                } else {
> +                    LOG(ERROR, "Unable to detect graphics passthru kind,"
> +                        " please set gfx_passthru_kind. See xl.cfg(5) 
> for more"
> +                        " information.\n");

Please change this to "Unable to detect graphics passthru kind" to avoid
talking xl-isms in libxl.

> +                    return NULL;
> +                }
> +                break;
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;

This duplicates the code from above. I think this would be best done as:

static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
{
    if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
        return 0;

    if (libxl__is_igd_vga_passthru(gc, guest_config)) {
        b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
        return 0;
    }

    LOG(ERROR, "Unable to detect graphics passthru kind");
    return 1;
}

Then for the code in libxl__build_device_model_args_new:

         if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
             if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
                  return NULL
             switch (b_info->u.hvm.gfx_passthru_kind) {
             case LIBXL_GFX_PASSTHRU_KIND_IGD:
                 machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
                 break;
             default:
                 LOG(ERROR, "unknown gfx_passthru_kind\n");
                return NULL;
             }
        }

That is, a helper to try and autodetect kind if it is default and then a
single switch entry for each kind.

> +            default:
> +                LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");

Please return an error here, as I've shown above.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c97c62d..8912421 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3632,6 +3632,11 @@ static inline void 
> libxl__update_config_vtpm(libxl__gc *gc,
>    */
>   void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>                                       const libxl_bitmap *sptr);
> +
> +#ifdef LIBXL_HAVE_GFX_PASSTHRU_KIND

No need for this ifdef.

Ian.