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

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

On 14/04/2025 16:50, Luca Fancellu wrote:
> 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.
So there can be max 255 regions. Ok.

> 
> Is it better if I use just the below?
> 
> #define MAX_MPU_REGIONS 255
If there are 255 regions, what NUM_MPU_REGIONS macro is for which stores 256?
These two macros confuse me. Or is it that by your macro you want to denote the
max region number? In that case, the macro should be named MAX_MPU_REGION_NR or
alike.

> 
>>
>>>
>>> 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.
Sorry, I don't see why you mention arm64 here. It is under arm64 directory, so
it's clear. I was referring to the word "definition".

~Michal