Hi Michal,
> On 14 Apr 2025, at 11:17, Orzel, Michal <michal.or...@amd.com> wrote:
>
>
>
> On 11/04/2025 16:56, Luca Fancellu wrote:
>> From: Penny Zheng <penny.zh...@arm.com>
>>
>> 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.zh...@arm.com>
>> Signed-off-by: Wei Chen <wei.c...@arm.com>
>> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
>> ---
>> 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
