Re: [PATCH v4 2/3] arm/mpu: Introduce `v8r_el1_msa` device tree property for domains

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

On 20-Apr-26 16:22, Luca Fancellu wrote:
> From: Harry Ramsey <harry.ramsey@xxxxxxx>
> 
> Add a new device tree property `v8r_el1_msa` to select the MSA (memory
> system architecture) at EL1 for Armv8-R architecture: MPU or MMU, the
> former is the default if the property is not passed.
> 
> The check and setting of this new input parameter for the guest
> configuration is performed in arch_domain_create() instead of the more
> usual arch_sanitise_domain_config() because the former has access to the
> Xen internal guest creation flags which are required to ensure PMSA can
> work (domain requires static allocation and direct mapping).
> 
> The property is valid only when used on MPU systems and will result in
> a panic on MMU ones.
> 
> Bumped XEN_DOMCTL_INTERFACE_VERSION because of the new domctl input
> parameter.
> 
> Signed-off-by: Harry Ramsey <harry.ramsey@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v4:
> - Rework the patch to have the v8r_el1_msa input parameter more
>   enclosed in the Armv8-A (mmu)/Armv8-R (mpu) space.
> v3:
> - Improve commit message and device tree property description
> - Remove macro protection
> - Remove unused function is_mpu_domain
> - Code formatting
> ---
>  docs/misc/arm/device-tree/booting.txt       | 14 ++++
>  xen/arch/arm/dom0less-build.c               |  3 +
>  xen/arch/arm/domain.c                       |  4 ++
>  xen/arch/arm/include/asm/domain.h           |  4 ++
>  xen/arch/arm/include/asm/domain_build.h     |  8 +++
>  xen/arch/arm/include/asm/mmu/domain-build.h | 46 +++++++++++++
>  xen/arch/arm/include/asm/mpu.h              |  5 ++
>  xen/arch/arm/include/asm/mpu/domain-build.h | 27 ++++++++
>  xen/arch/arm/mpu/Makefile                   |  1 +
>  xen/arch/arm/mpu/arm32/mm.c                 |  5 ++
>  xen/arch/arm/mpu/arm64/mm.c                 |  5 ++
>  xen/arch/arm/mpu/domain-build.c             | 76 +++++++++++++++++++++
>  xen/include/public/arch-arm.h               |  7 ++
>  xen/include/public/domctl.h                 |  4 +-
>  14 files changed, 207 insertions(+), 2 deletions(-)
>  create mode 100644 xen/arch/arm/include/asm/mmu/domain-build.h
>  create mode 100644 xen/arch/arm/include/asm/mpu/domain-build.h
>  create mode 100644 xen/arch/arm/mpu/domain-build.c
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index 977b4286082f..c3f484a3b01a 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -322,6 +322,20 @@ with the following properties:
>      Should be used together with scmi-smc-passthrough Xen command line
>      option.
>  
> +- v8r_el1_msa
> +
> +    A string property specifying whether, on Armv8-R systems, a domain
> +    should use PMSAv8 (MPU) at EL1 or VMSAv8 (MMU) at EL1.
Instead of repeating at EL1 you could move it next to "on Armv8R systems"

> +
> +    - "mmu"
> +    Enables VMSAv8 at EL1. This requires hardware support and is only
> +    optionally available on AArch64.
Maybe it's due to the combination of words but it does not immediately tell that
it's not present on AArch32. I would add: "Not supported on AArch32".

> +
> +    - "mpu"
> +    Enables PMSAv8 at EL1. This is the default behaviour when the property is
> +    not passed. This configuration requires static allocation 
> (xen,static-mem)
> +    and direct mapping (direct-map).
> +
>  Under the "xen,domain" compatible node, one or more sub-nodes are present
>  for the DomU kernel and ramdisk.
>  
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 4181c105389a..6f0256f9d825 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -24,6 +24,7 @@
>  #include <asm/domain_build.h>
>  #include <asm/firmware/sci.h>
>  #include <asm/grant_table.h>
> +#include <asm/mpu.h>
>  #include <asm/setup.h>
>  
>  #ifdef CONFIG_VGICV2
> @@ -322,6 +323,8 @@ int __init arch_parse_dom0less_node(struct dt_device_node 
> *node,
>      if ( domu_dt_sci_parse(node, d_cfg) )
>          panic("Error getting SCI configuration\n");
>  
> +    arch_dt_v8r_el1_msa_parse(node, d_cfg);
"arch" prefix should be used by functions called from the common code that have
arch-specific implementation. This is not the case for functions you're
introducing here, so please drop this prefix.

Also, I would continue using the SCI approach e.g. call it domu_dt_msa_parse()
and decide to panic at the call site rather than making this decision in the
function itself.

> +
>      if ( !dt_property_read_u32(node, "nr_spis", &d_cfg->arch.nr_spis) )
>      {
>          int vpl011_virq = GUEST_VPL011_SPI;
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 26380a807cad..dfa7ace1141b 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -18,6 +18,7 @@
>  #include <asm/cpuerrata.h>
>  #include <asm/cpufeature.h>
>  #include <asm/current.h>
> +#include <asm/domain_build.h>
>  #include <asm/event.h>
>  #include <asm/gic.h>
>  #include <asm/guest_atomics.h>
> @@ -725,6 +726,9 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = sci_domain_init(d, config)) != 0 )
>          goto fail;
>  
> +    if ( (rc = arch_set_v8r_el1_msa(d, config, flags)) != 0 )
> +        goto fail;
> +
>      return 0;
>  
>  fail:
> diff --git a/xen/arch/arm/include/asm/domain.h 
> b/xen/arch/arm/include/asm/domain.h
> index ffe5d0d9f0a6..4a3fb825962b 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -128,6 +128,10 @@ struct arch_domain
>  #endif
>  
>      struct resume_info resume_ctx;
> +
> +#ifdef CONFIG_MPU
> +    uint8_t v8r_el1_msa;
> +#endif
>  }  __cacheline_aligned;
>  
>  struct arch_vcpu
> diff --git a/xen/arch/arm/include/asm/domain_build.h 
> b/xen/arch/arm/include/asm/domain_build.h
> index 6674dac5e2f8..921d6f98f4f4 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -4,6 +4,14 @@
>  #include <xen/fdt-kernel.h>
>  #include <xen/sched.h>
>  
> +#if defined(CONFIG_MMU)
> +#include <asm/mmu/domain-build.h>
> +#elif defined(CONFIG_MPU)
> +#include <asm/mpu/domain-build.h>
> +#else
> +# error "Unknown memory management layout"
I don't think that this error msg is correct for domain build.

> +#endif
> +
>  typedef __be32 gic_interrupt_t[3];
>  int make_psci_node(void *fdt);
>  void evtchn_allocate(struct domain *d);
> diff --git a/xen/arch/arm/include/asm/mmu/domain-build.h 
> b/xen/arch/arm/include/asm/mmu/domain-build.h
> new file mode 100644
> index 000000000000..3e0d9a6a2a08
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mmu/domain-build.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_MMU_DOMAIN_BUILD_H__
> +#define __ARM_MMU_DOMAIN_BUILD_H__
> +
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +#include <public/domctl.h>
> +
> +static inline
> +void arch_dt_v8r_el1_msa_parse(struct dt_device_node *node,
> +                               struct xen_domctl_createdomain *d_cfg)
> +{
> +    const char *v8r_el1_msa;
> +
> +    if ( !dt_property_read_string(node, "v8r_el1_msa", &v8r_el1_msa) )
> +        panic("'v8r_el1_msa' property found, but CONFIG_MPU not selected\n");
> +}
> +
> +static inline
> +int arch_set_v8r_el1_msa(struct domain *d,
> +                         const struct xen_domctl_createdomain *config,
> +                         unsigned int flags)
> +{
> +    if ( config->arch.v8r_el1_msa )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "arch.v8r_el1_msa set, but CONFIG_MPU not selected\n");
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +#endif /* __ARM_MMU_DOMAIN_BUILD_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 72fa5b00b861..8a8c01086206 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -27,6 +27,11 @@
>  
>  #ifndef __ASSEMBLER__
>  
> +/*
> + * Utility function to determine if an Armv8-R processor supports VMSA.
> + */
NIT: No need for multi-line comment for a single sentence that can fit in one 
line.

> +bool has_v8r_vmsa_support(void);
> +
>  /*
>   * Set base address of MPU protection region.
>   *
> diff --git a/xen/arch/arm/include/asm/mpu/domain-build.h 
> b/xen/arch/arm/include/asm/mpu/domain-build.h
> new file mode 100644
> index 000000000000..463cd85b5b7e
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/domain-build.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_MPU_DOMAIN_BUILD_H__
> +#define __ARM_MPU_DOMAIN_BUILD_H__
> +
> +#include <xen/device_tree.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +#include <public/domctl.h>
> +
> +void arch_dt_v8r_el1_msa_parse(struct dt_device_node *node,
> +                               struct xen_domctl_createdomain *d_cfg);
> +
> +int arch_set_v8r_el1_msa(struct domain *d,
> +                         const struct xen_domctl_createdomain *config,
> +                         unsigned int flags);
> +
> +#endif /* __ARM_MPU_DOMAIN_BUILD_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
> index 3327fadd5d0e..e3997e41b81b 100644
> --- a/xen/arch/arm/mpu/Makefile
> +++ b/xen/arch/arm/mpu/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_ARM_32) += arm32/
>  obj-$(CONFIG_ARM_64) += arm64/
> +obj-y += domain-build.o
>  obj-y += domain-page.o
>  obj-y += mm.o
>  obj-y += p2m.o
> diff --git a/xen/arch/arm/mpu/arm32/mm.c b/xen/arch/arm/mpu/arm32/mm.c
> index a4673c351141..5eaeb3400e6c 100644
> --- a/xen/arch/arm/mpu/arm32/mm.c
> +++ b/xen/arch/arm/mpu/arm32/mm.c
> @@ -38,6 +38,11 @@
>          break;                                            \
>      }
>  
> +bool has_v8r_vmsa_support(void)
> +{
> +    return false;
> +}
> +
>  /*
>   * Armv8-R supports direct access and indirect access to the MPU regions 
> through
>   * registers:
> diff --git a/xen/arch/arm/mpu/arm64/mm.c b/xen/arch/arm/mpu/arm64/mm.c
> index ed643cad4073..b07e729a7d05 100644
> --- a/xen/arch/arm/mpu/arm64/mm.c
> +++ b/xen/arch/arm/mpu/arm64/mm.c
> @@ -32,6 +32,11 @@
>          break;                                                  \
>      }
>  
> +bool has_v8r_vmsa_support(void)
> +{
> +    return system_cpuinfo.mm64.msa_frac == MM64_MSA_FRAC_VMSA_SUPPORT;
> +}
> +
>  /*
>   * Armv8-R supports direct access and indirect access to the MPU regions 
> through
>   * registers:
> diff --git a/xen/arch/arm/mpu/domain-build.c b/xen/arch/arm/mpu/domain-build.c
> new file mode 100644
> index 000000000000..1bdd0ffedebb
> --- /dev/null
> +++ b/xen/arch/arm/mpu/domain-build.c
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/device_tree.h>
> +#include <xen/domain.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/types.h>
> +#include <xen/sched.h>
> +#include <asm/mpu.h>
> +#include <asm/mpu/domain-build.h>
> +#include <public/arch-arm.h>
> +#include <public/domctl.h>
> +
> +void __init arch_dt_v8r_el1_msa_parse(struct dt_device_node *node,
> +                                      struct xen_domctl_createdomain *d_cfg)
> +{
> +    const char *v8r_el1_msa;
> +
> +    d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;
> +
> +    if ( !dt_property_read_string(node, "v8r_el1_msa", &v8r_el1_msa) )
> +    {
> +        if ( !strcmp(v8r_el1_msa, "mmu") )
> +            d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA;
> +        else if ( !strcmp(v8r_el1_msa, "mpu") )
> +            d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
> +        else
> +            panic("Invalid device tree option for v8r_el1_msa\n");
> +    }
> +}
> +
> +int arch_set_v8r_el1_msa(struct domain *d,
> +                         const struct xen_domctl_createdomain *config,
> +                         unsigned int flags)
> +{
> +    switch ( config->arch.v8r_el1_msa )
> +    {
> +    case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE:
> +        fallthrough;
> +    case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA:
> +        if ( !(flags & CDF_staticmem) || !(flags & CDF_directmap) )
> +        {
> +            dprintk(XENLOG_INFO,
> +                    "PMSA is not valid for domain without static allocation 
> and direct map (v8r_el1_msa)\n");
> +            return -EINVAL;
> +        }
> +        break;
> +
> +    case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA:
> +        if ( !has_v8r_vmsa_support() )
> +        {
> +            dprintk(XENLOG_INFO,
> +                    "Platform does not support VMSA at EL1 (v8r_el1_msa)\n");
> +            return -EINVAL;
> +        }
> +        break;
> +
> +    default:
> +        dprintk(XENLOG_INFO, "Unsupported arch.v8r_el1_msa value (%u)\n",
> +                config->arch.v8r_el1_msa);
> +        return -EINVAL;
> +    }
Why do we even need this split. It seems like all the above checks could be done
in arch_dt_v8r_el1_msa_parse given that it is called after static-mem,direct-map
are set. This would simplify this file and we would not even need to introduce
new domain-build split for one function.

> +
> +    d->arch.v8r_el1_msa = config->arch.v8r_el1_msa;
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index cd563cf70684..7d6f87e8b2b1 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -330,6 +330,10 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>  #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
>  #define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC  1
>  
> +#define XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE    0
> +#define XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA    1
> +#define XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA    2
> +
>  struct xen_arch_domainconfig {
>      /* IN/OUT */
>      uint8_t gic_version;
> @@ -355,6 +359,9 @@ struct xen_arch_domainconfig {
>      uint32_t clock_frequency;
>      /* IN */
>      uint8_t arm_sci_type;
> +    /* IN */
> +    uint8_t v8r_el1_msa;
> +    uint16_t pad;
Before this change there were 3B of implicit padding. Now you added 1B of data
and 2B of explicit padding. The struct size is the same, so why bumping the
interface version? I don't see it necessary here. Also, why explicit padding?
With explicit padding I do think you need to now check that it's 0.

>  };
>  #endif /* __XEN__ || __XEN_TOOLS__ */
>  
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 8f6708c0a7cd..23124547f347 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -30,9 +30,9 @@
>   * fields) don't require a change of the version.
>   * Stable ops are NOT covered by XEN_DOMCTL_INTERFACE_VERSION!
>   *
> - * Last version bump: Xen 4.19
> + * Last version bump: Xen 4.22
>   */
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000018
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.

~Michal