Hi Michal,
On 05/06/2025 08:06, Orzel, Michal wrote:
On 04/06/2025 19:43, Ayan Kumar Halder wrote:
Introduce pr_t typedef which is a structure having the prbar and prlar members,
each being structured as the registers of the AArch32 Armv8-R architecture.
Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
BASE or LIMIT bitfields in prbar or prlar respectively.
Move pr_t definition to common code.
Also, enclose xn_0 within ARM64 as it is not present for ARM32.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.hal...@amd.com>
---
xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
xen/arch/arm/include/asm/arm64/mpu.h | 6 ------
xen/arch/arm/include/asm/mpu.h | 6 ++++++
xen/arch/arm/mpu/mm.c | 2 ++
4 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/include/asm/arm32/mpu.h
b/xen/arch/arm/include/asm/arm32/mpu.h
index f0d4d4055c..ae3b661fde 100644
--- a/xen/arch/arm/include/asm/arm32/mpu.h
+++ b/xen/arch/arm/include/asm/arm32/mpu.h
@@ -5,11 +5,31 @@
#ifndef __ASSEMBLY__
-/* MPU Protection Region */
-typedef struct {
- uint32_t prbar;
- uint32_t prlar;
-} pr_t;
+#define MPU_REGION_RES0 0x0
The name of the macro does not make a lot of sense in AArch32 context
and can create a confusion for the reader.
I know, but I want to avoid introducing ifdef or have separate
implementation (for arm32 and arm64) for the following
Refer xen/arch/arm/include/asm/mpu.h
static inline void pr_set_base(pr_t *pr, paddr_t base)
{
pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
}
Let me know your preference.
+
+/* Hypervisor Protection Region Base Address Register */
+typedef union {
+ struct {
+ unsigned int xn:1; /* Execute-Never */
+ unsigned int ap_0:1; /* Acess Permission */
If you write AP[1] below, I would expect here AP[0]
Again same reason as before, let me know if you want to have additional
ifdef in pr_of_addr() or separate functions for arm32 and arm64
pr_t pr_of_addr(...)
{...
prbar = (prbar_t) {
.reg = {
#ifdef CONFIG_ARM64
.xn_0 = 0,
#endif
.xn = PAGE_XN_MASK(flags),
.ap_0 = 0,
.ro = PAGE_RO_MASK(flags)
}};
...
}
Also s/acess/access/
Ack.
+ unsigned long ro:1; /* Access Permission AP[1] */
+ unsigned int sh:2; /* Sharebility */
+ unsigned int res0:1; /* Reserved as 0 */
Here your comment for RES0 (which IMO could be just RES0) does not match...
I will remove the comment here and ..
+ 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 */
+ unsigned int res0:2; /* Reserved 0 by hardware */
this one. I don't think you need to explain what RES0 means.
here.
- Ayan
~Michal
