xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++ xen/arch/arm/include/asm/mpu.h | 4 + xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3 HPRBAR<n>,
E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
Introduce pr_t typedef which is a structure having the prbar and prlar members,
each being structured as the registers of the AArch32-V8R architecture.
This is the arm32 equivalent of
"arm/mpu: Introduce MPU memory region map structure".
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
This patch should be applied after
"[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to enable
compilation for AArch32.
xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++
xen/arch/arm/include/asm/mpu.h | 4 +
xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
3 files changed, 198 insertions(+)
create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
new file mode 100644
index 0000000000..4aabd93479
--- /dev/null
+++ b/xen/arch/arm/include/asm/arm32/mpu.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * mpu.h: Arm Memory Protection Unit definitions for aarch64.
+ */
+
+#ifndef __ARM_ARM32_MPU_H
+#define __ARM_ARM32_MPU_H
+
+#define XN_EL2_ENABLED 0x1
+
+#ifndef __ASSEMBLY__
+
+/* Hypervisor Protection Region Base Address Register */
+typedef union {
+ struct {
+ unsigned int xn:1; /* Execute-Never */
+ unsigned int ap:2; /* Acess Permission */
+ unsigned int sh:2; /* Sharebility */
+ unsigned int res0:1; /* Reserved as 0 */
+ unsigned int base:26; /* Base Address */
+ } reg;
+ uint32_t bits;
+} prbar_t;
+
+/* Hypervisor Protection Region Limit Address Register */
+typedef union {
+ struct {
+ unsigned int en:1; /* Region enable */
+ unsigned int ai:3; /* Memory Attribute Index */
+ /*
+ * There is no actual ns bit in hardware. It is used here for
+ * compatibility with Armr64 code. Thus, we are reusing a res0 bit for ns.
+ */
+ unsigned int ns:1; /* Reserved 0 by hardware */
+ unsigned int res0:1; /* Reserved 0 by hardware */
+ unsigned int limit:26; /* Limit Address */
+ } reg;
+ uint32_t bits;
+} prlar_t;
+
+/* Protection Region */
+typedef struct {
+ prbar_t prbar;
+ prlar_t prlar;
+ uint64_t p2m_type; /* Used to store p2m types. */
+} pr_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __ARM_ARM32_MPU_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index 77d0566f97..67127149c0 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -8,8 +8,12 @@
#if defined(CONFIG_ARM_64)
# include <asm/arm64/mpu.h>
+#elif defined(CONFIG_ARM_32)
+# include <asm/arm32/mpu.h>
#endif
+#define PRENR_MASK GENMASK(31, 0)
+
#define MPU_REGION_SHIFT 6
#define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
#define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
index d5cd0e04d5..7cf52aa09a 100644
--- a/xen/arch/arm/include/asm/mpu/cpregs.h
+++ b/xen/arch/arm/include/asm/mpu/cpregs.h
@@ -6,18 +6,153 @@
/* CP15 CR0: MPU Type Register */
#define HMPUIR p15,4,c0,c0,4
+/* CP15 CR6: Protection Region Enable Register */
+#define HPRENR p15,4,c6,c1,1
+
/* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
#define HPRSELR p15,4,c6,c2,1
#define HPRBAR p15,4,c6,c3,0
#define HPRLAR p15,4,c6,c8,1
+/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
+#define HPRBAR0 p15,4,c6,c8,0
+#define HPRLAR0 p15,4,c6,c8,1
+#define HPRBAR1 p15,4,c6,c8,4
+#define HPRLAR1 p15,4,c6,c8,5
+#define HPRBAR2 p15,4,c6,c9,0
+#define HPRLAR2 p15,4,c6,c9,1
+#define HPRBAR3 p15,4,c6,c9,4
+#define HPRLAR3 p15,4,c6,c9,5
+#define HPRBAR4 p15,4,c6,c10,0
+#define HPRLAR4 p15,4,c6,c10,1
+#define HPRBAR5 p15,4,c6,c10,4
+#define HPRLAR5 p15,4,c6,c10,5
+#define HPRBAR6 p15,4,c6,c11,0
+#define HPRLAR6 p15,4,c6,c11,1
+#define HPRBAR7 p15,4,c6,c11,4
+#define HPRLAR7 p15,4,c6,c11,5
+#define HPRBAR8 p15,4,c6,c12,0
+#define HPRLAR8 p15,4,c6,c12,1
+#define HPRBAR9 p15,4,c6,c12,4
+#define HPRLAR9 p15,4,c6,c12,5
+#define HPRBAR10 p15,4,c6,c13,0
+#define HPRLAR10 p15,4,c6,c13,1
+#define HPRBAR11 p15,4,c6,c13,4
+#define HPRLAR11 p15,4,c6,c13,5
+#define HPRBAR12 p15,4,c6,c14,0
+#define HPRLAR12 p15,4,c6,c14,1
+#define HPRBAR13 p15,4,c6,c14,4
+#define HPRLAR13 p15,4,c6,c14,5
+#define HPRBAR14 p15,4,c6,c15,0
+#define HPRLAR14 p15,4,c6,c15,1
+#define HPRBAR15 p15,4,c6,c15,4
+#define HPRLAR15 p15,4,c6,c15,5
+#define HPRBAR16 p15,5,c6,c8,0
+#define HPRLAR16 p15,5,c6,c8,1
+#define HPRBAR17 p15,5,c6,c8,4
+#define HPRLAR17 p15,5,c6,c8,5
+#define HPRBAR18 p15,5,c6,c9,0
+#define HPRLAR18 p15,5,c6,c9,1
+#define HPRBAR19 p15,5,c6,c9,4
+#define HPRLAR19 p15,5,c6,c9,5
+#define HPRBAR20 p15,5,c6,c10,0
+#define HPRLAR20 p15,5,c6,c10,1
+#define HPRBAR21 p15,5,c6,c10,4
+#define HPRLAR21 p15,5,c6,c10,5
+#define HPRBAR22 p15,5,c6,c11,0
+#define HPRLAR22 p15,5,c6,c11,1
+#define HPRBAR23 p15,5,c6,c11,4
+#define HPRLAR23 p15,5,c6,c11,5
+#define HPRBAR24 p15,5,c6,c12,0
+#define HPRLAR24 p15,5,c6,c12,1
+#define HPRBAR25 p15,5,c6,c12,4
+#define HPRLAR25 p15,5,c6,c12,5
+#define HPRBAR26 p15,5,c6,c13,0
+#define HPRLAR26 p15,5,c6,c13,1
+#define HPRBAR27 p15,5,c6,c13,4
+#define HPRLAR27 p15,5,c6,c13,5
+#define HPRBAR28 p15,5,c6,c14,0
+#define HPRLAR28 p15,5,c6,c14,1
+#define HPRBAR29 p15,5,c6,c14,4
+#define HPRLAR29 p15,5,c6,c14,5
+#define HPRBAR30 p15,5,c6,c15,0
+#define HPRLAR30 p15,5,c6,c15,1
+#define HPRBAR31 p15,5,c6,c15,4
+#define HPRLAR31 p15,5,c6,c15,5
+
/* Aliases of AArch64 names for use in common code */
#ifdef CONFIG_ARM_32
/* Alphabetically... */
#define MPUIR_EL2 HMPUIR
#define PRBAR_EL2 HPRBAR
+#define PRBAR0_EL2 HPRBAR0
+#define PRBAR1_EL2 HPRBAR1
+#define PRBAR2_EL2 HPRBAR2
+#define PRBAR3_EL2 HPRBAR3
+#define PRBAR4_EL2 HPRBAR4
+#define PRBAR5_EL2 HPRBAR5
+#define PRBAR6_EL2 HPRBAR6
+#define PRBAR7_EL2 HPRBAR7
+#define PRBAR8_EL2 HPRBAR8
+#define PRBAR9_EL2 HPRBAR9
+#define PRBAR10_EL2 HPRBAR10
+#define PRBAR11_EL2 HPRBAR11
+#define PRBAR12_EL2 HPRBAR12
+#define PRBAR13_EL2 HPRBAR13
+#define PRBAR14_EL2 HPRBAR14
+#define PRBAR15_EL2 HPRBAR15
+#define PRBAR16_EL2 HPRBAR16
+#define PRBAR17_EL2 HPRBAR17
+#define PRBAR18_EL2 HPRBAR18
+#define PRBAR19_EL2 HPRBAR19
+#define PRBAR20_EL2 HPRBAR20
+#define PRBAR21_EL2 HPRBAR21
+#define PRBAR22_EL2 HPRBAR22
+#define PRBAR23_EL2 HPRBAR23
+#define PRBAR24_EL2 HPRBAR24
+#define PRBAR25_EL2 HPRBAR25
+#define PRBAR26_EL2 HPRBAR26
+#define PRBAR27_EL2 HPRBAR27
+#define PRBAR28_EL2 HPRBAR28
+#define PRBAR29_EL2 HPRBAR29
+#define PRBAR30_EL2 HPRBAR30
+#define PRBAR31_EL2 HPRBAR31
+#define PRENR_EL2 HPRENR
#define PRLAR_EL2 HPRLAR
+#define PRLAR0_EL2 HPRLAR0
+#define PRLAR1_EL2 HPRLAR1
+#define PRLAR2_EL2 HPRLAR2
+#define PRLAR3_EL2 HPRLAR3
+#define PRLAR4_EL2 HPRLAR4
+#define PRLAR5_EL2 HPRLAR5
+#define PRLAR6_EL2 HPRLAR6
+#define PRLAR7_EL2 HPRLAR7
+#define PRLAR8_EL2 HPRLAR8
+#define PRLAR9_EL2 HPRLAR9
+#define PRLAR10_EL2 HPRLAR10
+#define PRLAR11_EL2 HPRLAR11
+#define PRLAR12_EL2 HPRLAR12
+#define PRLAR13_EL2 HPRLAR13
+#define PRLAR14_EL2 HPRLAR14
+#define PRLAR15_EL2 HPRLAR15
+#define PRLAR16_EL2 HPRLAR16
+#define PRLAR17_EL2 HPRLAR17
+#define PRLAR18_EL2 HPRLAR18
+#define PRLAR19_EL2 HPRLAR19
+#define PRLAR20_EL2 HPRLAR20
+#define PRLAR21_EL2 HPRLAR21
+#define PRLAR22_EL2 HPRLAR22
+#define PRLAR23_EL2 HPRLAR23
+#define PRLAR24_EL2 HPRLAR24
+#define PRLAR25_EL2 HPRLAR25
+#define PRLAR26_EL2 HPRLAR26
+#define PRLAR27_EL2 HPRLAR27
+#define PRLAR28_EL2 HPRLAR28
+#define PRLAR29_EL2 HPRLAR29
+#define PRLAR30_EL2 HPRLAR30
+#define PRLAR31_EL2 HPRLAR31
#define PRSELR_EL2 HPRSELR
+
#endif /* CONFIG_ARM_32 */
#endif /* __ARM_MPU_CPREGS_H */
--
2.25.1Hi Ayan,
On 18/04/2025 00:55, Ayan Kumar Halder wrote:
> Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
> The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3 HPRBAR<n>,
> E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
>
> Introduce pr_t typedef which is a structure having the prbar and prlar members,
> each being structured as the registers of the AArch32-V8R architecture.
> This is the arm32 equivalent of
> "arm/mpu: Introduce MPU memory region map structure".
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> This patch should be applied after
> "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to enable
> compilation for AArch32.
>
> xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++
> xen/arch/arm/include/asm/mpu.h | 4 +
> xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
> 3 files changed, 198 insertions(+)
> create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
> new file mode 100644
> index 0000000000..4aabd93479
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
> + */
> +
> +#ifndef __ARM_ARM32_MPU_H
> +#define __ARM_ARM32_MPU_H
> +
> +#define XN_EL2_ENABLED 0x1
I understand the define is introduced in Luca's patch, but this a bit
non-descriptive... Can we find a better name? Maybe by adding the name
of the register and some documentation?
> +
> +#ifndef __ASSEMBLY__
> +
> +/* Hypervisor Protection Region Base Address Register */
> +typedef union {
> + struct {
> + unsigned int xn:1; /* Execute-Never */
> + unsigned int ap:2; /* Acess Permission */
> + unsigned int sh:2; /* Sharebility */
> + unsigned int res0:1; /* Reserved as 0 */
> + unsigned int base:26; /* Base Address */
> + } reg;
> + uint32_t bits;
> +} prbar_t;
> +
> +/* Hypervisor Protection Region Limit Address Register */
> +typedef union {
> + struct {
> + unsigned int en:1; /* Region enable */
> + unsigned int ai:3; /* Memory Attribute Index */
> + /*
> + * There is no actual ns bit in hardware. It is used here for
> + * compatibility with Armr64 code. Thus, we are reusing a res0 bit for ns.
typo: Arm64.
> + */
Hmmmm, this would mean someone may mistakenly set the bit to 0 by
mistake. If the field is always meant to be 0 on arm64, then I would
consider to name is res0 on arm64 with an explanation.
This would make clear the bit is not supposed to have a value other than 0.
> + unsigned int ns:1; /* Reserved 0 by hardware */
> + unsigned int res0:1; /* Reserved 0 by hardware */
> + unsigned int limit:26; /* Limit Address */
NIT: Can we align the comments?
> + } reg;
> + uint32_t bits;
> +} prlar_t;
> +
> +/* Protection Region */
> +typedef struct {
> + prbar_t prbar;
> + prlar_t prlar;
> + uint64_t p2m_type; /* Used to store p2m types. */
> +} pr_t;
This looks to be common with arm64. If so, I would prefer if the
structure is in a common header.
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* __ARM_ARM32_MPU_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 77d0566f97..67127149c0 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -8,8 +8,12 @@
>
> #if defined(CONFIG_ARM_64)
> # include <asm/arm64/mpu.h>
> +#elif defined(CONFIG_ARM_32)
> +# include <asm/arm32/mpu.h>
> #endif
>
> +#define PRENR_MASK GENMASK(31, 0)
> +
> #define MPU_REGION_SHIFT 6
> #define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
> #define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
> index d5cd0e04d5..7cf52aa09a 100644
> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> @@ -6,18 +6,153 @@
> /* CP15 CR0: MPU Type Register */
> #define HMPUIR p15,4,c0,c0,4
>
> +/* CP15 CR6: Protection Region Enable Register */
> +#define HPRENR p15,4,c6,c1,1
> +
> /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
> #define HPRSELR p15,4,c6,c2,1
> #define HPRBAR p15,4,c6,c3,0
> #define HPRLAR p15,4,c6,c8,1
>
> +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
> +#define HPRBAR0 p15,4,c6,c8,0
> +#define HPRLAR0 p15,4,c6,c8,1
> +#define HPRBAR1 p15,4,c6,c8,4
> +#define HPRLAR1 p15,4,c6,c8,5
> +#define HPRBAR2 p15,4,c6,c9,0
> +#define HPRLAR2 p15,4,c6,c9,1
> +#define HPRBAR3 p15,4,c6,c9,4
> +#define HPRLAR3 p15,4,c6,c9,5
> +#define HPRBAR4 p15,4,c6,c10,0
> +#define HPRLAR4 p15,4,c6,c10,1
> +#define HPRBAR5 p15,4,c6,c10,4
> +#define HPRLAR5 p15,4,c6,c10,5
> +#define HPRBAR6 p15,4,c6,c11,0
> +#define HPRLAR6 p15,4,c6,c11,1
> +#define HPRBAR7 p15,4,c6,c11,4
> +#define HPRLAR7 p15,4,c6,c11,5
> +#define HPRBAR8 p15,4,c6,c12,0
> +#define HPRLAR8 p15,4,c6,c12,1
> +#define HPRBAR9 p15,4,c6,c12,4
> +#define HPRLAR9 p15,4,c6,c12,5
> +#define HPRBAR10 p15,4,c6,c13,0
> +#define HPRLAR10 p15,4,c6,c13,1
> +#define HPRBAR11 p15,4,c6,c13,4
> +#define HPRLAR11 p15,4,c6,c13,5
> +#define HPRBAR12 p15,4,c6,c14,0
> +#define HPRLAR12 p15,4,c6,c14,1
> +#define HPRBAR13 p15,4,c6,c14,4
> +#define HPRLAR13 p15,4,c6,c14,5
> +#define HPRBAR14 p15,4,c6,c15,0
> +#define HPRLAR14 p15,4,c6,c15,1
> +#define HPRBAR15 p15,4,c6,c15,4
> +#define HPRLAR15 p15,4,c6,c15,5
> +#define HPRBAR16 p15,5,c6,c8,0
> +#define HPRLAR16 p15,5,c6,c8,1
> +#define HPRBAR17 p15,5,c6,c8,4
> +#define HPRLAR17 p15,5,c6,c8,5
> +#define HPRBAR18 p15,5,c6,c9,0
> +#define HPRLAR18 p15,5,c6,c9,1
> +#define HPRBAR19 p15,5,c6,c9,4
> +#define HPRLAR19 p15,5,c6,c9,5
> +#define HPRBAR20 p15,5,c6,c10,0
> +#define HPRLAR20 p15,5,c6,c10,1
> +#define HPRBAR21 p15,5,c6,c10,4
> +#define HPRLAR21 p15,5,c6,c10,5
> +#define HPRBAR22 p15,5,c6,c11,0
> +#define HPRLAR22 p15,5,c6,c11,1
> +#define HPRBAR23 p15,5,c6,c11,4
> +#define HPRLAR23 p15,5,c6,c11,5
> +#define HPRBAR24 p15,5,c6,c12,0
> +#define HPRLAR24 p15,5,c6,c12,1
> +#define HPRBAR25 p15,5,c6,c12,4
> +#define HPRLAR25 p15,5,c6,c12,5
> +#define HPRBAR26 p15,5,c6,c13,0
> +#define HPRLAR26 p15,5,c6,c13,1
> +#define HPRBAR27 p15,5,c6,c13,4
> +#define HPRLAR27 p15,5,c6,c13,5
> +#define HPRBAR28 p15,5,c6,c14,0
> +#define HPRLAR28 p15,5,c6,c14,1
> +#define HPRBAR29 p15,5,c6,c14,4
> +#define HPRLAR29 p15,5,c6,c14,5
> +#define HPRBAR30 p15,5,c6,c15,0
> +#define HPRLAR30 p15,5,c6,c15,1
> +#define HPRBAR31 p15,5,c6,c15,4
> +#define HPRLAR31 p15,5,c6,c15,5
NIT: Is there any way we could generate the values using macros?
> +
> /* Aliases of AArch64 names for use in common code */
> #ifdef CONFIG_ARM_32
> /* Alphabetically... */
> #define MPUIR_EL2 HMPUIR
> #define PRBAR_EL2 HPRBAR
> +#define PRBAR0_EL2 HPRBAR0
AFAIU, the alias will be mainly used in the macro generate
the switch. Rather than open-coding all the PR*AR_EL2, can we
provide two macros PR{B, L}AR_N that will be implemented as
HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
> #define PRSELR_EL2 HPRSELR
> +
> #endif /* CONFIG_ARM_32 */
>
> #endif /* __ARM_MPU_CPREGS_H */
Cheers,
--
Julien Grall
Hi Julien,
cc-ed Luca for feedback on specific points.
On 18/04/2025 05:54, Julien Grall wrote:
> Hi Ayan,
>
> On 18/04/2025 00:55, Ayan Kumar Halder wrote:
>> Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
>> The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3
>> HPRBAR<n>,
>> E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
>>
>> Introduce pr_t typedef which is a structure having the prbar and
>> prlar members,
>> each being structured as the registers of the AArch32-V8R architecture.
>> This is the arm32 equivalent of
>> "arm/mpu: Introduce MPU memory region map structure".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> This patch should be applied after
>> "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to
>> enable
>> compilation for AArch32.
>>
>> xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++
>> xen/arch/arm/include/asm/mpu.h | 4 +
>> xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
>> 3 files changed, 198 insertions(+)
>> create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>>
>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h
>> b/xen/arch/arm/include/asm/arm32/mpu.h
>> new file mode 100644
>> index 0000000000..4aabd93479
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>> @@ -0,0 +1,59 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
>> + */
>> +
>> +#ifndef __ARM_ARM32_MPU_H
>> +#define __ARM_ARM32_MPU_H
>> +
>> +#define XN_EL2_ENABLED 0x1
>
> I understand the define is introduced in Luca's patch, but this a bit
> non-descriptive... Can we find a better name? Maybe by adding the name
> of the register and some documentation?
We can rename this as PRBAR_EL2_XN if this sounds better (cc @Luca) in
Luca's patch
The description can be
Refer ARM DDI 0568A.c ID110520 , E2.2.2
HPRBAR [0:1] Execute Never
>
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +/* Hypervisor Protection Region Base Address Register */
>> +typedef union {
>> + struct {
>> + unsigned int xn:1; /* Execute-Never */
>> + unsigned int ap:2; /* Acess Permission */
>> + unsigned int sh:2; /* Sharebility */
>> + unsigned int res0:1; /* Reserved as 0 */
>> + unsigned int base:26; /* Base Address */
>> + } reg;
>> + uint32_t bits;
>> +} prbar_t;
>> +
>> +/* Hypervisor Protection Region Limit Address Register */
>> +typedef union {
>> + struct {
>> + unsigned int en:1; /* Region enable */
>> + unsigned int ai:3; /* Memory Attribute Index */
>> + /*
>> + * There is no actual ns bit in hardware. It is used here for
>> + * compatibility with Armr64 code. Thus, we are reusing a
>> res0 bit for ns.
>
> typo: Arm64.
Ack
>
>> + */
>
> Hmmmm, this would mean someone may mistakenly set the bit to 0 by
> mistake. If the field is always meant to be 0 on arm64, then I would
> consider to name is res0 on arm64 with an explanation.
>
> This would make clear the bit is not supposed to have a value other
> than 0.
On Arm64, ns == 0 as it can only support secure mode.
So we can change this on Arm64 as well :-
unsigned int res0:2; /* ns == 0 as only secure mode is supported */
@Luca to clarify
>
>> + unsigned int ns:1; /* Reserved 0 by hardware */
>> + unsigned int res0:1; /* Reserved 0 by hardware */
Then, we can change this on Arm32 as well.
>> + unsigned int limit:26; /* Limit Address */
>
> NIT: Can we align the comments?
Ack.
>
>> + } reg;
>> + uint32_t bits;
>> +} prlar_t;
>> +
>> +/* Protection Region */
>> +typedef struct {
>> + prbar_t prbar;
>> + prlar_t prlar;
>> + uint64_t p2m_type; /* Used to store p2m types. */
>> +} pr_t;
>
> This looks to be common with arm64. If so, I would prefer if the
> structure is in a common header.
No, in arm64 this is
typedef struct {
prbar_t prbar;
prlar_t prlar;
} pr_t;
The reason being Arm64 uses unused bits (ie 'pad') to store the type.
>
>> +
>> +#endif /* __ASSEMBLY__ */
>> +
>> +#endif /* __ARM_ARM32_MPU_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/include/asm/mpu.h
>> b/xen/arch/arm/include/asm/mpu.h
>> index 77d0566f97..67127149c0 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -8,8 +8,12 @@
>> #if defined(CONFIG_ARM_64)
>> # include <asm/arm64/mpu.h>
>> +#elif defined(CONFIG_ARM_32)
>> +# include <asm/arm32/mpu.h>
>> #endif
>> +#define PRENR_MASK GENMASK(31, 0)
>> +
>> #define MPU_REGION_SHIFT 6
>> #define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
>> #define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
>> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h
>> b/xen/arch/arm/include/asm/mpu/cpregs.h
>> index d5cd0e04d5..7cf52aa09a 100644
>> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
>> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
>> @@ -6,18 +6,153 @@
>> /* CP15 CR0: MPU Type Register */
>> #define HMPUIR p15,4,c0,c0,4
>> +/* CP15 CR6: Protection Region Enable Register */
>> +#define HPRENR p15,4,c6,c1,1
>> +
>> /* CP15 CR6: MPU Protection Region Base/Limit/Select Address
>> Register */
>> #define HPRSELR p15,4,c6,c2,1
>> #define HPRBAR p15,4,c6,c3,0
>> #define HPRLAR p15,4,c6,c8,1
>> +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
>> +#define HPRBAR0 p15,4,c6,c8,0
>> +#define HPRLAR0 p15,4,c6,c8,1
>> +#define HPRBAR1 p15,4,c6,c8,4
>> +#define HPRLAR1 p15,4,c6,c8,5
>> +#define HPRBAR2 p15,4,c6,c9,0
>> +#define HPRLAR2 p15,4,c6,c9,1
>> +#define HPRBAR3 p15,4,c6,c9,4
>> +#define HPRLAR3 p15,4,c6,c9,5
>> +#define HPRBAR4 p15,4,c6,c10,0
>> +#define HPRLAR4 p15,4,c6,c10,1
>> +#define HPRBAR5 p15,4,c6,c10,4
>> +#define HPRLAR5 p15,4,c6,c10,5
>> +#define HPRBAR6 p15,4,c6,c11,0
>> +#define HPRLAR6 p15,4,c6,c11,1
>> +#define HPRBAR7 p15,4,c6,c11,4
>> +#define HPRLAR7 p15,4,c6,c11,5
>> +#define HPRBAR8 p15,4,c6,c12,0
>> +#define HPRLAR8 p15,4,c6,c12,1
>> +#define HPRBAR9 p15,4,c6,c12,4
>> +#define HPRLAR9 p15,4,c6,c12,5
>> +#define HPRBAR10 p15,4,c6,c13,0
>> +#define HPRLAR10 p15,4,c6,c13,1
>> +#define HPRBAR11 p15,4,c6,c13,4
>> +#define HPRLAR11 p15,4,c6,c13,5
>> +#define HPRBAR12 p15,4,c6,c14,0
>> +#define HPRLAR12 p15,4,c6,c14,1
>> +#define HPRBAR13 p15,4,c6,c14,4
>> +#define HPRLAR13 p15,4,c6,c14,5
>> +#define HPRBAR14 p15,4,c6,c15,0
>> +#define HPRLAR14 p15,4,c6,c15,1
>> +#define HPRBAR15 p15,4,c6,c15,4
>> +#define HPRLAR15 p15,4,c6,c15,5
>> +#define HPRBAR16 p15,5,c6,c8,0
>> +#define HPRLAR16 p15,5,c6,c8,1
>> +#define HPRBAR17 p15,5,c6,c8,4
>> +#define HPRLAR17 p15,5,c6,c8,5
>> +#define HPRBAR18 p15,5,c6,c9,0
>> +#define HPRLAR18 p15,5,c6,c9,1
>> +#define HPRBAR19 p15,5,c6,c9,4
>> +#define HPRLAR19 p15,5,c6,c9,5
>> +#define HPRBAR20 p15,5,c6,c10,0
>> +#define HPRLAR20 p15,5,c6,c10,1
>> +#define HPRBAR21 p15,5,c6,c10,4
>> +#define HPRLAR21 p15,5,c6,c10,5
>> +#define HPRBAR22 p15,5,c6,c11,0
>> +#define HPRLAR22 p15,5,c6,c11,1
>> +#define HPRBAR23 p15,5,c6,c11,4
>> +#define HPRLAR23 p15,5,c6,c11,5
>> +#define HPRBAR24 p15,5,c6,c12,0
>> +#define HPRLAR24 p15,5,c6,c12,1
>> +#define HPRBAR25 p15,5,c6,c12,4
>> +#define HPRLAR25 p15,5,c6,c12,5
>> +#define HPRBAR26 p15,5,c6,c13,0
>> +#define HPRLAR26 p15,5,c6,c13,1
>> +#define HPRBAR27 p15,5,c6,c13,4
>> +#define HPRLAR27 p15,5,c6,c13,5
>> +#define HPRBAR28 p15,5,c6,c14,0
>> +#define HPRLAR28 p15,5,c6,c14,1
>> +#define HPRBAR29 p15,5,c6,c14,4
>> +#define HPRLAR29 p15,5,c6,c14,5
>> +#define HPRBAR30 p15,5,c6,c15,0
>> +#define HPRLAR30 p15,5,c6,c15,1
>> +#define HPRBAR31 p15,5,c6,c15,4
>> +#define HPRLAR31 p15,5,c6,c15,5
>
> NIT: Is there any way we could generate the values using macros?
This looks tricky. So I will prefer to keep this as it is.
>
>> +
>> /* Aliases of AArch64 names for use in common code */
>> #ifdef CONFIG_ARM_32
>> /* Alphabetically... */
>> #define MPUIR_EL2 HMPUIR
>> #define PRBAR_EL2 HPRBAR
>> +#define PRBAR0_EL2 HPRBAR0
>
> AFAIU, the alias will be mainly used in the macro generate
> the switch. Rather than open-coding all the PR*AR_EL2, can we
> provide two macros PR{B, L}AR_N that will be implemented as
> HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
Yes , we can have
#define PR{B,L}AR_EL2_(n) HPR{B,L}AR##n for arm32
#define PR{B,L}AR_EL2_(n) PR{B,L}AR##n##_EL2
I will send a v2 with these changes. Please have a look at that.
- Ayan
>
>> #define PRSELR_EL2 HPRSELR
>> +
>> #endif /* CONFIG_ARM_32 */
>> #endif /* __ARM_MPU_CPREGS_H */
>
> Cheers,
>
Hi Ayan,
> On 25 Apr 2025, at 13:00, Ayan Kumar Halder <ayankuma@amd.com> wrote:
>
> Hi Julien,
>
> cc-ed Luca for feedback on specific points.
>
> On 18/04/2025 05:54, Julien Grall wrote:
>> Hi Ayan,
>>
>> On 18/04/2025 00:55, Ayan Kumar Halder wrote:
>>> Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
>>> The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3 HPRBAR<n>,
>>> E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
>>>
>>> Introduce pr_t typedef which is a structure having the prbar and prlar members,
>>> each being structured as the registers of the AArch32-V8R architecture.
>>> This is the arm32 equivalent of
>>> "arm/mpu: Introduce MPU memory region map structure".
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>> ---
>>> This patch should be applied after
>>> "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to enable
>>> compilation for AArch32.
>>>
>>> xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++
>>> xen/arch/arm/include/asm/mpu.h | 4 +
>>> xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
>>> 3 files changed, 198 insertions(+)
>>> create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>>>
>>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
>>> new file mode 100644
>>> index 0000000000..4aabd93479
>>> --- /dev/null
>>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>>> @@ -0,0 +1,59 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
>>> + */
>>> +
>>> +#ifndef __ARM_ARM32_MPU_H
>>> +#define __ARM_ARM32_MPU_H
>>> +
>>> +#define XN_EL2_ENABLED 0x1
>>
>> I understand the define is introduced in Luca's patch, but this a bit non-descriptive... Can we find a better name? Maybe by adding the name of the register and some documentation?
>
> We can rename this as PRBAR_EL2_XN if this sounds better (cc @Luca) in Luca's patch
what about PRBAR_EL2_XN_ENABLED? I can change it in my serie
>
> The description can be
>
> Refer ARM DDI 0568A.c ID110520 , E2.2.2
>
> HPRBAR [0:1] Execute Never
>
>>
>>> +
>>> +#ifndef __ASSEMBLY__
>>> +
>>> +/* Hypervisor Protection Region Base Address Register */
>>> +typedef union {
>>> + struct {
>>> + unsigned int xn:1; /* Execute-Never */
>>> + unsigned int ap:2; /* Acess Permission */
>>> + unsigned int sh:2; /* Sharebility */
>>> + unsigned int res0:1; /* Reserved as 0 */
>>> + unsigned int base:26; /* Base Address */
>>> + } reg;
>>> + uint32_t bits;
>>> +} prbar_t;
>>> +
>>> +/* Hypervisor Protection Region Limit Address Register */
>>> +typedef union {
>>> + struct {
>>> + unsigned int en:1; /* Region enable */
>>> + unsigned int ai:3; /* Memory Attribute Index */
>>> + /*
>>> + * There is no actual ns bit in hardware. It is used here for
>>> + * compatibility with Armr64 code. Thus, we are reusing a res0 bit for ns.
>>
>> typo: Arm64.
> Ack
>>
>>> + */
>>
>> Hmmmm, this would mean someone may mistakenly set the bit to 0 by mistake. If the field is always meant to be 0 on arm64, then I would consider to name is res0 on arm64 with an explanation.
>>
>> This would make clear the bit is not supposed to have a value other than 0.
>
> On Arm64, ns == 0 as it can only support secure mode.
>
> So we can change this on Arm64 as well :-
>
> unsigned int res0:2; /* ns == 0 as only secure mode is supported */
>
> @Luca to clarify
From the specifications: "Non-secure bit. Specifies whether the output address is in the Secure or Non-secure memory”, I’m not sure
that we should remove it from Arm64, so I don’t think you should have something only for compatibility, maybe the code accessing .ns
can be compiled out for Arm32 or we can have arch-specific implementation. I think you are referring to pr_of_xenaddr when you say
“compatibility with Arm64 code"
>
>>
>>> + unsigned int ns:1; /* Reserved 0 by hardware */
>>> + unsigned int res0:1; /* Reserved 0 by hardware */
> Then, we can change this on Arm32 as well.
>>> + unsigned int limit:26; /* Limit Address */
>>
>> NIT: Can we align the comments?
> Ack.
>>
>>> + } reg;
>>> + uint32_t bits;
>>> +} prlar_t;
>>> +
>>> +/* Protection Region */
>>> +typedef struct {
>>> + prbar_t prbar;
>>> + prlar_t prlar;
>>> + uint64_t p2m_type; /* Used to store p2m types. */
>>> +} pr_t;
>>
>> This looks to be common with arm64. If so, I would prefer if the structure is in a common header.
>
> No, in arm64 this is
>
> typedef struct {
> prbar_t prbar;
> prlar_t prlar;
> } pr_t;
>
> The reason being Arm64 uses unused bits (ie 'pad') to store the type.
>
>>
>>> +
>>> +#endif /* __ASSEMBLY__ */
>>> +
>>> +#endif /* __ARM_ARM32_MPU_H */
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>>> index 77d0566f97..67127149c0 100644
>>> --- a/xen/arch/arm/include/asm/mpu.h
>>> +++ b/xen/arch/arm/include/asm/mpu.h
>>> @@ -8,8 +8,12 @@
>>> #if defined(CONFIG_ARM_64)
>>> # include <asm/arm64/mpu.h>
>>> +#elif defined(CONFIG_ARM_32)
>>> +# include <asm/arm32/mpu.h>
>>> #endif
>>> +#define PRENR_MASK GENMASK(31, 0)
>>> +
>>> #define MPU_REGION_SHIFT 6
>>> #define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
>>> #define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
>>> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
>>> index d5cd0e04d5..7cf52aa09a 100644
>>> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
>>> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
>>> @@ -6,18 +6,153 @@
>>> /* CP15 CR0: MPU Type Register */
>>> #define HMPUIR p15,4,c0,c0,4
>>> +/* CP15 CR6: Protection Region Enable Register */
>>> +#define HPRENR p15,4,c6,c1,1
>>> +
>>> /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
>>> #define HPRSELR p15,4,c6,c2,1
>>> #define HPRBAR p15,4,c6,c3,0
>>> #define HPRLAR p15,4,c6,c8,1
>>> +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
>>> +#define HPRBAR0 p15,4,c6,c8,0
>>> +#define HPRLAR0 p15,4,c6,c8,1
>>> +#define HPRBAR1 p15,4,c6,c8,4
>>> +#define HPRLAR1 p15,4,c6,c8,5
>>> +#define HPRBAR2 p15,4,c6,c9,0
>>> +#define HPRLAR2 p15,4,c6,c9,1
>>> +#define HPRBAR3 p15,4,c6,c9,4
>>> +#define HPRLAR3 p15,4,c6,c9,5
>>> +#define HPRBAR4 p15,4,c6,c10,0
>>> +#define HPRLAR4 p15,4,c6,c10,1
>>> +#define HPRBAR5 p15,4,c6,c10,4
>>> +#define HPRLAR5 p15,4,c6,c10,5
>>> +#define HPRBAR6 p15,4,c6,c11,0
>>> +#define HPRLAR6 p15,4,c6,c11,1
>>> +#define HPRBAR7 p15,4,c6,c11,4
>>> +#define HPRLAR7 p15,4,c6,c11,5
>>> +#define HPRBAR8 p15,4,c6,c12,0
>>> +#define HPRLAR8 p15,4,c6,c12,1
>>> +#define HPRBAR9 p15,4,c6,c12,4
>>> +#define HPRLAR9 p15,4,c6,c12,5
>>> +#define HPRBAR10 p15,4,c6,c13,0
>>> +#define HPRLAR10 p15,4,c6,c13,1
>>> +#define HPRBAR11 p15,4,c6,c13,4
>>> +#define HPRLAR11 p15,4,c6,c13,5
>>> +#define HPRBAR12 p15,4,c6,c14,0
>>> +#define HPRLAR12 p15,4,c6,c14,1
>>> +#define HPRBAR13 p15,4,c6,c14,4
>>> +#define HPRLAR13 p15,4,c6,c14,5
>>> +#define HPRBAR14 p15,4,c6,c15,0
>>> +#define HPRLAR14 p15,4,c6,c15,1
>>> +#define HPRBAR15 p15,4,c6,c15,4
>>> +#define HPRLAR15 p15,4,c6,c15,5
>>> +#define HPRBAR16 p15,5,c6,c8,0
>>> +#define HPRLAR16 p15,5,c6,c8,1
>>> +#define HPRBAR17 p15,5,c6,c8,4
>>> +#define HPRLAR17 p15,5,c6,c8,5
>>> +#define HPRBAR18 p15,5,c6,c9,0
>>> +#define HPRLAR18 p15,5,c6,c9,1
>>> +#define HPRBAR19 p15,5,c6,c9,4
>>> +#define HPRLAR19 p15,5,c6,c9,5
>>> +#define HPRBAR20 p15,5,c6,c10,0
>>> +#define HPRLAR20 p15,5,c6,c10,1
>>> +#define HPRBAR21 p15,5,c6,c10,4
>>> +#define HPRLAR21 p15,5,c6,c10,5
>>> +#define HPRBAR22 p15,5,c6,c11,0
>>> +#define HPRLAR22 p15,5,c6,c11,1
>>> +#define HPRBAR23 p15,5,c6,c11,4
>>> +#define HPRLAR23 p15,5,c6,c11,5
>>> +#define HPRBAR24 p15,5,c6,c12,0
>>> +#define HPRLAR24 p15,5,c6,c12,1
>>> +#define HPRBAR25 p15,5,c6,c12,4
>>> +#define HPRLAR25 p15,5,c6,c12,5
>>> +#define HPRBAR26 p15,5,c6,c13,0
>>> +#define HPRLAR26 p15,5,c6,c13,1
>>> +#define HPRBAR27 p15,5,c6,c13,4
>>> +#define HPRLAR27 p15,5,c6,c13,5
>>> +#define HPRBAR28 p15,5,c6,c14,0
>>> +#define HPRLAR28 p15,5,c6,c14,1
>>> +#define HPRBAR29 p15,5,c6,c14,4
>>> +#define HPRLAR29 p15,5,c6,c14,5
>>> +#define HPRBAR30 p15,5,c6,c15,0
>>> +#define HPRLAR30 p15,5,c6,c15,1
>>> +#define HPRBAR31 p15,5,c6,c15,4
>>> +#define HPRLAR31 p15,5,c6,c15,5
>>
>> NIT: Is there any way we could generate the values using macros?
> This looks tricky. So I will prefer to keep this as it is.
>>
>>> +
>>> /* Aliases of AArch64 names for use in common code */
>>> #ifdef CONFIG_ARM_32
>>> /* Alphabetically... */
>>> #define MPUIR_EL2 HMPUIR
>>> #define PRBAR_EL2 HPRBAR
>>> +#define PRBAR0_EL2 HPRBAR0
>>
>> AFAIU, the alias will be mainly used in the macro generate
>> the switch. Rather than open-coding all the PR*AR_EL2, can we
>> provide two macros PR{B, L}AR_N that will be implemented as
>> HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
>
> Yes , we can have
>
> #define PR{B,L}AR_EL2_(n) HPR{B,L}AR##n for arm32
>
> #define PR{B,L}AR_EL2_(n) PR{B,L}AR##n##_EL2
we could have them in mm.c, I see in your v2 you’ve put them in cpregs.h,
but since they are only used by the generator, I would put them there.
On 28/04/2025 20:04, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
>
>> On 25 Apr 2025, at 13:00, Ayan Kumar Halder <ayankuma@amd.com> wrote:
>>
>> Hi Julien,
>>
>> cc-ed Luca for feedback on specific points.
>>
>> On 18/04/2025 05:54, Julien Grall wrote:
>>> Hi Ayan,
>>>
>>> On 18/04/2025 00:55, Ayan Kumar Halder wrote:
>>>> Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
>>>> The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3 HPRBAR<n>,
>>>> E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
>>>>
>>>> Introduce pr_t typedef which is a structure having the prbar and prlar members,
>>>> each being structured as the registers of the AArch32-V8R architecture.
>>>> This is the arm32 equivalent of
>>>> "arm/mpu: Introduce MPU memory region map structure".
>>>>
>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>>>> ---
>>>> This patch should be applied after
>>>> "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to enable
>>>> compilation for AArch32.
>>>>
>>>> xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++
>>>> xen/arch/arm/include/asm/mpu.h | 4 +
>>>> xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
>>>> 3 files changed, 198 insertions(+)
>>>> create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>>>>
>>>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
>>>> new file mode 100644
>>>> index 0000000000..4aabd93479
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>>>> @@ -0,0 +1,59 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
>>>> + */
>>>> +
>>>> +#ifndef __ARM_ARM32_MPU_H
>>>> +#define __ARM_ARM32_MPU_H
>>>> +
>>>> +#define XN_EL2_ENABLED 0x1
>>> I understand the define is introduced in Luca's patch, but this a bit non-descriptive... Can we find a better name? Maybe by adding the name of the register and some documentation?
>> We can rename this as PRBAR_EL2_XN if this sounds better (cc @Luca) in Luca's patch
> what about PRBAR_EL2_XN_ENABLED? I can change it in my serie
I am ok with the name.
>
>> The description can be
>>
>> Refer ARM DDI 0568A.c ID110520 , E2.2.2
>>
>> HPRBAR [0:1] Execute Never
>>
>>>> +
>>>> +#ifndef __ASSEMBLY__
>>>> +
>>>> +/* Hypervisor Protection Region Base Address Register */
>>>> +typedef union {
>>>> + struct {
>>>> + unsigned int xn:1; /* Execute-Never */
>>>> + unsigned int ap:2; /* Acess Permission */
>>>> + unsigned int sh:2; /* Sharebility */
>>>> + unsigned int res0:1; /* Reserved as 0 */
>>>> + unsigned int base:26; /* Base Address */
>>>> + } reg;
>>>> + uint32_t bits;
>>>> +} prbar_t;
>>>> +
>>>> +/* Hypervisor Protection Region Limit Address Register */
>>>> +typedef union {
>>>> + struct {
>>>> + unsigned int en:1; /* Region enable */
>>>> + unsigned int ai:3; /* Memory Attribute Index */
>>>> + /*
>>>> + * There is no actual ns bit in hardware. It is used here for
>>>> + * compatibility with Armr64 code. Thus, we are reusing a res0 bit for ns.
>>> typo: Arm64.
>> Ack
>>>> + */
>>> Hmmmm, this would mean someone may mistakenly set the bit to 0 by mistake. If the field is always meant to be 0 on arm64, then I would consider to name is res0 on arm64 with an explanation.
>>>
>>> This would make clear the bit is not supposed to have a value other than 0.
>> On Arm64, ns == 0 as it can only support secure mode.
>>
>> So we can change this on Arm64 as well :-
>>
>> unsigned int res0:2; /* ns == 0 as only secure mode is supported */
>>
>> @Luca to clarify
> From the specifications: "Non-secure bit. Specifies whether the output address is in the Secure or Non-secure memory”, I’m not sure
> that we should remove it from Arm64, so I don’t think you should have something only for compatibility, maybe the code accessing .ns
> can be compiled out for Arm32 or we can have arch-specific implementation. I think you are referring to pr_of_xenaddr when you say
> “compatibility with Arm64 code"
Yes, that is correct. So are you saying that we should have an "ifdef"
in the function.
+++ b/xen/arch/arm/mpu/mm.c
@@ -221,7 +221,9 @@ pr_t pr_of_xenaddr(paddr_t base, paddr_t limit,
unsigned attr)
/* Build up value for PRLAR_EL2. */
prlar = (prlar_t) {
.reg = {
+#ifdef CONFIG_ARM_64
.ns = 0, /* Hyp mode is in secure world */
+#endif
.ai = attr,
.en = 1, /* Region enabled */
}};
I am ok with this. I just want to know if you and Julien are aligned as
well.
>
>>>> + unsigned int ns:1; /* Reserved 0 by hardware */
>>>> + unsigned int res0:1; /* Reserved 0 by hardware */
>> Then, we can change this on Arm32 as well.
>>>> + unsigned int limit:26; /* Limit Address */
>>> NIT: Can we align the comments?
>> Ack.
>>>> + } reg;
>>>> + uint32_t bits;
>>>> +} prlar_t;
>>>> +
>>>> +/* Protection Region */
>>>> +typedef struct {
>>>> + prbar_t prbar;
>>>> + prlar_t prlar;
>>>> + uint64_t p2m_type; /* Used to store p2m types. */
>>>> +} pr_t;
>>> This looks to be common with arm64. If so, I would prefer if the structure is in a common header.
>> No, in arm64 this is
>>
>> typedef struct {
>> prbar_t prbar;
>> prlar_t prlar;
>> } pr_t;
>>
>> The reason being Arm64 uses unused bits (ie 'pad') to store the type.
>>
>>>> +
>>>> +#endif /* __ASSEMBLY__ */
>>>> +
>>>> +#endif /* __ARM_ARM32_MPU_H */
>>>> +
>>>> +/*
>>>> + * Local variables:
>>>> + * mode: C
>>>> + * c-file-style: "BSD"
>>>> + * c-basic-offset: 4
>>>> + * indent-tabs-mode: nil
>>>> + * End:
>>>> + */
>>>> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
>>>> index 77d0566f97..67127149c0 100644
>>>> --- a/xen/arch/arm/include/asm/mpu.h
>>>> +++ b/xen/arch/arm/include/asm/mpu.h
>>>> @@ -8,8 +8,12 @@
>>>> #if defined(CONFIG_ARM_64)
>>>> # include <asm/arm64/mpu.h>
>>>> +#elif defined(CONFIG_ARM_32)
>>>> +# include <asm/arm32/mpu.h>
>>>> #endif
>>>> +#define PRENR_MASK GENMASK(31, 0)
>>>> +
>>>> #define MPU_REGION_SHIFT 6
>>>> #define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
>>>> #define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
>>>> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
>>>> index d5cd0e04d5..7cf52aa09a 100644
>>>> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
>>>> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
>>>> @@ -6,18 +6,153 @@
>>>> /* CP15 CR0: MPU Type Register */
>>>> #define HMPUIR p15,4,c0,c0,4
>>>> +/* CP15 CR6: Protection Region Enable Register */
>>>> +#define HPRENR p15,4,c6,c1,1
>>>> +
>>>> /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
>>>> #define HPRSELR p15,4,c6,c2,1
>>>> #define HPRBAR p15,4,c6,c3,0
>>>> #define HPRLAR p15,4,c6,c8,1
>>>> +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
>>>> +#define HPRBAR0 p15,4,c6,c8,0
>>>> +#define HPRLAR0 p15,4,c6,c8,1
>>>> +#define HPRBAR1 p15,4,c6,c8,4
>>>> +#define HPRLAR1 p15,4,c6,c8,5
>>>> +#define HPRBAR2 p15,4,c6,c9,0
>>>> +#define HPRLAR2 p15,4,c6,c9,1
>>>> +#define HPRBAR3 p15,4,c6,c9,4
>>>> +#define HPRLAR3 p15,4,c6,c9,5
>>>> +#define HPRBAR4 p15,4,c6,c10,0
>>>> +#define HPRLAR4 p15,4,c6,c10,1
>>>> +#define HPRBAR5 p15,4,c6,c10,4
>>>> +#define HPRLAR5 p15,4,c6,c10,5
>>>> +#define HPRBAR6 p15,4,c6,c11,0
>>>> +#define HPRLAR6 p15,4,c6,c11,1
>>>> +#define HPRBAR7 p15,4,c6,c11,4
>>>> +#define HPRLAR7 p15,4,c6,c11,5
>>>> +#define HPRBAR8 p15,4,c6,c12,0
>>>> +#define HPRLAR8 p15,4,c6,c12,1
>>>> +#define HPRBAR9 p15,4,c6,c12,4
>>>> +#define HPRLAR9 p15,4,c6,c12,5
>>>> +#define HPRBAR10 p15,4,c6,c13,0
>>>> +#define HPRLAR10 p15,4,c6,c13,1
>>>> +#define HPRBAR11 p15,4,c6,c13,4
>>>> +#define HPRLAR11 p15,4,c6,c13,5
>>>> +#define HPRBAR12 p15,4,c6,c14,0
>>>> +#define HPRLAR12 p15,4,c6,c14,1
>>>> +#define HPRBAR13 p15,4,c6,c14,4
>>>> +#define HPRLAR13 p15,4,c6,c14,5
>>>> +#define HPRBAR14 p15,4,c6,c15,0
>>>> +#define HPRLAR14 p15,4,c6,c15,1
>>>> +#define HPRBAR15 p15,4,c6,c15,4
>>>> +#define HPRLAR15 p15,4,c6,c15,5
>>>> +#define HPRBAR16 p15,5,c6,c8,0
>>>> +#define HPRLAR16 p15,5,c6,c8,1
>>>> +#define HPRBAR17 p15,5,c6,c8,4
>>>> +#define HPRLAR17 p15,5,c6,c8,5
>>>> +#define HPRBAR18 p15,5,c6,c9,0
>>>> +#define HPRLAR18 p15,5,c6,c9,1
>>>> +#define HPRBAR19 p15,5,c6,c9,4
>>>> +#define HPRLAR19 p15,5,c6,c9,5
>>>> +#define HPRBAR20 p15,5,c6,c10,0
>>>> +#define HPRLAR20 p15,5,c6,c10,1
>>>> +#define HPRBAR21 p15,5,c6,c10,4
>>>> +#define HPRLAR21 p15,5,c6,c10,5
>>>> +#define HPRBAR22 p15,5,c6,c11,0
>>>> +#define HPRLAR22 p15,5,c6,c11,1
>>>> +#define HPRBAR23 p15,5,c6,c11,4
>>>> +#define HPRLAR23 p15,5,c6,c11,5
>>>> +#define HPRBAR24 p15,5,c6,c12,0
>>>> +#define HPRLAR24 p15,5,c6,c12,1
>>>> +#define HPRBAR25 p15,5,c6,c12,4
>>>> +#define HPRLAR25 p15,5,c6,c12,5
>>>> +#define HPRBAR26 p15,5,c6,c13,0
>>>> +#define HPRLAR26 p15,5,c6,c13,1
>>>> +#define HPRBAR27 p15,5,c6,c13,4
>>>> +#define HPRLAR27 p15,5,c6,c13,5
>>>> +#define HPRBAR28 p15,5,c6,c14,0
>>>> +#define HPRLAR28 p15,5,c6,c14,1
>>>> +#define HPRBAR29 p15,5,c6,c14,4
>>>> +#define HPRLAR29 p15,5,c6,c14,5
>>>> +#define HPRBAR30 p15,5,c6,c15,0
>>>> +#define HPRLAR30 p15,5,c6,c15,1
>>>> +#define HPRBAR31 p15,5,c6,c15,4
>>>> +#define HPRLAR31 p15,5,c6,c15,5
>>> NIT: Is there any way we could generate the values using macros?
>> This looks tricky. So I will prefer to keep this as it is.
>>>> +
>>>> /* Aliases of AArch64 names for use in common code */
>>>> #ifdef CONFIG_ARM_32
>>>> /* Alphabetically... */
>>>> #define MPUIR_EL2 HMPUIR
>>>> #define PRBAR_EL2 HPRBAR
>>>> +#define PRBAR0_EL2 HPRBAR0
>>> AFAIU, the alias will be mainly used in the macro generate
>>> the switch. Rather than open-coding all the PR*AR_EL2, can we
>>> provide two macros PR{B, L}AR_N that will be implemented as
>>> HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
>> Yes , we can have
>>
>> #define PR{B,L}AR_EL2_(n) HPR{B,L}AR##n for arm32
>>
>> #define PR{B,L}AR_EL2_(n) PR{B,L}AR##n##_EL2
> we could have them in mm.c, I see in your v2 you’ve put them in cpregs.h,
> but since they are only used by the generator, I would put them there.
You mean the above two macros should be moved to mm.c. I am ok with that.
Just 2 more things to be aligned :-
1. Are we ok to use PRBAR_EL2_(num) and PRLAR_EL2_(num) in the common
code (ie mpu/mm.c) ?
2. Are you ok to introduce ifdef in prepare_selector() ?
Please have a look at my v2 for reference.
- Ayan
>
>
>
Hi Ayan,
>>>>> + /*
>>>>> + * There is no actual ns bit in hardware. It is used here for
>>>>> + * compatibility with Armr64 code. Thus, we are reusing a res0 bit for ns.
>>>> typo: Arm64.
>>> Ack
>>>>> + */
>>>> Hmmmm, this would mean someone may mistakenly set the bit to 0 by mistake. If the field is always meant to be 0 on arm64, then I would consider to name is res0 on arm64 with an explanation.
>>>>
>>>> This would make clear the bit is not supposed to have a value other than 0.
>>> On Arm64, ns == 0 as it can only support secure mode.
>>>
>>> So we can change this on Arm64 as well :-
>>>
>>> unsigned int res0:2; /* ns == 0 as only secure mode is supported */
>>>
>>> @Luca to clarify
>> From the specifications: "Non-secure bit. Specifies whether the output address is in the Secure or Non-secure memory”, I’m not sure
>> that we should remove it from Arm64, so I don’t think you should have something only for compatibility, maybe the code accessing .ns
>> can be compiled out for Arm32 or we can have arch-specific implementation. I think you are referring to pr_of_xenaddr when you say
>> “compatibility with Arm64 code"
>
> Yes, that is correct. So are you saying that we should have an "ifdef" in the function.
>
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -221,7 +221,9 @@ pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned attr)
> /* Build up value for PRLAR_EL2. */
> prlar = (prlar_t) {
> .reg = {
> +#ifdef CONFIG_ARM_64
> .ns = 0, /* Hyp mode is in secure world */
> +#endif
> .ai = attr,
> .en = 1, /* Region enabled */
> }};
>
> I am ok with this. I just want to know if you and Julien are aligned as well.
this is my proposal, yes
>>>>>
>>>> NIT: Is there any way we could generate the values using macros?
>>> This looks tricky. So I will prefer to keep this as it is.
>>>>> +
>>>>> /* Aliases of AArch64 names for use in common code */
>>>>> #ifdef CONFIG_ARM_32
>>>>> /* Alphabetically... */
>>>>> #define MPUIR_EL2 HMPUIR
>>>>> #define PRBAR_EL2 HPRBAR
>>>>> +#define PRBAR0_EL2 HPRBAR0
>>>> AFAIU, the alias will be mainly used in the macro generate
>>>> the switch. Rather than open-coding all the PR*AR_EL2, can we
>>>> provide two macros PR{B, L}AR_N that will be implemented as
>>>> HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
>>> Yes , we can have
>>>
>>> #define PR{B,L}AR_EL2_(n) HPR{B,L}AR##n for arm32
>>>
>>> #define PR{B,L}AR_EL2_(n) PR{B,L}AR##n##_EL2
>> we could have them in mm.c, I see in your v2 you’ve put them in cpregs.h,
>> but since they are only used by the generator, I would put them there.
>
> You mean the above two macros should be moved to mm.c. I am ok with that.
>
> Just 2 more things to be aligned :-
>
> 1. Are we ok to use PRBAR_EL2_(num) and PRLAR_EL2_(num) in the common code (ie mpu/mm.c) ?
>
> 2. Are you ok to introduce ifdef in prepare_selector() ?
>
> Please have a look at my v2 for reference.
I will change my implementation to introduce on Arm64 PR{B,L}AR_EL2_(num) -> PR{B,L}AR##num##_EL2,
I will then protect everything with CONFIG_ARM_64, this will ensure proper compilation on both architecture.
When you will introduce your changes, you will have only to revert the #ifdef CONFIG_ARM_64 that protects
the common code and implement the changes to build on Arm32.
I think this is the best way to ensure we are not blocked by each other while keeping the churn as low as possible.
Please @julien and/or other maintainers let me know your opinion on this.
Cheers,
Luca
Hi Ayan
On 25/04/2025 13:00, Ayan Kumar Halder wrote:
>>> + unsigned int ns:1; /* Reserved 0 by hardware */
>>> + unsigned int res0:1; /* Reserved 0 by hardware */
> Then, we can change this on Arm32 as well.
>>> + unsigned int limit:26; /* Limit Address */
>>
>> NIT: Can we align the comments?
> Ack.
>>
>>> + } reg;
>>> + uint32_t bits;
>>> +} prlar_t;
>>> +
>>> +/* Protection Region */
>>> +typedef struct {
>>> + prbar_t prbar;
>>> + prlar_t prlar;
>>> + uint64_t p2m_type; /* Used to store p2m types. */
>>> +} pr_t;
>>
>> This looks to be common with arm64. If so, I would prefer if the
>> structure is in a common header.
>
> No, in arm64 this is
>
> typedef struct {
> prbar_t prbar;
> prlar_t prlar;
> } pr_t;
>
> The reason being Arm64 uses unused bits (ie 'pad') to store the type.
Hmmm... They are bits from the system register (prbar), right? If so,
you can't use them for storing the p2m_type as AFAICT they are RES0.
You could possibly mask the bits before writing them. But then, it will
become risky if the fields are defined.
Also, the number of MPU regions is fairly small. So, I don't think it is
worth it to store p2m_type.
Regardless that, I think it would be better to defer the introduction of
p2m_type until guest support is added. So it will be easier to
understand how this is going to be used and the size (for instance, I
doubt 64-bit is necessary...).
Cheers,
--
Julien Grall
> On 25 Apr 2025, at 14:32, Julien Grall <julien@xen.org> wrote:
>
> Hi Ayan
>
> On 25/04/2025 13:00, Ayan Kumar Halder wrote:
>>>> + unsigned int ns:1; /* Reserved 0 by hardware */
>>>> + unsigned int res0:1; /* Reserved 0 by hardware */
>> Then, we can change this on Arm32 as well.
>>>> + unsigned int limit:26; /* Limit Address */
>>>
>>> NIT: Can we align the comments?
>> Ack.
>>>
>>>> + } reg;
>>>> + uint32_t bits;
>>>> +} prlar_t;
>>>> +
>>>> +/* Protection Region */
>>>> +typedef struct {
>>>> + prbar_t prbar;
>>>> + prlar_t prlar;
>>>> + uint64_t p2m_type; /* Used to store p2m types. */
>>>> +} pr_t;
>>>
>>> This looks to be common with arm64. If so, I would prefer if the structure is in a common header.
>> No, in arm64 this is
>> typedef struct {
>> prbar_t prbar;
>> prlar_t prlar;
>> } pr_t;
>> The reason being Arm64 uses unused bits (ie 'pad') to store the type.
>
> Hmmm... They are bits from the system register (prbar), right? If so, you can't use them for storing the p2m_type as AFAICT they are RES0.
>
> You could possibly mask the bits before writing them.
This was the approach taken in the Arm64, I think the reason was because in this way the overall maximum structure was less than a page
and it was easier to manage something, I’ll see if I can find what, I think it was related to p2m...
> But then, it will become risky if the fields are defined.
>
> Also, the number of MPU regions is fairly small. So, I don't think it is worth it to store p2m_type.
>
> Regardless that, I think it would be better to defer the introduction of p2m_type until guest support is added. So it will be easier to understand how this is going to be used and the size (for instance, I doubt 64-bit is necessary...).
>
> Cheers,
>
> --
> Julien Grall
>
Hello all,
This is my first post to xen-devel. Since I'm new, let me give a brief intro about "Why?"
1. I am interested in easing the ongoing burden of repeatedly obtaining safety
certification for Xen for eventual use in high assurance operational *systems*.
Call this an evergreen certification effort.
2. When I read the ISO26262 and avionics cert standards, I note that there is a system
architecture (it has its own "System V" lifecycle) that encompasses a set-of-n
hardware items (e.g. fabricated SoC analysis and also FPGA soft IP development) and
also a set-of-m software items (e.g., bootloader, hypervisor, guest OSes, apps, containers,
etc.). For high assurance, we must evaluate the entire system and not just some of its items.
3. Also from the standards the govern the lifecycle and processes for developing
high assurance systems, hardware, and software, maintaining (automated) bidirectional
requirements identifier traceability from safety requirements "down" (i.e., through
levels of requirements decomposition, into *-architecture level designs, into software
unit designs, into software unit code, and into software verification tests at the unit,
library, module / architecture, and system level. From my perspective, there's a lot
riding on these requirements IDs. I would guess that many in this community
would feel concerned about dumping many (hundreds?) of requirements IDs
directly into the Xen code base, even if these were located within comments.
4. Because of 1, 2, and 3 (above), when I look to understand Xen requirements sources,
my first pass is to remember that Xen is just one software item, and it will need to accept
integration requirements flowing in from the system (i.e., via KConfig settings and Device
Tree / SDT language) and also from the core types that it relies upon (i.e., scoped down
to the operationally-deployed core types including Arm Cortex-R52 during boot,
but extending to co-processors it uses and then to IO devices that are used, etc.)
Finally, sometimes other software items flow requirements into Xen, e.g., when
Linux paravirtualization or virtio implementations are scoped-in.
In summary, finding an agreeable way to embed requirements IDs into the Xen
source code seems essential to me, so that we can achieve evergreen high
assurance certifications for Xen.
Proposal 1: Rather than start by adding many requirements IDs into comments,
we might instead start out by using a less-impactful approach.
I propose that we could hand-pick some C data types (CDTs) that were known to be
critically important to Xen's high assurance, and simply (re)name these CDTs,
for example, structs and unions.
This proposal is to adopt a naming convention for handpicked safety-relevant
CDTs, starting with the first two identified below.
For example in this patch, e.g. in xen/arch/arm/include/asm/arm32/mpu.h,
two CDT structs convey the bitfield requirements from the new MPU co-processors.
Specifically, in the typedefs "prbar_t" and "prlar_t" no struct tag (name) is used
for either of the bitfield struct. I propose adding struct tag names to both, and
then leveraging these struct tag names as requirements IDs.
Suggested Naming Convention:
(a) E.g., For prbar_t, tag its bitfield struct with the name "PRBAR_BF"
(b) E.g., For prlar_t, tag its bitfied struct with the name "PRLAR_BF"
(c) Proposed bitfield struct naming convention: <register>_BF
(d) Going forward, for bitfield structs that flow-in safety-relevant
requirements from the core's Technical Reference Manual (TRM)
and registers, to use the suffix "_BF" to identify these structs. And
to faithfully reuse the TRM's name for the register. (application to structs)
(e) Going forward, for unions that wrap "_BF" structs, to name any
union member name corresponding to the "_BF" as "bf" and any
union member name corresponding to the whole as "whole".
(application to unions that immediately wrap "_BF" structs)
The benefits of systematically following these naming conventions will be:
1) Even simple tools like grep -R can find all "_BF" requirements IDs that flow from
hardware registers that have been handpicked due to their safety-relevance.
2) Any CDT-parsing tool such as Doxygen, etc., can automatically find and trace
all usages of "_BF" structs at several levels
(a) Dependent structs such as "pr_t" can be automatically traced "up" to
their "_BF" ancestor CDTs.
(b) C function argument CDTs can be automatically traced up to any
ancestor "_BF" CDTs, with the implication that the function may be safety-relevant
because it accesses one or more "_BF" CDTs.
(c) Simple pointer-to-CDT usages should not obscure any facts that
either the ".bf.*" (accessing a safety-relevant bitfield by name) or the
".whole" (accessing the entire register contents by value) were accessed.
3) This convention will naturally allow the new "_BF" requirements IDs
to flow into all future use / access of these safety-relevant registers, including
future verification tests.
4) Also, it will naturally allow the new "<register>_BF" requirements IDs
to be traced "up" into the TRM and automatically de-reference the semantics
and usage and safety-relevant notes about the bitfield, the register, and the
larger context of use for that register (e.g., at which instruction will the PMU's
enforcement of "HPRBAR10" / "p15,4,c6,c13,0" be activated?)
5) The xen-devel community may find that using this naming convention
provides an easier on-ramp to "start seeing safety and high assurance."
This is because engineers who read any future C source code,
or review any future patch series will see that specific lines in that C source code
are indelibly tattooed when they refer to a safety-relevant register. Whether any
line of code access the "_BF" register in part or in whole, that will become explicitly visible.
The goal is to raise community awareness by flagging access / use.
Thank you!
Sincerely,
--mark
-----Original Message-----
From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Julien Grall
Sent: Thursday, April 17, 2025 11:55 PM
To: Ayan Kumar Halder <ayan.kumar.halder@amd.com>; xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis <bertrand.marquis@arm.com>; Michal Orzel <michal.orzel@amd.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v1] xen/arm: AArch32-V8R: Add MPU register definitions
[You don't often get email from julien@xen.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
Hi Ayan,
On 18/04/2025 00:55, Ayan Kumar Halder wrote:
> Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
> The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3
> HPRBAR<n>,
> E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
>
> Introduce pr_t typedef which is a structure having the prbar and prlar
> members, each being structured as the registers of the AArch32-V8R architecture.
> This is the arm32 equivalent of
> "arm/mpu: Introduce MPU memory region map structure".
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> This patch should be applied after
> "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to
> enable compilation for AArch32.
>
> xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++
> xen/arch/arm/include/asm/mpu.h | 4 +
> xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
> 3 files changed, 198 insertions(+)
> create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h
> b/xen/arch/arm/include/asm/arm32/mpu.h
> new file mode 100644
> index 0000000000..4aabd93479
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
> + */
> +
> +#ifndef __ARM_ARM32_MPU_H
> +#define __ARM_ARM32_MPU_H
> +
> +#define XN_EL2_ENABLED 0x1
I understand the define is introduced in Luca's patch, but this a bit non-descriptive... Can we find a better name? Maybe by adding the name of the register and some documentation?
> +
> +#ifndef __ASSEMBLY__
> +
> +/* Hypervisor Protection Region Base Address Register */ typedef
> +union {
> + struct {
> + unsigned int xn:1; /* Execute-Never */
> + unsigned int ap:2; /* Acess Permission */
> + unsigned int sh:2; /* Sharebility */
> + unsigned int res0:1; /* Reserved as 0 */
> + unsigned int base:26; /* Base Address */
> + } reg;
> + uint32_t bits;
> +} prbar_t;
> +
> +/* Hypervisor Protection Region Limit Address Register */ typedef
> +union {
> + struct {
> + unsigned int en:1; /* Region enable */
> + unsigned int ai:3; /* Memory Attribute Index */
> + /*
> + * There is no actual ns bit in hardware. It is used here for
> + * compatibility with Armr64 code. Thus, we are reusing a res0 bit for ns.
typo: Arm64.
> + */
Hmmmm, this would mean someone may mistakenly set the bit to 0 by mistake. If the field is always meant to be 0 on arm64, then I would consider to name is res0 on arm64 with an explanation.
This would make clear the bit is not supposed to have a value other than 0.
> + unsigned int ns:1; /* Reserved 0 by hardware */
> + unsigned int res0:1; /* Reserved 0 by hardware */
> + unsigned int limit:26; /* Limit Address */
NIT: Can we align the comments?
> + } reg;
> + uint32_t bits;
> +} prlar_t;
> +
> +/* Protection Region */
> +typedef struct {
> + prbar_t prbar;
> + prlar_t prlar;
> + uint64_t p2m_type; /* Used to store p2m types. */
> +} pr_t;
This looks to be common with arm64. If so, I would prefer if the structure is in a common header.
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* __ARM_ARM32_MPU_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/mpu.h
> b/xen/arch/arm/include/asm/mpu.h index 77d0566f97..67127149c0 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -8,8 +8,12 @@
>
> #if defined(CONFIG_ARM_64)
> # include <asm/arm64/mpu.h>
> +#elif defined(CONFIG_ARM_32)
> +# include <asm/arm32/mpu.h>
> #endif
>
> +#define PRENR_MASK GENMASK(31, 0)
> +
> #define MPU_REGION_SHIFT 6
> #define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
> #define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h
> b/xen/arch/arm/include/asm/mpu/cpregs.h
> index d5cd0e04d5..7cf52aa09a 100644
> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> @@ -6,18 +6,153 @@
> /* CP15 CR0: MPU Type Register */
> #define HMPUIR p15,4,c0,c0,4
>
> +/* CP15 CR6: Protection Region Enable Register */
> +#define HPRENR p15,4,c6,c1,1
> +
> /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
> #define HPRSELR p15,4,c6,c2,1
> #define HPRBAR p15,4,c6,c3,0
> #define HPRLAR p15,4,c6,c8,1
>
> +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
> +#define HPRBAR0 p15,4,c6,c8,0
> +#define HPRLAR0 p15,4,c6,c8,1
> +#define HPRBAR1 p15,4,c6,c8,4
> +#define HPRLAR1 p15,4,c6,c8,5
> +#define HPRBAR2 p15,4,c6,c9,0
> +#define HPRLAR2 p15,4,c6,c9,1
> +#define HPRBAR3 p15,4,c6,c9,4
> +#define HPRLAR3 p15,4,c6,c9,5
> +#define HPRBAR4 p15,4,c6,c10,0
> +#define HPRLAR4 p15,4,c6,c10,1
> +#define HPRBAR5 p15,4,c6,c10,4
> +#define HPRLAR5 p15,4,c6,c10,5
> +#define HPRBAR6 p15,4,c6,c11,0
> +#define HPRLAR6 p15,4,c6,c11,1
> +#define HPRBAR7 p15,4,c6,c11,4
> +#define HPRLAR7 p15,4,c6,c11,5
> +#define HPRBAR8 p15,4,c6,c12,0
> +#define HPRLAR8 p15,4,c6,c12,1
> +#define HPRBAR9 p15,4,c6,c12,4
> +#define HPRLAR9 p15,4,c6,c12,5
> +#define HPRBAR10 p15,4,c6,c13,0
> +#define HPRLAR10 p15,4,c6,c13,1
> +#define HPRBAR11 p15,4,c6,c13,4
> +#define HPRLAR11 p15,4,c6,c13,5
> +#define HPRBAR12 p15,4,c6,c14,0
> +#define HPRLAR12 p15,4,c6,c14,1
> +#define HPRBAR13 p15,4,c6,c14,4
> +#define HPRLAR13 p15,4,c6,c14,5
> +#define HPRBAR14 p15,4,c6,c15,0
> +#define HPRLAR14 p15,4,c6,c15,1
> +#define HPRBAR15 p15,4,c6,c15,4
> +#define HPRLAR15 p15,4,c6,c15,5
> +#define HPRBAR16 p15,5,c6,c8,0
> +#define HPRLAR16 p15,5,c6,c8,1
> +#define HPRBAR17 p15,5,c6,c8,4
> +#define HPRLAR17 p15,5,c6,c8,5
> +#define HPRBAR18 p15,5,c6,c9,0
> +#define HPRLAR18 p15,5,c6,c9,1
> +#define HPRBAR19 p15,5,c6,c9,4
> +#define HPRLAR19 p15,5,c6,c9,5
> +#define HPRBAR20 p15,5,c6,c10,0
> +#define HPRLAR20 p15,5,c6,c10,1
> +#define HPRBAR21 p15,5,c6,c10,4
> +#define HPRLAR21 p15,5,c6,c10,5
> +#define HPRBAR22 p15,5,c6,c11,0
> +#define HPRLAR22 p15,5,c6,c11,1
> +#define HPRBAR23 p15,5,c6,c11,4
> +#define HPRLAR23 p15,5,c6,c11,5
> +#define HPRBAR24 p15,5,c6,c12,0
> +#define HPRLAR24 p15,5,c6,c12,1
> +#define HPRBAR25 p15,5,c6,c12,4
> +#define HPRLAR25 p15,5,c6,c12,5
> +#define HPRBAR26 p15,5,c6,c13,0
> +#define HPRLAR26 p15,5,c6,c13,1
> +#define HPRBAR27 p15,5,c6,c13,4
> +#define HPRLAR27 p15,5,c6,c13,5
> +#define HPRBAR28 p15,5,c6,c14,0
> +#define HPRLAR28 p15,5,c6,c14,1
> +#define HPRBAR29 p15,5,c6,c14,4
> +#define HPRLAR29 p15,5,c6,c14,5
> +#define HPRBAR30 p15,5,c6,c15,0
> +#define HPRLAR30 p15,5,c6,c15,1
> +#define HPRBAR31 p15,5,c6,c15,4
> +#define HPRLAR31 p15,5,c6,c15,5
NIT: Is there any way we could generate the values using macros?
> +
> /* Aliases of AArch64 names for use in common code */
> #ifdef CONFIG_ARM_32
> /* Alphabetically... */
> #define MPUIR_EL2 HMPUIR
> #define PRBAR_EL2 HPRBAR
> +#define PRBAR0_EL2 HPRBAR0
AFAIU, the alias will be mainly used in the macro generate the switch. Rather than open-coding all the PR*AR_EL2, can we provide two macros PR{B, L}AR_N that will be implemented as HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
> #define PRSELR_EL2 HPRSELR
> +
> #endif /* CONFIG_ARM_32 */
>
> #endif /* __ARM_MPU_CPREGS_H */
Cheers,
--
Julien Grall
________________________________
The information contained in this e-mail and any attachments from Parry Labs may contain confidential and/or proprietary information, and is intended only for the named recipient to whom it was originally addressed. If you are not the intended recipient, any disclosure, distribution, or copying of this e-mail or its attachments is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately by return e-mail and permanently delete the e-mail and any attachments.
Hi Mark,
On 18/04/2025 16:00, Mark Brown wrote:
> Hello all,
> This is my first post to xen-devel. Since I'm new, let me give a brief intro about "Why?"
Welcome :)
>
> 1. I am interested in easing the ongoing burden of repeatedly obtaining safety
> certification for Xen for eventual use in high assurance operational *systems*.
> Call this an evergreen certification effort.
>
> 2. When I read the ISO26262 and avionics cert standards, I note that there is a system
> architecture (it has its own "System V" lifecycle) that encompasses a set-of-n
> hardware items (e.g. fabricated SoC analysis and also FPGA soft IP development) and
> also a set-of-m software items (e.g., bootloader, hypervisor, guest OSes, apps, containers,
> etc.). For high assurance, we must evaluate the entire system and not just some of its items.
>
> 3. Also from the standards the govern the lifecycle and processes for developing
> high assurance systems, hardware, and software, maintaining (automated) bidirectional
> requirements identifier traceability from safety requirements "down" (i.e., through
> levels of requirements decomposition, into *-architecture level designs, into software
> unit designs, into software unit code, and into software verification tests at the unit,
> library, module / architecture, and system level. From my perspective, there's a lot
> riding on these requirements IDs. I would guess that many in this community
> would feel concerned about dumping many (hundreds?) of requirements IDs
> directly into the Xen code base, even if these were located within comments.
>
> 4. Because of 1, 2, and 3 (above), when I look to understand Xen requirements sources,
> my first pass is to remember that Xen is just one software item, and it will need to accept
> integration requirements flowing in from the system (i.e., via KConfig settings and Device
> Tree / SDT language) and also from the core types that it relies upon (i.e., scoped down
> to the operationally-deployed core types including Arm Cortex-R52 during boot,
> but extending to co-processors it uses and then to IO devices that are used, etc.)
> Finally, sometimes other software items flow requirements into Xen, e.g., when
> Linux paravirtualization or virtio implementations are scoped-in.
Thanks for the context. This is exciting as Xen finds new use cases in
avionics.
However I think the right way to go on this is to send a RFC (request
for comments) patch on the coding style.
Especially on ...
>
> In summary, finding an agreeable way to embed requirements IDs into the Xen
> source code seems essential to me, so that we can achieve evergreen high
> assurance certifications for Xen.
>
> Proposal 1: Rather than start by adding many requirements IDs into comments,
> we might instead start out by using a less-impactful approach.
> I propose that we could hand-pick some C data types (CDTs) that were known to be
> critically important to Xen's high assurance, and simply (re)name these CDTs,
> for example, structs and unions.
>
> This proposal is to adopt a naming convention for handpicked safety-relevant
> CDTs, starting with the first two identified below.
>
> For example in this patch, e.g. in xen/arch/arm/include/asm/arm32/mpu.h,
> two CDT structs convey the bitfield requirements from the new MPU co-processors.
> Specifically, in the typedefs "prbar_t" and "prlar_t" no struct tag (name) is used
> for either of the bitfield struct. I propose adding struct tag names to both, and
> then leveraging these struct tag names as requirements IDs.
>
> Suggested Naming Convention:
> (a) E.g., For prbar_t, tag its bitfield struct with the name "PRBAR_BF"
> (b) E.g., For prlar_t, tag its bitfied struct with the name "PRLAR_BF"
> (c) Proposed bitfield struct naming convention: <register>_BF
You can send a patch to update
https://gitlab.com/xen-project/xen/-/blob/staging/CODING_STYLE?ref_type=heads
with the suggested naming convention.
However, we need a rationale for the above proposal. So..
> (d) Going forward, for bitfield structs that flow-in safety-relevant
> requirements from the core's Technical Reference Manual (TRM)
> and registers, to use the suffix "_BF" to identify these structs. And
> to faithfully reuse the TRM's name for the register. (application to structs)
> (e) Going forward, for unions that wrap "_BF" structs, to name any
> union member name corresponding to the "_BF" as "bf" and any
> union member name corresponding to the whole as "whole".
> (application to unions that immediately wrap "_BF" structs)
>
> The benefits of systematically following these naming conventions will be:
> 1) Even simple tools like grep -R can find all "_BF" requirements IDs that flow from
> hardware registers that have been handpicked due to their safety-relevance.
> 2) Any CDT-parsing tool such as Doxygen, etc., can automatically find and trace
> all usages of "_BF" structs at several levels
> (a) Dependent structs such as "pr_t" can be automatically traced "up" to
> their "_BF" ancestor CDTs.
> (b) C function argument CDTs can be automatically traced up to any
> ancestor "_BF" CDTs, with the implication that the function may be safety-relevant
> because it accesses one or more "_BF" CDTs.
> (c) Simple pointer-to-CDT usages should not obscure any facts that
> either the ".bf.*" (accessing a safety-relevant bitfield by name) or the
> ".whole" (accessing the entire register contents by value) were accessed.
This should be a part of the commit message. I would avoid adding any
mention of requirements or req-ids in the coding style. You can put this
in the cover letter to explain what is the long term view on this.
In short I am suggesting that the changes suggested for the coding style
should be concise, so the maintainers can give their opinion.
Also if you can prepare a proof of concept with a sample Doxygen report,
that might be helpful.
What might help the maintainers is to understand :-
1. What is the minimum change you are suggesting ?
Proposed bitfield struct naming convention: <register>_BF ....
2. How is it going to be helpful to the community in general (eg better
code readability, ease of review, ease of mantainance)
"<register>_BF" can be linked to the specific section of Architecture manual within the code itself. It helps to review, mantain and update the code.
> 3) This convention will naturally allow the new "_BF" requirements IDs
> to flow into all future use / access of these safety-relevant registers, including
> future verification tests.
> 4) Also, it will naturally allow the new "<register>_BF" requirements IDs
> to be traced "up" into the TRM and automatically de-reference the semantics
> and usage and safety-relevant notes about the bitfield, the register, and the
> larger context of use for that register (e.g., at which instruction will the PMU's
> enforcement of "HPRBAR10" / "p15,4,c6,c13,0" be activated?)
> 5) The xen-devel community may find that using this naming convention
> provides an easier on-ramp to "start seeing safety and high assurance."
> This is because engineers who read any future C source code,
> or review any future patch series will see that specific lines in that C source code
> are indelibly tattooed when they refer to a safety-relevant register. Whether any
> line of code access the "_BF" register in part or in whole, that will become explicitly visible.
> The goal is to raise community awareness by flagging access / use.
You can mention this as part of the commit message.
Also it will be helpful if you can pick up an existing file (eg anything
arch/arm64/mmu ) to explain the concept. As the new code is in flux, so
it make things easier to understand the proposed coding style changes in
the existing code.
You can take a look at
https://lists.xen.org/archives/html/xen-devel/2022-05/msg01995.html to
understand how we propose new changes to the coding style.
Hope this helps.
- Ayan
>
> Thank you!
>
> Sincerely,
> --mark
>
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Julien Grall
> Sent: Thursday, April 17, 2025 11:55 PM
> To: Ayan Kumar Halder <ayan.kumar.halder@amd.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis <bertrand.marquis@arm.com>; Michal Orzel <michal.orzel@amd.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1] xen/arm: AArch32-V8R: Add MPU register definitions
>
> [You don't often get email from julien@xen.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi Ayan,
>
> On 18/04/2025 00:55, Ayan Kumar Halder wrote:
>> Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
>> The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3
>> HPRBAR<n>,
>> E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
>>
>> Introduce pr_t typedef which is a structure having the prbar and prlar
>> members, each being structured as the registers of the AArch32-V8R architecture.
>> This is the arm32 equivalent of
>> "arm/mpu: Introduce MPU memory region map structure".
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> This patch should be applied after
>> "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to
>> enable compilation for AArch32.
>>
>> xen/arch/arm/include/asm/arm32/mpu.h | 59 +++++++++++
>> xen/arch/arm/include/asm/mpu.h | 4 +
>> xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
>> 3 files changed, 198 insertions(+)
>> create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>>
>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h
>> b/xen/arch/arm/include/asm/arm32/mpu.h
>> new file mode 100644
>> index 0000000000..4aabd93479
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>> @@ -0,0 +1,59 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
>> + */
>> +
>> +#ifndef __ARM_ARM32_MPU_H
>> +#define __ARM_ARM32_MPU_H
>> +
>> +#define XN_EL2_ENABLED 0x1
> I understand the define is introduced in Luca's patch, but this a bit non-descriptive... Can we find a better name? Maybe by adding the name of the register and some documentation?
>
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +/* Hypervisor Protection Region Base Address Register */ typedef
>> +union {
>> + struct {
>> + unsigned int xn:1; /* Execute-Never */
>> + unsigned int ap:2; /* Acess Permission */
>> + unsigned int sh:2; /* Sharebility */
>> + unsigned int res0:1; /* Reserved as 0 */
>> + unsigned int base:26; /* Base Address */
>> + } reg;
>> + uint32_t bits;
>> +} prbar_t;
>> +
>> +/* Hypervisor Protection Region Limit Address Register */ typedef
>> +union {
>> + struct {
>> + unsigned int en:1; /* Region enable */
>> + unsigned int ai:3; /* Memory Attribute Index */
>> + /*
>> + * There is no actual ns bit in hardware. It is used here for
>> + * compatibility with Armr64 code. Thus, we are reusing a res0 bit for ns.
> typo: Arm64.
>
>> + */
> Hmmmm, this would mean someone may mistakenly set the bit to 0 by mistake. If the field is always meant to be 0 on arm64, then I would consider to name is res0 on arm64 with an explanation.
>
> This would make clear the bit is not supposed to have a value other than 0.
>
>> + unsigned int ns:1; /* Reserved 0 by hardware */
>> + unsigned int res0:1; /* Reserved 0 by hardware */
>> + unsigned int limit:26; /* Limit Address */
> NIT: Can we align the comments?
>
>> + } reg;
>> + uint32_t bits;
>> +} prlar_t;
>> +
>> +/* Protection Region */
>> +typedef struct {
>> + prbar_t prbar;
>> + prlar_t prlar;
>> + uint64_t p2m_type; /* Used to store p2m types. */
>> +} pr_t;
> This looks to be common with arm64. If so, I would prefer if the structure is in a common header.
>
>> +
>> +#endif /* __ASSEMBLY__ */
>> +
>> +#endif /* __ARM_ARM32_MPU_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/include/asm/mpu.h
>> b/xen/arch/arm/include/asm/mpu.h index 77d0566f97..67127149c0 100644
>> --- a/xen/arch/arm/include/asm/mpu.h
>> +++ b/xen/arch/arm/include/asm/mpu.h
>> @@ -8,8 +8,12 @@
>>
>> #if defined(CONFIG_ARM_64)
>> # include <asm/arm64/mpu.h>
>> +#elif defined(CONFIG_ARM_32)
>> +# include <asm/arm32/mpu.h>
>> #endif
>>
>> +#define PRENR_MASK GENMASK(31, 0)
>> +
>> #define MPU_REGION_SHIFT 6
>> #define MPU_REGION_ALIGN (_AC(1, UL) << MPU_REGION_SHIFT)
>> #define MPU_REGION_MASK (~(MPU_REGION_ALIGN - 1))
>> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h
>> b/xen/arch/arm/include/asm/mpu/cpregs.h
>> index d5cd0e04d5..7cf52aa09a 100644
>> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
>> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
>> @@ -6,18 +6,153 @@
>> /* CP15 CR0: MPU Type Register */
>> #define HMPUIR p15,4,c0,c0,4
>>
>> +/* CP15 CR6: Protection Region Enable Register */
>> +#define HPRENR p15,4,c6,c1,1
>> +
>> /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
>> #define HPRSELR p15,4,c6,c2,1
>> #define HPRBAR p15,4,c6,c3,0
>> #define HPRLAR p15,4,c6,c8,1
>>
>> +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
>> +#define HPRBAR0 p15,4,c6,c8,0
>> +#define HPRLAR0 p15,4,c6,c8,1
>> +#define HPRBAR1 p15,4,c6,c8,4
>> +#define HPRLAR1 p15,4,c6,c8,5
>> +#define HPRBAR2 p15,4,c6,c9,0
>> +#define HPRLAR2 p15,4,c6,c9,1
>> +#define HPRBAR3 p15,4,c6,c9,4
>> +#define HPRLAR3 p15,4,c6,c9,5
>> +#define HPRBAR4 p15,4,c6,c10,0
>> +#define HPRLAR4 p15,4,c6,c10,1
>> +#define HPRBAR5 p15,4,c6,c10,4
>> +#define HPRLAR5 p15,4,c6,c10,5
>> +#define HPRBAR6 p15,4,c6,c11,0
>> +#define HPRLAR6 p15,4,c6,c11,1
>> +#define HPRBAR7 p15,4,c6,c11,4
>> +#define HPRLAR7 p15,4,c6,c11,5
>> +#define HPRBAR8 p15,4,c6,c12,0
>> +#define HPRLAR8 p15,4,c6,c12,1
>> +#define HPRBAR9 p15,4,c6,c12,4
>> +#define HPRLAR9 p15,4,c6,c12,5
>> +#define HPRBAR10 p15,4,c6,c13,0
>> +#define HPRLAR10 p15,4,c6,c13,1
>> +#define HPRBAR11 p15,4,c6,c13,4
>> +#define HPRLAR11 p15,4,c6,c13,5
>> +#define HPRBAR12 p15,4,c6,c14,0
>> +#define HPRLAR12 p15,4,c6,c14,1
>> +#define HPRBAR13 p15,4,c6,c14,4
>> +#define HPRLAR13 p15,4,c6,c14,5
>> +#define HPRBAR14 p15,4,c6,c15,0
>> +#define HPRLAR14 p15,4,c6,c15,1
>> +#define HPRBAR15 p15,4,c6,c15,4
>> +#define HPRLAR15 p15,4,c6,c15,5
>> +#define HPRBAR16 p15,5,c6,c8,0
>> +#define HPRLAR16 p15,5,c6,c8,1
>> +#define HPRBAR17 p15,5,c6,c8,4
>> +#define HPRLAR17 p15,5,c6,c8,5
>> +#define HPRBAR18 p15,5,c6,c9,0
>> +#define HPRLAR18 p15,5,c6,c9,1
>> +#define HPRBAR19 p15,5,c6,c9,4
>> +#define HPRLAR19 p15,5,c6,c9,5
>> +#define HPRBAR20 p15,5,c6,c10,0
>> +#define HPRLAR20 p15,5,c6,c10,1
>> +#define HPRBAR21 p15,5,c6,c10,4
>> +#define HPRLAR21 p15,5,c6,c10,5
>> +#define HPRBAR22 p15,5,c6,c11,0
>> +#define HPRLAR22 p15,5,c6,c11,1
>> +#define HPRBAR23 p15,5,c6,c11,4
>> +#define HPRLAR23 p15,5,c6,c11,5
>> +#define HPRBAR24 p15,5,c6,c12,0
>> +#define HPRLAR24 p15,5,c6,c12,1
>> +#define HPRBAR25 p15,5,c6,c12,4
>> +#define HPRLAR25 p15,5,c6,c12,5
>> +#define HPRBAR26 p15,5,c6,c13,0
>> +#define HPRLAR26 p15,5,c6,c13,1
>> +#define HPRBAR27 p15,5,c6,c13,4
>> +#define HPRLAR27 p15,5,c6,c13,5
>> +#define HPRBAR28 p15,5,c6,c14,0
>> +#define HPRLAR28 p15,5,c6,c14,1
>> +#define HPRBAR29 p15,5,c6,c14,4
>> +#define HPRLAR29 p15,5,c6,c14,5
>> +#define HPRBAR30 p15,5,c6,c15,0
>> +#define HPRLAR30 p15,5,c6,c15,1
>> +#define HPRBAR31 p15,5,c6,c15,4
>> +#define HPRLAR31 p15,5,c6,c15,5
> NIT: Is there any way we could generate the values using macros?
>
>> +
>> /* Aliases of AArch64 names for use in common code */
>> #ifdef CONFIG_ARM_32
>> /* Alphabetically... */
>> #define MPUIR_EL2 HMPUIR
>> #define PRBAR_EL2 HPRBAR
>> +#define PRBAR0_EL2 HPRBAR0
> AFAIU, the alias will be mainly used in the macro generate the switch. Rather than open-coding all the PR*AR_EL2, can we provide two macros PR{B, L}AR_N that will be implemented as HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?
>
>> #define PRSELR_EL2 HPRSELR
>> +
>> #endif /* CONFIG_ARM_32 */
>>
>> #endif /* __ARM_MPU_CPREGS_H */
> Cheers,
>
> --
> Julien Grall
>
>
> ________________________________
> The information contained in this e-mail and any attachments from Parry Labs may contain confidential and/or proprietary information, and is intended only for the named recipient to whom it was originally addressed. If you are not the intended recipient, any disclosure, distribution, or copying of this e-mail or its attachments is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately by return e-mail and permanently delete the e-mail and any attachments.
© 2016 - 2025 Red Hat, Inc.