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

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

On 08-May-26 16:33, 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.
> 
> Implement the dom0less path to parse the new device tree property, add
> a new domctl hypercall input parameter `v8r_el1_msa` for arm and
> add the sanitisation in arch_sanitise_domain_config(), the parameter
> is intended to be used on CONFIG_MPU systems and returns an error if
> selected for MMU.
> 
> Given the breaking change, bump the XEN_DOMCTL_INTERFACE_VERSION.
> 
> Signed-off-by: Harry Ramsey <harry.ramsey@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v5:
>  - follow the way domu_dt_sci_parse and ARM_SCI is doing, but
>    return error if property is present in MMU systems.
>  - Align the commit message on the new changes
>  - fix documentation
>  - fix multiline comment
>  - drop the explicit padding from struct xen_arch_domainconfig
> 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           | 74 +++++++++++++++++++++++++
>  xen/arch/arm/domain.c                   | 11 ++++
>  xen/arch/arm/include/asm/domain.h       |  4 ++
>  xen/arch/arm/include/asm/domain_build.h | 21 +++++++
>  xen/arch/arm/mpu/arm32/mm.c             | 17 ++++++
>  xen/arch/arm/mpu/arm64/mm.c             | 18 ++++++
>  xen/include/public/arch-arm.h           |  6 ++
>  xen/include/public/domctl.h             |  4 +-
>  9 files changed, 167 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index 977b4286082f..2389ae610963 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 at EL1, a domain
> +    should use PMSAv8 (MPU) or VMSAv8 (MMU).
> +
> +    - "mmu"
> +    Enables VMSAv8 at EL1. This requires hardware support and is only
> +    optionally available on AArch64. Not supported on AArch32.
> +
> +    - "mpu"
> +    Enables PMSAv8 at EL1. This is the default behaviour when the property is
s/behaviour/behavior/

> +    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 6f73c65e5151..2a0671bd0e8e 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -295,6 +295,77 @@ static int __init domu_dt_sci_parse(struct 
> dt_device_node *node,
>      return 0;
>  }
>  
> +static int __init
> +domu_dt_v8r_el1_msa_parse(struct dt_device_node *node,
node can be const

> +                          struct xen_domctl_createdomain *d_cfg,
> +                          unsigned int flags)
> +{
> +    bool property_present = dt_property_read_bool(node, "v8r_el1_msa");
> +
> +    if ( !property_present )
> +        d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;
> +    else
> +    {
> +        const char *v8r_el1_msa;
> +        int ret = dt_property_read_string(node, "v8r_el1_msa", &v8r_el1_msa);
> +
> +        if ( ret )
> +            return ret;
> +
> +        if ( !strcmp(v8r_el1_msa, "mpu") )
> +            d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
> +        else if ( !strcmp(v8r_el1_msa, "mmu") )
> +            d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA;
> +        else
> +        {
> +            printk(XENLOG_ERR
> +                   "v8r_el1_msa value (%s) not valid for domain %s\n",
> +                   v8r_el1_msa, dt_node_name(node));
dt_node_name can be any arbitrary name not necessarily denoting a domain in a
meaningful way. Without a full path it can be misleading. Either print a full
path or don't print it at all. Here and elsewhere.

> +            return -EINVAL;
> +        }
> +    }
> +
> +    if ( !IS_ENABLED(CONFIG_MPU) )
> +    {
> +        if ( !property_present )
> +            return 0;
> +
> +        printk(XENLOG_ERR
> +               "Not supported 'v8r_el1_msa' DT property found for domain 
> %s\n",
> +               dt_node_name(node));
> +        return -EINVAL;
> +    }
> +
> +    switch ( d_cfg->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) )
> +        {
> +            printk(XENLOG_ERR
> +                   "PMSA is not valid for domain (%s) without static 
> allocation and direct map (v8r_el1_msa)\n",
> +                   dt_node_name(node));
> +            return -EINVAL;
> +        }
> +        break;
> +
> +    case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA:
> +        if ( !has_v8r_vmsa_support() )
> +        {
> +            printk(XENLOG_ERR
> +                   "Platform doesn't support VMSA at EL1 (v8r_el1_msa)\n");
> +            return -EINVAL;
> +        }
> +        break;
> +
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  int __init arch_parse_dom0less_node(struct dt_device_node *node,
>                                      struct boot_domain *bd)
>  {
> @@ -308,6 +379,9 @@ 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");
>  
> +    if ( domu_dt_v8r_el1_msa_parse(node, d_cfg, flags) )
> +        panic("Error getting v8r_el1_msa configuration\n");
> +
>      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..ac7d1abd9c7c 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>
> @@ -630,6 +631,12 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( !v8r_el1_msa_domain_sanitise_config(config) )
> +    {
> +        dprintk(XENLOG_INFO, "Unsupported v8r_el1_msa value\n");
> +        return -EINVAL;
> +    }
> +
>      return sci_domain_sanitise_config(config);
>  }
>  
> @@ -722,6 +729,10 @@ int arch_domain_create(struct domain *d,
>      d->arch.sve_vl = config->arch.sve_vl;
>  #endif
>  
> +#ifdef CONFIG_MPU
> +    d->arch.v8r_el1_msa = config->arch.v8r_el1_msa;
> +#endif
> +
>      if ( (rc = sci_domain_init(d, config)) != 0 )
>          goto fail;
>  
> diff --git a/xen/arch/arm/include/asm/domain.h 
> b/xen/arch/arm/include/asm/domain.h
> index b24f02d269be..ac7300e41fcd 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -112,6 +112,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..50ddc0511e7e 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -19,6 +19,27 @@ int prepare_acpi(struct domain *d, struct kernel_info 
> *kinfo);
>  
>  int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
>  
> +#ifdef CONFIG_MPU
> +/* Utility function to determine if an Armv8-R processor supports VMSA. */
> +bool has_v8r_vmsa_support(void);
> +bool v8r_el1_msa_domain_sanitise_config(
> +    const struct xen_domctl_createdomain *config);
> +#else
> +static inline bool has_v8r_vmsa_support(void)
> +{
> +    return false;
> +}
> +
> +static inline bool v8r_el1_msa_domain_sanitise_config(
> +    const struct xen_domctl_createdomain *config)
Why can't this function be common? I can see 3 definitions (Arm64 MPU, Arm32
MPU, MMU) but they do not have anything that would prevent from generalizing
them in a single function.

> +{
> +    if ( config->arch.v8r_el1_msa != XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE )
> +        return false;
> +
> +    return true;
> +}
> +#endif /* CONFIG_MPU */
> +
>  #endif
>  
>  /*
> diff --git a/xen/arch/arm/mpu/arm32/mm.c b/xen/arch/arm/mpu/arm32/mm.c
> index a4673c351141..a759ebdfc124 100644
> --- a/xen/arch/arm/mpu/arm32/mm.c
> +++ b/xen/arch/arm/mpu/arm32/mm.c
> @@ -5,6 +5,8 @@
>  #include <asm/mpu.h>
>  #include <asm/sysregs.h>
>  #include <asm/system.h>
> +#include <public/arch-arm.h>
> +#include <public/domctl.h>
>  
>  #define GENERATE_WRITE_PR_REG_CASE(num, pr)               \
>      case num:                                             \
> @@ -38,6 +40,21 @@
>          break;                                            \
>      }
>  
> +bool has_v8r_vmsa_support(void)
> +{
> +    return false;
> +}
> +
> +bool v8r_el1_msa_domain_sanitise_config(
> +    const struct xen_domctl_createdomain *config)
> +{
> +    if ( config->arch.v8r_el1_msa != XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE 
> &&
> +         config->arch.v8r_el1_msa != XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA )
Please add brackets ( () && () )


~Michal