Re: [PATCH v2 14/15] arm: Allow the user to specify the GIC version

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

On Fri, 2015-06-26 at 10:34 +0100, Julien Grall wrote:
> A platform may have a GIC compatible with previous version of the
> device.
> 
> This is allow to virtualize an unmodified OS on new hardware if the GIC
> is compatible with older version.
> 
> When a guest is created, the vGIC will emulate same version as the
> hardware. Although, the user can specify in the configuration file the
> preferred version (currently on GICv2 and GICv3 are supported).
                                ^ly?

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
> ---
>     The hypervisor will check if the GIC is able to virtualize the
>     version specified by the user (via the DOMCTL createdomain).
>     If it's not compatible an error will be send on the Xen console
>     which will make the error not obvious for user.
> 
>     I left aside a user error reporting for a follow-up as I'm not
>     sure how to notify the user which GIC versions are available. May be
>     by a new mechanism similar to xen_get_caps?
> 
>     Changes in v2:
>         - Introduce arch_arm in libxl_domain_build_info to store ARM
>         specific field
>         - Add docs
>         - Remove code that is not necessary with the new version
> ---
>  docs/man/xl.cfg.pod.5        | 27 ++++++++++++++++++++++++++
>  tools/libxl/libxl.h          |  5 +++++
>  tools/libxl/libxl_arm.c      | 16 +++++++++++++++-
>  tools/libxl/libxl_types.idl  | 11 +++++++++++
>  tools/libxl/xl_cmdimpl.c     | 12 ++++++++++++
>  xen/arch/arm/domain.c        | 45 ++++++++++++++++++++++++++------------------
>  xen/arch/arm/vgic.c          |  2 +-
>  xen/include/asm-arm/domain.h |  2 ++
>  8 files changed, 100 insertions(+), 20 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a3e0e2e..663eb2d 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1688,6 +1688,33 @@ The default is B<en-us>.
>  
>  See L<qemu(1)> for more information.
>  
> +=head2 Architecture Specific options
> +
> +=head3 ARM
> +
> +=over 4
> +
> +=item B<gic_version="vN">
> +
> +Version of the GIC emulated for the guest. Currently, the following versions
> +are supported:
> +
> +=over 4
> +
> +=item B<v2>
> +
> +Emulate a GICv2 hardware
> +
> +=item B<v3>
> +
> +Emulate a GICv3 hardware. Note that the GICv2 compatibility is not supported.

"Note that the emulated GIC does not support the GICv2 compatibility
mode". (To avoid confusion with such compatibility used to provide the
other option to the guest)

> +
> +=back
> +
> +Although, all the versions may not be supported on the host.

"This requires hardware compatibility with the requested version. Either
natively or via hardware backwards compatibility support".


> +
> +=back
> +
>  =head1 SEE ALSO
>  
>  =over 4
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 0a7913b..68bc954 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -200,6 +200,11 @@
>  #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>  
>  /*
> + * libxl_domain_build_info has the gic_version field.
> + */
> +#define LIBXL_HAVE_BUILDINFO_GIC_VERSION 1

Mention ARM in the comment and/or name?

> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index f09c860..5b637e9 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -61,7 +61,21 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      xc_config->nr_spis = nr_spis;
>      LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
>  
> -    xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
> +    switch (d_config->b_info.arch_arm.gic_version) {
> +    case LIBXL_GIC_VERSION_DEFAULT:
> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
> +        break;
> +    case LIBXL_GIC_VERSION_2:
> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
> +        break;
> +    case LIBXL_GIC_VERSION_3:
> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
> +        break;
> +    default:
> +        LOG(ERROR, "Unkwnon GIC version %s\n",

"Unknown"
>  
> +libxl_gic_version = Enumeration("gic_version", [
> +    (0, "DEFAULT"),
> +    (1, "2"),
> +    (2, "3")

If you call these "V2" and "V3" then you can use
libxl_gic_version_from_string to make the parsing much easier. If you
subsequently add e.g. "v2m" then the parsing code in xl will also just
work,

Better to line up the values with the gic version if possible too. Enums
don't need to be contiguous. Or maybe use 0x20, 0x30 etc for the value,
leaving space for gicv2m in 0x21 etc.
 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c858068..dcf1963 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2242,6 +2242,18 @@ skip_vfb:
>          }
>      }
>  
> +    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
> +        if (!strcmp(buf, "v2"))
> +            b_info->arch_arm.gic_version = LIBXL_GIC_VERSION_2;
> +        else if (!strcmp(buf, "v3"))
> +            b_info->arch_arm.gic_version = LIBXL_GIC_VERSION_3;
> +        else {
> +            fprintf(stderr,
> +                    "Uknown gic_version \"%s\" specified\n", buf);

"Unknown".

Ian.