| Message ID | 20250411145655.140667-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | First chunk for Arm R82 and MPU support | expand |
On 11/04/2025 16:56, Luca Fancellu wrote: > From: Penny Zheng <Penny.Zheng@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.zheng@arm.com> > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Luca Fancellu <luca.fancellu@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
Hi Michal, > On 14 Apr 2025, at 11:17, Orzel, Michal <michal.orzel@amd.com> wrote: > > > > On 11/04/2025 16:56, Luca Fancellu wrote: >> From: Penny Zheng <Penny.Zheng@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. 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. Is it better if I use just the below? #define MAX_MPU_REGIONS 255 > >> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> Signed-off-by: Wei Chen <wei.chen@arm.com> >> Signed-off-by: Luca Fancellu <luca.fancellu@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? 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. > >> + */ >> + >> +#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. Thanks I will fix all these findings Cheers, Luca
On 14/04/2025 16:50, Luca Fancellu wrote: > Hi Michal, > >> On 14 Apr 2025, at 11:17, Orzel, Michal <michal.orzel@amd.com> wrote: >> >> >> >> On 11/04/2025 16:56, Luca Fancellu wrote: >>> From: Penny Zheng <Penny.Zheng@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. > > 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@arm.com> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> Signed-off-by: Luca Fancellu <luca.fancellu@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? > > 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
Hi Michal, > On 14 Apr 2025, at 16:01, Orzel, Michal <Michal.Orzel@amd.com> wrote: > > > > On 14/04/2025 16:50, Luca Fancellu wrote: >> Hi Michal, >> >>> On 14 Apr 2025, at 11:17, Orzel, Michal <michal.orzel@amd.com> wrote: >>> >>> >>> >>> On 11/04/2025 16:56, Luca Fancellu wrote: >>>> From: Penny Zheng <Penny.Zheng@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. >> >> 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. I know, NUM_MPU_REGIONS should have a different name as it’s a bit misleading, ok I’ll name the macro I use here as MAX_MPU_REGION_NR. Cheers, Luca
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. + */ + +#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 */ + unsigned long sh:2; /* Sharebility */ + unsigned long base:46; /* Base Address */ + unsigned long pad:12; + } 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 */ + unsigned long limit:46; /* Limit Address */ + unsigned long pad:12; + } 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 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) { /*