Re: [PATCH v2 3/4] xen/arm64: allow to make aarch32 support optional

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Grygorii,

Grygorii Strashko <grygorii_strashko@xxxxxxxx> writes:

> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
>
> Now Arm64 AArch32 guest support is always enabled and built-in while not
> all Arm64 platforms supports AArch32 (for exmaple on Armv9A) or this
> support might not be needed (Arm64 AArch32 is used quite rarely in embedded
> systems). More over, when focusing on safety certification, AArch32 related
> code in Xen leaves a gap in terms of coverage that cannot really be
> justified in words. This leaves two options: either support it (lots of
> additional testing, requirements and documents would be needed) or compile
> it out.
>
> Hence, this patch introduces basic support to allow make Arm64
> AArch32 guest support optional. The following changes are introduced:
>
> - Introduce Kconfig option CONFIG_ARM64_AARCH32 to allow enable/disable
>   Arm64 AArch32 guest support (default y)
>
> - Introduce is_aarch32_enabled() helper which accounts Arm64 HW capability
>   and CONFIG_ARM64_AARCH32 setting
>
> - Introduce arm64_set_domain_type() to configure Arm64 domain type in
>   unified way instead of open coding (d)->arch.type, and account
>   CONFIG_ARM64_AARCH32 configuration.
>
> - toolstack: do not advertise "xen-3.0-armv7l " capability if AArch32 is
>   disabled.
>
> - do not expose EL1 AArch32 support to guest in ID_AA64PFR0_EL1 reg if
>   AArch32 is disabled.
>
> - Set Arm64 domain type to DOMAIN_64BIT by default.
>   - the Arm Xen boot code is handling this case properly already;
>   - for toolstack case the XEN_DOMCTL_set_address_size hypercall handling
>     updated to forcibly configure domain type regardless of current domain
>     type configuration, so toolstack behavior is unchanged.
>
> With CONFIG_ARM64_AARCH32=n the Xen will reject AArch32 guests (kernels) if
> configured by user in the following way:
> - Xen boot will fail with panic during dom0 or dom0less domains creation
> - toolstack domain creation will be rejected due to xc_dom_compat_check()
>   failure.
>
> Making Arm64 AArch32 guest support open further possibilities for build
> optimizations of Arm64 AArch32 guest support code.
>
> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> ---
> changes in v2:
> - use Arm64 "cpu_has_el1_32" in all places to check if HW has AArch32 support
> - rework Arm64 XEN_DOMCTL_set_address_size hypercall handling to work with any
>   initial domain type set (32bit or 64 bit)
> - fix comments related to macro parameters evaluation issues
> - do not expose EL1 AArch32 support to guest in ID_AA64PFR0_EL1 reg if
>   AArch32 is disabled
>
>  xen/arch/arm/Kconfig                    |  7 ++++
>  xen/arch/arm/arm64/domain-build.c       | 46 +++++++++++++++++++++++--
>  xen/arch/arm/arm64/domctl.c             | 16 +++++----
>  xen/arch/arm/arm64/vsysreg.c            |  9 +++++
>  xen/arch/arm/domain.c                   |  9 +++++
>  xen/arch/arm/domain_build.c             | 21 +++--------
>  xen/arch/arm/include/asm/arm32/domain.h | 12 +++++++
>  xen/arch/arm/include/asm/arm64/domain.h | 23 +++++++++++++
>  xen/arch/arm/setup.c                    |  2 +-
>  9 files changed, 119 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index a0c816047427..bf6dd73caf73 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -266,6 +266,13 @@ config PCI_PASSTHROUGH
>       help
>         This option enables PCI device passthrough
>  
> +config ARM64_AARCH32
> +     bool "AArch32 Guests support on on ARM64 (UNSUPPORTED)" if UNSUPPORTED

But aarch32 guests are supported... I understand that you wanted to say
"Disabling aarch32 support is unsupported". But currently this entry
reads backwards. I think it should be reworded better. But I have no
idea - how to do this.

> +     depends on ARM_64
> +     default y
> +     help
> +       This option enables AArch32 Guests on ARM64.
> +
>  endmenu
>  
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/arm64/domain-build.c 
> b/xen/arch/arm/arm64/domain-build.c
> index 3a89ee46b8c6..5951f002e3af 100644
> --- a/xen/arch/arm/arm64/domain-build.c
> +++ b/xen/arch/arm/arm64/domain-build.c
> @@ -4,13 +4,55 @@
>  #include <xen/sched.h>
>  
>  #include <asm/domain.h>
> +#include <asm/arm64/sve.h>
> +
> +int __init arm64_set_domain_type(struct kernel_info *kinfo)
> +{
> +    struct domain *d = kinfo->bd.d;
> +    enum domain_type type;
> +
> +    ASSERT(d);
> +    ASSERT(kinfo);

I don't think there is a sense in asserting that kinfo != NULL after you
dereferenced it retrieve "d"

> +
> +    type = kinfo->arch.type;
> +
> +    if ( !is_aarch32_enabled() )
> +    {
> +        ASSERT(d->arch.type == DOMAIN_64BIT);
> +
> +        if ( type == DOMAIN_32BIT )
> +        {
> +            const char *str = "not available";
> +
> +            if ( !IS_ENABLED(CONFIG_ARM64_AARCH32) )
> +                str = "disabled";
> +            printk("aarch32 guests support is %s\n", str);

XENLOG_ERROR?

> +            return -EINVAL;
> +        }
> +
> +        return 0;
> +    }
> +
> +    if ( is_sve_domain(d) && type == DOMAIN_32BIT )
> +    {
> +        printk("SVE is not available for 32-bit domain\n");

XENLOG_ERROR?

> +        return -EINVAL;
> +    }
> +
> +    d->arch.type = type;
> +
> +    return 0;
> +}
>  
>  #ifdef CONFIG_DOM0LESS_BOOT
>  /* TODO: make arch.type generic ? */
>  void __init set_domain_type(struct domain *d, struct kernel_info *kinfo)
>  {
> -    /* type must be set before allocate memory */
> -    d->arch.type = kinfo->arch.type;
> +    int rc;
> +
> +    rc = arm64_set_domain_type(kinfo);
> +    if ( rc < 0 )
> +        panic("Unsupported guest type\n");
>  }
>  #endif
>  
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 8720d126c97d..4f0f143daef8 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -13,6 +13,11 @@
>  #include <asm/arm64/sve.h>
>  #include <asm/cpufeature.h>
>  
> +static void vcpu_switch_to_aarch32_mode(struct vcpu *v)
> +{
> +    v->arch.hcr_el2 &= ~HCR_RW;
> +}
> +
>  static long switch_mode(struct domain *d, enum domain_type type)
>  {
>      struct vcpu *v;
> @@ -21,14 +26,14 @@ static long switch_mode(struct domain *d, enum 
> domain_type type)
>          return -EINVAL;
>      if ( domain_tot_pages(d) != 0 )
>          return -EBUSY;
> -    if ( d->arch.type == type )
> -        return 0;
>  
>      d->arch.type = type;
>  
> -    if ( is_64bit_domain(d) )
> -        for_each_vcpu(d, v)
> +    for_each_vcpu(d, v)
> +        if ( is_64bit_domain(d) )

Do you really need to check domain type for every vCPU? I think that
original variant was better.

>              vcpu_switch_to_aarch64_mode(v);
> +        else
> +            vcpu_switch_to_aarch32_mode(v);
>  
>      return 0;
>  }
> @@ -38,7 +43,7 @@ static long set_address_size(struct domain *d, uint32_t 
> address_size)
>      switch ( address_size )
>      {
>      case 32:
> -        if ( !cpu_has_el1_32 )
> +        if ( !is_aarch32_enabled() )
>              return -EINVAL;
>          /* SVE is not supported for 32 bit domain */
>          if ( is_sve_domain(d) )
> @@ -58,7 +63,6 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct 
> domain *d,
>      {
>      case XEN_DOMCTL_set_address_size:
>          return set_address_size(d, domctl->u.address_size.size);
> -

Stray change?

>      default:
>          return -ENOSYS;
>      }
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index d14258290ff0..9f7e735c9b74 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -330,6 +330,15 @@ void do_sysreg(struct cpu_user_regs *regs,
>      {
>          register_t guest_reg_value = domain_cpuinfo.pfr64.bits[0];
>  
> +        if ( !is_aarch32_enabled() )
> +        {
> +            /* do not expose EL1 AArch32 support if disabled */
> +            register_t mask = GENMASK(ID_AA64PFR0_EL1_SHIFT + 4 - 1,
> +                                      ID_AA64PFR0_EL1_SHIFT);
> +            guest_reg_value &= ~mask;
> +            guest_reg_value |= (1 << ID_AA64PFR0_EL1_SHIFT) & mask;

Why do you need to apply mask here?

> +        }
> +
>          if ( is_sve_domain(v->domain) )
>          {
>              /* 4 is the SVE field width in id_aa64pfr0_el1 */
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 310c5789096d..544d1422a793 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -791,6 +791,15 @@ int arch_domain_create(struct domain *d,
>      d->arch.sve_vl = config->arch.sve_vl;
>  #endif
>  
> +#ifdef CONFIG_ARM_64
> +    /*
> +     * Set default Execution State to AArch64 (64bit)
> +     * - Xen will reconfigure it properly at boot time
> +     * - toolstack will reconfigure it properly by 
> XEN_DOMCTL_set_address_size
> +     */
> +    d->arch.type = DOMAIN_64BIT;
> +#endif
> +
>      return 0;
>  
>  fail:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 47ba920fc6b0..c616127e8da5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1873,19 +1873,6 @@ int __init construct_domain(struct domain *d, struct 
> kernel_info *kinfo)
>      BUG_ON(v->is_initialised);
>  
>  #ifdef CONFIG_ARM_64
> -    /* if aarch32 mode is not supported at EL1 do not allow 32-bit domain */
> -    if ( !(cpu_has_el1_32) && kinfo->arch.type == DOMAIN_32BIT )
> -    {
> -        printk("Platform does not support 32-bit domain\n");
> -        return -EINVAL;
> -    }
> -
> -    if ( is_sve_domain(d) && (kinfo->arch.type == DOMAIN_32BIT) )
> -    {
> -        printk("SVE is not available for 32-bit domain\n");
> -        return -EINVAL;
> -    }
> -
>      if ( is_64bit_domain(d) )
>          vcpu_switch_to_aarch64_mode(v);
>  
> @@ -1983,6 +1970,10 @@ static int __init construct_dom0(struct domain *d)
>      if ( rc < 0 )
>          return rc;
>  
> +    rc = arm64_set_domain_type(&kinfo);
> +    if ( rc < 0 )
> +        return rc;
> +
>      return construct_hwdom(&kinfo, NULL);
>  }
>  
> @@ -1994,10 +1985,6 @@ int __init construct_hwdom(struct kernel_info *kinfo,
>  
>      iommu_hwdom_init(d);
>  
> -#ifdef CONFIG_ARM_64
> -    /* type must be set before allocate_memory */
> -    d->arch.type = kinfo->arch.type;
> -#endif
>      find_gnttab_region(d, kinfo);
>      if ( is_domain_direct_mapped(d) )
>          allocate_memory_11(d, kinfo);
> diff --git a/xen/arch/arm/include/asm/arm32/domain.h 
> b/xen/arch/arm/include/asm/arm32/domain.h
> index 1bd0735c9194..30e8818ff2ae 100644
> --- a/xen/arch/arm/include/asm/arm32/domain.h
> +++ b/xen/arch/arm/include/asm/arm32/domain.h
> @@ -3,11 +3,23 @@
>  #ifndef ARM_ARM32_DOMAIN_H
>  #define ARM_ARM32_DOMAIN_H
>  
> +struct kernel_info;
> +
>  /* Arm32 always runs guests in AArch32 mode */
>  
>  #define is_32bit_domain(d) ((void)(d), 1)
>  #define is_64bit_domain(d) ((void)(d), 0)
>  
> +static inline bool is_aarch32_enabled(void)
> +{
> +    return true;
> +}
> +
> +static inline int arm64_set_domain_type(struct kernel_info *kinfo)
> +{
> +    return 0;
> +}
> +
>  #endif /* ARM_ARM32_DOMAIN_H */
>  
>  /*
> diff --git a/xen/arch/arm/include/asm/arm64/domain.h 
> b/xen/arch/arm/include/asm/arm64/domain.h
> index 1a2d73950bf0..bebcbc582f97 100644
> --- a/xen/arch/arm/include/asm/arm64/domain.h
> +++ b/xen/arch/arm/include/asm/arm64/domain.h
> @@ -3,6 +3,10 @@
>  #ifndef ARM_ARM64_DOMAIN_H
>  #define ARM_ARM64_DOMAIN_H
>  
> +#include <asm/cpufeature.h>
> +
> +struct kernel_info;
> +
>  /*
>   * Returns true if guest execution state is AArch32
>   *
> @@ -17,6 +21,25 @@
>   */
>  #define is_64bit_domain(d) ((d)->arch.type == DOMAIN_64BIT)
>  
> +/*
> + * Arm64 declares AArch32 (32bit) Execution State support in the
> + * Processor Feature Registers (PFR0), but also can be disabled manually.
> + */
> +static inline bool is_aarch32_enabled(void)
> +{
> +    return IS_ENABLED(CONFIG_ARM64_AARCH32) && cpu_has_el1_32;
> +}
> +
> +/*
> + * Set domain type from struct kernel_info which defines guest Execution
> + * State AArch32/AArch64 during regular dom0 or predefined (dom0less)
> + * domains creation .

Extra space before full stop

> + * Type must be set before allocate_memory or create vcpus.
> + *
> + * @kinfo: pointer to the kinfo structure.
> + */
> +int arm64_set_domain_type(struct kernel_info *kinfo);
> +
>  #endif /* ARM_ARM64_DOMAIN_H */
>  
>  /*
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index bb35afe56cec..c29573f0ffec 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -530,7 +530,7 @@ static int __init init_xen_cap_info(void)
>  #ifdef CONFIG_ARM_64
>      safe_strcat(xen_cap_info, "xen-3.0-aarch64 ");
>  #endif
> -    if ( cpu_has_aarch32 )
> +    if ( is_aarch32_enabled() )
>          safe_strcat(xen_cap_info, "xen-3.0-armv7l ");
>  
>      return 0;

-- 
WBR, Volodymyr