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