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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
> 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.

> +#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)
>
> +/* Access permission attributes. */
> +/* Read/Write at EL2, No Access at EL1/EL0. */
> +#define AP_RW_EL2 0x0
> +/* Read/Write at EL2/EL1/EL0 all levels. */
> +#define AP_RW_ALL 0x1
> +/* Read-only at EL2, No Access at EL1/EL0. */
> +#define AP_RO_EL2 0x2
> +/* Read-only at EL2/EL1/EL0 all levels. */
> +#define AP_RO_ALL 0x3
> +
> +/*
> + * Excute never.
> + * Stage 1 EL2 translation regime.
> + * XN[1] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + * Stage 2 EL1/EL0 translation regime.
> + * XN[0] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + */
> +#define XN_DISABLED    0x0
> +#define XN_P2M_ENABLED 0x1
> +#define XN_ENABLED     0x2
> +
>   #ifndef __ASSEMBLY__
>
> +#define INVALID_REGION_IDX 0xff
> +
>   /* Protection Region Base Address Register */
>   typedef union {
>       struct __packed {
> @@ -54,6 +83,41 @@ typedef struct {
>       prlar_t prlar;
>   } pr_t;
>
> +/* Access to set base address of MPU protection region(pr_t). */
> +static inline void pr_set_base(pr_t *pr, paddr_t base)
> +{
> +    pr->prbar.reg.base = (base >> MPU_REGION_SHIFT);
> +}
> +
> +/* Access to set limit address of MPU protection region(pr_t). */
> +static inline void pr_set_limit(pr_t *pr, paddr_t limit)
> +{
> +    pr->prlar.reg.limit = (limit >> MPU_REGION_SHIFT);
> +}
> +
> +/*
> + * Access to get base address of MPU protection region(pr_t).
> + * The base address shall be zero extended.
> + */
> +static inline paddr_t pr_get_base(pr_t *pr)
> +{
> +    return (paddr_t)(pr->prbar.reg.base << MPU_REGION_SHIFT);
> +}
> +
> +/*
> + * Access to get limit address of MPU protection region(pr_t).
> + * The limit address shall be concatenated with 0x3f.
> + */
> +static inline paddr_t pr_get_limit(pr_t *pr)
> +{
> +    return (paddr_t)((pr->prlar.reg.limit << MPU_REGION_SHIFT) | 
> ~MPU_REGION_MASK);
> +}
> +
> +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.
- Ayan