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

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

On 12-May-26 19:57, 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.
> 
> While there, add explicit padding and check that it's zero during
> arch domain config sanitisation, 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>
> ---
> v6:
>  - Add explicit padding to `struct xen_arch_domainconfig` and check it
>  - Provide common v8r_el1_msa_domain_sanitise_config() for MMU/MPU
>  - During domu_dt_v8r_el1_msa_parse() set PMSA when property not present
>  - constify *node in domu_dt_v8r_el1_msa_parse()
>  - Print full path of the node in domu_dt_v8r_el1_msa_parse() error msgs
>  - typo fixing
> 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                   | 37 +++++++++++++
>  xen/arch/arm/include/asm/domain.h       |  4 ++
>  xen/arch/arm/include/asm/domain_build.h | 10 ++++
>  xen/arch/arm/mpu/arm32/mm.c             |  7 +++
>  xen/arch/arm/mpu/arm64/mm.c             |  7 +++
>  xen/include/public/arch-arm.h           |  7 +++
>  xen/include/public/domctl.h             |  4 +-
>  9 files changed, 162 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt 
> b/docs/misc/arm/device-tree/booting.txt
> index 977b4286082f..f73839df090b 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 behavior 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 52cf788a45ea..fecdb33d3e3a 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -302,6 +302,77 @@ static int __init domu_dt_sci_parse(struct 
> dt_device_node *node,
>      return 0;
>  }
>  
> +static int __init
> +domu_dt_v8r_el1_msa_parse(const struct dt_device_node *node,
> +                          struct xen_domctl_createdomain *d_cfg,
> +                          unsigned int flags)
> +{
> +    bool property_present = dt_property_read_bool(node, "v8r_el1_msa");
I know it's present in the code for SCI, but it's not really necessary to do the
DT parsing twice (once for boot, second for string given that
dt_property_read_string returns -EINVAL if no property found).

You could move this bool check ...

> +
> +    if ( !IS_ENABLED(CONFIG_MPU) )
> +    {
> +        d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;
> +
> +        if ( !property_present )
... here.

> +            return 0;
> +
> +        printk(XENLOG_ERR
> +               "Not supported 'v8r_el1_msa' DT property found for domain 
> %s\n",
> +               dt_node_full_name(node));
> +        return -EINVAL;
> +    }
> +
> +    if ( !property_present )
> +        d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
> +    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_full_name(node));
> +            return -EINVAL;
> +        }
> +    }
> +
> +    switch ( d_cfg->arch.v8r_el1_msa )
> +    {
> +    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_full_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;
This does not look very clean. How about:

static int __init
domu_dt_v8r_el1_msa_parse(const struct dt_device_node *node,
                          struct xen_domctl_createdomain *d_cfg,
                          unsigned int flags)
{
    const char *value;
    int ret;

    if ( !IS_ENABLED(CONFIG_MPU) )
    {
        d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;

        if ( !dt_property_read_bool(node, "v8r_el1_msa") )
            return 0;

        printk(XENLOG_ERR
               "v8r_el1_msa not supported on this build for domain %s\n",
               dt_node_full_name(node));
        return -EINVAL;
    }

    ret = dt_property_read_string(node, "v8r_el1_msa", &value);
    /* property absent: PMSA is the default */
    if ( ret == -EINVAL )
        value = "mpu";
    else if ( ret )
        return ret;

    if ( !strcmp(value, "mpu") )
    {
        if ( !(flags & CDF_staticmem) || !(flags & CDF_directmap) )
        {
            printk(XENLOG_ERR
                   "v8r_el1_msa=mpu requires static-mem and direct-map for
domain %s\n",
                   dt_node_full_name(node));
            return -EINVAL;
        }
        d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
        return 0;
    }

    if ( !strcmp(value, "mmu") )
    {
        if ( !has_v8r_vmsa_support() )
        {
            printk(XENLOG_ERR
                   "v8r_el1_msa=mmu unsupported by platform for domain %s\n",
                   dt_node_full_name(node));
            return -EINVAL;
        }
        d_cfg->arch.v8r_el1_msa = XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA;
        return 0;
    }

    printk(XENLOG_ERR "v8r_el1_msa value '%s' not valid for domain %s\n",
           value, dt_node_full_name(node));
    return -EINVAL;
}


> +}
> +
>  int __init arch_parse_dom0less_node(struct dt_device_node *node,
>                                      struct boot_domain *bd)
>  {
> @@ -315,6 +386,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..e579c3b1bb3c 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>
You don't seem to use anything from this header.

>  #include <asm/event.h>
>  #include <asm/gic.h>
>  #include <asm/guest_atomics.h>
> @@ -538,6 +539,24 @@ void vcpu_switch_to_aarch64_mode(struct vcpu *v)
>      v->arch.hcr_el2 |= HCR_RW;
>  }
>  
> +static bool v8r_el1_msa_domain_sanitise_config(
> +    const struct xen_domctl_createdomain *config)
> +{
> +    uint8_t v8r_el1_msa = config->arch.v8r_el1_msa;
That is not a useful assignment.

> +
> +    if ( !IS_ENABLED(CONFIG_MPU) )
> +        return v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE;
> +
> +    if ( IS_ENABLED(CONFIG_ARM_32) )
> +        return v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA;
> +
> +    if ( IS_ENABLED(CONFIG_ARM_64) )
> +        return (v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA) ||
> +               (v8r_el1_msa == XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA);
> +
> +    return false;
Arm32 and Arm64 are mutually exclusive, so this line is unreachable.
How about:
  static bool v8r_el1_msa_domain_sanitise_config(
      const struct xen_domctl_createdomain *config)
  {
      switch ( config->arch.v8r_el1_msa )
      {
      case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_NONE:
          return !IS_ENABLED(CONFIG_MPU);

      case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_PMSA:
          return IS_ENABLED(CONFIG_MPU);

      case XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA:
          return IS_ENABLED(CONFIG_MPU) && IS_ENABLED(CONFIG_ARM_64);

      default:
          return false;
      }
  }

> +}
> +
>  int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>  {
>      unsigned int max_vcpus;
> @@ -554,6 +573,14 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    /* Check config structure padding */
> +    if ( config->arch.pad )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "Invalid domain configuration during domain creation\n");
> +        return -EINVAL;
> +    }
> +
>      /* Check feature flags */
>      if ( sve_vl_bits > 0 )
>      {
> @@ -630,6 +657,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 +755,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..13e88fc0891b 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -19,6 +19,16 @@ 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
You could do: && CONFIG_ARM64 and then ...

> +/* Utility function to determine if an Armv8-R processor supports VMSA. */
> +bool has_v8r_vmsa_support(void);
> +#else
> +static inline bool has_v8r_vmsa_support(void)
> +{
> +    return false;
> +}
> +#endif /* CONFIG_MPU */
> +
>  #endif
>  
>  /*
> diff --git a/xen/arch/arm/mpu/arm32/mm.c b/xen/arch/arm/mpu/arm32/mm.c
> index a4673c351141..702bea804acd 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>
You don't seem to add anything from these headers.

>  
>  #define GENERATE_WRITE_PR_REG_CASE(num, pr)               \
>      case num:                                             \
> @@ -38,6 +40,11 @@
>          break;                                            \
>      }
>  
> +bool has_v8r_vmsa_support(void)
> +{
> +    return false;
> +}
... and then you could get rid of this stub
> +
>  /*
>   * 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..b8abcc6f7bc6 100644
> --- a/xen/arch/arm/mpu/arm64/mm.c
> +++ b/xen/arch/arm/mpu/arm64/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>
You don't seem to add anything from these headers.

~Michal