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

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

On 29-Apr-26 14:46, Luca Fancellu wrote:
> Hi Michal,
> 
> thanks for your review,
> 
>>>
>>> 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"
> 
> ok
> 
>>
>>> +
>>> +    - "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".
> 
> Ok
> 
>>
>>> +
>>> +    - "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.
> 
> sounds good
> 
>>
>>> +
>>>     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.
> 
> are you suggesting dropping the error or modifying the message? 
modifying the message

> 
>>>
>>> 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.
> 
> ok
> 
>>
>>> +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.
> 
> Please correct me if I’m wrong, if I move the checks in 
> arch_dt_v8r_el1_msa_parse() they will
> be handled only on dom0less domain creation only.
> If I have it in the current way it will be handled by dom0less and XL 
> toolstack creation.
> I understand that for now the toolstack path is not implemented, but it felt 
> better to do this now
> rather than change it later.
> 
> Let me know what you think.
MPU == everything static, so I'm not sure how dynamic domain creation via libxl
would come into play here.

> 
>>
>>> +
>>> +    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.
> 
> I’ve bumped because if an older toolstack is passing something that is not 
> zero
> in the v8r_el1_msa we would return an error and stop the domain creation by 
> the
> above:
I guess we won't have this issue if we drop arch_set_v8r_el1_msa as suggested 
above.

~Michal