Re: [PATCH v3 1/7] arm/mpu: Introduce MPU memory region map structure

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

> On 14 Apr 2025, at 11:17, Orzel, Michal <michal.orzel@xxxxxxx> wrote:
> 
> 
> 
> On 11/04/2025 16:56, Luca Fancellu wrote:
>> From: Penny Zheng <Penny.Zheng@xxxxxxx>
>> 
>> Introduce pr_t typedef which is a structure having the prbar
>> and prlar members, each being structured as the registers of
>> the aarch64 armv8-r architecture.
>> 
>> Introduce the array 'xen_mpumap' that will store a view of
>> the content of the MPU regions.
>> 
>> Introduce MAX_MPU_REGIONS macro that uses the value of
>> NUM_MPU_REGIONS_MASK just for clarity, because using the
>> latter as number of elements of the xen_mpumap array might
>> be misleading.
> What should be the size of this array? I thought NUM_MPU_REGIONS indicates how
> many regions there can be (i.e. 256) and this should be the size. Yet you use
> MASK for size which is odd.

So the maximum number of regions for aarch64 armv8-r are 255, MPUIR_EL2.REGION 
is an
8 bit field advertising the number of region supported.

Is it better if I use just the below?

#define MAX_MPU_REGIONS 255

> 
>> 
>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> ---
>> xen/arch/arm/include/asm/arm64/mpu.h | 44 ++++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/mpu.h       |  5 ++++
>> xen/arch/arm/mpu/mm.c                |  4 +++
>> 3 files changed, 53 insertions(+)
>> create mode 100644 xen/arch/arm/include/asm/arm64/mpu.h
>> 
>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
>> b/xen/arch/arm/include/asm/arm64/mpu.h
>> new file mode 100644
>> index 000000000000..4d2bd7d7877f
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
> NIT: Do we really see the benefit in having such generic comments? What if you
> add a prototype of some function here. Will it fit into a definition scope?

I can remove the comment, but I would say that if I put some function prototype 
here
it should be related to arm64, being this file under arm64.

> 
>> + */
>> +
>> +#ifndef __ARM_ARM64_MPU_H__
>> +#define __ARM_ARM64_MPU_H__
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +/* Protection Region Base Address Register */
>> +typedef union {
>> +    struct __packed {
>> +        unsigned long xn:2;       /* Execute-Never */
>> +        unsigned long ap:2;       /* Acess Permission */
> s/Acess/Access/
> 
>> +        unsigned long sh:2;       /* Sharebility */
> s/Sharebility/Shareability/
> 
>> +        unsigned long base:46;    /* Base Address */
>> +        unsigned long pad:12;
> If you describe the register 1:1, why "pad" and not "res" or "res0"?
> 
>> +    } reg;
>> +    uint64_t bits;
>> +} prbar_t;
>> +
>> +/* Protection Region Limit Address Register */
>> +typedef union {
>> +    struct __packed {
>> +        unsigned long en:1;     /* Region enable */
>> +        unsigned long ai:3;     /* Memory Attribute Index */
>> +        unsigned long ns:1;     /* Not-Secure */
>> +        unsigned long res:1;    /* Reserved 0 by hardware */
> res0 /* RES0 */
> 
>> +        unsigned long limit:46; /* Limit Address */
>> +        unsigned long pad:12;
> res1 /* RES0 */
> 
>> +    } reg;
>> +    uint64_t bits;
>> +} prlar_t;
>> +
>> +/* MPU Protection Region */
>> +typedef struct {
>> +    prbar_t prbar;
>> +    prlar_t prlar;
>> +} pr_t;
>> +
>> +#endif /* __ASSEMBLY__ */
>> +
>> +#endif /* __ARM_ARM64_MPU_H__ */
>> \ No newline at end of file
> Please add a new line at the end
> 
> Also, EMACS comment is missing.

Thanks I will fix all these findings

Cheers,
Luca