[PATCH v2 0/3] Enable R52 support for the first chunk of MPU support

[PATCH v2 0/3] Enable R52 support for the first chunk of MPU support

[Ayan Kumar Halder]

Hi all,

This patch serie enables R52 support based on Luca's series.
"[PATCH v6 0/6] First chunk for Arm R82 and MPU support".

Ayan Kumar Halder (3):
  arm/mpu: Introduce MPU memory region map structure
  arm/mpu: Provide and populate MPU C data structures
  arm/mpu: Provide access to the MPU region from the C code

 xen/arch/arm/arm32/Makefile              |  1 +
 xen/arch/arm/arm32/asm-offsets.c         |  6 ++
 xen/arch/arm/arm32/cache.S               | 41 ++++++++++++++
 xen/arch/arm/arm32/mpu/head.S            | 25 ++++++++
 xen/arch/arm/include/asm/arm32/mpu.h     | 34 ++++++++++-
 xen/arch/arm/include/asm/mpu.h           |  2 -
 xen/arch/arm/include/asm/mpu/cpregs.h    | 72 +++++++++++++++++++++++-
 xen/arch/arm/include/asm/mpu/regions.inc |  2 +-
 xen/arch/arm/mpu/mm.c                    | 51 +++++++++++++++--
 9 files changed, 224 insertions(+), 10 deletions(-)
 create mode 100644 xen/arch/arm/arm32/cache.S

-- 
2.25.1

[PATCH v2 1/3] arm/mpu: Introduce MPU memory region map structure

[Ayan Kumar Halder]

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.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from v1 :-

1. Preserve pr_t typedef in arch specific files.

2. Fix typo.

 xen/arch/arm/include/asm/arm32/mpu.h | 34 ++++++++++++++++++++++++++--
 xen/arch/arm/mpu/mm.c                |  2 ++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
index f0d4d4055c..fe139a2abe 100644
--- a/xen/arch/arm/include/asm/arm32/mpu.h
+++ b/xen/arch/arm/include/asm/arm32/mpu.h
@@ -5,10 +5,40 @@
 
 #ifndef __ASSEMBLY__
 
+/*
+ * Unlike arm64, there are no reserved 0 bits beyond base and limit bitfield in
+ * prbar and prlar registers respectively.
+ */
+#define MPU_REGION_RES0       0x0
+
+/* Hypervisor Protection Region Base Address Register */
+typedef union {
+    struct {
+        unsigned int xn:1;       /* Execute-Never */
+        unsigned int ap_0:1;     /* Access Permission AP[0] */
+        unsigned long ro:1;      /* Access Permission AP[1] */
+        unsigned int sh:2;       /* Shareability */
+        unsigned int res0:1;
+        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;
+        unsigned int limit:26; /* Limit Address */
+    } reg;
+    uint32_t bits;
+} prlar_t;
+
 /* MPU Protection Region */
 typedef struct {
-    uint32_t prbar;
-    uint32_t prlar;
+    prbar_t prbar;
+    prlar_t prlar;
 } pr_t;
 
 #endif /* __ASSEMBLY__ */
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 86fbe105af..2fb6b822c6 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -167,7 +167,9 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
     /* Build up value for PRBAR_EL2. */
     prbar = (prbar_t) {
         .reg = {
+#ifdef CONFIG_ARM64
             .xn_0 = 0,
+#endif
             .xn = PAGE_XN_MASK(flags),
             .ap_0 = 0,
             .ro = PAGE_RO_MASK(flags)
-- 
2.25.1

[PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Ayan Kumar Halder]

Modify Arm32 assembly boot code to reset any unused MPU region, initialise
'max_mpu_regions' with the number of supported MPU regions and set/clear the
bitmap 'xen_mpumap_mask' used to track the enabled regions.

Use the macro definition for "dcache_line_size" from linux.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from v1 :-

1. Introduce cache.S to hold arm32 cache initialization instructions.

2. Use dcache_line_size macro definition from linux.

3. Use mov_w instead of ldr.

4. Use a single stm instruction for 'store_pair' macro definition.

 xen/arch/arm/arm32/Makefile              |  1 +
 xen/arch/arm/arm32/asm-offsets.c         |  6 ++++
 xen/arch/arm/arm32/cache.S               | 41 ++++++++++++++++++++++++
 xen/arch/arm/arm32/mpu/head.S            | 25 +++++++++++++++
 xen/arch/arm/include/asm/mpu/regions.inc |  2 +-
 5 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/arm32/cache.S

diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index 537969d753..531168f58a 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -2,6 +2,7 @@ obj-y += lib/
 obj-$(CONFIG_MMU) += mmu/
 obj-$(CONFIG_MPU) += mpu/
 
+obj-y += cache.o
 obj-$(CONFIG_EARLY_PRINTK) += debug.o
 obj-y += domctl.o
 obj-y += domain.o
diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index 8bbb0f938e..c203ce269d 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -75,6 +75,12 @@ void __dummy__(void)
 
    OFFSET(INITINFO_stack, struct init_info, stack);
    BLANK();
+
+#ifdef CONFIG_MPU
+   DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
+   DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
+   BLANK();
+#endif
 }
 
 /*
diff --git a/xen/arch/arm/arm32/cache.S b/xen/arch/arm/arm32/cache.S
new file mode 100644
index 0000000000..00ea390ce4
--- /dev/null
+++ b/xen/arch/arm/arm32/cache.S
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Cache maintenance */
+
+#include <asm/arm32/sysregs.h>
+
+/* dcache_line_size - get the minimum D-cache line size from the CTR register */
+    .macro  dcache_line_size, reg, tmp
+    mrc p15, 0, \tmp, c0, c0, 1     /* read ctr */
+    lsr \tmp, \tmp, #16
+    and \tmp, \tmp, #0xf            /* cache line size encoding */
+    mov \reg, #4                    /* bytes per word */
+    mov \reg, \reg, lsl \tmp        /* actual cache line size */
+    .endm
+
+/*
+ * __invalidate_dcache_area(addr, size)
+ *
+ * Ensure that the data held in the cache for the buffer is invalidated.
+ *
+ * - addr - start address of the buffer
+ * - size - size of the buffer
+ */
+FUNC(__invalidate_dcache_area)
+    dcache_line_size r2, r3
+    add   r1, r0, r1
+    sub   r3, r2, #1
+    bic   r0, r0, r3
+1:  mcr   CP32(r0, DCIMVAC)     /* invalidate D line / unified line */
+    add   r0, r0, r2
+    cmp   r0, r1
+    blo   1b
+    dsb   sy
+    ret
+END(__invalidate_dcache_area)
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
index b2c5245e51..435b140d09 100644
--- a/xen/arch/arm/arm32/mpu/head.S
+++ b/xen/arch/arm/arm32/mpu/head.S
@@ -49,6 +49,10 @@ FUNC(enable_boot_cpu_mm)
     mrc   CP32(r5, MPUIR_EL2)
     and   r5, r5, #NUM_MPU_REGIONS_MASK
 
+    mov_w   r0, max_mpu_regions
+    str   r5, [r0]
+    mcr   CP32(r0, DCIMVAC) /* Invalidate cache for max_mpu_regions addr */
+
     /* x0: region sel */
     mov   r0, #0
     /* Xen text section. */
@@ -83,6 +87,27 @@ FUNC(enable_boot_cpu_mm)
     prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
 #endif
 
+zero_mpu:
+    /* Reset remaining MPU regions */
+    cmp   r0, r5
+    beq   out_zero_mpu
+    mov   r1, #0
+    mov   r2, #1
+    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prlar=REGION_DISABLED_PRLAR
+    b     zero_mpu
+
+out_zero_mpu:
+    /* Invalidate data cache for MPU data structures */
+    mov r4, lr
+    mov_w r0, xen_mpumap_mask
+    mov r1, #XEN_MPUMAP_MASK_sizeof
+    bl __invalidate_dcache_area
+
+    ldr r0, =xen_mpumap
+    mov r1, #XEN_MPUMAP_sizeof
+    bl __invalidate_dcache_area
+    mov lr, r4
+
     b    enable_mpu
 END(enable_boot_cpu_mm)
 
diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
index 6b8c233e6c..631b0b2b86 100644
--- a/xen/arch/arm/include/asm/mpu/regions.inc
+++ b/xen/arch/arm/include/asm/mpu/regions.inc
@@ -24,7 +24,7 @@
 #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
 
 .macro store_pair reg1, reg2, dst
-    .word 0xe7f000f0                    /* unimplemented */
+    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register than reg1 */
 .endm
 
 #endif
-- 
2.25.1

[PATCH v2 3/3] arm/mpu: Provide access to the MPU region from the C code

[Ayan Kumar Halder]

Enable the helper functions defined in mpu/mm.c and asm/mpu.h for ARM32.
Define the register definitions for HPRBAR{0..31} and HPRLAR{0..31}.
One can directly access the first 32 MPU regions using the above registers
without the use of PRSELR.

Also fix the register definition for HPRLAR.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Change from :-

v1 - 1. Enable write_protection_region() for aarch32.

 xen/arch/arm/include/asm/mpu.h        |  2 -
 xen/arch/arm/include/asm/mpu/cpregs.h | 72 ++++++++++++++++++++++++++-
 xen/arch/arm/mpu/mm.c                 | 49 ++++++++++++++++--
 3 files changed, 116 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index 8f06ddac0f..63560c613b 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -25,7 +25,6 @@
 
 #ifndef __ASSEMBLY__
 
-#ifdef CONFIG_ARM_64
 /*
  * Set base address of MPU protection region.
  *
@@ -85,7 +84,6 @@ static inline bool region_is_valid(const pr_t *pr)
 {
     return pr->prlar.reg.en;
 }
-#endif /* CONFIG_ARM_64 */
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
index d5cd0e04d5..9f3b32acd7 100644
--- a/xen/arch/arm/include/asm/mpu/cpregs.h
+++ b/xen/arch/arm/include/asm/mpu/cpregs.h
@@ -6,16 +6,86 @@
 /* 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
+#define HPRLAR          p15,4,c6,c3,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 PRENR_EL2       HPRENR
 #define PRLAR_EL2       HPRLAR
 #define PRSELR_EL2      HPRSELR
 #endif /* CONFIG_ARM_32 */
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 2fb6b822c6..74e96ca571 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -40,7 +40,10 @@ pr_t __cacheline_aligned __section(".data") xen_mpumap[MAX_MPU_REGION_NR];
 #define PRBAR_EL2_(n)   PRBAR##n##_EL2
 #define PRLAR_EL2_(n)   PRLAR##n##_EL2
 
-#endif /* CONFIG_ARM_64 */
+#else  /* CONFIG_ARM_64 */
+#define PRBAR_EL2_(n)   HPRBAR##n
+#define PRLAR_EL2_(n)   HPRLAR##n
+#endif /* !CONFIG_ARM_64 */
 
 #define GENERATE_WRITE_PR_REG_CASE(num, pr)                                 \
     case num:                                                               \
@@ -68,7 +71,6 @@ static void __init __maybe_unused build_assertions(void)
     BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
 }
 
-#ifdef CONFIG_ARM_64
 /*
  * Armv8-R supports direct access and indirect access to the MPU regions through
  * registers:
@@ -85,6 +87,7 @@ static void __init __maybe_unused build_assertions(void)
  */
 static void prepare_selector(uint8_t *sel)
 {
+#ifdef CONFIG_ARM_64
     uint8_t cur_sel = *sel;
 
     /*
@@ -98,7 +101,8 @@ static void prepare_selector(uint8_t *sel)
         WRITE_SYSREG(cur_sel, PRSELR_EL2);
         isb();
     }
-    *sel &= 0xFU;
+    *sel = *sel & 0xFU;
+#endif
 }
 
 void read_protection_region(pr_t *pr_read, uint8_t sel)
@@ -123,6 +127,24 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
         GENERATE_READ_PR_REG_CASE(13, pr_read);
         GENERATE_READ_PR_REG_CASE(14, pr_read);
         GENERATE_READ_PR_REG_CASE(15, pr_read);
+#ifdef CONFIG_ARM_32
+        GENERATE_READ_PR_REG_CASE(16, pr_read);
+        GENERATE_READ_PR_REG_CASE(17, pr_read);
+        GENERATE_READ_PR_REG_CASE(18, pr_read);
+        GENERATE_READ_PR_REG_CASE(19, pr_read);
+        GENERATE_READ_PR_REG_CASE(20, pr_read);
+        GENERATE_READ_PR_REG_CASE(21, pr_read);
+        GENERATE_READ_PR_REG_CASE(22, pr_read);
+        GENERATE_READ_PR_REG_CASE(23, pr_read);
+        GENERATE_READ_PR_REG_CASE(24, pr_read);
+        GENERATE_READ_PR_REG_CASE(25, pr_read);
+        GENERATE_READ_PR_REG_CASE(26, pr_read);
+        GENERATE_READ_PR_REG_CASE(27, pr_read);
+        GENERATE_READ_PR_REG_CASE(28, pr_read);
+        GENERATE_READ_PR_REG_CASE(29, pr_read);
+        GENERATE_READ_PR_REG_CASE(30, pr_read);
+        GENERATE_READ_PR_REG_CASE(31, pr_read);
+#endif
     default:
         BUG(); /* Can't happen */
         break;
@@ -151,6 +173,24 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel)
         GENERATE_WRITE_PR_REG_CASE(13, pr_write);
         GENERATE_WRITE_PR_REG_CASE(14, pr_write);
         GENERATE_WRITE_PR_REG_CASE(15, pr_write);
+#ifdef CONFIG_ARM_32
+        GENERATE_WRITE_PR_REG_CASE(16, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(17, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(18, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(19, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(20, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(21, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(22, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(23, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(24, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(25, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(26, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(27, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(28, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(29, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(30, pr_write);
+        GENERATE_WRITE_PR_REG_CASE(31, pr_write);
+#endif
     default:
         BUG(); /* Can't happen */
         break;
@@ -208,7 +248,9 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
     /* 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_idx,
             .en = 1,        /* Region enabled */
         }};
@@ -225,7 +267,6 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
 
     return region;
 }
-#endif /* CONFIG_ARM_64 */
 
 void __init setup_mm(void)
 {
-- 
2.25.1

Re: [PATCH v2 1/3] arm/mpu: Introduce MPU memory region map structure

[Orzel, Michal]

On 06/06/2025 18:48, 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.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from v1 :-
> 
> 1. Preserve pr_t typedef in arch specific files.
> 
> 2. Fix typo.
> 
>  xen/arch/arm/include/asm/arm32/mpu.h | 34 ++++++++++++++++++++++++++--
>  xen/arch/arm/mpu/mm.c                |  2 ++
>  2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
> index f0d4d4055c..fe139a2abe 100644
> --- a/xen/arch/arm/include/asm/arm32/mpu.h
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -5,10 +5,40 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +/*
> + * Unlike arm64, there are no reserved 0 bits beyond base and limit bitfield in
> + * prbar and prlar registers respectively.
> + */
> +#define MPU_REGION_RES0       0x0
> +
> +/* Hypervisor Protection Region Base Address Register */
> +typedef union {
> +    struct {
> +        unsigned int xn:1;       /* Execute-Never */
> +        unsigned int ap_0:1;     /* Access Permission AP[0] */
> +        unsigned long ro:1;      /* Access Permission AP[1] */
It should be unsigned int, not long.

With that fixed:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Orzel, Michal]

On 06/06/2025 18:48, Ayan Kumar Halder wrote:
> Modify Arm32 assembly boot code to reset any unused MPU region, initialise
> 'max_mpu_regions' with the number of supported MPU regions and set/clear the
> bitmap 'xen_mpumap_mask' used to track the enabled regions.
> 
> Use the macro definition for "dcache_line_size" from linux.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from v1 :-
> 
> 1. Introduce cache.S to hold arm32 cache initialization instructions.
> 
> 2. Use dcache_line_size macro definition from linux.
> 
> 3. Use mov_w instead of ldr.
> 
> 4. Use a single stm instruction for 'store_pair' macro definition.
> 
>  xen/arch/arm/arm32/Makefile              |  1 +
>  xen/arch/arm/arm32/asm-offsets.c         |  6 ++++
>  xen/arch/arm/arm32/cache.S               | 41 ++++++++++++++++++++++++
>  xen/arch/arm/arm32/mpu/head.S            | 25 +++++++++++++++
>  xen/arch/arm/include/asm/mpu/regions.inc |  2 +-
>  5 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/arm32/cache.S
> 
> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> index 537969d753..531168f58a 100644
> --- a/xen/arch/arm/arm32/Makefile
> +++ b/xen/arch/arm/arm32/Makefile
> @@ -2,6 +2,7 @@ obj-y += lib/
>  obj-$(CONFIG_MMU) += mmu/
>  obj-$(CONFIG_MPU) += mpu/
>  
> +obj-y += cache.o
>  obj-$(CONFIG_EARLY_PRINTK) += debug.o
>  obj-y += domctl.o
>  obj-y += domain.o
> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
> index 8bbb0f938e..c203ce269d 100644
> --- a/xen/arch/arm/arm32/asm-offsets.c
> +++ b/xen/arch/arm/arm32/asm-offsets.c
> @@ -75,6 +75,12 @@ void __dummy__(void)
>  
>     OFFSET(INITINFO_stack, struct init_info, stack);
>     BLANK();
> +
> +#ifdef CONFIG_MPU
> +   DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
> +   DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
> +   BLANK();
> +#endif
>  }
>  
>  /*
> diff --git a/xen/arch/arm/arm32/cache.S b/xen/arch/arm/arm32/cache.S
> new file mode 100644
> index 0000000000..00ea390ce4
> --- /dev/null
> +++ b/xen/arch/arm/arm32/cache.S
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Cache maintenance */
> +
> +#include <asm/arm32/sysregs.h>
> +
> +/* dcache_line_size - get the minimum D-cache line size from the CTR register */
> +    .macro  dcache_line_size, reg, tmp
> +    mrc p15, 0, \tmp, c0, c0, 1     /* read ctr */
Why open coding macro CTR? Especially if below you use DCIMVAC.

> +    lsr \tmp, \tmp, #16
> +    and \tmp, \tmp, #0xf            /* cache line size encoding */
> +    mov \reg, #4                    /* bytes per word */
> +    mov \reg, \reg, lsl \tmp        /* actual cache line size */
> +    .endm
> +
> +/*
> + * __invalidate_dcache_area(addr, size)
> + *
> + * Ensure that the data held in the cache for the buffer is invalidated.
> + *
> + * - addr - start address of the buffer
> + * - size - size of the buffer
> + */
I do think that for new functions in assembly we should write what registers are
clobbered. Arm64 cache.S originated from Linux, hence it lacks this information.

> +FUNC(__invalidate_dcache_area)
> +    dcache_line_size r2, r3
> +    add   r1, r0, r1
> +    sub   r3, r2, #1
> +    bic   r0, r0, r3
> +1:  mcr   CP32(r0, DCIMVAC)     /* invalidate D line / unified line */
> +    add   r0, r0, r2
> +    cmp   r0, r1
> +    blo   1b
> +    dsb   sy
> +    ret
> +END(__invalidate_dcache_area)
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
> index b2c5245e51..435b140d09 100644
> --- a/xen/arch/arm/arm32/mpu/head.S
> +++ b/xen/arch/arm/arm32/mpu/head.S
> @@ -49,6 +49,10 @@ FUNC(enable_boot_cpu_mm)
>      mrc   CP32(r5, MPUIR_EL2)
>      and   r5, r5, #NUM_MPU_REGIONS_MASK
>  
> +    mov_w   r0, max_mpu_regions
> +    str   r5, [r0]
> +    mcr   CP32(r0, DCIMVAC) /* Invalidate cache for max_mpu_regions addr */
> +
>      /* x0: region sel */
>      mov   r0, #0
>      /* Xen text section. */
> @@ -83,6 +87,27 @@ FUNC(enable_boot_cpu_mm)
>      prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
>  #endif
>  
> +zero_mpu:
> +    /* Reset remaining MPU regions */
> +    cmp   r0, r5
> +    beq   out_zero_mpu
> +    mov   r1, #0
> +    mov   r2, #1
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prlar=REGION_DISABLED_PRLAR
> +    b     zero_mpu
> +
> +out_zero_mpu:
> +    /* Invalidate data cache for MPU data structures */
> +    mov r4, lr
> +    mov_w r0, xen_mpumap_mask
> +    mov r1, #XEN_MPUMAP_MASK_sizeof
> +    bl __invalidate_dcache_area
> +
> +    ldr r0, =xen_mpumap
> +    mov r1, #XEN_MPUMAP_sizeof
> +    bl __invalidate_dcache_area
> +    mov lr, r4
> +
>      b    enable_mpu
>  END(enable_boot_cpu_mm)
>  
> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
> index 6b8c233e6c..631b0b2b86 100644
> --- a/xen/arch/arm/include/asm/mpu/regions.inc
> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
> @@ -24,7 +24,7 @@
>  #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
>  
>  .macro store_pair reg1, reg2, dst
> -    .word 0xe7f000f0                    /* unimplemented */
> +    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register than reg1 */
Didn't we agree not to use STM (I suggested it but then Julien pointed out that
it's use in macro might not be the best)?

~Michal

Re: [PATCH v2 3/3] arm/mpu: Provide access to the MPU region from the C code

[Orzel, Michal]

On 06/06/2025 18:48, Ayan Kumar Halder wrote:
> Enable the helper functions defined in mpu/mm.c and asm/mpu.h for ARM32.
> Define the register definitions for HPRBAR{0..31} and HPRLAR{0..31}.
> One can directly access the first 32 MPU regions using the above registers
> without the use of PRSELR.
> 
> Also fix the register definition for HPRLAR.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
It looks good apart from ...

[snip]

> @@ -98,7 +101,8 @@ static void prepare_selector(uint8_t *sel)
>          WRITE_SYSREG(cur_sel, PRSELR_EL2);
>          isb();
>      }
> -    *sel &= 0xFU;
> +    *sel = *sel & 0xFU;
this change. Why?

Apart from that:
Acked-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Ayan Kumar Halder]

Hi Michal,

On 09/06/2025 08:41, Orzel, Michal wrote:
>
> On 06/06/2025 18:48, Ayan Kumar Halder wrote:
>> Modify Arm32 assembly boot code to reset any unused MPU region, initialise
>> 'max_mpu_regions' with the number of supported MPU regions and set/clear the
>> bitmap 'xen_mpumap_mask' used to track the enabled regions.
>>
>> Use the macro definition for "dcache_line_size" from linux.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from v1 :-
>>
>> 1. Introduce cache.S to hold arm32 cache initialization instructions.
>>
>> 2. Use dcache_line_size macro definition from linux.
>>
>> 3. Use mov_w instead of ldr.
>>
>> 4. Use a single stm instruction for 'store_pair' macro definition.
>>
>>   xen/arch/arm/arm32/Makefile              |  1 +
>>   xen/arch/arm/arm32/asm-offsets.c         |  6 ++++
>>   xen/arch/arm/arm32/cache.S               | 41 ++++++++++++++++++++++++
>>   xen/arch/arm/arm32/mpu/head.S            | 25 +++++++++++++++
>>   xen/arch/arm/include/asm/mpu/regions.inc |  2 +-
>>   5 files changed, 74 insertions(+), 1 deletion(-)
>>   create mode 100644 xen/arch/arm/arm32/cache.S
>>
>> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
>> index 537969d753..531168f58a 100644
>> --- a/xen/arch/arm/arm32/Makefile
>> +++ b/xen/arch/arm/arm32/Makefile
>> @@ -2,6 +2,7 @@ obj-y += lib/
>>   obj-$(CONFIG_MMU) += mmu/
>>   obj-$(CONFIG_MPU) += mpu/
>>   
>> +obj-y += cache.o
>>   obj-$(CONFIG_EARLY_PRINTK) += debug.o
>>   obj-y += domctl.o
>>   obj-y += domain.o
>> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
>> index 8bbb0f938e..c203ce269d 100644
>> --- a/xen/arch/arm/arm32/asm-offsets.c
>> +++ b/xen/arch/arm/arm32/asm-offsets.c
>> @@ -75,6 +75,12 @@ void __dummy__(void)
>>   
>>      OFFSET(INITINFO_stack, struct init_info, stack);
>>      BLANK();
>> +
>> +#ifdef CONFIG_MPU
>> +   DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
>> +   DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
>> +   BLANK();
>> +#endif
>>   }
>>   
>>   /*
>> diff --git a/xen/arch/arm/arm32/cache.S b/xen/arch/arm/arm32/cache.S
>> new file mode 100644
>> index 0000000000..00ea390ce4
>> --- /dev/null
>> +++ b/xen/arch/arm/arm32/cache.S
>> @@ -0,0 +1,41 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Cache maintenance */
>> +
>> +#include <asm/arm32/sysregs.h>
>> +
>> +/* dcache_line_size - get the minimum D-cache line size from the CTR register */
>> +    .macro  dcache_line_size, reg, tmp
>> +    mrc p15, 0, \tmp, c0, c0, 1     /* read ctr */
> Why open coding macro CTR? Especially if below you use DCIMVAC.
>
>> +    lsr \tmp, \tmp, #16
>> +    and \tmp, \tmp, #0xf            /* cache line size encoding */
>> +    mov \reg, #4                    /* bytes per word */
>> +    mov \reg, \reg, lsl \tmp        /* actual cache line size */
>> +    .endm
>> +
>> +/*
>> + * __invalidate_dcache_area(addr, size)
>> + *
>> + * Ensure that the data held in the cache for the buffer is invalidated.
>> + *
>> + * - addr - start address of the buffer
>> + * - size - size of the buffer
>> + */
> I do think that for new functions in assembly we should write what registers are
> clobbered. Arm64 cache.S originated from Linux, hence it lacks this information.
>
>> +FUNC(__invalidate_dcache_area)
>> +    dcache_line_size r2, r3
>> +    add   r1, r0, r1
>> +    sub   r3, r2, #1
>> +    bic   r0, r0, r3
>> +1:  mcr   CP32(r0, DCIMVAC)     /* invalidate D line / unified line */
>> +    add   r0, r0, r2
>> +    cmp   r0, r1
>> +    blo   1b
>> +    dsb   sy
>> +    ret
>> +END(__invalidate_dcache_area)
>> +
>> +/*
>> + * Local variables:
>> + * mode: ASM
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
>> index b2c5245e51..435b140d09 100644
>> --- a/xen/arch/arm/arm32/mpu/head.S
>> +++ b/xen/arch/arm/arm32/mpu/head.S
>> @@ -49,6 +49,10 @@ FUNC(enable_boot_cpu_mm)
>>       mrc   CP32(r5, MPUIR_EL2)
>>       and   r5, r5, #NUM_MPU_REGIONS_MASK
>>   
>> +    mov_w   r0, max_mpu_regions
>> +    str   r5, [r0]
>> +    mcr   CP32(r0, DCIMVAC) /* Invalidate cache for max_mpu_regions addr */
>> +
>>       /* x0: region sel */
>>       mov   r0, #0
>>       /* Xen text section. */
>> @@ -83,6 +87,27 @@ FUNC(enable_boot_cpu_mm)
>>       prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
>>   #endif
>>   
>> +zero_mpu:
>> +    /* Reset remaining MPU regions */
>> +    cmp   r0, r5
>> +    beq   out_zero_mpu
>> +    mov   r1, #0
>> +    mov   r2, #1
>> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prlar=REGION_DISABLED_PRLAR
>> +    b     zero_mpu
>> +
>> +out_zero_mpu:
>> +    /* Invalidate data cache for MPU data structures */
>> +    mov r4, lr
>> +    mov_w r0, xen_mpumap_mask
>> +    mov r1, #XEN_MPUMAP_MASK_sizeof
>> +    bl __invalidate_dcache_area
>> +
>> +    ldr r0, =xen_mpumap
>> +    mov r1, #XEN_MPUMAP_sizeof
>> +    bl __invalidate_dcache_area
>> +    mov lr, r4
>> +
>>       b    enable_mpu
>>   END(enable_boot_cpu_mm)
>>   
>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
>> index 6b8c233e6c..631b0b2b86 100644
>> --- a/xen/arch/arm/include/asm/mpu/regions.inc
>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
>> @@ -24,7 +24,7 @@
>>   #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
>>   
>>   .macro store_pair reg1, reg2, dst
>> -    .word 0xe7f000f0                    /* unimplemented */
>> +    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register than reg1 */
> Didn't we agree not to use STM (I suggested it but then Julien pointed out that
> it's use in macro might not be the best)?

Ah my last response was not sent.

I realized that I cannot use strd due to the following error

Error: first transfer register must be even -- `strd r3,r4,[r1]'

So,  I am using stm with the following comment

stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register than reg1 */

- Ayan

>
> ~Michal
>

Re: [PATCH v2 3/3] arm/mpu: Provide access to the MPU region from the C code

[Luca Fancellu]

Hi Ayan,

If I understand correctly Armv8-R AArch32 supports up to 255 regions, so I would expect ...

> /*
>  * Armv8-R supports direct access and indirect access to the MPU regions through
>  * registers:
> @@ -85,6 +87,7 @@ static void __init __maybe_unused build_assertions(void)
>  */
> static void prepare_selector(uint8_t *sel)
> {
> +#ifdef CONFIG_ARM_64
>     uint8_t cur_sel = *sel;
> 
>     /*
> @@ -98,7 +101,8 @@ static void prepare_selector(uint8_t *sel)
>         WRITE_SYSREG(cur_sel, PRSELR_EL2);
>         isb();
>     }
> -    *sel &= 0xFU;
> +    *sel = *sel & 0xFU;
> +#endif

something here to check if the selector is 0-31 or not and:

- set the selector to 0 if set is 0-31
- set the selector to 32-255 if sel > 32

And ...


> }
> 
> void read_protection_region(pr_t *pr_read, uint8_t sel)
> @@ -123,6 +127,24 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
>         GENERATE_READ_PR_REG_CASE(13, pr_read);
>         GENERATE_READ_PR_REG_CASE(14, pr_read);
>         GENERATE_READ_PR_REG_CASE(15, pr_read);
> +#ifdef CONFIG_ARM_32
> +        GENERATE_READ_PR_REG_CASE(16, pr_read);
> +        GENERATE_READ_PR_REG_CASE(17, pr_read);
> +        GENERATE_READ_PR_REG_CASE(18, pr_read);
> +        GENERATE_READ_PR_REG_CASE(19, pr_read);
> +        GENERATE_READ_PR_REG_CASE(20, pr_read);
> +        GENERATE_READ_PR_REG_CASE(21, pr_read);
> +        GENERATE_READ_PR_REG_CASE(22, pr_read);
> +        GENERATE_READ_PR_REG_CASE(23, pr_read);
> +        GENERATE_READ_PR_REG_CASE(24, pr_read);
> +        GENERATE_READ_PR_REG_CASE(25, pr_read);
> +        GENERATE_READ_PR_REG_CASE(26, pr_read);
> +        GENERATE_READ_PR_REG_CASE(27, pr_read);
> +        GENERATE_READ_PR_REG_CASE(28, pr_read);
> +        GENERATE_READ_PR_REG_CASE(29, pr_read);
> +        GENERATE_READ_PR_REG_CASE(30, pr_read);
> +        GENERATE_READ_PR_REG_CASE(31, pr_read);
> +#endif
>     default:

have something here for Arm32 to access the regions 32-255


>         BUG(); /* Can't happen */
>         break;
> @@ -151,6 +173,24 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel)
>         GENERATE_WRITE_PR_REG_CASE(13, pr_write);
>         GENERATE_WRITE_PR_REG_CASE(14, pr_write);
>         GENERATE_WRITE_PR_REG_CASE(15, pr_write);
> +#ifdef CONFIG_ARM_32
> +        GENERATE_WRITE_PR_REG_CASE(16, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(17, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(18, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(19, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(20, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(21, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(22, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(23, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(24, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(25, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(26, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(27, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(28, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(29, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(30, pr_write);
> +        GENERATE_WRITE_PR_REG_CASE(31, pr_write);
> +#endif
>     default:

also here have something for Arm32 to access the regions 32-255

>         BUG(); /* Can't happen */
>         break;


Please let me know your thoughts.

Cheers,
Luca

Re: [PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Julien Grall]

Hi Ayan,

On 09/06/2025 09:27, Ayan Kumar Halder wrote:
> On 09/06/2025 08:41, Orzel, Michal wrote:
>>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/ 
>>> include/asm/mpu/regions.inc
>>> index 6b8c233e6c..631b0b2b86 100644
>>> --- a/xen/arch/arm/include/asm/mpu/regions.inc
>>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
>>> @@ -24,7 +24,7 @@
>>>   #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
>>>   .macro store_pair reg1, reg2, dst
>>> -    .word 0xe7f000f0                    /* unimplemented */
>>> +    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register 
>>> than reg1 */
>> Didn't we agree not to use STM (I suggested it but then Julien pointed 
>> out that
>> it's use in macro might not be the best)?
> 
> Ah my last response was not sent.
> 
> I realized that I cannot use strd due to the following error
> 
> Error: first transfer register must be even -- `strd r3,r4,[r1]'

Ah I missed the "even" part when reading the specification. However, we 
control the set of registers, so we can't we follow the restriction? 
This would be better...

> 
> So,  I am using stm with the following comment

... than a comment and hoping the developper/reviewer will notice it 
(the code is also misplaced). I am ready to bet this will likely cause 
some problem in the future.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/3] arm/mpu: Provide access to the MPU region from the C code

[Luca Fancellu]

Oh sorry forgot one thing ...
> 
>> /*
>> * Armv8-R supports direct access and indirect access to the MPU regions through
>> * registers:
>> @@ -85,6 +87,7 @@ static void __init __maybe_unused build_assertions(void)
>> */
>> static void prepare_selector(uint8_t *sel)
>> {
>> +#ifdef CONFIG_ARM_64
>>    uint8_t cur_sel = *sel;
>> 
>>    /*
>> @@ -98,7 +101,8 @@ static void prepare_selector(uint8_t *sel)
>>        WRITE_SYSREG(cur_sel, PRSELR_EL2);
>>        isb();
>>    }
>> -    *sel &= 0xFU;
>> +    *sel = *sel & 0xFU;

this line is not part of this commit, maybe rebase clash? 

Cheers,
Luca

Re: [PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Ayan Kumar Halder]

On 09/06/2025 09:42, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> On 09/06/2025 09:27, Ayan Kumar Halder wrote:
>> On 09/06/2025 08:41, Orzel, Michal wrote:
>>>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc 
>>>> b/xen/arch/arm/ include/asm/mpu/regions.inc
>>>> index 6b8c233e6c..631b0b2b86 100644
>>>> --- a/xen/arch/arm/include/asm/mpu/regions.inc
>>>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
>>>> @@ -24,7 +24,7 @@
>>>>   #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
>>>>   .macro store_pair reg1, reg2, dst
>>>> -    .word 0xe7f000f0                    /* unimplemented */
>>>> +    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register 
>>>> than reg1 */
>>> Didn't we agree not to use STM (I suggested it but then Julien 
>>> pointed out that
>>> it's use in macro might not be the best)?
>>
>> Ah my last response was not sent.
>>
>> I realized that I cannot use strd due to the following error
>>
>> Error: first transfer register must be even -- `strd r3,r4,[r1]'
>
> Ah I missed the "even" part when reading the specification. However, 
> we control the set of registers, so we can't we follow the 
> restriction? This would be better...

I am ok to follow this. I just want to make sure that this looks ok to you

prepare_xen_region() invokes store_pair(). They are in common header.

So we need to make the change wherever prepare_xen_region() is invoked 
from arm32/mpu/head.S. This would look like

--- a/xen/arch/arm/arm32/mpu/head.S
+++ b/xen/arch/arm/arm32/mpu/head.S
@@ -58,33 +58,33 @@ FUNC(enable_boot_cpu_mm)
      /* Xen text section. */
      mov_w   r1, _stext
      mov_w   r2, _etext
-    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
+    prepare_xen_region r0, r1, r2, r4, r3, r5, attr_prbar=REGION_TEXT_PRBAR

      /* Xen read-only data section. */
      mov_w   r1, _srodata
      mov_w   r2, _erodata
-    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_RO_PRBAR
+    prepare_xen_region r0, r1, r2, r4, r3, r5, attr_prbar=REGION_RO_PRBAR

      /* Xen read-only after init and data section. (RW data) */
      mov_w   r1, __ro_after_init_start
      mov_w   r2, __init_begin
-    prepare_xen_region r0, r1, r2, r3, r4, r5
+    prepare_xen_region r0, r1, r2, r4, r3, r5

      /* Xen code section. */
      mov_w   r1, __init_begin
      mov_w   r2, __init_data_begin
-    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
+    prepare_xen_region r0, r1, r2, r4, r3, r5, attr_prbar=REGION_TEXT_PRBAR

      /* Xen data and BSS section. */
      mov_w   r1, __init_data_begin
      mov_w   r2, __bss_end
-    prepare_xen_region r0, r1, r2, r3, r4, r5
+    prepare_xen_region r0, r1, r2, r4, r3, r5

  #ifdef CONFIG_EARLY_PRINTK
      /* Xen early UART section. */
      mov_w   r1, CONFIG_EARLY_UART_BASE_ADDRESS
      mov_w   r2, (CONFIG_EARLY_UART_BASE_ADDRESS + CONFIG_EARLY_UART_SIZE)
-    prepare_xen_region r0, r1, r2, r3, r4, r5, 
attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
+    prepare_xen_region r0, r1, r2, r4, r3, r5, 
attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
  #endif

  zero_mpu:
@@ -93,7 +93,7 @@ zero_mpu:
      beq   out_zero_mpu
      mov   r1, #0
      mov   r2, #1
-    prepare_xen_region r0, r1, r2, r3, r4, r5, 
attr_prlar=REGION_DISABLED_PRLAR
+    prepare_xen_region r0, r1, r2, r4, r3, r5, 
attr_prlar=REGION_DISABLED_PRLAR

So this would look a different different from its arm64 counterpart.

Are you ok with this change ?

- Ayan

>
>>
>> So,  I am using stm with the following comment
>
> ... than a comment and hoping the developper/reviewer will notice it 
> (the code is also misplaced). I am ready to bet this will likely cause 
> some problem in the future.
>
> Cheers,
>

Re: [PATCH v2 3/3] arm/mpu: Provide access to the MPU region from the C code

[Ayan Kumar Halder]

On 09/06/2025 09:37, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
>
> If I understand correctly Armv8-R AArch32 supports up to 255 regions, so I would expect ...
>
>> /*
>>   * Armv8-R supports direct access and indirect access to the MPU regions through
>>   * registers:
>> @@ -85,6 +87,7 @@ static void __init __maybe_unused build_assertions(void)
>>   */
>> static void prepare_selector(uint8_t *sel)
>> {
>> +#ifdef CONFIG_ARM_64
>>      uint8_t cur_sel = *sel;
>>
>>      /*
>> @@ -98,7 +101,8 @@ static void prepare_selector(uint8_t *sel)
>>          WRITE_SYSREG(cur_sel, PRSELR_EL2);
>>          isb();
>>      }
>> -    *sel &= 0xFU;
>> +    *sel = *sel & 0xFU;
>> +#endif
> something here to check if the selector is 0-31 or not and:
>
> - set the selector to 0 if set is 0-31
> - set the selector to 32-255 if sel > 32

yes, good catch. I was initially thinking of supporting only the first 
32 regions for arm32. So, it would BUG() for region numbered 32 onwards.

I can expand the patch to support all the 255 regions.

>
> And ...
>
>
>> }
>>
>> void read_protection_region(pr_t *pr_read, uint8_t sel)
>> @@ -123,6 +127,24 @@ void read_protection_region(pr_t *pr_read, uint8_t sel)
>>          GENERATE_READ_PR_REG_CASE(13, pr_read);
>>          GENERATE_READ_PR_REG_CASE(14, pr_read);
>>          GENERATE_READ_PR_REG_CASE(15, pr_read);
>> +#ifdef CONFIG_ARM_32
>> +        GENERATE_READ_PR_REG_CASE(16, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(17, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(18, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(19, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(20, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(21, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(22, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(23, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(24, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(25, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(26, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(27, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(28, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(29, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(30, pr_read);
>> +        GENERATE_READ_PR_REG_CASE(31, pr_read);
>> +#endif
>>      default:
> have something here for Arm32 to access the regions 32-255
>
>
>>          BUG(); /* Can't happen */
>>          break;
>> @@ -151,6 +173,24 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel)
>>          GENERATE_WRITE_PR_REG_CASE(13, pr_write);
>>          GENERATE_WRITE_PR_REG_CASE(14, pr_write);
>>          GENERATE_WRITE_PR_REG_CASE(15, pr_write);
>> +#ifdef CONFIG_ARM_32
>> +        GENERATE_WRITE_PR_REG_CASE(16, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(17, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(18, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(19, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(20, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(21, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(22, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(23, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(24, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(25, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(26, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(27, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(28, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(29, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(30, pr_write);
>> +        GENERATE_WRITE_PR_REG_CASE(31, pr_write);
>> +#endif
>>      default:
> also here have something for Arm32 to access the regions 32-255
>
>>          BUG(); /* Can't happen */
>>          break;
>
> Please let me know your thoughts.

Ack

- Ayan

>
> Cheers,
> Luca
>
>

Re: [PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Ayan Kumar Halder]

Hi,

On 09/06/2025 10:16, Ayan Kumar Halder wrote:
>
> On 09/06/2025 09:42, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien,
>>
>> On 09/06/2025 09:27, Ayan Kumar Halder wrote:
>>> On 09/06/2025 08:41, Orzel, Michal wrote:
>>>>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc 
>>>>> b/xen/arch/arm/ include/asm/mpu/regions.inc
>>>>> index 6b8c233e6c..631b0b2b86 100644
>>>>> --- a/xen/arch/arm/include/asm/mpu/regions.inc
>>>>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
>>>>> @@ -24,7 +24,7 @@
>>>>>   #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
>>>>>   .macro store_pair reg1, reg2, dst
>>>>> -    .word 0xe7f000f0                    /* unimplemented */
>>>>> +    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register 
>>>>> than reg1 */
>>>> Didn't we agree not to use STM (I suggested it but then Julien 
>>>> pointed out that
>>>> it's use in macro might not be the best)?
>>>
>>> Ah my last response was not sent.
>>>
>>> I realized that I cannot use strd due to the following error
>>>
>>> Error: first transfer register must be even -- `strd r3,r4,[r1]'
>>
>> Ah I missed the "even" part when reading the specification. However, 
>> we control the set of registers, so we can't we follow the 
>> restriction? This would be better...
>
> I am ok to follow this. I just want to make sure that this looks ok to 
> you
>
> prepare_xen_region() invokes store_pair(). They are in common header.
>
> So we need to make the change wherever prepare_xen_region() is invoked 
> from arm32/mpu/head.S. This would look like
>
> --- a/xen/arch/arm/arm32/mpu/head.S
> +++ b/xen/arch/arm/arm32/mpu/head.S
> @@ -58,33 +58,33 @@ FUNC(enable_boot_cpu_mm)
>      /* Xen text section. */
>      mov_w   r1, _stext
>      mov_w   r2, _etext
> -    prepare_xen_region r0, r1, r2, r3, r4, r5, 
> attr_prbar=REGION_TEXT_PRBAR
> +    prepare_xen_region r0, r1, r2, r4, r3, r5, 
> attr_prbar=REGION_TEXT_PRBAR

My mistake, It should be

prepare_xen_region r0, r1, r2, r4, r5, r6, attr_prbar=REGION_TEXT_PRBAR

We will be clobbering an extra register.

- Ayan

Re: [PATCH v2 2/3] arm/mpu: Provide and populate MPU C data structures

[Julien Grall]

Hi Ayan,

On 09/06/2025 11:43, Ayan Kumar Halder wrote:
> Hi,
> 
> On 09/06/2025 10:16, Ayan Kumar Halder wrote:
>>
>> On 09/06/2025 09:42, Julien Grall wrote:
>>> Hi Ayan,
>> Hi Julien,
>>>
>>> On 09/06/2025 09:27, Ayan Kumar Halder wrote:
>>>> On 09/06/2025 08:41, Orzel, Michal wrote:
>>>>>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/ 
>>>>>> arm/ include/asm/mpu/regions.inc
>>>>>> index 6b8c233e6c..631b0b2b86 100644
>>>>>> --- a/xen/arch/arm/include/asm/mpu/regions.inc
>>>>>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
>>>>>> @@ -24,7 +24,7 @@
>>>>>>   #define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
>>>>>>   .macro store_pair reg1, reg2, dst
>>>>>> -    .word 0xe7f000f0                    /* unimplemented */
>>>>>> +    stm \dst, {\reg1, \reg2}  /* reg2 should be a higher register 
>>>>>> than reg1 */
>>>>> Didn't we agree not to use STM (I suggested it but then Julien 
>>>>> pointed out that
>>>>> it's use in macro might not be the best)?
>>>>
>>>> Ah my last response was not sent.
>>>>
>>>> I realized that I cannot use strd due to the following error
>>>>
>>>> Error: first transfer register must be even -- `strd r3,r4,[r1]'
>>>
>>> Ah I missed the "even" part when reading the specification. However, 
>>> we control the set of registers, so we can't we follow the 
>>> restriction? This would be better...
>>
>> I am ok to follow this. I just want to make sure that this looks ok to 
>> you
>>
>> prepare_xen_region() invokes store_pair(). They are in common header.
>>
>> So we need to make the change wherever prepare_xen_region() is invoked 
>> from arm32/mpu/head.S. This would look like
>>
>> --- a/xen/arch/arm/arm32/mpu/head.S
>> +++ b/xen/arch/arm/arm32/mpu/head.S
>> @@ -58,33 +58,33 @@ FUNC(enable_boot_cpu_mm)
>>      /* Xen text section. */
>>      mov_w   r1, _stext
>>      mov_w   r2, _etext
>> -    prepare_xen_region r0, r1, r2, r3, r4, r5, 
>> attr_prbar=REGION_TEXT_PRBAR
>> +    prepare_xen_region r0, r1, r2, r4, r3, r5, 
>> attr_prbar=REGION_TEXT_PRBAR
> 
> My mistake, It should be
> 
> prepare_xen_region r0, r1, r2, r4, r5, r6, attr_prbar=REGION_TEXT_PRBAR
> 
> We will be clobbering an extra register.

Why can't we use r3 instead of r6?

 > So this would look a different different from its arm64 counterpart.

I don't see the problem with that. The code is not common. And TBH 
prepare_xen_region should never have been common ... but I will not 
re-open this discussion :).

Cheers,

-- 
Julien Grall