Re: [PATCH v4 2/3] arm: Allow the user to specify the GIC version

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

On Tue, 2015-07-07 at 15:08 +0100, Julien Grall wrote:
> A platform may have a GIC compatible with previous version of the
> device.

"...a GIC which is compatible with a previous version..."

> 
> This is allow to virtualize an unmodified OS on new hardware if the GIC
> is compatible with older version.

"This is to allow virtualization of an unmodified..."

"...with the 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 only GICv2 and GICv3 are supported).

I think "Unless the user has specified in the configuration file..."

> 
> 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?
> 
>     It may be possible to rework the libxl code to restrict the scope of
>     xc_config in libxl_domain_make. This can be done in a follow-up if
>     we figure what to do the frequency field.
> 
>     Changes in v4:
>         - Update the documentation to specify the default behavior
>         - Update the domain configuration with the GIC version returned
>         by the hypervisor.
> 
>     Changes in v3:
>         - Rename GIC_VERSION define in LIBXL_GIC_VERSION_Vn.
>         - Change the value of each define
>         - Use libxl_gic_version_from_string rather than custom if/else
>         - Rename LIBXL_HAVE_BUILDINFO_GIC_VERSION into
>         LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION and update the comment
>         - Update doc with Ian's suggestion
>         - Typoes
> 
>     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        | 34 +++++++++++++++++++++++++++++++++
>  tools/libxl/libxl.h          |  5 +++++
>  tools/libxl/libxl_arch.h     |  6 ++++++
>  tools/libxl/libxl_arm.c      | 35 +++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl_create.c   |  7 +++++++
>  tools/libxl/libxl_types.idl  | 11 +++++++++++
>  tools/libxl/libxl_x86.c      |  7 +++++++
>  tools/libxl/xl_cmdimpl.c     | 12 ++++++++++++
>  xen/arch/arm/domain.c        | 45 ++++++++++++++++++++++++++------------------
>  xen/arch/arm/vgic.c          |  4 ++--
>  xen/include/asm-arm/domain.h |  2 ++
>  11 files changed, 147 insertions(+), 21 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a3e0e2e..12765d5 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1688,6 +1688,40 @@ 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

"Emulate the GICv2 hardware" or just "Emulate a GICv2" (without the
h/w).

> +
> +=item B<v3>
> +
> +Emulate a GICv3 hardware.

As above.

>  Note that the emulated GIC does not support the
> +GICv2 compatibility mode.
> +
> +=item B<default>
> +
> +Emulate the same version as the native GIC hardware used by host where the
> +the domain has been created.

"... where the domain was created".

> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 03a9205..8ac90c4 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -61,7 +61,40 @@ 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_NATIVE;
> +    switch (d_config->b_info.arch_arm.gic_version) {
> +    case LIBXL_GIC_VERSION_DEFAULT:
> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> +        break;
> +    case LIBXL_GIC_VERSION_V2:
> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
> +        break;
> +    case LIBXL_GIC_VERSION_V3:
> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
> +        break;
> +    default:
> +        LOG(ERROR, "Unknown GIC version %s\n",
> +            libxl_gic_version_to_string(d_config->b_info.arch_arm.gic_version));

libxl_gic_version_to_string will return NULL for a truly unknown value.
Perhaps print %d version too/instead?

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index f799081..2b8a506 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -579,6 +579,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>          goto out;
>      }
>  
> +    ret = libxl__arch_domain_save_config(gc, d_config, xc_config);


> +    if (ret < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fail to save domain config");

Please use the shorter LOG* macros, and you don't need/want to log errno
here since the function doesn't touch it.

Actually, the function logs on failure itself, no need to repeat it I
think, just document "logs on failure" next to the prototype.

> +        rc = ERROR_FAIL;

libxl__arch_domain_save_config already returns a libxl ERROR_*, so it
should be stored into rc directly and propagated.

> +        goto out;
> +    }
> +
>      ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
>      if (ret < 0) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain move fail");
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1be3f8b..e825315 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2250,6 +2250,18 @@ skip_vfb:
>          }
>      }
>  
> +    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
> +        libxl_gic_version v;
> +
> +        e = libxl_gic_version_from_string(buf, &v);
> +        if (e) {
> +            fprintf(stderr,
> +                    "Unknown gic_version \"%s\" specified\n", buf);
> +            exit(-ERROR_FAIL);
> +        }
> +        b_info->arch_arm.gic_version = v;

You can just pass &b_info....gic_version straight to the from_string
function.

Ian.