Re: [PATCH v2] Add device_model_pvdevice parameter for HVM guests

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

On Thu, 2013-07-18 at 14:47 +0100, Paul Durrant wrote:
> The parameter determines which, if any, xen-pvdevice is specified on the
> QEMU command line. The default value is 'none' which means no argument will
> be passed. A value of 'xenserver' specifies a xen-pvdevice with device-id
> 0xc000 (the initial value in the xenserver namespace - see
> docs/misc/pci-device-reservations.txt).
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@citrix.com>
> ---
> v2:
>  - Added LIBXL_HAVE_PVDEVICE to libxl.h
>  - Added more text to xl.cfg manpage to call out the device-id of
>    the xenserver pvdevice and point readers at
>    docs/misc/pci-device-reservations.txt
> 
>  docs/man/xl.cfg.pod.5       |   23 +++++++++++++++++++++++
>  tools/libxl/libxl.h         |    8 ++++++++
>  tools/libxl/libxl_dm.c      |    9 +++++++++
>  tools/libxl/libxl_types.idl |    5 +++++
>  tools/libxl/xl_cmdimpl.c    |   14 ++++++++++++++
>  5 files changed, 59 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 069b73f..bf6760f 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1227,6 +1227,29 @@ you have selected.
>  
>  Assign an XSM security label to the device-model stubdomain.
>  
> +=item B<device_model_pvdevice="PVDEVICE">
> +
> +Selects which variant of the xen-pvdevice should be used for this
> +guest. Valid values are:
> +
> +=over 4
> +
> +=item B<none>
> +
> +The xen-pvdevice should be omitted. This is the default.
> +
> +=item B<xenserver>
> +
> +The xenserver variant of the xen-pvdevice (device-id=C000) will be
> +specified, enabling the use of XenServer PV drivers in the guest.
> +
> +=back
> +
> +This parameter only takes effect when device_model_version=qemu-xen.
> +See F<docs/misc/pci-device-reservations.txt> for more information.
> +
> +=back
> +
>  =item B<device_model_args=[ "ARG", "ARG", ...]>
>  
>  Pass additional arbitrary options on the device-model command
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 37e4d82..4170861 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -82,6 +82,14 @@
>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>  
>  /*
> + * LIBXL_HAVE_PVDEVICE indicates that the libxl_device_model_pvdevice
> + * field is present in the hvm sections of libxl_domain_build_info.
> + * This field tells libxl which flavour of xen-pvdevice to enable in
> + * QEMU.
> + */
> +#define LIBXL_HAVE_PVDEVICE 1

Can we expand this name a bit, to make it clear what PVDEVICE refers to?
LIBXL_HAVE_PLATFORM_PVDEVICE_TYPES? or _VARIANTS? Bit clumsy?

While at it I'd be tempted to
s/device_model_pvdevice/platform_device_type/ or something in the API
(both the Enum and the field). The related overall boolean control is
xen_platform_pci, so perhaps something derived from that?

Sorry for spotting this after so long.

> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 7e54c02..2a2665b 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -647,6 +647,15 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-drive");
>              flexarray_append(dm_args, drive);
>          }
> +
> +        switch (b_info->u.hvm.device_model_pvdevice) {
> +        case LIBXL_DEVICE_MODEL_PVDEVICE_XENSERVER:
> +            flexarray_append(dm_args, "-device");
> +            flexarray_append(dm_args, "xen-pvdevice,device-id=0xc000");
> +            break;
> +        default:
> +            break;
> +        }
>      }
>      flexarray_append(dm_args, NULL);
>      return (char **) flexarray_contents(dm_args);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d218a2d..7165139 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -132,6 +132,10 @@ libxl_vga_interface_type = Enumeration("vga_interface_type", [
>      (2, "STD"),
>      ], init_val = 0)
>  
> +libxl_device_model_pvdevice = Enumeration("device_model_pvdevice", [
> +    (0, "NONE"),
> +    (1, "XENSERVER"),
> +    ])
>  #
>  # Complex libxl types
>  #
> @@ -332,6 +336,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("soundhw",          string),
>                                         ("xen_platform_pci", libxl_defbool),
>                                         ("usbdevice_list",   libxl_string_list),
> +                                       ("device_model_pvdevice", libxl_device_model_pvdevice),
>                                         ])),
>                   ("pv", Struct(None, [("kernel", string),
>                                        ("slack_memkb", MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8a478ba..cfaa54e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1526,6 +1526,20 @@ skip_vfb:
>              exit (1);
>  
>          }
> +
> +        if (!xlu_cfg_get_string (config, "device_model_pvdevice", &buf, 0)) {
> +            libxl_device_model_pvdevice d;
> +
> +            e = libxl_device_model_pvdevice_from_string(buf, &d);
> +            if (e) {
> +                fprintf(stderr,
> +                        "xl: unknown device_model_pvdevice '%s'\n",
> +                        buf);
> +                exit(-ERROR_FAIL);
> +            }
> +                
> +            b_info->u.hvm.device_model_pvdevice = d;
> +        }
>      }
>  
>      xlu_cfg_destroy(config);