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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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? 

>> 
>> 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.

> 
>> +
>> +    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:

---
>> 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;
>> +}
---

So I thought this was a breaking change on it’s own, regarless of checking that 
the
padding is zero.

Let me know what you think, I would drop the explicit padding and version bump 
if
you say that adding v8r_el1_msa is not breaking the interface.

> 
>> };
>> #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
> 

Cheers,
Luca