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

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

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.

> 
> 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?

> + */
> +
> +#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.

> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index d4ec4248b62b..e148c705b82c 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -6,6 +6,10 @@
>  #ifndef __ARM_MPU_H__
>  #define __ARM_MPU_H__
>  
> +#if defined(CONFIG_ARM_64)
> +# include <asm/arm64/mpu.h>
> +#endif
> +
>  #define MPU_REGION_SHIFT  6
>  #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
>  #define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
> @@ -13,6 +17,7 @@
>  #define NUM_MPU_REGIONS_SHIFT   8
>  #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
> +#define MAX_MPU_REGIONS         NUM_MPU_REGIONS_MASK
>  
>  #endif /* __ARM_MPU_H__ */
>  
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 07c8959f4ee9..f83ce04fef8a 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -7,9 +7,13 @@
>  #include <xen/mm.h>
>  #include <xen/sizes.h>
>  #include <xen/types.h>
> +#include <asm/mpu.h>
>  
>  struct page_info *frame_table;
>  
> +/* EL2 Xen MPU memory region mapping table. */
> +pr_t xen_mpumap[MAX_MPU_REGIONS];
> +
>  static void __init __maybe_unused build_assertions(void)
>  {
>      /*

~Michal