[PATCH 0/5] Enable early bootup of AArch32 MPU systems

[PATCH 0/5] Enable early bootup of AArch32 MPU systems

[Ayan Kumar Halder]

Enabled early booting of R52.

Ayan Kumar Halder (5):
  xen/arm: mpu: Ensure that the page size is 4KB (arm32)
  xen/arm: mpu: Enclose access to MMU specific registers under
    CONFIG_MMU (arm32)
  xen/arm: mpu: Move some of the definitions to common file
  xen/arm: mpu: Create boot-time MPU protection regions (arm32)
  xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm (arm32)

 xen/arch/arm/arm32/Makefile                |   1 +
 xen/arch/arm/arm32/head.S                  |   2 +
 xen/arch/arm/arm32/mpu/Makefile            |   1 +
 xen/arch/arm/arm32/mpu/head.S              | 174 +++++++++++++++++++++
 xen/arch/arm/arm32/mpu/mm.c                |  15 ++
 xen/arch/arm/arm64/mpu/head.S              |   2 +-
 xen/arch/arm/include/asm/cpregs.h          |   4 +
 xen/arch/arm/include/asm/early_printk.h    |   2 +-
 xen/arch/arm/include/asm/{arm64 => }/mpu.h |   6 +-
 xen/arch/arm/include/asm/mpu/cpregs.h      |  21 +++
 10 files changed, 223 insertions(+), 5 deletions(-)
 create mode 100644 xen/arch/arm/arm32/mpu/Makefile
 create mode 100644 xen/arch/arm/arm32/mpu/head.S
 create mode 100644 xen/arch/arm/arm32/mpu/mm.c
 rename xen/arch/arm/include/asm/{arm64 => }/mpu.h (87%)
 create mode 100644 xen/arch/arm/include/asm/mpu/cpregs.h

-- 
2.25.1

[PATCH 1/5] xen/arm: mpu: Ensure that the page size is 4KB (arm32)

[Ayan Kumar Halder]

Similar to "xen/arm: mpu: Define Xen start address for MPU systems", added
a build assertion to ensure that the page size is 4KB.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/arm32/Makefile     |  1 +
 xen/arch/arm/arm32/mpu/Makefile |  1 +
 xen/arch/arm/arm32/mpu/mm.c     | 15 +++++++++++++++
 3 files changed, 17 insertions(+)
 create mode 100644 xen/arch/arm/arm32/mpu/Makefile
 create mode 100644 xen/arch/arm/arm32/mpu/mm.c

diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index 40a2b4803f..537969d753 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -1,5 +1,6 @@
 obj-y += lib/
 obj-$(CONFIG_MMU) += mmu/
+obj-$(CONFIG_MPU) += mpu/
 
 obj-$(CONFIG_EARLY_PRINTK) += debug.o
 obj-y += domctl.o
diff --git a/xen/arch/arm/arm32/mpu/Makefile b/xen/arch/arm/arm32/mpu/Makefile
new file mode 100644
index 0000000000..b18cec4836
--- /dev/null
+++ b/xen/arch/arm/arm32/mpu/Makefile
@@ -0,0 +1 @@
+obj-y += mm.o
diff --git a/xen/arch/arm/arm32/mpu/mm.c b/xen/arch/arm/arm32/mpu/mm.c
new file mode 100644
index 0000000000..0b8748e575
--- /dev/null
+++ b/xen/arch/arm/arm32/mpu/mm.c
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/lib.h>
+#include <xen/init.h>
+#include <xen/sizes.h>
+
+static void __init __maybe_unused build_assertions(void)
+{
+    /*
+     * Unlike MMU, MPU does not use pages for translation. However, we continue
+     * to use PAGE_SIZE to denote 4KB. This is so that the existing memory
+     * management based on pages, continue to work for now.
+     */
+    BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
+}
-- 
2.25.1

[PATCH 2/5] xen/arm: mpu: Enclose access to MMU specific registers under CONFIG_MMU (arm32)

[Ayan Kumar Halder]

All the EL2 MMU specific registers in head.S are enclosed within CONFIG_MMU.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/arm32/head.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 4ff5c220bc..1d0f84b18f 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -224,6 +224,7 @@ cpu_init_done:
         mcr   CP32(r0, HMAIR0)
         mcr   CP32(r1, HMAIR1)
 
+#ifdef CONFIG_MMU
         /*
          * Set up the HTCR:
          * PT walks use Inner-Shareable accesses,
@@ -232,6 +233,7 @@ cpu_init_done:
          */
         mov_w r0, (TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
         mcr   CP32(r0, HTCR)
+#endif
 
         mov_w r0, HSCTLR_SET
         mcr   CP32(r0, HSCTLR)
-- 
2.25.1

[PATCH 3/5] xen/arm: mpu: Move some of the definitions to common file

[Ayan Kumar Halder]

For AArch32, refer to ARM DDI 0568A.c ID110520.
MPU_REGION_SHIFT is same between AArch32 and AArch64 (HPRBAR).
Also, NUM_MPU_REGIONS_SHIFT is same between AArch32 and AArch64
(HMPUIR).

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/arm64/mpu/head.S              | 2 +-
 xen/arch/arm/include/asm/early_printk.h    | 2 +-
 xen/arch/arm/include/asm/{arm64 => }/mpu.h | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename xen/arch/arm/include/asm/{arm64 => }/mpu.h (87%)

diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
index e4f2021f45..7b659aa42b 100644
--- a/xen/arch/arm/arm64/mpu/head.S
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -3,7 +3,7 @@
  * Start-of-day code for an Armv8-R MPU system.
  */
 
-#include <asm/arm64/mpu.h>
+#include <asm/mpu.h>
 #include <asm/early_printk.h>
 
 /* Backgroud region enable/disable */
diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h
index 219705a8b6..644fd0fcfb 100644
--- a/xen/arch/arm/include/asm/early_printk.h
+++ b/xen/arch/arm/include/asm/early_printk.h
@@ -11,7 +11,7 @@
 #define __ARM_EARLY_PRINTK_H__
 
 #include <xen/page-size.h>
-#include <asm/arm64/mpu.h>
+#include <asm/mpu.h>
 #include <asm/fixmap.h>
 
 #ifdef CONFIG_EARLY_PRINTK
diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/mpu.h
similarity index 87%
rename from xen/arch/arm/include/asm/arm64/mpu.h
rename to xen/arch/arm/include/asm/mpu.h
index f8a029f1a1..40fa6eaaca 100644
--- a/xen/arch/arm/include/asm/arm64/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -3,8 +3,8 @@
  * mpu.h: Arm Memory Protection Unit definitions.
  */
 
-#ifndef __ARM64_MPU_H__
-#define __ARM64_MPU_H__
+#ifndef __ARM_MPU_H__
+#define __ARM_MPU_H__
 
 #define MPU_REGION_SHIFT  6
 #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
@@ -13,7 +13,7 @@
 #define NUM_MPU_REGIONS_SHIFT   8
 #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
 #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
-#endif /* __ARM64_MPU_H__ */
+#endif /* __ARM_MPU_H__ */
 
 /*
  * Local variables:
-- 
2.25.1

[PATCH 4/5] xen/arm: mpu: Create boot-time MPU protection regions (arm32)

[Ayan Kumar Halder]

Define enable_boot_cpu_mm() for the Armv8-R AArch64.

Like boot-time page table in MMU system, we need a boot-time MPU protection
region configuration in MPU system so Xen can fetch code and data from normal
memory.

To do this, Xen maps the following sections of the binary as separate regions
(with permissions) :-
1. Text (Read only at EL2, execution is permitted)
2. RO data (Read only at EL2)
3. RO after init data and RW data (Read/Write at EL2)
4. Init Text (Read only at EL2, execution is permitted)
5. Init data and BSS (Read/Write at EL2)

Before creating a region, we check if the count exceeds the number defined in
MPUIR_EL2. If so, then the boot fails.

Also we check if the region is empty or not. IOW, if the start and end address
are same, we skip mapping the region.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/arm32/mpu/head.S         | 164 ++++++++++++++++++++++++++
 xen/arch/arm/include/asm/cpregs.h     |   4 +
 xen/arch/arm/include/asm/mpu/cpregs.h |  21 ++++
 3 files changed, 189 insertions(+)
 create mode 100644 xen/arch/arm/arm32/mpu/head.S
 create mode 100644 xen/arch/arm/include/asm/mpu/cpregs.h

diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
new file mode 100644
index 0000000000..4aad3c6b5d
--- /dev/null
+++ b/xen/arch/arm/arm32/mpu/head.S
@@ -0,0 +1,164 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Start-of-day code for an Armv8-R MPU system.
+ */
+
+#include <asm/early_printk.h>
+#include <asm/arm32/sysregs.h>
+
+/* Backgroud region enable/disable */
+#define SCTLR_ELx_BR    BIT(17, UL)
+
+#define REGION_TEXT_PRBAR       0x18    /* SH=11 AP=10 XN=0 */
+#define REGION_RO_PRBAR         0x1D    /* SH=11 AP=10 XN=1 */
+#define REGION_DATA_PRBAR       0x19    /* SH=11 AP=00 XN=1 */
+#define REGION_DEVICE_PRBAR     0x11    /* SH=10 AP=00 XN=1 */
+
+#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
+#define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
+
+/*
+ * Macro to prepare and set a EL2 MPU memory region.
+ * We will also create an according MPU memory region entry, which
+ * is a structure of pr_t,  in table \prmap.
+ *
+ * sel:         region selector
+ * base:        reg storing base address
+ * limit:       reg storing limit address
+ * prbar:       store computed PRBAR_EL2 value
+ * prlar:       store computed PRLAR_EL2 value
+ * maxcount:    maximum number of EL2 regions supported
+ * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it will be
+ *              REGION_DATA_PRBAR
+ * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it will be
+ *              REGION_NORMAL_PRLAR
+ *
+ * Preserves \maxcount
+ * Output:
+ *  \sel: Next available region selector index.
+ * Clobbers \base, \limit, \prbar, \prlar
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
+    /* Check if the region is empty */
+    cmp   \base, \limit
+    beq   1f
+
+    /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */
+    cmp   \sel, \maxcount
+    bge   fail_insufficient_regions
+
+    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
+    and   \base, \base, #MPU_REGION_MASK
+    mov   \prbar, #\attr_prbar
+    orr   \prbar, \prbar, \base
+
+    /* Limit address should be inclusive */
+    sub   \limit, \limit, #1
+    and   \limit, \limit, #MPU_REGION_MASK
+    mov   \prlar, #\attr_prlar
+    orr   \prlar, \prlar, \limit
+
+    mcr   CP32(\sel, PRSELR_EL2)
+    isb
+    mcr   CP32(\prbar, PRBAR_EL2)
+    mcr   CP32(\prlar,  PRLAR_EL2)
+    dsb   sy
+    isb
+
+    add   \sel, \sel, #1
+
+1:
+.endm
+
+/*
+ * Failure caused due to insufficient MPU regions.
+ */
+FUNC_LOCAL(fail_insufficient_regions)
+    PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n")
+1:  wfe
+    b   1b
+END(fail_insufficient_regions)
+
+/*
+ * Enable EL2 MPU and data cache
+ * If the Background region is enabled, then the MPU uses the default memory
+ * map as the Background region for generating the memory
+ * attributes when MPU is disabled.
+ * Since the default memory map of the Armv8-R AArch64 architecture is
+ * IMPLEMENTATION DEFINED, we intend to turn off the Background region here.
+ *
+ * Clobbers x0
+ *
+ */
+FUNC_LOCAL(enable_mpu)
+    mrc   CP32(r0, HSCTLR)
+    bic   r0, r0, #SCTLR_ELx_BR       /* Disable Background region */
+    orr   r0, r0, #SCTLR_Axx_ELx_M    /* Enable MPU */
+    orr   r0, r0, #SCTLR_Axx_ELx_C    /* Enable D-cache */
+    mcr   CP32(r0, HSCTLR)
+    isb
+
+    ret
+END(enable_mpu)
+
+/*
+ * Maps the various sections of Xen (decsribed in xen.lds.S) as different MPU
+ * regions.
+ *
+ * Clobbers r0
+ *
+ */
+#define NORMAL_MEM_SIZE         0x001fffff    /* 2MB - 1 */
+
+FUNC(enable_boot_cpu_mm)
+    /* Get the number of regions specified in MPUIR_EL2 */
+    mrc   CP32(r5, MPUIR_EL2)
+    and   r5, r5, #NUM_MPU_REGIONS_MASK
+
+    /* x0: region sel */
+    mov   r0, #0
+
+    /* Xen text section. */
+    ldr   r1, =_stext
+    ldr   r2, =_etext
+    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
+
+    /* Xen read-only data section. */
+    ldr   r1, =_srodata
+    ldr   r2, =_erodata
+    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_RO_PRBAR
+
+    /* Xen read-only after init and data section. (RW data) */
+    ldr   r1, =__ro_after_init_start
+    ldr   r2, =__init_begin
+    prepare_xen_region r0, r1, r2, r3, r4, r5
+
+    /* Xen code section. */
+    ldr   r1, =__init_begin
+    ldr   r2, =__init_data_begin
+    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
+
+    /* Xen data and BSS section. */
+    ldr   r1, =__init_data_begin
+    ldr   r2, =__bss_end
+    prepare_xen_region r0, r1, r2, r3, r4, r5
+
+#ifdef CONFIG_EARLY_PRINTK
+    /* Xen early UART section. */
+    ldr   r1, =CONFIG_EARLY_UART_BASE_ADDRESS
+    ldr   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
+#endif
+
+    b    enable_mpu
+    ret
+END(enable_boot_cpu_mm)
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
index aec9e8f329..6019a2cbdd 100644
--- a/xen/arch/arm/include/asm/cpregs.h
+++ b/xen/arch/arm/include/asm/cpregs.h
@@ -1,6 +1,10 @@
 #ifndef __ASM_ARM_CPREGS_H
 #define __ASM_ARM_CPREGS_H
 
+#ifdef CONFIG_MPU
+#include <asm/mpu/cpregs.h>
+#endif
+
 /*
  * AArch32 Co-processor registers.
  *
diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
new file mode 100644
index 0000000000..bd17a8c75a
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/cpregs.h
@@ -0,0 +1,21 @@
+#ifndef __ASM_ARM_MPU_CPREGS_H
+#define __ASM_ARM_MPU_CPREGS_H
+
+#define HMPUIR          p15,4,c0,c0,4
+
+/* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
+#define HPRSELR         p15,4,c6,c2,1
+#define PRBAR_EL2       p15,4,c6,c3,0
+#define PRLAR_EL2       p15,4,c6,c8,1
+
+#define MPUIR_EL2               HMPUIR
+#define PRSELR_EL2              HPRSELR
+
+#endif
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.25.1

[PATCH 5/5] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm (arm32)

[Ayan Kumar Halder]

Secondary cpus initialization is not yet supported. Thus, we print an
appropriate message and put the secondary cpus in WFE state.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
 xen/arch/arm/arm32/mpu/head.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
index 4aad3c6b5d..49ab5fc9c0 100644
--- a/xen/arch/arm/arm32/mpu/head.S
+++ b/xen/arch/arm/arm32/mpu/head.S
@@ -156,6 +156,16 @@ FUNC(enable_boot_cpu_mm)
     ret
 END(enable_boot_cpu_mm)
 
+/*
+ * We don't yet support secondary CPUs bring-up. Implement a dummy helper to
+ * please the common code.
+ */
+ENTRY(enable_secondary_cpu_mm)
+    PRINT("- SMP not enabled yet -\r\n")
+1:  wfe
+    b 1b
+ENDPROC(enable_secondary_cpu_mm)
+
 /*
  * Local variables:
  * mode: ASM
-- 
2.25.1

Re: [PATCH 1/5] xen/arm: mpu: Ensure that the page size is 4KB (arm32)

[Luca Fancellu]

Hi Ayan,

> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> Similar to "xen/arm: mpu: Define Xen start address for MPU systems", added
> a build assertion to ensure that the page size is 4KB.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

This looks ok to me and in line with what is done for arm64.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [PATCH 2/5] xen/arm: mpu: Enclose access to MMU specific registers under CONFIG_MMU (arm32)

[Luca Fancellu]

Hi Ayan,

> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> All the EL2 MMU specific registers in head.S are enclosed within CONFIG_MMU.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> xen/arch/arm/arm32/head.S | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 4ff5c220bc..1d0f84b18f 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -224,6 +224,7 @@ cpu_init_done:
>         mcr   CP32(r0, HMAIR0)
>         mcr   CP32(r1, HMAIR1)
> 
> +#ifdef CONFIG_MMU
>         /*
>          * Set up the HTCR:
>          * PT walks use Inner-Shareable accesses,
> @@ -232,6 +233,7 @@ cpu_init_done:
>          */
>         mov_w r0, (TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
>         mcr   CP32(r0, HTCR)
> +#endif

I was wondering if here it was better, for readability, to have this part defined in the arm32/mmu/head.S and
arm32/mpu/head.S could have implemented a stub, maybe the maintainer could help with that.

Anyway this solution works for me.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> 
>         mov_w r0, HSCTLR_SET
>         mcr   CP32(r0, HSCTLR)
> -- 
> 2.25.1
> 
> 

Re: [PATCH 3/5] xen/arm: mpu: Move some of the definitions to common file

[Luca Fancellu]

Hi Ayan,

> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> For AArch32, refer to ARM DDI 0568A.c ID110520.
> MPU_REGION_SHIFT is same between AArch32 and AArch64 (HPRBAR).
> Also, NUM_MPU_REGIONS_SHIFT is same between AArch32 and AArch64
> (HMPUIR).
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> xen/arch/arm/arm64/mpu/head.S              | 2 +-
> xen/arch/arm/include/asm/early_printk.h    | 2 +-
> xen/arch/arm/include/asm/{arm64 => }/mpu.h | 6 +++---
> 3 files changed, 5 insertions(+), 5 deletions(-)
> rename xen/arch/arm/include/asm/{arm64 => }/mpu.h (87%)
> 
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> index e4f2021f45..7b659aa42b 100644
> --- a/xen/arch/arm/arm64/mpu/head.S
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -3,7 +3,7 @@
>  * Start-of-day code for an Armv8-R MPU system.
>  */
> 
> -#include <asm/arm64/mpu.h>
> +#include <asm/mpu.h>
> #include <asm/early_printk.h>
> 
> /* Backgroud region enable/disable */
> diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h
> index 219705a8b6..644fd0fcfb 100644
> --- a/xen/arch/arm/include/asm/early_printk.h
> +++ b/xen/arch/arm/include/asm/early_printk.h
> @@ -11,7 +11,7 @@
> #define __ARM_EARLY_PRINTK_H__
> 
> #include <xen/page-size.h>
> -#include <asm/arm64/mpu.h>
> +#include <asm/mpu.h>
> #include <asm/fixmap.h>
> 
> #ifdef CONFIG_EARLY_PRINTK
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/mpu.h

Why not in include/mpu/ ?

Cheers,
Luca

Re: [PATCH 4/5] xen/arm: mpu: Create boot-time MPU protection regions (arm32)

[Luca Fancellu]

Hi Ayan,

> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> Define enable_boot_cpu_mm() for the Armv8-R AArch64.
> 
> Like boot-time page table in MMU system, we need a boot-time MPU protection
> region configuration in MPU system so Xen can fetch code and data from normal
> memory.
> 
> To do this, Xen maps the following sections of the binary as separate regions
> (with permissions) :-
> 1. Text (Read only at EL2, execution is permitted)
> 2. RO data (Read only at EL2)
> 3. RO after init data and RW data (Read/Write at EL2)
> 4. Init Text (Read only at EL2, execution is permitted)
> 5. Init data and BSS (Read/Write at EL2)
> 
> Before creating a region, we check if the count exceeds the number defined in
> MPUIR_EL2. If so, then the boot fails.
> 
> Also we check if the region is empty or not. IOW, if the start and end address
> are same, we skip mapping the region.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---

With this one there is quite some duplication now between arm64/mpu/head.S
and arm32/mpu/head.S, do you think it is necessary?

> xen/arch/arm/arm32/mpu/head.S         | 164 ++++++++++++++++++++++++++
> xen/arch/arm/include/asm/cpregs.h     |   4 +
> xen/arch/arm/include/asm/mpu/cpregs.h |  21 ++++
> 3 files changed, 189 insertions(+)
> create mode 100644 xen/arch/arm/arm32/mpu/head.S
> create mode 100644 xen/arch/arm/include/asm/mpu/cpregs.h
> 
> diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
> new file mode 100644
> index 0000000000..4aad3c6b5d
> --- /dev/null
> +++ b/xen/arch/arm/arm32/mpu/head.S
> @@ -0,0 +1,164 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include <asm/early_printk.h>
> +#include <asm/arm32/sysregs.h>
> +
> +/* Backgroud region enable/disable */
> +#define SCTLR_ELx_BR    BIT(17, UL)

This is the same as arm64

> +
> +#define REGION_TEXT_PRBAR       0x18    /* SH=11 AP=10 XN=0 */
> +#define REGION_RO_PRBAR         0x1D    /* SH=11 AP=10 XN=1 */
> +#define REGION_DATA_PRBAR       0x19    /* SH=11 AP=00 XN=1 */
> +#define REGION_DEVICE_PRBAR     0x11    /* SH=10 AP=00 XN=1 */

these are the same as arm64 but shifted right by 1, we might want to ask the maintainers
about what is better here


> +
> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
> +#define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */

same as arm64

> +
> +/*
> + * Macro to prepare and set a EL2 MPU memory region.
> + * We will also create an according MPU memory region entry, which
> + * is a structure of pr_t,  in table \prmap.
> + *
> + * sel:         region selector
> + * base:        reg storing base address
> + * limit:       reg storing limit address
> + * prbar:       store computed PRBAR_EL2 value
> + * prlar:       store computed PRLAR_EL2 value
> + * maxcount:    maximum number of EL2 regions supported
> + * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it will be
> + *              REGION_DATA_PRBAR
> + * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it will be
> + *              REGION_NORMAL_PRLAR
> + *
> + * Preserves \maxcount
> + * Output:
> + *  \sel: Next available region selector index.
> + * Clobbers \base, \limit, \prbar, \prlar
> + *
> + * Note that all parameters using registers should be distinct.
> + */
> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> +    /* Check if the region is empty */
> +    cmp   \base, \limit
> +    beq   1f
> +
> +    /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */
> +    cmp   \sel, \maxcount
> +    bge   fail_insufficient_regions
> +
> +    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> +    and   \base, \base, #MPU_REGION_MASK
> +    mov   \prbar, #\attr_prbar
> +    orr   \prbar, \prbar, \base
> +
> +    /* Limit address should be inclusive */
> +    sub   \limit, \limit, #1
> +    and   \limit, \limit, #MPU_REGION_MASK
> +    mov   \prlar, #\attr_prlar
> +    orr   \prlar, \prlar, \limit

Up to here this is the same as arm64

> +
> +    mcr   CP32(\sel, PRSELR_EL2)
> +    isb
> +    mcr   CP32(\prbar, PRBAR_EL2)
> +    mcr   CP32(\prlar,  PRLAR_EL2)
> +    dsb   sy
> +    isb

here we have something specific for arm32 for what it concern the register write,
maybe we could do something around that area to have a common code that
calls specific arch-related methods to write the registers on arm32 and arm64.

> +
> +    add   \sel, \sel, #1
> +
> +1:
> +.endm
> +
> +/*
> + * Failure caused due to insufficient MPU regions.
> + */
> +FUNC_LOCAL(fail_insufficient_regions)
> +    PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n")
> +1:  wfe
> +    b   1b
> +END(fail_insufficient_regions)

same as arm64

> +
> +/*
> + * Enable EL2 MPU and data cache
> + * If the Background region is enabled, then the MPU uses the default memory
> + * map as the Background region for generating the memory
> + * attributes when MPU is disabled.
> + * Since the default memory map of the Armv8-R AArch64 architecture is
										    ^— this needs to be updated

> + * IMPLEMENTATION DEFINED, we intend to turn off the Background region here.
> + *
> + * Clobbers x0
> + *
> + */
> +FUNC_LOCAL(enable_mpu)
> +    mrc   CP32(r0, HSCTLR)
> +    bic   r0, r0, #SCTLR_ELx_BR       /* Disable Background region */
> +    orr   r0, r0, #SCTLR_Axx_ELx_M    /* Enable MPU */
> +    orr   r0, r0, #SCTLR_Axx_ELx_C    /* Enable D-cache */
> +    mcr   CP32(r0, HSCTLR)
> +    isb
> +
> +    ret
> +END(enable_mpu)
> +
> +/*
> + * Maps the various sections of Xen (decsribed in xen.lds.S) as different MPU
> + * regions.
> + *
> + * Clobbers r0
> + *
> + */
> +#define NORMAL_MEM_SIZE         0x001fffff    /* 2MB - 1 */

this is not used here

> +
> +FUNC(enable_boot_cpu_mm)
> +    /* Get the number of regions specified in MPUIR_EL2 */
> +    mrc   CP32(r5, MPUIR_EL2)
> +    and   r5, r5, #NUM_MPU_REGIONS_MASK
> +
> +    /* x0: region sel */
> +    mov   r0, #0
> +
> +    /* Xen text section. */
> +    ldr   r1, =_stext
> +    ldr   r2, =_etext
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen read-only data section. */
> +    ldr   r1, =_srodata
> +    ldr   r2, =_erodata
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_RO_PRBAR
> +
> +    /* Xen read-only after init and data section. (RW data) */
> +    ldr   r1, =__ro_after_init_start
> +    ldr   r2, =__init_begin
> +    prepare_xen_region r0, r1, r2, r3, r4, r5
> +
> +    /* Xen code section. */
> +    ldr   r1, =__init_begin
> +    ldr   r2, =__init_data_begin
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen data and BSS section. */
> +    ldr   r1, =__init_data_begin
> +    ldr   r2, =__bss_end
> +    prepare_xen_region r0, r1, r2, r3, r4, r5
> +
> +#ifdef CONFIG_EARLY_PRINTK
> +    /* Xen early UART section. */
> +    ldr   r1, =CONFIG_EARLY_UART_BASE_ADDRESS
> +    ldr   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
> +#endif
> +
> +    b    enable_mpu
> +    ret
> +END(enable_boot_cpu_mm)

This one is equal to arm64 apart from the registers xY -> rY, but I’m not sure we would
want to consolidate that.

> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
> index aec9e8f329..6019a2cbdd 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -1,6 +1,10 @@
> #ifndef __ASM_ARM_CPREGS_H
> #define __ASM_ARM_CPREGS_H
> 
> +#ifdef CONFIG_MPU
> +#include <asm/mpu/cpregs.h>
> +#endif
> +
> /*
>  * AArch32 Co-processor registers.
>  *
> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h

xen/arch/arm/include/asm/mpu/arm32/mpu.h? Where you define all the MPU registers but with
a translation from the aarch64 name to the arm32? Not sure about that, better ask a maintainer.

> new file mode 100644
> index 0000000000..bd17a8c75a
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> @@ -0,0 +1,21 @@
> +#ifndef __ASM_ARM_MPU_CPREGS_H
> +#define __ASM_ARM_MPU_CPREGS_H
> +
> +#define HMPUIR          p15,4,c0,c0,4
> +
> +/* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
> +#define HPRSELR         p15,4,c6,c2,1
> +#define PRBAR_EL2       p15,4,c6,c3,0
> +#define PRLAR_EL2       p15,4,c6,c8,1
> +
> +#define MPUIR_EL2               HMPUIR
> +#define PRSELR_EL2              HPRSELR
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.25.1
> 
> 

Re: [PATCH 5/5] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm (arm32)

[Luca Fancellu]

Hi Ayan,

> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> Secondary cpus initialization is not yet supported. Thus, we print an
> appropriate message and put the secondary cpus in WFE state.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [PATCH 4/5] xen/arm: mpu: Create boot-time MPU protection regions (arm32)

[Ayan Kumar Halder]

On 06/02/2025 15:29, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
>
>> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>>
>> Define enable_boot_cpu_mm() for the Armv8-R AArch64.
>>
>> Like boot-time page table in MMU system, we need a boot-time MPU protection
>> region configuration in MPU system so Xen can fetch code and data from normal
>> memory.
>>
>> To do this, Xen maps the following sections of the binary as separate regions
>> (with permissions) :-
>> 1. Text (Read only at EL2, execution is permitted)
>> 2. RO data (Read only at EL2)
>> 3. RO after init data and RW data (Read/Write at EL2)
>> 4. Init Text (Read only at EL2, execution is permitted)
>> 5. Init data and BSS (Read/Write at EL2)
>>
>> Before creating a region, we check if the count exceeds the number defined in
>> MPUIR_EL2. If so, then the boot fails.
>>
>> Also we check if the region is empty or not. IOW, if the start and end address
>> are same, we skip mapping the region.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
> With this one there is quite some duplication now between arm64/mpu/head.S
> and arm32/mpu/head.S, do you think it is necessary?
>
>> xen/arch/arm/arm32/mpu/head.S         | 164 ++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/cpregs.h     |   4 +
>> xen/arch/arm/include/asm/mpu/cpregs.h |  21 ++++
>> 3 files changed, 189 insertions(+)
>> create mode 100644 xen/arch/arm/arm32/mpu/head.S
>> create mode 100644 xen/arch/arm/include/asm/mpu/cpregs.h
>>
>> diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
>> new file mode 100644
>> index 0000000000..4aad3c6b5d
>> --- /dev/null
>> +++ b/xen/arch/arm/arm32/mpu/head.S
>> @@ -0,0 +1,164 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Start-of-day code for an Armv8-R MPU system.
>> + */
>> +
>> +#include <asm/early_printk.h>
>> +#include <asm/arm32/sysregs.h>
>> +
>> +/* Backgroud region enable/disable */
>> +#define SCTLR_ELx_BR    BIT(17, UL)
> This is the same as arm64
This can be moved to a common header file if it makes sense.
>
>> +
>> +#define REGION_TEXT_PRBAR       0x18    /* SH=11 AP=10 XN=0 */
>> +#define REGION_RO_PRBAR         0x1D    /* SH=11 AP=10 XN=1 */
>> +#define REGION_DATA_PRBAR       0x19    /* SH=11 AP=00 XN=1 */
>> +#define REGION_DEVICE_PRBAR     0x11    /* SH=10 AP=00 XN=1 */
> these are the same as arm64 but shifted right by 1, we might want to ask the maintainers
> about what is better here
I wouldn't read it in this way. The main difference is XN is 2 bits in 
arm64 and 1 bit in arm32. So, I will prefer to keep the definitions 
separate to avoid any confusion.
>
>
>> +
>> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
>> +#define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
> same as arm64
>
>> +
>> +/*
>> + * Macro to prepare and set a EL2 MPU memory region.
>> + * We will also create an according MPU memory region entry, which
>> + * is a structure of pr_t,  in table \prmap.
>> + *
>> + * sel:         region selector
>> + * base:        reg storing base address
>> + * limit:       reg storing limit address
>> + * prbar:       store computed PRBAR_EL2 value
>> + * prlar:       store computed PRLAR_EL2 value
>> + * maxcount:    maximum number of EL2 regions supported
>> + * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it will be
>> + *              REGION_DATA_PRBAR
>> + * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it will be
>> + *              REGION_NORMAL_PRLAR
>> + *
>> + * Preserves \maxcount
>> + * Output:
>> + *  \sel: Next available region selector index.
>> + * Clobbers \base, \limit, \prbar, \prlar
>> + *
>> + * Note that all parameters using registers should be distinct.
>> + */
>> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
>> +    /* Check if the region is empty */
>> +    cmp   \base, \limit
>> +    beq   1f
>> +
>> +    /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */
>> +    cmp   \sel, \maxcount
>> +    bge   fail_insufficient_regions
>> +
>> +    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
>> +    and   \base, \base, #MPU_REGION_MASK
>> +    mov   \prbar, #\attr_prbar
>> +    orr   \prbar, \prbar, \base
>> +
>> +    /* Limit address should be inclusive */
>> +    sub   \limit, \limit, #1
>> +    and   \limit, \limit, #MPU_REGION_MASK
>> +    mov   \prlar, #\attr_prlar
>> +    orr   \prlar, \prlar, \limit
> Up to here this is the same as arm64
>
>> +
>> +    mcr   CP32(\sel, PRSELR_EL2)
>> +    isb
>> +    mcr   CP32(\prbar, PRBAR_EL2)
>> +    mcr   CP32(\prlar,  PRLAR_EL2)
>> +    dsb   sy
>> +    isb
> here we have something specific for arm32 for what it concern the register write,
> maybe we could do something around that area to have a common code that
> calls specific arch-related methods to write the registers on arm32 and arm64.
>
>> +
>> +    add   \sel, \sel, #1
>> +
>> +1:
>> +.endm
>> +
>> +/*
>> + * Failure caused due to insufficient MPU regions.
>> + */
>> +FUNC_LOCAL(fail_insufficient_regions)
>> +    PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n")
>> +1:  wfe
>> +    b   1b
>> +END(fail_insufficient_regions)
> same as arm64
>
>> +
>> +/*
>> + * Enable EL2 MPU and data cache
>> + * If the Background region is enabled, then the MPU uses the default memory
>> + * map as the Background region for generating the memory
>> + * attributes when MPU is disabled.
>> + * Since the default memory map of the Armv8-R AArch64 architecture is
> 										    ^— this needs to be updated
yes
>
>> + * IMPLEMENTATION DEFINED, we intend to turn off the Background region here.
>> + *
>> + * Clobbers x0
>> + *
>> + */
>> +FUNC_LOCAL(enable_mpu)
>> +    mrc   CP32(r0, HSCTLR)
>> +    bic   r0, r0, #SCTLR_ELx_BR       /* Disable Background region */
>> +    orr   r0, r0, #SCTLR_Axx_ELx_M    /* Enable MPU */
>> +    orr   r0, r0, #SCTLR_Axx_ELx_C    /* Enable D-cache */
>> +    mcr   CP32(r0, HSCTLR)
>> +    isb
>> +
>> +    ret
>> +END(enable_mpu)
>> +
>> +/*
>> + * Maps the various sections of Xen (decsribed in xen.lds.S) as different MPU
>> + * regions.
>> + *
>> + * Clobbers r0
>> + *
>> + */
>> +#define NORMAL_MEM_SIZE         0x001fffff    /* 2MB - 1 */
> this is not used here
sorry, this should be dropped.
>
>> +
>> +FUNC(enable_boot_cpu_mm)
>> +    /* Get the number of regions specified in MPUIR_EL2 */
>> +    mrc   CP32(r5, MPUIR_EL2)
>> +    and   r5, r5, #NUM_MPU_REGIONS_MASK
>> +
>> +    /* x0: region sel */
>> +    mov   r0, #0
>> +
>> +    /* Xen text section. */
>> +    ldr   r1, =_stext
>> +    ldr   r2, =_etext
>> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
>> +
>> +    /* Xen read-only data section. */
>> +    ldr   r1, =_srodata
>> +    ldr   r2, =_erodata
>> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_RO_PRBAR
>> +
>> +    /* Xen read-only after init and data section. (RW data) */
>> +    ldr   r1, =__ro_after_init_start
>> +    ldr   r2, =__init_begin
>> +    prepare_xen_region r0, r1, r2, r3, r4, r5
>> +
>> +    /* Xen code section. */
>> +    ldr   r1, =__init_begin
>> +    ldr   r2, =__init_data_begin
>> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
>> +
>> +    /* Xen data and BSS section. */
>> +    ldr   r1, =__init_data_begin
>> +    ldr   r2, =__bss_end
>> +    prepare_xen_region r0, r1, r2, r3, r4, r5
>> +
>> +#ifdef CONFIG_EARLY_PRINTK
>> +    /* Xen early UART section. */
>> +    ldr   r1, =CONFIG_EARLY_UART_BASE_ADDRESS
>> +    ldr   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
>> +#endif
>> +
>> +    b    enable_mpu
>> +    ret
>> +END(enable_boot_cpu_mm)
> This one is equal to arm64 apart from the registers xY -> rY, but I’m not sure we would
> want to consolidate that.
I am not sure either.
>
>> +
>> +/*
>> + * Local variables:
>> + * mode: ASM
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/include/asm/cpregs.h b/xen/arch/arm/include/asm/cpregs.h
>> index aec9e8f329..6019a2cbdd 100644
>> --- a/xen/arch/arm/include/asm/cpregs.h
>> +++ b/xen/arch/arm/include/asm/cpregs.h
>> @@ -1,6 +1,10 @@
>> #ifndef __ASM_ARM_CPREGS_H
>> #define __ASM_ARM_CPREGS_H
>>
>> +#ifdef CONFIG_MPU
>> +#include <asm/mpu/cpregs.h>
>> +#endif
>> +
>> /*
>>   * AArch32 Co-processor registers.
>>   *
>> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
> xen/arch/arm/include/asm/mpu/arm32/mpu.h? Where you define all the MPU registers but with
> a translation from the aarch64 name to the arm32? Not sure about that, better ask a maintainer.

I think this will be confusing. IMO, better to keep them separate unless 
Arm ARM specifies some translation.

- Ayan

>
>> new file mode 100644
>> index 0000000000..bd17a8c75a
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
>> @@ -0,0 +1,21 @@
>> +#ifndef __ASM_ARM_MPU_CPREGS_H
>> +#define __ASM_ARM_MPU_CPREGS_H
>> +
>> +#define HMPUIR          p15,4,c0,c0,4
>> +
>> +/* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
>> +#define HPRSELR         p15,4,c6,c2,1
>> +#define PRBAR_EL2       p15,4,c6,c3,0
>> +#define PRLAR_EL2       p15,4,c6,c8,1
>> +
>> +#define MPUIR_EL2               HMPUIR
>> +#define PRSELR_EL2              HPRSELR
>> +
>> +#endif
>> +
>> +/*
>> + * Local variables:
>> + * mode: ASM
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> -- 
>> 2.25.1
>>
>>

Re: [PATCH 1/5] xen/arm: mpu: Ensure that the page size is 4KB (arm32)

[Julien Grall]

Hi Ayan,

On 04/02/2025 19:23, Ayan Kumar Halder wrote:
> Similar to "xen/arm: mpu: Define Xen start address for MPU systems", added

Can you provide the commit ID? Also, we tend to use present for 
describing changes, so s/added/add/

> a build assertion to ensure that the page size is 4KB.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/arm32/Makefile     |  1 +
>   xen/arch/arm/arm32/mpu/Makefile |  1 +
>   xen/arch/arm/arm32/mpu/mm.c     | 15 +++++++++++++++
>   3 files changed, 17 insertions(+)
>   create mode 100644 xen/arch/arm/arm32/mpu/Makefile
>   create mode 100644 xen/arch/arm/arm32/mpu/mm.c
> 
> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> index 40a2b4803f..537969d753 100644
> --- a/xen/arch/arm/arm32/Makefile
> +++ b/xen/arch/arm/arm32/Makefile
> @@ -1,5 +1,6 @@
>   obj-y += lib/
>   obj-$(CONFIG_MMU) += mmu/
> +obj-$(CONFIG_MPU) += mpu/
>   
>   obj-$(CONFIG_EARLY_PRINTK) += debug.o
>   obj-y += domctl.o
> diff --git a/xen/arch/arm/arm32/mpu/Makefile b/xen/arch/arm/arm32/mpu/Makefile
> new file mode 100644
> index 0000000000..b18cec4836
> --- /dev/null
> +++ b/xen/arch/arm/arm32/mpu/Makefile
> @@ -0,0 +1 @@
> +obj-y += mm.o
> diff --git a/xen/arch/arm/arm32/mpu/mm.c b/xen/arch/arm/arm32/mpu/mm.c
> new file mode 100644
> index 0000000000..0b8748e575
> --- /dev/null
> +++ b/xen/arch/arm/arm32/mpu/mm.c
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/sizes.h>
> +
> +static void __init __maybe_unused build_assertions(void)
> +{
> +    /*
> +     * Unlike MMU, MPU does not use pages for translation. However, we continue
> +     * to use PAGE_SIZE to denote 4KB. This is so that the existing memory
> +     * management based on pages, continue to work for now.
> +     */
> +    BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
> +}

I think it would be better if we create an arm/mpu/mm.c which would 
contain any common code/requirements between arm64 and arm32 (I assume 
there will be quire a few).

Cheers,

-- 
Julien Grall

Re: [PATCH 2/5] xen/arm: mpu: Enclose access to MMU specific registers under CONFIG_MMU (arm32)

[Julien Grall]

Hi,

On 06/02/2025 14:48, Luca Fancellu wrote:
>> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>>
>> All the EL2 MMU specific registers in head.S are enclosed within CONFIG_MMU.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> xen/arch/arm/arm32/head.S | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 4ff5c220bc..1d0f84b18f 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -224,6 +224,7 @@ cpu_init_done:
>>          mcr   CP32(r0, HMAIR0)
>>          mcr   CP32(r1, HMAIR1)
>>
>> +#ifdef CONFIG_MMU
>>          /*
>>           * Set up the HTCR:
>>           * PT walks use Inner-Shareable accesses,
>> @@ -232,6 +233,7 @@ cpu_init_done:
>>           */
>>          mov_w r0, (TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(0))
>>          mcr   CP32(r0, HTCR)
>> +#endif
> 
> I was wondering if here it was better, for readability, to have this part defined in the arm32/mmu/head.S and
> arm32/mpu/head.S could have implemented a stub, maybe the maintainer could help with that.

The current logic is a bit odd because the MM specific registers are 
initialized in two different places (cpu_init and enable_mmu).

It would be better if we have a single place. So I would move setting 
HTCR (and event HMAIR{0,1} even if it means duplication) to enable_mmu.

The same would apply for arm64.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/5] xen/arm: mpu: Move some of the definitions to common file

[Julien Grall]

Hi Luca,

On 06/02/2025 15:01, Luca Fancellu wrote:
> Hi Ayan,
> 
>> On 4 Feb 2025, at 19:23, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>>
>> For AArch32, refer to ARM DDI 0568A.c ID110520.
>> MPU_REGION_SHIFT is same between AArch32 and AArch64 (HPRBAR).
>> Also, NUM_MPU_REGIONS_SHIFT is same between AArch32 and AArch64
>> (HMPUIR).
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> xen/arch/arm/arm64/mpu/head.S              | 2 +-
>> xen/arch/arm/include/asm/early_printk.h    | 2 +-
>> xen/arch/arm/include/asm/{arm64 => }/mpu.h | 6 +++---
>> 3 files changed, 5 insertions(+), 5 deletions(-)
>> rename xen/arch/arm/include/asm/{arm64 => }/mpu.h (87%)
>>
>> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
>> index e4f2021f45..7b659aa42b 100644
>> --- a/xen/arch/arm/arm64/mpu/head.S
>> +++ b/xen/arch/arm/arm64/mpu/head.S
>> @@ -3,7 +3,7 @@
>>   * Start-of-day code for an Armv8-R MPU system.
>>   */
>>
>> -#include <asm/arm64/mpu.h>
>> +#include <asm/mpu.h>
>> #include <asm/early_printk.h>
>>
>> /* Backgroud region enable/disable */
>> diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h
>> index 219705a8b6..644fd0fcfb 100644
>> --- a/xen/arch/arm/include/asm/early_printk.h
>> +++ b/xen/arch/arm/include/asm/early_printk.h
>> @@ -11,7 +11,7 @@
>> #define __ARM_EARLY_PRINTK_H__
>>
>> #include <xen/page-size.h>
>> -#include <asm/arm64/mpu.h>
>> +#include <asm/mpu.h>
>> #include <asm/fixmap.h>
>>
>> #ifdef CONFIG_EARLY_PRINTK
>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/mpu.h
> 
> Why not in include/mpu/ ?

Do you mean include/asm/mpu? or something different?

Cheers,

-- 
Julien Grall

Re: [PATCH 3/5] xen/arm: mpu: Move some of the definitions to common file

[Luca Fancellu]

Hi Julien,

>>> 
>>> #ifdef CONFIG_EARLY_PRINTK
>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/mpu.h
>> Why not in include/mpu/ ?
> 
> Do you mean include/asm/mpu? or something different?

Yes, sorry typo, I mean include/asm/mpu/mpu.h

Cheers,
Luca

Re: [PATCH 3/5] xen/arm: mpu: Move some of the definitions to common file

[Julien Grall]

On 26/02/2025 08:28, Luca Fancellu wrote:
> Hi Julien,

Hi Luca,

> 
>>>>
>>>> #ifdef CONFIG_EARLY_PRINTK
>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/mpu.h
>>> Why not in include/mpu/ ?
>>
>> Do you mean include/asm/mpu? or something different?
> 
> Yes, sorry typo, I mean include/asm/mpu/mpu.h

Thanks for the clarification. I don't have a strong opinion either way. 
I will let Ayan decide.

Cheers,

-- 
Julien Grall

Re: [PATCH 3/5] xen/arm: mpu: Move some of the definitions to common file

[Ayan Kumar Halder]

Hi Julien/Luca,

On 28/02/2025 09:22, Julien Grall wrote:
>
>
> On 26/02/2025 08:28, Luca Fancellu wrote:
>> Hi Julien,
>
> Hi Luca,
>
>>
>>>>>
>>>>> #ifdef CONFIG_EARLY_PRINTK
>>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
>>>>> b/xen/arch/arm/include/asm/mpu.h
>>>> Why not in include/mpu/ ?
>>>
>>> Do you mean include/asm/mpu? or something different?
>>
>> Yes, sorry typo, I mean include/asm/mpu/mpu.h
>
> Thanks for the clarification. I don't have a strong opinion either 
> way. I will let Ayan decide.
Can I leave as it is for the time being ?

I mean I will create "xen/arch/arm/include/asm/mpu/" directory when I 
know there will be more files.

Let me know what you suggest.

- Ayan

>
> Cheers,
>

Re: [PATCH 3/5] xen/arm: mpu: Move some of the definitions to common file

[Luca Fancellu]

> On 28 Feb 2025, at 10:34, Ayan Kumar Halder <ayankuma@amd.com> wrote:
> 
> Hi Julien/Luca,
> 
> On 28/02/2025 09:22, Julien Grall wrote:
>> 
>> 
>> On 26/02/2025 08:28, Luca Fancellu wrote:
>>> Hi Julien,
>> 
>> Hi Luca,
>> 
>>> 
>>>>>> 
>>>>>> #ifdef CONFIG_EARLY_PRINTK
>>>>>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/mpu.h
>>>>> Why not in include/mpu/ ?
>>>> 
>>>> Do you mean include/asm/mpu? or something different?
>>> 
>>> Yes, sorry typo, I mean include/asm/mpu/mpu.h
>> 
>> Thanks for the clarification. I don't have a strong opinion either way. I will let Ayan decide.
> Can I leave as it is for the time being ?
> 
> I mean I will create "xen/arch/arm/include/asm/mpu/" directory when I know there will be more files.
> 
> Let me know what you suggest.
> 
> - Ayan

Ok, as agreed on Matrix that Ayan would like to go with include/asm/mpu.h:

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>


> 
>> 
>> Cheers,
>>