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

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

On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote:
> Although we already have 'gfx_passthru' in b_info, this doesn' suffice

                                                                ^t

> 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
                                                                   true

You had it right in the code.

>                                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>
> ---
>  docs/man/xl.cfg.pod.5        | 11 +++++++----
>  tools/libxl/libxl.h          |  6 ++++++
>  tools/libxl/libxl_dm.c       | 36 +++++++++++++++++++++++++++++++++---
>  tools/libxl/libxl_internal.h |  4 ++++
>  tools/libxl/libxl_types.idl  |  6 ++++++
>  tools/libxl/xl_cmdimpl.c     | 14 ++++++++++++--
>  6 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 408653f..c29bcbf 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
>  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
>  above.
>  
> -=item B<gfx_passthru=BOOLEAN>
> +=item B<gfx_passthru="STRING">

I think B<gfx_passthru=BOOLEAN|"STRING"> is more accurate.

>  Enable graphics device PCI passthrough. This option makes an assigned
>  PCI graphics card become primary graphics card in the VM. The QEMU
> @@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
>  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>  for currently supported graphics cards for gfx_passthru.
>  
> -gfx_passthru is currently only supported with the qemu-xen-traditional
> -device-model. Upstream qemu-xen device-model currently does not have
> -support for gfx_passthru.
> +gfx_passthru is currently supported both with the qemu-xen-traditional
> +device-model and upstream qemu-xen device-model. Note with the
> +qemu-xen-traditional device-model this option is just treated as BOOLEAN
> +actually, but with upstream qemu-xen device-model this option is extended
> +to pass a specific device name to force work. Currently just 'igd' is
> +defined to support Intel graphics device.

How about:

        When given as a boolean the B<gfx_passthru> option either
        disables gfx passthru or enables autodetection.
        
        When given as a string the B<gfx_passthru> option describes the
        type of passthru to enable.
        
        Valid options are:
        
        =over 4
        
        =item B<gfx_passthru=0>
        
        Disables gfx_passthru.
        
        =item B<gfx_passthru=1>, B<gfx_passthru="default">
        
        Enables gfx_passthru and autodetects the type of device which is
        being used.
        
        =item "igd"
        
        Enables gfx_passthru of the Intel Graphics Device.
        
        =back

        Note: When used with the qemu-xen-traditional device model only
        IGD passthru is supported.

(do check my pod syntax, I'm mostly making it up!)

The note at the end makes me thing that perhaps something ought to check
this constraint in the qemu-xen-traditional case. It might be easiest to
do it in libxl__build_device_model_args_old using the
libxl__detect_gfx_passthru_kind helper you have added

e.g. where libxl__build_device_model_args_old has:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
            flexarray_append(dm_args, "-gfx_passthru");
        }
would become something like:
        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
            if (libxl__detect_gfx_passthru_kind(gc, guest_config) !=
                LIBXL_GFX_PASSTHRU_KIND_IGD) {
                LOG("only IGD gfx_passthru is supported with qemu-xen-traditional");
                return NULL;
            }
            flexarray_append(dm_args, "-gfx_passthru");
        }

Or something like that. What do you think?

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c97c62d..26a01cb 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3632,6 +3632,10 @@ 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);
> +
> +bool libxl__is_igd_vga_passthru(libxl__gc *gc,
> +                                const libxl_domain_config *d_config);

This should be in the previous patch. In fact I think it is and this is
a redundant second instance.

Ian.