[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1] xen/arm: AArch32-V8R: Add MPU register definitions
- To: Luca Fancellu <Luca.Fancellu@xxxxxxx>
- From: Ayan Kumar Halder <ayankuma@xxxxxxx>
- Date: Tue, 29 Apr 2025 13:04:41 +0100
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=EOCkHerxvDxmbeBQR30NNEMstB8L6draFAV62NRgmMM=; b=A+GGVZFAifOoYwcecCL49bpisitW29LSf5gOGkOhRP/AS/tJImSzLhz0qu1JZ5Jlhtiy7dj4ln4sxA67gvFv0x+uoo5ueJbV9ScQxw/G9lccVKAR+Lsk02dd3TToed+gFQJPAfZqIZqQFcx4mXL1peb2QkVxGIlUpr0kgL9+r6PaA8z8SCejbcfqAgAGu8zF+M/WbvbJ7vianyrHsaSqgrHxHQR5UytBZfPgnrKR+wf/knfHt0C6oxQJH5EdNp6wSMkqYlJXXlvjpNU9YHGG2C5ghT6b/8zcjFGIKhReD41vQLPjJL8IewOTJ8UXnsJc6PVJx6LMBR1qkxp1x8s/0Q==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=GhPdyxHf/BRudvYBK3lm91RhWvX/ynujEoX4eIzPRK3ddGmch0/SO/gqxMqbdtW/xdvKOpRm5ZSlLfcG6NtvLx2b2eM906TzaIrhPSIMGUWpvodT7B7n3NP5WyWe9Bj+CKQg3o4yADpjI4XepsBISt4nCbIQJcc6wJuCwRxn3X0q94tsGfjs4o2DIyhgxAHjTmzT6FP5IK6ZpQiwlHDc2nZaQIuu6i0C282Efk2KlPWtjNd0A2FlFo3SHQ4gs4VHsEOALEMI4PCPrEVqKanMgBqz0FvINNlOLHX1krjhWYcjQwbjPfSx5XBoPhY6kGHLZe3xAI6nv1UpRj9dLaXKsg==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
- Cc: Julien Grall <julien@xxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
- Delivery-date: Tue, 29 Apr 2025 12:04:54 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 28/04/2025 20:04, Luca Fancellu wrote:
Hi Ayan,
Hi Luca,
On 25 Apr 2025, at 13:00, Ayan Kumar Halder <ayankuma@xxxxxxx> 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@xxxxxxx>
---
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
|
