Re: [PATCH v5 3/6] tools/arm: choose GIC version explicitly instead of relying on GIC_NATIVE

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 11-Sep-26 14:47, Julian Vetter wrote:
> XEN_DOMCTL_CONFIG_GIC_NATIVE lets the toolstack ask Xen to silently
> resolve the domain's GIC version to whatever the host hardware has. Xen
> then writes the resolved value back into the same in/out
> xen_arch_domainconfig the toolstack used as input, which is the kind of
> API abuse we're trying to get rid of. The struct passed to createdomain
> should only be an input parameter.
> 
> Move the "pick the best available GIC version" decision to the
> toolstack, using the XEN_SYSCTL_PHYSCAP_ARM_GIC_V2/V3 capability bits
> already exposed via XEN_SYSCTL_physinfo:
> 
>  * libxl__arch_domain_build_info_setdefault() resolves the GIC version
>    against those bits before the config is built. An unspecified version
>    becomes v3 if available, else v2, else fails. An explicitly requested
>    v2/v3 is validated against the same bits, so a version the host
>    cannot provide is directly rejected in the toolstack.
>  * The Python xc.domain_create() binding does the same via a call to
>    xc_physinfo().
>  * libxl__arch_domain_prepare_config() therefore only ever sees a
>    concrete v2/v3 request and just validates it. The GIC_NATIVE case is
>    dropped since setdefault() always resolves it first.
> 
> The LIBXL_GIC_VERSION enum value 0 is renamed from DEFAULT to NONE to
> reflect that it now only means "the user did not pick a version".
> setdefault() resolves it before anything else can observe it, so there
> is no longer a "default" left in the config. The xl.cfg(5) gic_version
> documentation is updated to match.
> 
> This guarantees no toolstack path can still produce
> XEN_DOMCTL_CONFIG_GIC_NATIVE, in preparation for removing it from the
> Xen side and from the ABI entirely.
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v5:
> - Go back to the initial per-version arch_capabilities_arm_gic_{v2,v3}()
>   helpers instead of a generic arch_capabilities_arm_has(caps, mask)
> - Validate an explicitly requested GIC version against the host
>   capabilities, not just resolve an unspecified one
> - Rename LIBXL_GIC_VERSION_DEFAULT to LIBXL_GIC_VERSION_NONE
This one is on me. I just realized that libxl compares user provided string with
the IDL types, so a xl.cfg file specifying "default" would fail now. This would
be wrong given that libxl API is stable. Let's keep the DEFAULT as it was (for
NONE you would also need to change the golang bindings). With that changed:
Acked-by: Michal Orzel <michal.orzel@xxxxxxx>

You will still need Rb from Anthony as toolstack maintainer.

> - Document the change in xl.cfg(5)
> ---
>  docs/man/xl.cfg.5.pod.in                      |  9 ++--
>  .../include/xen-tools/arm-arch-capabilities.h | 21 +++++++++
>  tools/libs/light/libxl_arm.c                  | 45 +++++++++++++++++--
>  tools/libs/light/libxl_types.idl              |  4 +-
>  tools/python/xen/lowlevel/xc/xc.c             | 18 +++++++-
>  5 files changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index d34951edb9..6a5e75eeab 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -3081,15 +3081,16 @@ Emulate a GICv2
>  Emulate a GICv3. Note that the emulated GIC does not support the
>  GICv2 compatibility mode.
>  
> -=item B<default>
> +=item B<none>
>  
> -Emulate the same version as the native GIC hardware used by the host where
> -the domain was created.
> +Let the toolstack choose the GIC version: GICv3 if the host supports it,
> +otherwise GICv2. This is the default when C<gic_version> is not specified.
>  
>  =back
>  
>  This requires hardware compatibility with the requested version, either
> -natively or via hardware backwards compatibility support.
> +natively or via hardware backwards compatibility support. The GIC versions
> +the host can emulate for a guest are reported via C<XEN_SYSCTL_physinfo>.
>  
>  =item B<vuart="uart">
>  
> diff --git a/tools/include/xen-tools/arm-arch-capabilities.h 
> b/tools/include/xen-tools/arm-arch-capabilities.h
> index 4aa4c6c34a..21e3c73bd1 100644
> --- a/tools/include/xen-tools/arm-arch-capabilities.h
> +++ b/tools/include/xen-tools/arm-arch-capabilities.h
> @@ -6,6 +6,7 @@
>  #ifndef ARM_ARCH_CAPABILITIES_H
>  #define ARM_ARCH_CAPABILITIES_H
>  
> +#include <stdbool.h>
>  #include <stdint.h>
>  #include <xen/sysctl.h>
>  
> @@ -25,4 +26,24 @@ unsigned int arch_capabilities_arm_sve(unsigned int 
> arch_capabilities)
>  #endif
>  }
>  
> +static inline
> +bool arch_capabilities_arm_gic_v2(unsigned int arch_capabilities)
> +{
> +#if defined(__arm__) || defined(__aarch64__)
> +    return MASK_EXTR(arch_capabilities, XEN_SYSCTL_PHYSCAP_ARM_GIC_V2);
> +#else
> +    return false;
> +#endif
> +}
> +
> +static inline
> +bool arch_capabilities_arm_gic_v3(unsigned int arch_capabilities)
> +{
> +#if defined(__arm__) || defined(__aarch64__)
> +    return MASK_EXTR(arch_capabilities, XEN_SYSCTL_PHYSCAP_ARM_GIC_V3);
> +#else
> +    return false;
> +#endif
> +}
> +
>  #endif /* ARM_ARCH_CAPABILITIES_H */
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 7e9f8a1bc3..283cfb749b 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -196,9 +196,6 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      LOG(DEBUG, " - Allocate %u SPIs", config->arch.nr_spis);
>  
>      switch (d_config->b_info.arch_arm.gic_version) {
> -    case LIBXL_GIC_VERSION_DEFAULT:
> -        config->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> -        break;
>      case LIBXL_GIC_VERSION_V2:
>          config->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
>          break;
> @@ -1800,6 +1797,48 @@ int libxl__arch_domain_build_info_setdefault(libxl__gc 
> *gc,
>      /* Trapping of unmapped accesses enabled by default.  */
>      libxl_defbool_setdefault(&b_info->trap_unmapped_accesses, true);
>  
> +    /*
> +     * Resolve the GIC version against the host capabilities reported by
> +     * XEN_SYSCTL_physinfo. If the user didn't request a specific version, 
> pick
> +     * the best one available. Otherwise validate the requested version here,
> +     * so a bad request fails early instead of in the hypervisor.
> +     */
> +    {
> +        bool has_v3 = 
> arch_capabilities_arm_gic_v3(physinfo->arch_capabilities);
> +        bool has_v2 = 
> arch_capabilities_arm_gic_v2(physinfo->arch_capabilities);
NIT: you can move them to the top to prevent the need for indentation.

~Michal