Re: [PATCH v3 30/52] xen/mpu: populate a new region in Xen MPU mapping table

[Penny Zheng <penny.zheng@arm.com>]

Hi Ayan

On 2023/6/28 18:08, Ayan Kumar Halder wrote:
> (Forgot to cc)
> 
> On 28/06/2023 11:05, Ayan Kumar Halder wrote:
>> Hi Penny,
>>
>> On 26/06/2023 04:34, Penny Zheng wrote:
>>> CAUTION: This message has originated from an External Source. Please 
>>> use proper judgment and caution when opening attachments, clicking 
>>> links, or responding to this email.
>>>
>>>
>>> The new helper xen_mpumap_update() is responsible for updating Xen 
>>> MPU memory
>>> mapping table(xen_mpumap), including creating a new entry, updating
>>> or destroying an existing one. It is equivalent to xen_pt_update in MMU.
>>> This commit only talks about populating a new entry in Xen MPU memory 
>>> mapping
>>> table(xen_mpumap). Others will be introduced in the following commits.
>>>
>>> When populating a new entry in Xen MPU memory mapping table(xen_mpumap),
>>> firstly, we shall check if requested address range [base, limit) is 
>>> mapped.
>>> If not, we shall find a free slot in xen_mpumap to insert, based on 
>>> bitmap
>>> xen_mpumap_mask, and use standard entry pr_of_xenaddr() to build up 
>>> MPU memory
>>> region structure(pr_t)
>>> In the last, we set memory attribute and permission based on variable 
>>> @flags.
>>>
>>> To summarize all region attributes in one variable @flags, layout of the
>>> flags is elaborated as follows:
>>> [0:2] Memory attribute Index
>>> [3:4] Execute Never
>>> [5:6] Access Permission
>>> [7]   Region Present
>>> Also, we provide a set of definitions(REGION_HYPERVISOR_RW, etc) that 
>>> combine
>>> the memory attribute and permission for common combinations.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>> ---
>>> v3:
>>> - implement pr_set_base/pr_set_limit/region_is_valid using static
>>> inline.
>>> - define index uint8_t to limit its size
>>> - stay the same major entry map_pages_to_xen, then go different path
>>> in different context(xen_pt_update in MMU, and xen_mpumap_update in MPU)
>>> ---
>>>   xen/arch/arm/include/asm/arm64/mpu.h |  64 +++++++
>>>   xen/arch/arm/include/asm/mm.h        |   3 +
>>>   xen/arch/arm/include/asm/mpu/mm.h    |  16 ++
>>>   xen/arch/arm/include/asm/page.h      |  22 +++
>>>   xen/arch/arm/mm.c                    |  20 +++
>>>   xen/arch/arm/mmu/mm.c                |   9 +-
>>>   xen/arch/arm/mpu/mm.c                | 255 +++++++++++++++++++++++++++
>>>   7 files changed, 381 insertions(+), 8 deletions(-)
>>>   create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
>>>
>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
>>> b/xen/arch/arm/include/asm/arm64/mpu.h
>>> index 407fec66c9..a6b07bab02 100644
>>> --- a/xen/arch/arm/include/asm/arm64/mpu.h
>>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
>>> @@ -6,6 +6,10 @@
>>>   #ifndef __ARM64_MPU_H__
>>>   #define __ARM64_MPU_H__
>>>
>>> +#define MPU_REGION_SHIFT  6
>>
>> You are using this macro in patch 24/52 and you are defining it here.
>>
>> Please consider moving the definition to patch 24/52.
>>

Thanks for pointing out. I'll move.

>>> +#define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
>>> +#define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
>>> +
>>>   /*
>>>    * MPUIR_EL2.Region identifies the number of regions supported by 
>>> the EL2 MPU.
>>>    * It is a 8-bit field, so 255 MPU memory regions at most.
>>> @@ -21,8 +25,33 @@
>>>   #define REGION_UART_SEL            0x07
>>>   #define MPUIR_REGION_MASK          ((_AC(1, UL) << 8) - 1)
>>>
...
>>> +static inline bool region_is_valid(pr_t *pr)
>>> +{
>>> +    return pr->prlar.reg.en;
>>> +}
>>
>> A lot of these macros and inlines can be reused on arm32 as well. I 
>> have split them as follows :-
>>
>> Refer 
>> https://github.com/Xilinx/xen/blob/xlnx_rebase_4.17/xen/arch/arm_mpu/include/asm/armv8r/mpu.h for the common definitions.
>>
>> Refer 
>> https://github.com/Xilinx/xen/blob/xlnx_rebase_4.17/xen/arch/arm_mpu/include/asm/armv8r/arm64/mpu.h for the 64 bit specific definitions
>>
>> Refer 
>> https://github.com/Xilinx/xen/blob/xlnx_rebase_4.17/xen/arch/arm_mpu/include/asm/armv8r/arm32/mpu.h for the 32 bit specific definitions (This I can add later as part of R52 port).
>>
>> Please consider splitting the definitions so that R52 can reuse the 
>> common ones.
>>

Sure. I'll try to split.

>> - Ayan
>>
>>