[PATCH v4 0/6] Enable early bootup of AArch64 MPU systems

[PATCH v4 0/6] Enable early bootup of AArch64 MPU systems

[Ayan Kumar Halder]

We have enabled early booting of R82.

Changes from v2 :-
1. Added a new patch "xen/arm: Skip initializing the BSS section when it is empty".
2. Split "xen/arm: mpu: Create boot-time MPU protection regions" into 2 patches.

Changes from v3 :-
1. Removed some of the R-b as the patches have been modified.

Ayan Kumar Halder (6):
  xen/arm: Skip initializing the BSS section when it is empty
  xen/arm: mpu: Introduce choice between MMU and MPU
  xen/arm: mpu: Define Xen start address for MPU systems
  xen/arm: mpu: Create boot-time MPU protection regions
  xen/arm: mpu: Enable MPU
  xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

 CHANGELOG.md                                 |   2 +
 SUPPORT.md                                   |   1 +
 xen/arch/Kconfig                             |   2 +
 xen/arch/arm/Kconfig                         |  27 +++-
 xen/arch/arm/arm32/head.S                    |   3 +
 xen/arch/arm/arm64/Makefile                  |   1 +
 xen/arch/arm/arm64/head.S                    |   2 +
 xen/arch/arm/arm64/mpu/Makefile              |   2 +
 xen/arch/arm/arm64/mpu/head.S                | 158 +++++++++++++++++++
 xen/arch/arm/arm64/mpu/mm.c                  |  15 ++
 xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  30 ++++
 xen/arch/arm/include/asm/config.h            |   4 +-
 xen/arch/arm/include/asm/mm.h                |   2 +
 xen/arch/arm/include/asm/mpu/arm64/mm.h      |  22 +++
 xen/arch/arm/include/asm/mpu/layout.h        |  33 ++++
 xen/arch/arm/include/asm/mpu/mm.h            |  20 +++
 xen/arch/arm/platforms/Kconfig               |   2 +-
 xen/arch/arm/smp.c                           |  11 ++
 xen/arch/arm/xen.lds.S                       |   8 +
 19 files changed, 342 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/arm/arm64/mpu/Makefile
 create mode 100644 xen/arch/arm/arm64/mpu/head.S
 create mode 100644 xen/arch/arm/arm64/mpu/mm.c
 create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
 create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
 create mode 100644 xen/arch/arm/include/asm/mpu/layout.h
 create mode 100644 xen/arch/arm/include/asm/mpu/mm.h

-- 
2.25.1

[PATCH v4 1/6] xen/arm: Skip initializing the BSS section when it is empty

[Ayan Kumar Halder]

If the BSS section is empty, then the function should return.
If one does not check whether the BSS section is empty or not, then there is a
risk of writing 0s outside of BSS section (which may contain critical data).

Fixes: dac84b66cc9a ("xen: arm64: initial build + config changes, start of day code")
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-

v1..v2 - New patch introduced in v3.

v3 - 1. Update the check in arm32 as well.
2. Drop the R-bs.

 xen/arch/arm/arm32/head.S | 3 +++
 xen/arch/arm/arm64/head.S | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index a96d5d3503..4ff5c220bc 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -185,12 +185,15 @@ zero_bss:
         PRINT("- Zero BSS -\r\n")
         mov_w r0, __bss_start        /* r0 := vaddr(__bss_start) */
         mov_w r1, __bss_end          /* r1 := vaddr(__bss_end)   */
+        cmp   r1, r0
+        beq   skip_bss
 
         mov   r2, #0
 1:      str   r2, [r0], #4
         cmp   r0, r1
         blo   1b
 
+skip_bss:
         mov   pc, lr
 ENDPROC(zero_bss)
 
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 14c3720d80..72c7b24498 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -346,6 +346,8 @@ FUNC_LOCAL(zero_bss)
         PRINT("- Zero BSS -\r\n")
         ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
         ldr   x1, =__bss_end         /* x1 := vaddr(__bss_end)   */
+        cmp   x1, x0
+        beq   skip_bss
 
 1:      str   xzr, [x0], #8
         cmp   x0, x1
-- 
2.25.1

Re: [PATCH v4 1/6] xen/arm: Skip initializing the BSS section when it is empty

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> If the BSS section is empty, then the function should return.
> If one does not check whether the BSS section is empty or not, then there is a
> risk of writing 0s outside of BSS section (which may contain critical data).
> 
> Fixes: dac84b66cc9a ("xen: arm64: initial build + config changes, start of day code")
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> —

Looks good to me

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

Re: [PATCH v4 1/6] xen/arm: Skip initializing the BSS section when it is empty

[Julien Grall]

Hi Ayan,

On 28/10/2024 12:45, Ayan Kumar Halder wrote:
> If the BSS section is empty, then the function should return.
> If one does not check whether the BSS section is empty or not, then there is a
> risk of writing 0s outside of BSS section (which may contain critical data).
> 
> Fixes: dac84b66cc9a ("xen: arm64: initial build + config changes, start of day code")
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v4 2/6] xen/arm: mpu: Introduce choice between MMU and MPU

[Ayan Kumar Halder]

There are features in the forthcoming patches which are dependent on
MPU. For eg fixed start address.
Also, some of the Xen features (eg STATIC_MEMORY) will be selected
by the MPU configuration.

Thus, this patch introduces a choice between MMU and MPU for the type
of memory management system. By default, MMU is selected.
MPU is now gated by UNSUPPORTED.

Update SUPPORT.md to state that the support for MPU is experimental.
Also updated CHANGELOG.md as well.

Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---

Changes from :-

v1 - 1. Reword the help messages.
2. Update Support.md.

v2 - 1. Reword the help message.

v3 - 1. Update Changelog.
2. Add R-b and Ack.

 CHANGELOG.md                   |  2 ++
 SUPPORT.md                     |  1 +
 xen/arch/arm/Kconfig           | 17 ++++++++++++++++-
 xen/arch/arm/platforms/Kconfig |  2 +-
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c499d12dc4..79524cc15f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
 
 ### Added
+ - On Arm:
+   - Support for earlyboot of Xen on Armv8-R (experimental).
 
 ### Removed
  - On x86:
diff --git a/SUPPORT.md b/SUPPORT.md
index 23dd7e6424..94610d3c91 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -40,6 +40,7 @@ supported in this document.
 
     Status, Xen in AArch64 mode: Supported
     Status, Xen in AArch32 mode: Tech Preview
+    Status, Xen in Armv8-R: Experimental
     Status, Cortex A57 r0p0-r1p1: Supported, not security supported
     Status, Cortex A77 r0p0-r1p0: Supported, not security supported
 
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 323c967361..ed92eb67cb 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -58,10 +58,25 @@ config PADDR_BITS
 	default 40 if ARM_PA_BITS_40
 	default 48 if ARM_64
 
+choice
+	prompt "Memory management system"
+	default MMU
+	help
+	  User can choose between the different forms of memory management system.
+
 config MMU
-	def_bool y
+	bool "MMU"
 	select HAS_PMAP
 	select HAS_VMAP
+	help
+	  Select it if you plan to run Xen on A-profile Armv7+
+
+config MPU
+	bool "MPU" if UNSUPPORTED
+	help
+	  Memory Protection Unit (MPU). Select if you plan to run Xen on ARMv8-R
+	  systems supporting EL2. (UNSUPPORTED)
+endchoice
 
 source "arch/Kconfig"
 
diff --git a/xen/arch/arm/platforms/Kconfig b/xen/arch/arm/platforms/Kconfig
index 76f7e76b1b..02322c259c 100644
--- a/xen/arch/arm/platforms/Kconfig
+++ b/xen/arch/arm/platforms/Kconfig
@@ -1,5 +1,5 @@
 choice
-	prompt "Platform Support"
+	prompt "Platform Support" if MMU
 	default ALL_PLAT
 	help
 	Choose which hardware platform to enable in Xen.
-- 
2.25.1

Re: [PATCH v4 2/6] xen/arm: mpu: Introduce choice between MMU and MPU

[Andrew Cooper]

On 28/10/2024 12:45 pm, Ayan Kumar Halder wrote:
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index c499d12dc4..79524cc15f 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -12,6 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>     - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
>  
>  ### Added
> + - On Arm:
> +   - Support for earlyboot of Xen on Armv8-R (experimental).

Simply "Experimental support for Armv8-R".

The rest of that sentence is going to go stale quite quickly.

~Andrew

Re: [PATCH v4 2/6] xen/arm: mpu: Introduce choice between MMU and MPU

[oleksii.kurochko]

On Tue, 2024-10-29 at 09:53 +0000, Andrew Cooper wrote:
> On 28/10/2024 12:45 pm, Ayan Kumar Halder wrote:
> > diff --git a/CHANGELOG.md b/CHANGELOG.md
> > index c499d12dc4..79524cc15f 100644
> > --- a/CHANGELOG.md
> > +++ b/CHANGELOG.md
> > @@ -12,6 +12,8 @@ The format is based on [Keep a
> > Changelog](https://keepachangelog.com/en/1.0.0/)
> >     - Prefer ACPI reboot over UEFI ResetSystem() run time service
> > call.
> >  
> >  ### Added
> > + - On Arm:
> > +   - Support for earlyboot of Xen on Armv8-R (experimental).
> 
> Simply "Experimental support for Armv8-R".
> 
> The rest of that sentence is going to go stale quite quickly.
With suggested by Andrew: Acked-by: Oleksii Kurochko
<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

[PATCH v4 3/6] xen/arm: mpu: Define Xen start address for MPU systems

[Ayan Kumar Halder]

From: Wei Chen <wei.chen@arm.com>

On Armv8-A, Xen has a fixed virtual start address (link address too) for all
Armv8-A platforms. In an MMU based system, Xen can map its loaded address to
this virtual start address. So, on Armv8-A platforms, the Xen start address does
not need to be configurable. But on Armv8-R platforms, there is no MMU to map
loaded address to a fixed virtual address and different platforms will have very
different address space layout. So Xen cannot use a fixed physical address on
MPU based system and need to have it configurable.

So, we introduce a Kconfig option for users to set the start address. The start
address needs to be aligned to 4KB. We have a check for this alignment.

MPU allows us to define regions which are 64 bits aligned. This restriction
comes from the bitfields of PRBAR, PRLAR (the lower 6 bits are 0 extended to
provide the base and limit address of a region). This means that the start
address of Xen needs to be at least 64 bits aligned (as it will correspond to
the start address of memory protection region).

As for now Xen on MPU tries to use the same memory alignment restrictions as it
has been for MMU. We have added a build assertion to ensure that the page size
is 4KB. Unlike MMU where the starting virtual address is 2MB, Xen on MPU needs
the start address to be 4KB (ie page size) aligned.

In case if the user forgets to set the start address, then 0xffffffff is used
as default. This is to trigger the error (on alignment check) and thereby prompt
user to set the start address.

Also updated config.h so that it includes mpu/layout.h when CONFIG_MPU is
defined.

Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com>
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from :-

v1 - 1. Fix some of the coding style issues.
2. Reword the help message.
3. Updat the commit message.

v2 - Add clarification for the use of page and page size.

v3 - 1. Add a new file arm64/mpu/mm.c to contain the build assertion for page
size.
2. Enclosed the check for the start address within "#ifdef CONFIG_MPU".

 xen/arch/arm/Kconfig                  | 10 ++++++++
 xen/arch/arm/arm64/Makefile           |  1 +
 xen/arch/arm/arm64/mpu/Makefile       |  1 +
 xen/arch/arm/arm64/mpu/mm.c           | 15 ++++++++++++
 xen/arch/arm/include/asm/config.h     |  4 +++-
 xen/arch/arm/include/asm/mpu/layout.h | 33 +++++++++++++++++++++++++++
 xen/arch/arm/xen.lds.S                |  7 ++++++
 7 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/arm64/mpu/Makefile
 create mode 100644 xen/arch/arm/arm64/mpu/mm.c
 create mode 100644 xen/arch/arm/include/asm/mpu/layout.h

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ed92eb67cb..15b2e4a227 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -23,6 +23,16 @@ config ARCH_DEFCONFIG
 	default "arch/arm/configs/arm32_defconfig" if ARM_32
 	default "arch/arm/configs/arm64_defconfig" if ARM_64
 
+config XEN_START_ADDRESS
+	hex "Xen start address: keep default to use platform defined address"
+	default 0xFFFFFFFF
+	depends on MPU
+	help
+	  Used to set customized address at which which Xen will be linked on MPU
+	  systems. Must be aligned to 4KB.
+	  0xFFFFFFFF is used as default value to indicate that user has not
+	  customized this address.
+
 menu "Architecture Features"
 
 choice
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index 72161ff22e..6491c5350b 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -1,5 +1,6 @@
 obj-y += lib/
 obj-$(CONFIG_MMU) += mmu/
+obj-$(CONFIG_MPU) += mpu/
 
 obj-y += cache.o
 obj-y += cpufeature.o
diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
new file mode 100644
index 0000000000..b18cec4836
--- /dev/null
+++ b/xen/arch/arm/arm64/mpu/Makefile
@@ -0,0 +1 @@
+obj-y += mm.o
diff --git a/xen/arch/arm/arm64/mpu/mm.c b/xen/arch/arm/arm64/mpu/mm.c
new file mode 100644
index 0000000000..0b8748e575
--- /dev/null
+++ b/xen/arch/arm/arm64/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);
+}
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index a2e22b659d..0a51142efd 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -69,8 +69,10 @@
 #include <xen/const.h>
 #include <xen/page-size.h>
 
-#ifdef CONFIG_MMU
+#if defined(CONFIG_MMU)
 #include <asm/mmu/layout.h>
+#elif defined(CONFIG_MPU)
+#include <asm/mpu/layout.h>
 #else
 # error "Unknown memory management layout"
 #endif
diff --git a/xen/arch/arm/include/asm/mpu/layout.h b/xen/arch/arm/include/asm/mpu/layout.h
new file mode 100644
index 0000000000..d6d397f4c2
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/layout.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ARM_MPU_LAYOUT_H__
+#define __ARM_MPU_LAYOUT_H__
+
+#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS
+
+/*
+ * All MPU platforms need to provide a XEN_START_ADDRESS for linker. This
+ * address indicates where Xen image will be loaded and run from. This
+ * address must be aligned to a PAGE_SIZE.
+ */
+#if (XEN_START_ADDRESS % PAGE_SIZE) != 0
+#error "XEN_START_ADDRESS must be aligned to 4KB"
+#endif
+
+/*
+ * For MPU, XEN's virtual start address is same as the physical address.
+ * The reason being MPU treats VA == PA. IOW, it cannot map the physical
+ * address to a different fixed virtual address. So, the virtual start
+ * address is determined by the physical address at which Xen is loaded.
+ */
+#define XEN_VIRT_START         _AT(paddr_t, XEN_START_ADDRESS)
+
+#endif /* __ARM_MPU_LAYOUT_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 5b9abc9a2d..d1e579e8a8 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -213,6 +213,13 @@ SECTIONS
  * match the context.
  */
 ASSERT(_start == XEN_VIRT_START, "_start != XEN_VIRT_START")
+#ifdef CONFIG_MPU
+/*
+ * On MPU based platforms, the starting address is to be provided by user.
+ * One need to check that it is 4KB aligned.
+ */
+ASSERT(IS_ALIGNED(_start,       4096), "starting address should be aligned to 4KB")
+#endif
 
 /*
  * We require that Xen is loaded at a page boundary, so this ensures that any
-- 
2.25.1

Re: [PATCH v4 3/6] xen/arm: mpu: Define Xen start address for MPU systems

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> From: Wei Chen <wei.chen@arm.com>
> 
> On Armv8-A, Xen has a fixed virtual start address (link address too) for all
> Armv8-A platforms. In an MMU based system, Xen can map its loaded address to
> this virtual start address. So, on Armv8-A platforms, the Xen start address does
> not need to be configurable. But on Armv8-R platforms, there is no MMU to map
> loaded address to a fixed virtual address and different platforms will have very
> different address space layout. So Xen cannot use a fixed physical address on
> MPU based system and need to have it configurable.
> 
> So, we introduce a Kconfig option for users to set the start address. The start
> address needs to be aligned to 4KB. We have a check for this alignment.
> 
> MPU allows us to define regions which are 64 bits aligned. This restriction
> comes from the bitfields of PRBAR, PRLAR (the lower 6 bits are 0 extended to
> provide the base and limit address of a region). This means that the start
> address of Xen needs to be at least 64 bits aligned (as it will correspond to
> the start address of memory protection region).
> 
> As for now Xen on MPU tries to use the same memory alignment restrictions as it
> has been for MMU. We have added a build assertion to ensure that the page size
> is 4KB. Unlike MMU where the starting virtual address is 2MB, Xen on MPU needs
> the start address to be 4KB (ie page size) aligned.
> 
> In case if the user forgets to set the start address, then 0xffffffff is used
> as default. This is to trigger the error (on alignment check) and thereby prompt
> user to set the start address.
> 
> Also updated config.h so that it includes mpu/layout.h when CONFIG_MPU is
> defined.
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> —

Looks good to me

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

Re: [PATCH v4 3/6] xen/arm: mpu: Define Xen start address for MPU systems

[Julien Grall]

Hi Ayan,

On 28/10/2024 12:45, Ayan Kumar Halder wrote:
> From: Wei Chen <wei.chen@arm.com>
> 
> On Armv8-A, Xen has a fixed virtual start address (link address too) for all
> Armv8-A platforms. In an MMU based system, Xen can map its loaded address to
> this virtual start address. So, on Armv8-A platforms, the Xen start address does
> not need to be configurable. But on Armv8-R platforms, there is no MMU to map
> loaded address to a fixed virtual address and different platforms will have very
> different address space layout. So Xen cannot use a fixed physical address on
> MPU based system and need to have it configurable.
> 
> So, we introduce a Kconfig option for users to set the start address. The start
> address needs to be aligned to 4KB. We have a check for this alignment.
> 
> MPU allows us to define regions which are 64 bits aligned. This restriction
> comes from the bitfields of PRBAR, PRLAR (the lower 6 bits are 0 extended to
> provide the base and limit address of a region). This means that the start
> address of Xen needs to be at least 64 bits aligned (as it will correspond to
> the start address of memory protection region).
> 
> As for now Xen on MPU tries to use the same memory alignment restrictions as it
> has been for MMU. We have added a build assertion to ensure that the page size
> is 4KB. Unlike MMU where the starting virtual address is 2MB, Xen on MPU needs
> the start address to be 4KB (ie page size) aligned.
> 
> In case if the user forgets to set the start address, then 0xffffffff is used
> as default. This is to trigger the error (on alignment check) and thereby prompt
> user to set the start address.
> 
> Also updated config.h so that it includes mpu/layout.h when CONFIG_MPU is
> defined.
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Jiamei.Xie <jiamei.xie@arm.com>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder]

Define enable_boot_cpu_mm() for the AArch64-V8R system.

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.

To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers.
One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
Registers", to get the definitions of these registers. Also, refer to G1.2
"Accessing MPU memory region registers", the following

```
The MPU provides two register interfaces to program the MPU regions:
- Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and
PRLAR<n>_ELx.
```

We use the above mechanism to create the MPU memory regions.

MPU specific registers are defined in
xen/arch/arm/include/asm/arm64/mpu/sysregs.h.

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

v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region,
we have separate MPU regions for different parts of the Xen binary. The reason
being different regions will nned different permissions (as mentioned in the
linker script).

2. Introduced a label (__init_data_begin) to mark the beginning of the init data
section.

3. Moved MPU specific register definitions to mpu/sysregs.h.

4. Fixed coding style issues.

5. Included page.h in mpu/head.S as page.h includes sysregs.h.
I haven't seen sysregs.h included directly from head.S or mmu/head.S.
(Outstanding comment not addressed).

v2 - 1. Extracted "enable_mpu()" in a separate patch.

2. Removed alignment for limit address.

3. Merged some of the sections for preparing the early boot regions.

4. Checked for the max limit of MPU regions before creating a new region.

5. Checked for empty regions.

v3 :- 1. Modified prepare_xen_region() so that we check for empty region within
this. Also, index of regions (to be programmed in PRSELR_EL2) should start from
0.

2. Removed load_paddr() as the offset is 0.

3. Introduced fail_insufficient_regions() to handle failure caused when the
number of regions to be allocated is not sufficient.

 xen/arch/arm/arm64/mpu/Makefile              |   1 +
 xen/arch/arm/arm64/mpu/head.S                | 122 +++++++++++++++++++
 xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  27 ++++
 xen/arch/arm/include/asm/mm.h                |   2 +
 xen/arch/arm/include/asm/mpu/arm64/mm.h      |  22 ++++
 xen/arch/arm/include/asm/mpu/mm.h            |  20 +++
 xen/arch/arm/xen.lds.S                       |   1 +
 7 files changed, 195 insertions(+)
 create mode 100644 xen/arch/arm/arm64/mpu/head.S
 create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
 create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
 create mode 100644 xen/arch/arm/include/asm/mpu/mm.h

diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
index b18cec4836..a8a750a3d0 100644
--- a/xen/arch/arm/arm64/mpu/Makefile
+++ b/xen/arch/arm/arm64/mpu/Makefile
@@ -1 +1,2 @@
+obj-y += head.o
 obj-y += mm.o
diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
new file mode 100644
index 0000000000..9377ae778c
--- /dev/null
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -0,0 +1,122 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Start-of-day code for an Armv8-R MPU system.
+ */
+
+#include <asm/mm.h>
+#include <asm/arm64/mpu/sysregs.h>
+
+#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
+#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
+#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
+
+#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 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.
+ *
+ * Inputs:
+ * sel:         region selector
+ * base:        reg storing base address (should be page-aligned)
+ * 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
+ */
+.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
+
+    msr   PRSELR_EL2, \sel
+    isb
+    msr   PRBAR_EL2, \prbar
+    msr   PRLAR_EL2, \prlar
+    dsb   sy
+    isb
+
+1:
+.endm
+
+/*
+ * Failure caused due to insufficient MPU regions.
+ */
+FUNC_LOCAL(fail_insufficient_regions)
+    PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")
+1:  wfe
+    b   1b
+END(fail_insufficient_regions)
+
+/*
+ * Maps the various sections of Xen (described in xen.lds.S) as different MPU
+ * regions.
+ *
+ * Inputs:
+ *   lr : Address to return to.
+ *
+ * Clobbers x0 - x5
+ *
+ */
+FUNC(enable_boot_cpu_mm)
+
+    /* Get the number of regions specified in MPUIR_EL2 */
+    mrs   x5, MPUIR_EL2
+
+    /* x0: region sel */
+    mov   x0, xzr
+    /* Xen text section. */
+    ldr   x1, =_stext
+    ldr   x2, =_etext
+    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
+
+    /* Xen read-only data section. */
+    ldr   x1, =_srodata
+    ldr   x2, =_erodata
+    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
+
+    /* Xen read-only after init and data section. (RW data) */
+    ldr   x1, =__ro_after_init_start
+    ldr   x2, =__init_begin
+    prepare_xen_region x0, x1, x2, x3, x4, x5
+
+    /* Xen code section. */
+    ldr   x1, =__init_begin
+    ldr   x2, =__init_data_begin
+    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
+
+    /* Xen data and BSS section. */
+    ldr   x1, =__init_data_begin
+    ldr   x2, =__bss_end
+    prepare_xen_region x0, x1, x2, x3, x4, x5
+
+    ret
+
+END(enable_boot_cpu_mm)
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
new file mode 100644
index 0000000000..b0c31a58ec
--- /dev/null
+++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ASM_ARM_ARM64_MPU_SYSREGS_H
+#define __ASM_ARM_ARM64_MPU_SYSREGS_H
+
+/* Number of EL2 MPU regions */
+#define MPUIR_EL2   S3_4_C0_C0_4
+
+/* EL2 MPU Protection Region Base Address Register encode */
+#define PRBAR_EL2   S3_4_C6_C8_0
+
+/* EL2 MPU Protection Region Limit Address Register encode */
+#define PRLAR_EL2   S3_4_C6_C8_1
+
+/* MPU Protection Region Selection Register encode */
+#define PRSELR_EL2  S3_4_C6_C2_1
+
+#endif /* __ASM_ARM_ARM64_MPU_SYSREGS_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 5abd4b0d1c..7e61f37612 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -16,6 +16,8 @@
 
 #if defined(CONFIG_MMU)
 # include <asm/mmu/mm.h>
+#elif defined(CONFIG_MPU)
+# include <asm/mpu/mm.h>
 #else
 # error "Unknown memory management layout"
 #endif
diff --git a/xen/arch/arm/include/asm/mpu/arm64/mm.h b/xen/arch/arm/include/asm/mpu/arm64/mm.h
new file mode 100644
index 0000000000..c2640b50df
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/arm64/mm.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * mm.h: Arm Memory Protection Unit definitions.
+ */
+
+#ifndef __ARM64_MPU_MM_H__
+#define __ARM64_MPU_MM_H__
+
+#define MPU_REGION_SHIFT  6
+#define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
+#define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
+
+#endif /* __ARM64_MPU_MM_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
new file mode 100644
index 0000000000..92599a1d75
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ARM_MPU_MM__
+#define __ARM_MPU_MM__
+
+#if defined(CONFIG_ARM_64)
+# include <asm/mpu/arm64/mm.h>
+#else
+# error "unknown ARM variant"
+#endif
+
+#endif /* __ARM_MPU_MM__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index d1e579e8a8..bbccff1a03 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -147,6 +147,7 @@ SECTIONS
        *(.altinstr_replacement)
   } :text
   . = ALIGN(PAGE_SIZE);
+  __init_data_begin = .;
   .init.data : {
        *(.init.rodata)
        *(.init.rodata.*)
-- 
2.25.1

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> Define enable_boot_cpu_mm() for the AArch64-V8R system.

Could you use here "Armv8-R AArch64” instead of “AArch64-V8R"

> 
> 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.
> 
> To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers.
> One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
> Registers", to get the definitions of these registers. Also, refer to G1.2
> "Accessing MPU memory region registers", the following
> 
> ```
> The MPU provides two register interfaces to program the MPU regions:
> - Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and
> PRLAR<n>_ELx.
> ```
> 
> We use the above mechanism to create the MPU memory regions.
> 
> MPU specific registers are defined in
> xen/arch/arm/include/asm/arm64/mpu/sysregs.h.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from :-
> 
> v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region,
> we have separate MPU regions for different parts of the Xen binary. The reason
> being different regions will nned different permissions (as mentioned in the
> linker script).
> 
> 2. Introduced a label (__init_data_begin) to mark the beginning of the init data
> section.
> 
> 3. Moved MPU specific register definitions to mpu/sysregs.h.
> 
> 4. Fixed coding style issues.
> 
> 5. Included page.h in mpu/head.S as page.h includes sysregs.h.
> I haven't seen sysregs.h included directly from head.S or mmu/head.S.
> (Outstanding comment not addressed).
> 
> v2 - 1. Extracted "enable_mpu()" in a separate patch.
> 
> 2. Removed alignment for limit address.
> 
> 3. Merged some of the sections for preparing the early boot regions.
> 
> 4. Checked for the max limit of MPU regions before creating a new region.
> 
> 5. Checked for empty regions.
> 
> v3 :- 1. Modified prepare_xen_region() so that we check for empty region within
> this. Also, index of regions (to be programmed in PRSELR_EL2) should start from
> 0.
> 
> 2. Removed load_paddr() as the offset is 0.
> 
> 3. Introduced fail_insufficient_regions() to handle failure caused when the
> number of regions to be allocated is not sufficient.
> 
> xen/arch/arm/arm64/mpu/Makefile              |   1 +
> xen/arch/arm/arm64/mpu/head.S                | 122 +++++++++++++++++++
> xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  27 ++++
> xen/arch/arm/include/asm/mm.h                |   2 +
> xen/arch/arm/include/asm/mpu/arm64/mm.h      |  22 ++++
> xen/arch/arm/include/asm/mpu/mm.h            |  20 +++
> xen/arch/arm/xen.lds.S                       |   1 +
> 7 files changed, 195 insertions(+)
> create mode 100644 xen/arch/arm/arm64/mpu/head.S
> create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
> create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
> 
> diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
> index b18cec4836..a8a750a3d0 100644
> --- a/xen/arch/arm/arm64/mpu/Makefile
> +++ b/xen/arch/arm/arm64/mpu/Makefile
> @@ -1 +1,2 @@
> +obj-y += head.o
> obj-y += mm.o
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 0000000000..9377ae778c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include <asm/mm.h>
> +#include <asm/arm64/mpu/sysregs.h>
> +
> +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
> +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
> +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */

NIT: alignment

> +
> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 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.
> + *
> + * Inputs:
> + * sel:         region selector
> + * base:        reg storing base address (should be page-aligned)
> + * 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

NIT: shall we also align the text after the colon?

> + */
> +.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
> +
> +    msr   PRSELR_EL2, \sel
> +    isb
> +    msr   PRBAR_EL2, \prbar
> +    msr   PRLAR_EL2, \prlar
> +    dsb   sy
> +    isb
> +
> +1:
> +.endm
> +
> +/*
> + * Failure caused due to insufficient MPU regions.
> + */
> +FUNC_LOCAL(fail_insufficient_regions)
> +    PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")

MPUIR_ELx is a read only register, so I would rephrase this message in something like:

“Selected MPU region is above the implemented number in MPUIR_EL2"

> +1:  wfe
> +    b   1b
> +END(fail_insufficient_regions)
> +
> +/*
> + * Maps the various sections of Xen (described in xen.lds.S) as different MPU
> + * regions.
> + *
> + * Inputs:
> + *   lr : Address to return to.
> + *
> + * Clobbers x0 - x5
> + *
> + */
> +FUNC(enable_boot_cpu_mm)
> +
> +    /* Get the number of regions specified in MPUIR_EL2 */
> +    mrs   x5, MPUIR_EL2
> +
> +    /* x0: region sel */
> +    mov   x0, xzr
> +    /* Xen text section. */
> +    ldr   x1, =_stext
> +    ldr   x2, =_etext
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR

After this region is written, there is no code to increment x0, so all the subsequent will override the
region 0.

> +
> +    /* Xen read-only data section. */
> +    ldr   x1, =_srodata
> +    ldr   x2, =_erodata
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> +
> +    /* Xen read-only after init and data section. (RW data) */
> +    ldr   x1, =__ro_after_init_start
> +    ldr   x2, =__init_begin
> +    prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +    /* Xen code section. */
> +    ldr   x1, =__init_begin
> +    ldr   x2, =__init_data_begin
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen data and BSS section. */
> +    ldr   x1, =__init_data_begin
> +    ldr   x2, =__bss_end
> +    prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +    ret
> +
> +END(enable_boot_cpu_mm)

The rest looks on to me

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hy Ayan,

>> +
>> +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
>> +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
>> +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
> 
> NIT: alignment
> 
>> +
>> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 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.
>> + *
>> + * Inputs:
>> + * sel:         region selector
>> + * base:        reg storing base address (should be page-aligned)
>> + * 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
> 
> NIT: shall we also align the text after the colon?
> 

Please forget about these comments, I’ve applied your patches and everything looks good in terms of alignment,
I was misled by my mail client.

Cheers,
Luca 

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder]

On 28/10/2024 15:14, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
>
>> On 28 Oct 2024, at 12:45, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
>>
>> Define enable_boot_cpu_mm() for the AArch64-V8R system.
> Could you use here "Armv8-R AArch64” instead of “AArch64-V8R"
Yes.
>
>> 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.
>>
>> To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers.
>> One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
>> Registers", to get the definitions of these registers. Also, refer to G1.2
>> "Accessing MPU memory region registers", the following
>>
>> ```
>> The MPU provides two register interfaces to program the MPU regions:
>> - Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and
>> PRLAR<n>_ELx.
>> ```
>>
>> We use the above mechanism to create the MPU memory regions.
>>
>> MPU specific registers are defined in
>> xen/arch/arm/include/asm/arm64/mpu/sysregs.h.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from :-
>>
>> v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region,
>> we have separate MPU regions for different parts of the Xen binary. The reason
>> being different regions will nned different permissions (as mentioned in the
>> linker script).
>>
>> 2. Introduced a label (__init_data_begin) to mark the beginning of the init data
>> section.
>>
>> 3. Moved MPU specific register definitions to mpu/sysregs.h.
>>
>> 4. Fixed coding style issues.
>>
>> 5. Included page.h in mpu/head.S as page.h includes sysregs.h.
>> I haven't seen sysregs.h included directly from head.S or mmu/head.S.
>> (Outstanding comment not addressed).
>>
>> v2 - 1. Extracted "enable_mpu()" in a separate patch.
>>
>> 2. Removed alignment for limit address.
>>
>> 3. Merged some of the sections for preparing the early boot regions.
>>
>> 4. Checked for the max limit of MPU regions before creating a new region.
>>
>> 5. Checked for empty regions.
>>
>> v3 :- 1. Modified prepare_xen_region() so that we check for empty region within
>> this. Also, index of regions (to be programmed in PRSELR_EL2) should start from
>> 0.
>>
>> 2. Removed load_paddr() as the offset is 0.
>>
>> 3. Introduced fail_insufficient_regions() to handle failure caused when the
>> number of regions to be allocated is not sufficient.
>>
>> xen/arch/arm/arm64/mpu/Makefile              |   1 +
>> xen/arch/arm/arm64/mpu/head.S                | 122 +++++++++++++++++++
>> xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  27 ++++
>> xen/arch/arm/include/asm/mm.h                |   2 +
>> xen/arch/arm/include/asm/mpu/arm64/mm.h      |  22 ++++
>> xen/arch/arm/include/asm/mpu/mm.h            |  20 +++
>> xen/arch/arm/xen.lds.S                       |   1 +
>> 7 files changed, 195 insertions(+)
>> create mode 100644 xen/arch/arm/arm64/mpu/head.S
>> create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
>> create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
>> create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
>>
>> diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
>> index b18cec4836..a8a750a3d0 100644
>> --- a/xen/arch/arm/arm64/mpu/Makefile
>> +++ b/xen/arch/arm/arm64/mpu/Makefile
>> @@ -1 +1,2 @@
>> +obj-y += head.o
>> obj-y += mm.o
>> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
>> new file mode 100644
>> index 0000000000..9377ae778c
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/mpu/head.S
>> @@ -0,0 +1,122 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Start-of-day code for an Armv8-R MPU system.
>> + */
>> +
>> +#include <asm/mm.h>
>> +#include <asm/arm64/mpu/sysregs.h>
>> +
>> +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
>> +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
>> +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
> NIT: alignment
>
>> +
>> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 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.
>> + *
>> + * Inputs:
>> + * sel:         region selector
>> + * base:        reg storing base address (should be page-aligned)
>> + * 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
> NIT: shall we also align the text after the colon?
>
>> + */
>> +.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
>> +
>> +    msr   PRSELR_EL2, \sel
>> +    isb
>> +    msr   PRBAR_EL2, \prbar
>> +    msr   PRLAR_EL2, \prlar
>> +    dsb   sy
>> +    isb
>> +
>> +1:
>> +.endm
>> +
>> +/*
>> + * Failure caused due to insufficient MPU regions.
>> + */
>> +FUNC_LOCAL(fail_insufficient_regions)
>> +    PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")
> MPUIR_ELx is a read only register, so I would rephrase this message in something like:
>
> “Selected MPU region is above the implemented number in MPUIR_EL2"
Ack.
>
>> +1:  wfe
>> +    b   1b
>> +END(fail_insufficient_regions)
>> +
>> +/*
>> + * Maps the various sections of Xen (described in xen.lds.S) as different MPU
>> + * regions.
>> + *
>> + * Inputs:
>> + *   lr : Address to return to.
>> + *
>> + * Clobbers x0 - x5
>> + *
>> + */
>> +FUNC(enable_boot_cpu_mm)
>> +
>> +    /* Get the number of regions specified in MPUIR_EL2 */
>> +    mrs   x5, MPUIR_EL2
>> +
>> +    /* x0: region sel */
>> +    mov   x0, xzr
>> +    /* Xen text section. */
>> +    ldr   x1, =_stext
>> +    ldr   x2, =_etext
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> After this region is written, there is no code to increment x0, so all the subsequent will override the
> region 0.
Ah yes, you are correct. I should increment \sel in prepare_xen_region().
>
>> +
>> +    /* Xen read-only data section. */
>> +    ldr   x1, =_srodata
>> +    ldr   x2, =_erodata
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
>> +
>> +    /* Xen read-only after init and data section. (RW data) */
>> +    ldr   x1, =__ro_after_init_start
>> +    ldr   x2, =__init_begin
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>> +
>> +    /* Xen code section. */
>> +    ldr   x1, =__init_begin
>> +    ldr   x2, =__init_data_begin
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
>> +
>> +    /* Xen data and BSS section. */
>> +    ldr   x1, =__init_data_begin
>> +    ldr   x2, =__bss_end
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>> +
>> +    ret
>> +
>> +END(enable_boot_cpu_mm)
> The rest looks on to me

Thanks.

- Ayan

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,

I forgot another thing:


> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 0000000000..9377ae778c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include <asm/mm.h>
     ^— This feels suspicious, this header cannot be included by an assembly file

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,

While I rebased the branch on top of your patches, I saw you’ve changed the number of regions
mapped at boot time, can I ask why? 

Compared to https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-Penny.Zheng@arm.com/:

> +FUNC(enable_boot_cpu_mm)
> +
> +    /* Get the number of regions specified in MPUIR_EL2 */
> +    mrs   x5, MPUIR_EL2
> +
> +    /* x0: region sel */
> +    mov   x0, xzr
> +    /* Xen text section. */
> +    ldr   x1, =_stext
> +    ldr   x2, =_etext
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen read-only data section. */
> +    ldr   x1, =_srodata
> +    ldr   x2, =_erodata
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> +
> +    /* Xen read-only after init and data section. (RW data) */
> +    ldr   x1, =__ro_after_init_start
> +    ldr   x2, =__init_begin
> +    prepare_xen_region x0, x1, x2, x3, x4, x5

         ^— this, for example, will block Xen to call init_done(void) later, I understand this is earlyboot,
               but I guess we don’t want to make subsequent changes to this part when introducing the
               patches to support start_xen()

> +
> +    /* Xen code section. */
> +    ldr   x1, =__init_begin
> +    ldr   x2, =__init_data_begin
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen data and BSS section. */
> +    ldr   x1, =__init_data_begin
> +    ldr   x2, =__bss_end
> +    prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +    ret
> +
> +END(enable_boot_cpu_mm)

I suggest to keep exactly the regions as are in Penny’s patch.

Cheers,
Luca

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Julien Grall]

On Wed, 30 Oct 2024 at 09:17, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>
> Hi Ayan,
>
> While I rebased the branch on top of your patches, I saw you’ve changed the number of regions
> mapped at boot time, can I ask why?

I have asked the change. If you look at the layout...

>
> Compared to https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-Penny.Zheng@arm.com/:


... you have two sections with the same permissions:

xen_mpumap[1] : Xen read-only data
xen_mpumap[2] : Xen read-only after init data
xen_mpumap[3] : Xen read-write data

During boot, [2] and [3] will share the same permissions. After boot,
this will be [1] and [2]. Given the number of MPU regions is limited,
this is a bit of a waste.

We also don't want to have a hole in the middle of Xen sections. So
folding seemed to be a good idea.

>
> > +FUNC(enable_boot_cpu_mm)
> > +
> > +    /* Get the number of regions specified in MPUIR_EL2 */
> > +    mrs   x5, MPUIR_EL2
> > +
> > +    /* x0: region sel */
> > +    mov   x0, xzr
> > +    /* Xen text section. */
> > +    ldr   x1, =_stext
> > +    ldr   x2, =_etext
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> > +
> > +    /* Xen read-only data section. */
> > +    ldr   x1, =_srodata
> > +    ldr   x2, =_erodata
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> > +
> > +    /* Xen read-only after init and data section. (RW data) */
> > +    ldr   x1, =__ro_after_init_start
> > +    ldr   x2, =__init_begin
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5
>
>          ^— this, for example, will block Xen to call init_done(void) later, I understand this is earlyboot,
>                but I guess we don’t want to make subsequent changes to this part when introducing the
>                patches to support start_xen()

Can you be a bit more descriptive... What will block?

>
> > +
> > +    /* Xen code section. */
> > +    ldr   x1, =__init_begin
> > +    ldr   x2, =__init_data_begin
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> > +
> > +    /* Xen data and BSS section. */
> > +    ldr   x1, =__init_data_begin
> > +    ldr   x2, =__bss_end
> > +    prepare_xen_region x0, x1, x2, x3, x4, x5
> > +
> > +    ret
> > +
> > +END(enable_boot_cpu_mm)
>
> I suggest to keep exactly the regions as are in Penny’s patch.

See above. Without any details on the exact problem, it is difficult
to agree on your suggestion.

Cheers,

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Julien,

> On 30 Oct 2024, at 09:52, Julien Grall <julien.grall.oss@gmail.com> wrote:
> 
> On Wed, 30 Oct 2024 at 09:17, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>> 
>> Hi Ayan,
>> 
>> While I rebased the branch on top of your patches, I saw you’ve changed the number of regions
>> mapped at boot time, can I ask why?
> 
> I have asked the change. If you look at the layout...

Apologies, I didn’t see you’ve asked the change

> 
>> 
>> Compared to https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-Penny.Zheng@arm.com/:
> 
> 
> ... you have two sections with the same permissions:
> 
> xen_mpumap[1] : Xen read-only data
> xen_mpumap[2] : Xen read-only after init data
> xen_mpumap[3] : Xen read-write data
> 
> During boot, [2] and [3] will share the same permissions. After boot,
> this will be [1] and [2]. Given the number of MPU regions is limited,
> this is a bit of a waste.
> 
> We also don't want to have a hole in the middle of Xen sections. So
> folding seemed to be a good idea.
> 
>> 
>>> +FUNC(enable_boot_cpu_mm)
>>> +
>>> +    /* Get the number of regions specified in MPUIR_EL2 */
>>> +    mrs   x5, MPUIR_EL2
>>> +
>>> +    /* x0: region sel */
>>> +    mov   x0, xzr
>>> +    /* Xen text section. */
>>> +    ldr   x1, =_stext
>>> +    ldr   x2, =_etext
>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
>>> +
>>> +    /* Xen read-only data section. */
>>> +    ldr   x1, =_srodata
>>> +    ldr   x2, =_erodata
>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
>>> +
>>> +    /* Xen read-only after init and data section. (RW data) */
>>> +    ldr   x1, =__ro_after_init_start
>>> +    ldr   x2, =__init_begin
>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>> 
>>         ^— this, for example, will block Xen to call init_done(void) later, I understand this is earlyboot,
>>               but I guess we don’t want to make subsequent changes to this part when introducing the
>>               patches to support start_xen()
> 
> Can you be a bit more descriptive... What will block?

This call in setup.c:
    rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
                             (unsigned long)&__ro_after_init_end,
                             PAGE_HYPERVISOR_RO);

Cannot work anymore because xen_mpumap[2] is wider than only (__ro_after_init_start, __ro_after_init_end).

If that is what we want, then we could wrap the above call into something MMU specific that will execute the above call and
something MPU specific that will modify xen_mpumap[1] from (_srodata, _erodata) to (_srodata, __ro_after_init_end)
and xen_mpumap[2] from (__ro_after_init_start, __init_begin) to (__ro_after_init_end, __init_begin).

Please, let me know your thoughts.

Cheers,
Luca

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Julien Grall]

On 30/10/2024 10:08, Luca Fancellu wrote:
> Hi Julien,
> 
>> On 30 Oct 2024, at 09:52, Julien Grall <julien.grall.oss@gmail.com> wrote:
>>
>> On Wed, 30 Oct 2024 at 09:17, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>>>
>>> Hi Ayan,
>>>
>>> While I rebased the branch on top of your patches, I saw you’ve changed the number of regions
>>> mapped at boot time, can I ask why?
>>
>> I have asked the change. If you look at the layout...
> 
> Apologies, I didn’t see you’ve asked the change

No need to apologies! I think I asked a few revisions ago.

> 
>>
>>>
>>> Compared to https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-Penny.Zheng@arm.com/:
>>
>>
>> ... you have two sections with the same permissions:
>>
>> xen_mpumap[1] : Xen read-only data
>> xen_mpumap[2] : Xen read-only after init data
>> xen_mpumap[3] : Xen read-write data
>>
>> During boot, [2] and [3] will share the same permissions. After boot,
>> this will be [1] and [2]. Given the number of MPU regions is limited,
>> this is a bit of a waste.
>>
>> We also don't want to have a hole in the middle of Xen sections. So
>> folding seemed to be a good idea.
>>
>>>
>>>> +FUNC(enable_boot_cpu_mm)
>>>> +
>>>> +    /* Get the number of regions specified in MPUIR_EL2 */
>>>> +    mrs   x5, MPUIR_EL2
>>>> +
>>>> +    /* x0: region sel */
>>>> +    mov   x0, xzr
>>>> +    /* Xen text section. */
>>>> +    ldr   x1, =_stext
>>>> +    ldr   x2, =_etext
>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
>>>> +
>>>> +    /* Xen read-only data section. */
>>>> +    ldr   x1, =_srodata
>>>> +    ldr   x2, =_erodata
>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
>>>> +
>>>> +    /* Xen read-only after init and data section. (RW data) */
>>>> +    ldr   x1, =__ro_after_init_start
>>>> +    ldr   x2, =__init_begin
>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>>>
>>>          ^— this, for example, will block Xen to call init_done(void) later, I understand this is earlyboot,
>>>                but I guess we don’t want to make subsequent changes to this part when introducing the
>>>                patches to support start_xen()
>>
>> Can you be a bit more descriptive... What will block?
> 
> This call in setup.c:
>      rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
>                               (unsigned long)&__ro_after_init_end,
>                               PAGE_HYPERVISOR_RO);
> 
> Cannot work anymore because xen_mpumap[2] is wider than only (__ro_after_init_start, __ro_after_init_end).

Is this because the implementation of modify_xen_mappings() is only able 
to modify a full region? IOW, it would not be able to split regions 
and/or merge them?

> 
> If that is what we want, then we could wrap the above call into something MMU specific that will execute the above call and
> something MPU specific that will modify xen_mpumap[1] from (_srodata, _erodata) to (_srodata, __ro_after_init_end)
> and xen_mpumap[2] from (__ro_after_init_start, __init_begin) to (__ro_after_init_end, __init_begin).

I think it would make sense to have the call mmu/mpu specific. This 
would allow to limit the number of MPU regions used by Xen itself.

The only problem is IIRC the region is not fixed because we will skip 
empty regions during earlyboot.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Julien,

> On 30 Oct 2024, at 10:32, Julien Grall <julien@xen.org> wrote:
> 
> On 30/10/2024 10:08, Luca Fancellu wrote:
>> Hi Julien,
>>> On 30 Oct 2024, at 09:52, Julien Grall <julien.grall.oss@gmail.com> wrote:
>>> 
>>> On Wed, 30 Oct 2024 at 09:17, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>>>> 
>>>> Hi Ayan,
>>>> 
>>>> While I rebased the branch on top of your patches, I saw you’ve changed the number of regions
>>>> mapped at boot time, can I ask why?
>>> 
>>> I have asked the change. If you look at the layout...
>> Apologies, I didn’t see you’ve asked the change
> 
> No need to apologies! I think I asked a few revisions ago.
> 
>>> 
>>>> 
>>>> Compared to https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-Penny.Zheng@arm.com/:
>>> 
>>> 
>>> ... you have two sections with the same permissions:
>>> 
>>> xen_mpumap[1] : Xen read-only data
>>> xen_mpumap[2] : Xen read-only after init data
>>> xen_mpumap[3] : Xen read-write data
>>> 
>>> During boot, [2] and [3] will share the same permissions. After boot,
>>> this will be [1] and [2]. Given the number of MPU regions is limited,
>>> this is a bit of a waste.
>>> 
>>> We also don't want to have a hole in the middle of Xen sections. So
>>> folding seemed to be a good idea.
>>> 
>>>> 
>>>>> +FUNC(enable_boot_cpu_mm)
>>>>> +
>>>>> +    /* Get the number of regions specified in MPUIR_EL2 */
>>>>> +    mrs   x5, MPUIR_EL2
>>>>> +
>>>>> +    /* x0: region sel */
>>>>> +    mov   x0, xzr
>>>>> +    /* Xen text section. */
>>>>> +    ldr   x1, =_stext
>>>>> +    ldr   x2, =_etext
>>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
>>>>> +
>>>>> +    /* Xen read-only data section. */
>>>>> +    ldr   x1, =_srodata
>>>>> +    ldr   x2, =_erodata
>>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
>>>>> +
>>>>> +    /* Xen read-only after init and data section. (RW data) */
>>>>> +    ldr   x1, =__ro_after_init_start
>>>>> +    ldr   x2, =__init_begin
>>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>>>> 
>>>>         ^— this, for example, will block Xen to call init_done(void) later, I understand this is earlyboot,
>>>>               but I guess we don’t want to make subsequent changes to this part when introducing the
>>>>               patches to support start_xen()
>>> 
>>> Can you be a bit more descriptive... What will block?
>> This call in setup.c:
>>     rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
>>                              (unsigned long)&__ro_after_init_end,
>>                              PAGE_HYPERVISOR_RO);
>> Cannot work anymore because xen_mpumap[2] is wider than only (__ro_after_init_start, __ro_after_init_end).
> 
> Is this because the implementation of modify_xen_mappings() is only able to modify a full region? IOW, it would not be able to split regions and/or merge them?

Yes, the code is, at the moment, not smart enough to do that, it will only modify a full region.

> 
>> If that is what we want, then we could wrap the above call into something MMU specific that will execute the above call and
>> something MPU specific that will modify xen_mpumap[1] from (_srodata, _erodata) to (_srodata, __ro_after_init_end)
>> and xen_mpumap[2] from (__ro_after_init_start, __init_begin) to (__ro_after_init_end, __init_begin).
> 
> I think it would make sense to have the call mmu/mpu specific. This would allow to limit the number of MPU regions used by Xen itself.
> 
> The only problem is IIRC the region is not fixed because we will skip empty regions during earlyboot.

Yes, but I think we can assume that X(_srodata, _erodata) and Y(__ro_after_init_start, __init_begin) won’t never be empty for Xen?

In that case, the call to mpumap_contain_region() should be able to retrieve the full region X and the partial region Y and
a specific function could modify the ranges of both given the respective indexes.

Code in my branch: https://gitlab.com/xen-project/people/lucafancellu/xen/-/blob/r82_rebased/xen/arch/arm/mpu/mm.c#L382

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder]

On 30/10/2024 10:51, Luca Fancellu wrote:
> Hi Julien,
Hi Luca/Julien,
>
>> On 30 Oct 2024, at 10:32, Julien Grall <julien@xen.org> wrote:
>>
>> On 30/10/2024 10:08, Luca Fancellu wrote:
>>> Hi Julien,
>>>> On 30 Oct 2024, at 09:52, Julien Grall <julien.grall.oss@gmail.com> wrote:
>>>>
>>>> On Wed, 30 Oct 2024 at 09:17, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>>>>> Hi Ayan,
>>>>>
>>>>> While I rebased the branch on top of your patches, I saw you’ve changed the number of regions
>>>>> mapped at boot time, can I ask why?
>>>> I have asked the change. If you look at the layout...
>>> Apologies, I didn’t see you’ve asked the change
>> No need to apologies! I think I asked a few revisions ago.
>>
>>>>> Compared to https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-Penny.Zheng@arm.com/:
>>>>
>>>> ... you have two sections with the same permissions:
>>>>
>>>> xen_mpumap[1] : Xen read-only data
>>>> xen_mpumap[2] : Xen read-only after init data
>>>> xen_mpumap[3] : Xen read-write data
>>>>
>>>> During boot, [2] and [3] will share the same permissions. After boot,
>>>> this will be [1] and [2]. Given the number of MPU regions is limited,
>>>> this is a bit of a waste.
>>>>
>>>> We also don't want to have a hole in the middle of Xen sections. So
>>>> folding seemed to be a good idea.
>>>>
>>>>>> +FUNC(enable_boot_cpu_mm)
>>>>>> +
>>>>>> +    /* Get the number of regions specified in MPUIR_EL2 */
>>>>>> +    mrs   x5, MPUIR_EL2
>>>>>> +
>>>>>> +    /* x0: region sel */
>>>>>> +    mov   x0, xzr
>>>>>> +    /* Xen text section. */
>>>>>> +    ldr   x1, =_stext
>>>>>> +    ldr   x2, =_etext
>>>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
>>>>>> +
>>>>>> +    /* Xen read-only data section. */
>>>>>> +    ldr   x1, =_srodata
>>>>>> +    ldr   x2, =_erodata
>>>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
>>>>>> +
>>>>>> +    /* Xen read-only after init and data section. (RW data) */
>>>>>> +    ldr   x1, =__ro_after_init_start
>>>>>> +    ldr   x2, =__init_begin
>>>>>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>>>>>          ^— this, for example, will block Xen to call init_done(void) later, I understand this is earlyboot,
>>>>>                but I guess we don’t want to make subsequent changes to this part when introducing the
>>>>>                patches to support start_xen()
>>>> Can you be a bit more descriptive... What will block?
>>> This call in setup.c:
>>>      rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
>>>                               (unsigned long)&__ro_after_init_end,
>>>                               PAGE_HYPERVISOR_RO);
>>> Cannot work anymore because xen_mpumap[2] is wider than only (__ro_after_init_start, __ro_after_init_end).
>> Is this because the implementation of modify_xen_mappings() is only able to modify a full region? IOW, it would not be able to split regions and/or merge them?
> Yes, the code is, at the moment, not smart enough to do that, it will only modify a full region.
>
>>> If that is what we want, then we could wrap the above call into something MMU specific that will execute the above call and
>>> something MPU specific that will modify xen_mpumap[1] from (_srodata, _erodata) to (_srodata, __ro_after_init_end)
>>> and xen_mpumap[2] from (__ro_after_init_start, __init_begin) to (__ro_after_init_end, __init_begin).
>> I think it would make sense to have the call mmu/mpu specific. This would allow to limit the number of MPU regions used by Xen itself.
>>
>> The only problem is IIRC the region is not fixed because we will skip empty regions during earlyboot.
> Yes, but I think we can assume that X(_srodata, _erodata) and Y(__ro_after_init_start, __init_begin) won’t never be empty for Xen?
>
> In that case, the call to mpumap_contain_region() should be able to retrieve the full region X and the partial region Y and
> a specific function could modify the ranges of both given the respective indexes.
>
> Code in my branch: https://gitlab.com/xen-project/people/lucafancellu/xen/-/blob/r82_rebased/xen/arch/arm/mpu/mm.c#L382

Can we keep the current patch as it is ? We can revisit 
enable_boot_cpu_mm() when we enable start_xen() for mpu.

I can send a respin once we are aligned.

- Ayan

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Julien Grall]

Hi Ayan,

On 28/10/2024 12:45, Ayan Kumar Halder wrote:
> Define enable_boot_cpu_mm() for the AArch64-V8R system.
> 
> 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.
> 
> To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers.
> One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
> Registers", to get the definitions of these registers. Also, refer to G1.2
> "Accessing MPU memory region registers", the following
> 
> ```
> The MPU provides two register interfaces to program the MPU regions:
> - Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and
> PRLAR<n>_ELx.
> ```
> 
> We use the above mechanism to create the MPU memory regions.
> 
> MPU specific registers are defined in
> xen/arch/arm/include/asm/arm64/mpu/sysregs.h.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from :-
> 
> v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region,
> we have separate MPU regions for different parts of the Xen binary. The reason
> being different regions will nned different permissions (as mentioned in the
> linker script).
> 
> 2. Introduced a label (__init_data_begin) to mark the beginning of the init data
> section.
> 
> 3. Moved MPU specific register definitions to mpu/sysregs.h.
> 
> 4. Fixed coding style issues.
> 
> 5. Included page.h in mpu/head.S as page.h includes sysregs.h.
> I haven't seen sysregs.h included directly from head.S or mmu/head.S.
> (Outstanding comment not addressed).
> 
> v2 - 1. Extracted "enable_mpu()" in a separate patch.
> 
> 2. Removed alignment for limit address.
> 
> 3. Merged some of the sections for preparing the early boot regions.
> 
> 4. Checked for the max limit of MPU regions before creating a new region.
> 
> 5. Checked for empty regions.
> 
> v3 :- 1. Modified prepare_xen_region() so that we check for empty region within
> this. Also, index of regions (to be programmed in PRSELR_EL2) should start from
> 0.
> 
> 2. Removed load_paddr() as the offset is 0.
> 
> 3. Introduced fail_insufficient_regions() to handle failure caused when the
> number of regions to be allocated is not sufficient.
> 
>   xen/arch/arm/arm64/mpu/Makefile              |   1 +
>   xen/arch/arm/arm64/mpu/head.S                | 122 +++++++++++++++++++
>   xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  27 ++++
>   xen/arch/arm/include/asm/mm.h                |   2 +
>   xen/arch/arm/include/asm/mpu/arm64/mm.h      |  22 ++++
>   xen/arch/arm/include/asm/mpu/mm.h            |  20 +++
>   xen/arch/arm/xen.lds.S                       |   1 +
>   7 files changed, 195 insertions(+)
>   create mode 100644 xen/arch/arm/arm64/mpu/head.S
>   create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
>   create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
>   create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
> 
> diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
> index b18cec4836..a8a750a3d0 100644
> --- a/xen/arch/arm/arm64/mpu/Makefile
> +++ b/xen/arch/arm/arm64/mpu/Makefile
> @@ -1 +1,2 @@
> +obj-y += head.o
>   obj-y += mm.o
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 0000000000..9377ae778c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include <asm/mm.h>
> +#include <asm/arm64/mpu/sysregs.h>
> +
> +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
> +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
> +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
> +
> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 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.
> + *
> + * Inputs:
> + * sel:         region selector
> + * base:        reg storing base address (should be page-aligned)
> + * 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
> + */
> +.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
> +
> +    msr   PRSELR_EL2, \sel
> +    isb
> +    msr   PRBAR_EL2, \prbar
> +    msr   PRLAR_EL2, \prlar
> +    dsb   sy
> +    isb
> +
> +1:
> +.endm
> +
> +/*
> + * Failure caused due to insufficient MPU regions.
> + */
> +FUNC_LOCAL(fail_insufficient_regions)
> +    PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")
> +1:  wfe
> +    b   1b
> +END(fail_insufficient_regions)
> +
> +/*
> + * Maps the various sections of Xen (described in xen.lds.S) as different MPU
> + * regions.
> + *
> + * Inputs:
> + *   lr : Address to return to.
> + *
> + * Clobbers x0 - x5
> + *
> + */
> +FUNC(enable_boot_cpu_mm)
> +

NIT: I would remove this empty line.

> +    /* Get the number of regions specified in MPUIR_EL2 */
> +    mrs   x5, MPUIR_EL2

Looking at the spec, the number of regions are only in the first 8-bits. 
So you want to mask before using it.

> +
> +    /* x0: region sel */
> +    mov   x0, xzr
> +    /* Xen text section. */
> +    ldr   x1, =_stext
> +    ldr   x2, =_etext
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen read-only data section. */
> +    ldr   x1, =_srodata
> +    ldr   x2, =_erodata
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> +
> +    /* Xen read-only after init and data section. (RW data) */
> +    ldr   x1, =__ro_after_init_start
> +    ldr   x2, =__init_begin
> +    prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +    /* Xen code section. */
> +    ldr   x1, =__init_begin
> +    ldr   x2, =__init_data_begin
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen data and BSS section. */
> +    ldr   x1, =__init_data_begin
> +    ldr   x2, =__bss_end
> +    prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +    ret
> +

NIT: I would remove this empty line.
	
> +END(enable_boot_cpu_mm)
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> new file mode 100644
> index 0000000000..b0c31a58ec
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_ARM_ARM64_MPU_SYSREGS_H
> +#define __ASM_ARM_ARM64_MPU_SYSREGS_H
> +
> +/* Number of EL2 MPU regions */
> +#define MPUIR_EL2   S3_4_C0_C0_4

Do you know why the compiler doesn't support MPUIR_EL2 & co natively? 
Any chance this is because we may build Xen with the wrong flags for the 
MPU port?

Cheers,

-- 
Julien Grall

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder]

On 01/11/2024 14:11, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
>
> On 28/10/2024 12:45, Ayan Kumar Halder wrote:
>> Define enable_boot_cpu_mm() for the AArch64-V8R system.
>>
>> 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.
>>
>> To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 
>> registers.
>> One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
>> Registers", to get the definitions of these registers. Also, refer to 
>> G1.2
>> "Accessing MPU memory region registers", the following
>>
>> ```
>> The MPU provides two register interfaces to program the MPU regions:
>> - Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and
>> PRLAR<n>_ELx.
>> ```
>>
>> We use the above mechanism to create the MPU memory regions.
>>
>> MPU specific registers are defined in
>> xen/arch/arm/include/asm/arm64/mpu/sysregs.h.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> Changes from :-
>>
>> v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single 
>> MPU region,
>> we have separate MPU regions for different parts of the Xen binary. 
>> The reason
>> being different regions will nned different permissions (as mentioned 
>> in the
>> linker script).
>>
>> 2. Introduced a label (__init_data_begin) to mark the beginning of 
>> the init data
>> section.
>>
>> 3. Moved MPU specific register definitions to mpu/sysregs.h.
>>
>> 4. Fixed coding style issues.
>>
>> 5. Included page.h in mpu/head.S as page.h includes sysregs.h.
>> I haven't seen sysregs.h included directly from head.S or mmu/head.S.
>> (Outstanding comment not addressed).
>>
>> v2 - 1. Extracted "enable_mpu()" in a separate patch.
>>
>> 2. Removed alignment for limit address.
>>
>> 3. Merged some of the sections for preparing the early boot regions.
>>
>> 4. Checked for the max limit of MPU regions before creating a new 
>> region.
>>
>> 5. Checked for empty regions.
>>
>> v3 :- 1. Modified prepare_xen_region() so that we check for empty 
>> region within
>> this. Also, index of regions (to be programmed in PRSELR_EL2) should 
>> start from
>> 0.
>>
>> 2. Removed load_paddr() as the offset is 0.
>>
>> 3. Introduced fail_insufficient_regions() to handle failure caused 
>> when the
>> number of regions to be allocated is not sufficient.
>>
>>   xen/arch/arm/arm64/mpu/Makefile              |   1 +
>>   xen/arch/arm/arm64/mpu/head.S                | 122 +++++++++++++++++++
>>   xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  27 ++++
>>   xen/arch/arm/include/asm/mm.h                |   2 +
>>   xen/arch/arm/include/asm/mpu/arm64/mm.h      |  22 ++++
>>   xen/arch/arm/include/asm/mpu/mm.h            |  20 +++
>>   xen/arch/arm/xen.lds.S                       |   1 +
>>   7 files changed, 195 insertions(+)
>>   create mode 100644 xen/arch/arm/arm64/mpu/head.S
>>   create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
>>   create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
>>   create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
>>
>> diff --git a/xen/arch/arm/arm64/mpu/Makefile 
>> b/xen/arch/arm/arm64/mpu/Makefile
>> index b18cec4836..a8a750a3d0 100644
>> --- a/xen/arch/arm/arm64/mpu/Makefile
>> +++ b/xen/arch/arm/arm64/mpu/Makefile
>> @@ -1 +1,2 @@
>> +obj-y += head.o
>>   obj-y += mm.o
>> diff --git a/xen/arch/arm/arm64/mpu/head.S 
>> b/xen/arch/arm/arm64/mpu/head.S
>> new file mode 100644
>> index 0000000000..9377ae778c
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/mpu/head.S
>> @@ -0,0 +1,122 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Start-of-day code for an Armv8-R MPU system.
>> + */
>> +
>> +#include <asm/mm.h>
>> +#include <asm/arm64/mpu/sysregs.h>
>> +
>> +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
>> +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
>> +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
>> +
>> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 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.
>> + *
>> + * Inputs:
>> + * sel:         region selector
>> + * base:        reg storing base address (should be page-aligned)
>> + * 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
>> + */
>> +.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
>> +
>> +    msr   PRSELR_EL2, \sel
>> +    isb
>> +    msr   PRBAR_EL2, \prbar
>> +    msr   PRLAR_EL2, \prlar
>> +    dsb   sy
>> +    isb
>> +
>> +1:
>> +.endm
>> +
>> +/*
>> + * Failure caused due to insufficient MPU regions.
>> + */
>> +FUNC_LOCAL(fail_insufficient_regions)
>> +    PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")
>> +1:  wfe
>> +    b   1b
>> +END(fail_insufficient_regions)
>> +
>> +/*
>> + * Maps the various sections of Xen (described in xen.lds.S) as 
>> different MPU
>> + * regions.
>> + *
>> + * Inputs:
>> + *   lr : Address to return to.
>> + *
>> + * Clobbers x0 - x5
>> + *
>> + */
>> +FUNC(enable_boot_cpu_mm)
>> +
>
> NIT: I would remove this empty line.
Ack
>
>> +    /* Get the number of regions specified in MPUIR_EL2 */
>> +    mrs   x5, MPUIR_EL2
>
> Looking at the spec, the number of regions are only in the first 
> 8-bits. So you want to mask before using it.
yes, I will do it.
>
>> +
>> +    /* x0: region sel */
>> +    mov   x0, xzr
>> +    /* Xen text section. */
>> +    ldr   x1, =_stext
>> +    ldr   x2, =_etext
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, 
>> attr_prbar=REGION_TEXT_PRBAR
>> +
>> +    /* Xen read-only data section. */
>> +    ldr   x1, =_srodata
>> +    ldr   x2, =_erodata
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, 
>> attr_prbar=REGION_RO_PRBAR
>> +
>> +    /* Xen read-only after init and data section. (RW data) */
>> +    ldr   x1, =__ro_after_init_start
>> +    ldr   x2, =__init_begin
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>> +
>> +    /* Xen code section. */
>> +    ldr   x1, =__init_begin
>> +    ldr   x2, =__init_data_begin
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5, 
>> attr_prbar=REGION_TEXT_PRBAR
>> +
>> +    /* Xen data and BSS section. */
>> +    ldr   x1, =__init_data_begin
>> +    ldr   x2, =__bss_end
>> +    prepare_xen_region x0, x1, x2, x3, x4, x5
>> +
>> +    ret
>> +
>
> NIT: I would remove this empty line.
ack.
>
>> +END(enable_boot_cpu_mm)
>> +
>> +/*
>> + * Local variables:
>> + * mode: ASM
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h 
>> b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
>> new file mode 100644
>> index 0000000000..b0c31a58ec
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef __ASM_ARM_ARM64_MPU_SYSREGS_H
>> +#define __ASM_ARM_ARM64_MPU_SYSREGS_H
>> +
>> +/* Number of EL2 MPU regions */
>> +#define MPUIR_EL2   S3_4_C0_C0_4
>
> Do you know why the compiler doesn't support MPUIR_EL2 & co natively? 
> Any chance this is because we may build Xen with the wrong flags for 
> the MPU port?

Yes, you are correct. We need the following change.

|--- a/xen/arch/arm/arch.mk +++ b/xen/arch/arm/arch.mk @@ -9,7 +9,11 @@ 
CFLAGS-$(CONFIG_ARM_32) += -msoft-float CFLAGS-$(CONFIG_ARM_32) += 
-mcpu=cortex-a15 CFLAGS-$(CONFIG_ARM_32) += -mno-unaligned-access +ifeq 
($(CONFIG_MPU),y) +CFLAGS-$(CONFIG_ARM_64) += -march=armv8-r +else 
CFLAGS-$(CONFIG_ARM_64) += -mcpu=generic +endif CFLAGS-$(CONFIG_ARM_64) 
+= -mgeneral-regs-only # No fp registers etc $(call 
cc-option-add,CFLAGS-$(CONFIG_ARM_64),CC,-mno-outline-atomics) - Ayan |

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Ayan Kumar Halder]

On 01/11/2024 17:08, Ayan Kumar Halder wrote:
>
> On 01/11/2024 14:11, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien,

> Yes, you are correct. We need the following change.
--- a/xen/arch/arm/arch.mk
+++ b/xen/arch/arm/arch.mk
@@ -9,7 +9,11 @@ CFLAGS-$(CONFIG_ARM_32) += -msoft-float
  CFLAGS-$(CONFIG_ARM_32) += -mcpu=cortex-a15
  CFLAGS-$(CONFIG_ARM_32) += -mno-unaligned-access

+ifeq ($(CONFIG_MPU),y)
+CFLAGS-$(CONFIG_ARM_64) += -march=armv8-r
+else
  CFLAGS-$(CONFIG_ARM_64) += -mcpu=generic
+endif
  CFLAGS-$(CONFIG_ARM_64) += -mgeneral-regs-only # No fp registers etc
  $(call cc-option-add,CFLAGS-$(CONFIG_ARM_64),CC,-mno-outline-atomics)


(Sorry for the misformatted snippet in the previous mail)

- Ayan

[PATCH v4 5/6] xen/arm: mpu: Enable MPU

[Ayan Kumar Halder]

After the regions have been created, now we enable the MPU. For this we disable
the background region so that the new memory map created for the regions take
effect. Also, we treat all RW regions as non executable and the data cache is
enabled.

As enable_mpu() is invoked from enable_boot_cpu_mm(), one needs to save and
restore the lr.

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

v2 - 1. Extracted from the previous patch into a new one.

2. Disabled background region.

v3 - 1. Removed dsb before setting SCTLR_EL2. The reason being
From ARM DDI 0487K.a D23-7349:
"Direct writes to these registers (includes SCTLR_EL2) are not allowed to affect
any instructions appearing in program order before the direct write."
So, we don't need a synchronization barrier before writing to SCTLR_EL2.
Further, we do have synchronization barriers after writing the MPU region
registers (which happens before we read SCTLR_EL2). So, SCTLR_EL2 is written
after the MPU registers are synchronized. And, thus adding a 'isb' to flush the
instruction pipeline ensures that the subsequent instructions are fetched after
the MPU has been enabled.

2. Saved and restored lr in enable_boot_cpu_mm().

 xen/arch/arm/arm64/mpu/head.S                | 30 ++++++++++++++++++--
 xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  3 ++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
index 9377ae778c..0edadb009c 100644
--- a/xen/arch/arm/arm64/mpu/head.S
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -68,6 +68,29 @@ FUNC_LOCAL(fail_insufficient_regions)
     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)
+    mrs   x0, SCTLR_EL2
+    bic   x0, x0, #SCTLR_ELx_BR       /* Disable Background region */
+    orr   x0, x0, #SCTLR_Axx_ELx_M    /* Enable MPU */
+    orr   x0, x0, #SCTLR_Axx_ELx_C    /* Enable D-cache */
+    orr   x0, x0, #SCTLR_Axx_ELx_WXN  /* Enable WXN */
+    msr   SCTLR_EL2, x0
+    isb
+
+    ret
+END(enable_mpu)
+
 /*
  * Maps the various sections of Xen (described in xen.lds.S) as different MPU
  * regions.
@@ -75,10 +98,11 @@ END(fail_insufficient_regions)
  * Inputs:
  *   lr : Address to return to.
  *
- * Clobbers x0 - x5
+ * Clobbers x0 - x6
  *
  */
 FUNC(enable_boot_cpu_mm)
+    mov   x6, lr
 
     /* Get the number of regions specified in MPUIR_EL2 */
     mrs   x5, MPUIR_EL2
@@ -110,8 +134,10 @@ FUNC(enable_boot_cpu_mm)
     ldr   x2, =__bss_end
     prepare_xen_region x0, x1, x2, x3, x4, x5
 
-    ret
+    bl    enable_mpu
 
+    mov   lr, x6
+    ret
 END(enable_boot_cpu_mm)
 
 /*
diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
index b0c31a58ec..3769d23c80 100644
--- a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
@@ -15,6 +15,9 @@
 /* MPU Protection Region Selection Register encode */
 #define PRSELR_EL2  S3_4_C6_C2_1
 
+/* Backgroud region enable/disable */
+#define SCTLR_ELx_BR    BIT(17, UL)
+
 #endif /* __ASM_ARM_ARM64_MPU_SYSREGS_H */
 
 /*
-- 
2.25.1

Re: [PATCH v4 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote:
> 
> After the regions have been created, now we enable the MPU. For this we disable
> the background region so that the new memory map created for the regions take
> effect. Also, we treat all RW regions as non executable and the data cache is
> enabled.
> 
> As enable_mpu() is invoked from enable_boot_cpu_mm(), one needs to save and
> restore the lr.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 

It looks good to me:

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

Re: [PATCH v4 5/6] xen/arm: mpu: Enable MPU

[Julien Grall]

Hi Ayan,

On 28/10/2024 12:45, Ayan Kumar Halder wrote:
> After the regions have been created, now we enable the MPU. For this we disable
> the background region so that the new memory map created for the regions take
> effect. Also, we treat all RW regions as non executable and the data cache is
> enabled.
> 
> As enable_mpu() is invoked from enable_boot_cpu_mm(), one needs to save and
> restore the lr.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from :-
> 
> v2 - 1. Extracted from the previous patch into a new one.
> 
> 2. Disabled background region.
> 
> v3 - 1. Removed dsb before setting SCTLR_EL2. The reason being
>  From ARM DDI 0487K.a D23-7349:
> "Direct writes to these registers (includes SCTLR_EL2) are not allowed to affect
> any instructions appearing in program order before the direct write."
> So, we don't need a synchronization barrier before writing to SCTLR_EL2.
> Further, we do have synchronization barriers after writing the MPU region
> registers (which happens before we read SCTLR_EL2). So, SCTLR_EL2 is written
> after the MPU registers are synchronized. And, thus adding a 'isb' to flush the
> instruction pipeline ensures that the subsequent instructions are fetched after
> the MPU has been enabled.
> 
> 2. Saved and restored lr in enable_boot_cpu_mm().
> 
>   xen/arch/arm/arm64/mpu/head.S                | 30 ++++++++++++++++++--
>   xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  3 ++
>   2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> index 9377ae778c..0edadb009c 100644
> --- a/xen/arch/arm/arm64/mpu/head.S
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -68,6 +68,29 @@ FUNC_LOCAL(fail_insufficient_regions)
>       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)
> +    mrs   x0, SCTLR_EL2
> +    bic   x0, x0, #SCTLR_ELx_BR       /* Disable Background region */
> +    orr   x0, x0, #SCTLR_Axx_ELx_M    /* Enable MPU */
> +    orr   x0, x0, #SCTLR_Axx_ELx_C    /* Enable D-cache */
> +    orr   x0, x0, #SCTLR_Axx_ELx_WXN  /* Enable WXN */
> +    msr   SCTLR_EL2, x0
> +    isb
> +
> +    ret
> +END(enable_mpu)
> +
>   /*
>    * Maps the various sections of Xen (described in xen.lds.S) as different MPU
>    * regions.
> @@ -75,10 +98,11 @@ END(fail_insufficient_regions)
>    * Inputs:
>    *   lr : Address to return to.
>    *
> - * Clobbers x0 - x5
> + * Clobbers x0 - x6
>    *
>    */
>   FUNC(enable_boot_cpu_mm)
> +    mov   x6, lr
>   
>       /* Get the number of regions specified in MPUIR_EL2 */
>       mrs   x5, MPUIR_EL2
> @@ -110,8 +134,10 @@ FUNC(enable_boot_cpu_mm)
>       ldr   x2, =__bss_end
>       prepare_xen_region x0, x1, x2, x3, x4, x5
>   
> -    ret
> +    bl    enable_mpu
>   
> +    mov   lr, x6
> +    ret

You should not need to save/restore 'lr'. You could write:

b enable_mpu

So when enable_mpu returns, it will go back to the caller of 
enable_boot_cpu_mm.

With that fixed:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Ayan Kumar Halder]

Secondary cpus initialization is not yet supported. Thus, we print an
appropriate message and put the secondary cpus in WFE state.
And we introduce to BUILD_BUG_ON to prevent users using from building Xen
on multiprocessor based MPU systems.

In Arm, there is no clean way to disable SMP. As of now, we wish to support
MPU on UNP only. So, we have defined the default range of NR_CPUs to be 1 for
MPU.

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

v1 - 1. NR_CPUS is defined as 1 for MPU

2. Added a message in enable_secondary_cpu_mm()

v2 - 1. Added the range

2. Clarified in the commit message why/how we have disabled SMP.

v3 - 1. BUILD_BUG_ON() is moved to smp.c.

 xen/arch/Kconfig              |  2 ++
 xen/arch/arm/arm64/mpu/head.S | 10 ++++++++++
 xen/arch/arm/smp.c            | 11 +++++++++++
 3 files changed, 23 insertions(+)

diff --git a/xen/arch/Kconfig b/xen/arch/Kconfig
index 308ce129a8..aa383577a4 100644
--- a/xen/arch/Kconfig
+++ b/xen/arch/Kconfig
@@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
 
 config NR_CPUS
 	int "Maximum number of CPUs"
+	range 1 1 if ARM && MPU
 	range 1 16383
 	default "256" if X86
 	default "8" if ARM && RCAR3
 	default "4" if ARM && QEMU
 	default "4" if ARM && MPSOC
+	default "1" if ARM && MPU
 	default "128" if ARM
 	help
 	  Controls the build-time size of various arrays and bitmaps
diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
index 0edadb009c..5a6aaf47cd 100644
--- a/xen/arch/arm/arm64/mpu/head.S
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -140,6 +140,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
diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
index c11bba93ad..b372472188 100644
--- a/xen/arch/arm/smp.c
+++ b/xen/arch/arm/smp.c
@@ -1,4 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
+#include <xen/init.h>
 #include <xen/mm.h>
 #include <asm/system.h>
 #include <asm/smp.h>
@@ -6,6 +7,16 @@
 #include <asm/gic.h>
 #include <asm/flushtlb.h>
 
+static void __init __maybe_unused build_assertions(void)
+{
+#ifdef CONFIG_MPU
+    /*
+     * Currently, SMP is not enabled on MPU based systems.
+     */
+    BUILD_BUG_ON(NR_CPUS > 1);
+#endif
+}
+
 void arch_flush_tlb_mask(const cpumask_t *mask)
 {
     /* No need to IPI other processors on ARM, the processor takes care of it. */
-- 
2.25.1

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Jan Beulich]

On 28.10.2024 13:45, Ayan Kumar Halder wrote:
> --- a/xen/arch/Kconfig
> +++ b/xen/arch/Kconfig
> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>  
>  config NR_CPUS
>  	int "Maximum number of CPUs"
> +	range 1 1 if ARM && MPU
>  	range 1 16383
>  	default "256" if X86
>  	default "8" if ARM && RCAR3
>  	default "4" if ARM && QEMU
>  	default "4" if ARM && MPSOC
> +	default "1" if ARM && MPU
>  	default "128" if ARM
>  	help
>  	  Controls the build-time size of various arrays and bitmaps

I'm afraid I can't easily tell whether MPU can be used together with any of
RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
up, as it's the first one that has a match on its condition which is being
used.

Jan

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Ayan Kumar Halder]

Hi Jan,

On 28/10/2024 12:55, Jan Beulich wrote:
> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>> --- a/xen/arch/Kconfig
>> +++ b/xen/arch/Kconfig
>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>   
>>   config NR_CPUS
>>   	int "Maximum number of CPUs"
>> +	range 1 1 if ARM && MPU
>>   	range 1 16383
>>   	default "256" if X86
>>   	default "8" if ARM && RCAR3
>>   	default "4" if ARM && QEMU
>>   	default "4" if ARM && MPSOC
>> +	default "1" if ARM && MPU
>>   	default "128" if ARM
>>   	help
>>   	  Controls the build-time size of various arrays and bitmaps
> I'm afraid I can't easily tell whether MPU can be used together with any of
> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
> up, as it's the first one that has a match on its condition which is being
> used.

MPU cannot be used with any of the existing platforms.

There will be patch in the future series where we will introduce a 
platform specific for MPU. That can be FVP for R82 and R52.

- Ayan

>
> Jan

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Jan Beulich]

On 28.10.2024 15:39, Ayan Kumar Halder wrote:
> On 28/10/2024 12:55, Jan Beulich wrote:
>> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>>> --- a/xen/arch/Kconfig
>>> +++ b/xen/arch/Kconfig
>>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>>   
>>>   config NR_CPUS
>>>   	int "Maximum number of CPUs"
>>> +	range 1 1 if ARM && MPU
>>>   	range 1 16383
>>>   	default "256" if X86
>>>   	default "8" if ARM && RCAR3
>>>   	default "4" if ARM && QEMU
>>>   	default "4" if ARM && MPSOC
>>> +	default "1" if ARM && MPU
>>>   	default "128" if ARM
>>>   	help
>>>   	  Controls the build-time size of various arrays and bitmaps
>> I'm afraid I can't easily tell whether MPU can be used together with any of
>> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
>> up, as it's the first one that has a match on its condition which is being
>> used.
> 
> MPU cannot be used with any of the existing platforms.

That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
don't go together?

Jan

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Ayan Kumar Halder]

Hi Jan,

On 28/10/2024 15:01, Jan Beulich wrote:
> On 28.10.2024 15:39, Ayan Kumar Halder wrote:
>> On 28/10/2024 12:55, Jan Beulich wrote:
>>> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>>>> --- a/xen/arch/Kconfig
>>>> +++ b/xen/arch/Kconfig
>>>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>>>    
>>>>    config NR_CPUS
>>>>    	int "Maximum number of CPUs"
>>>> +	range 1 1 if ARM && MPU
>>>>    	range 1 16383
>>>>    	default "256" if X86
>>>>    	default "8" if ARM && RCAR3
>>>>    	default "4" if ARM && QEMU
>>>>    	default "4" if ARM && MPSOC
>>>> +	default "1" if ARM && MPU
>>>>    	default "128" if ARM
>>>>    	help
>>>>    	  Controls the build-time size of various arrays and bitmaps
>>> I'm afraid I can't easily tell whether MPU can be used together with any of
>>> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
>>> up, as it's the first one that has a match on its condition which is being
>>> used.
>> MPU cannot be used with any of the existing platforms.
> That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
> don't go together?

Qemu has support for Aarch32 MPU at EL2 and EL1 (ie R52). As far as I am 
aware, there is no support for Aarch64 MPU in Qemu (ie R82).

Even for R52, I could not get the upstream Qemu working (emulating some 
Arm reference platform).

I could get the Xilinx fork of Qemu (https://github.com/Xilinx/qemu) 
working which emulates AMD's SoC using R52.

However, this should not impact the current patch. There is no Qemu in 
xen/arch/arm/platforms/*.

- Ayan

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Jan Beulich]

On 28.10.2024 18:38, Ayan Kumar Halder wrote:
> On 28/10/2024 15:01, Jan Beulich wrote:
>> On 28.10.2024 15:39, Ayan Kumar Halder wrote:
>>> On 28/10/2024 12:55, Jan Beulich wrote:
>>>> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>>>>> --- a/xen/arch/Kconfig
>>>>> +++ b/xen/arch/Kconfig
>>>>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>>>>    
>>>>>    config NR_CPUS
>>>>>    	int "Maximum number of CPUs"
>>>>> +	range 1 1 if ARM && MPU
>>>>>    	range 1 16383
>>>>>    	default "256" if X86
>>>>>    	default "8" if ARM && RCAR3
>>>>>    	default "4" if ARM && QEMU
>>>>>    	default "4" if ARM && MPSOC
>>>>> +	default "1" if ARM && MPU
>>>>>    	default "128" if ARM
>>>>>    	help
>>>>>    	  Controls the build-time size of various arrays and bitmaps
>>>> I'm afraid I can't easily tell whether MPU can be used together with any of
>>>> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
>>>> up, as it's the first one that has a match on its condition which is being
>>>> used.
>>> MPU cannot be used with any of the existing platforms.
>> That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
>> don't go together?
> 
> Qemu has support for Aarch32 MPU at EL2 and EL1 (ie R52). As far as I am 
> aware, there is no support for Aarch64 MPU in Qemu (ie R82).
> 
> Even for R52, I could not get the upstream Qemu working (emulating some 
> Arm reference platform).
> 
> I could get the Xilinx fork of Qemu (https://github.com/Xilinx/qemu) 
> working which emulates AMD's SoC using R52.
> 
> However, this should not impact the current patch. There is no Qemu in 
> xen/arch/arm/platforms/*.

Aiui that's not relevant. There is a QEMU item in xen/arch/arm/platforms/Kconfig.
I continue to fail to see why that couldn't be selected together with MPU. Yet if
it can be, you'd end up with a default of 4, not 1, if it actually _is_ selected.
Alternatively QEMU (and maybe also RCAR3 and MPSOC) need to be mutually exclusive
with MPU. Hmm, looks like that's already the case, by patch 2 suppressing the
"Platform Support" prompt. While that looks fragile to me, I'm sorry for the
noise then.

Jan

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Luca Fancellu]

Hi Jan,

> On 29 Oct 2024, at 08:08, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 28.10.2024 18:38, Ayan Kumar Halder wrote:
>> On 28/10/2024 15:01, Jan Beulich wrote:
>>> On 28.10.2024 15:39, Ayan Kumar Halder wrote:
>>>> On 28/10/2024 12:55, Jan Beulich wrote:
>>>>> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>>>>>> --- a/xen/arch/Kconfig
>>>>>> +++ b/xen/arch/Kconfig
>>>>>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>>>>> 
>>>>>>   config NR_CPUS
>>>>>>   	int "Maximum number of CPUs"
>>>>>> +	range 1 1 if ARM && MPU
>>>>>>   	range 1 16383
>>>>>>   	default "256" if X86
>>>>>>   	default "8" if ARM && RCAR3
>>>>>>   	default "4" if ARM && QEMU
>>>>>>   	default "4" if ARM && MPSOC
>>>>>> +	default "1" if ARM && MPU
>>>>>>   	default "128" if ARM
>>>>>>   	help
>>>>>>   	  Controls the build-time size of various arrays and bitmaps
>>>>> I'm afraid I can't easily tell whether MPU can be used together with any of
>>>>> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
>>>>> up, as it's the first one that has a match on its condition which is being
>>>>> used.
>>>> MPU cannot be used with any of the existing platforms.
>>> That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
>>> don't go together?
>> 
>> Qemu has support for Aarch32 MPU at EL2 and EL1 (ie R52). As far as I am 
>> aware, there is no support for Aarch64 MPU in Qemu (ie R82).
>> 
>> Even for R52, I could not get the upstream Qemu working (emulating some 
>> Arm reference platform).
>> 
>> I could get the Xilinx fork of Qemu (https://github.com/Xilinx/qemu) 
>> working which emulates AMD's SoC using R52.
>> 
>> However, this should not impact the current patch. There is no Qemu in 
>> xen/arch/arm/platforms/*.
> 
> Aiui that's not relevant. There is a QEMU item in xen/arch/arm/platforms/Kconfig.
> I continue to fail to see why that couldn't be selected together with MPU. Yet if
> it can be, you'd end up with a default of 4, not 1, if it actually _is_ selected.
> Alternatively QEMU (and maybe also RCAR3 and MPSOC) need to be mutually exclusive
> with MPU. Hmm, looks like that's already the case, by patch 2 suppressing the
> "Platform Support" prompt. While that looks fragile to me, I'm sorry for the
> noise then.

Are you suggesting to move "default "1" if ARM && MPU” right after “default "256" if X86”?

Cheers,
Luca

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Jan Beulich]

On 29.10.2024 10:30, Luca Fancellu wrote:
> Hi Jan,
> 
>> On 29 Oct 2024, at 08:08, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.10.2024 18:38, Ayan Kumar Halder wrote:
>>> On 28/10/2024 15:01, Jan Beulich wrote:
>>>> On 28.10.2024 15:39, Ayan Kumar Halder wrote:
>>>>> On 28/10/2024 12:55, Jan Beulich wrote:
>>>>>> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>>>>>>> --- a/xen/arch/Kconfig
>>>>>>> +++ b/xen/arch/Kconfig
>>>>>>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>>>>>>
>>>>>>>   config NR_CPUS
>>>>>>>   	int "Maximum number of CPUs"
>>>>>>> +	range 1 1 if ARM && MPU
>>>>>>>   	range 1 16383
>>>>>>>   	default "256" if X86
>>>>>>>   	default "8" if ARM && RCAR3
>>>>>>>   	default "4" if ARM && QEMU
>>>>>>>   	default "4" if ARM && MPSOC
>>>>>>> +	default "1" if ARM && MPU
>>>>>>>   	default "128" if ARM
>>>>>>>   	help
>>>>>>>   	  Controls the build-time size of various arrays and bitmaps
>>>>>> I'm afraid I can't easily tell whether MPU can be used together with any of
>>>>>> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
>>>>>> up, as it's the first one that has a match on its condition which is being
>>>>>> used.
>>>>> MPU cannot be used with any of the existing platforms.
>>>> That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
>>>> don't go together?
>>>
>>> Qemu has support for Aarch32 MPU at EL2 and EL1 (ie R52). As far as I am 
>>> aware, there is no support for Aarch64 MPU in Qemu (ie R82).
>>>
>>> Even for R52, I could not get the upstream Qemu working (emulating some 
>>> Arm reference platform).
>>>
>>> I could get the Xilinx fork of Qemu (https://github.com/Xilinx/qemu) 
>>> working which emulates AMD's SoC using R52.
>>>
>>> However, this should not impact the current patch. There is no Qemu in 
>>> xen/arch/arm/platforms/*.
>>
>> Aiui that's not relevant. There is a QEMU item in xen/arch/arm/platforms/Kconfig.
>> I continue to fail to see why that couldn't be selected together with MPU. Yet if
>> it can be, you'd end up with a default of 4, not 1, if it actually _is_ selected.
>> Alternatively QEMU (and maybe also RCAR3 and MPSOC) need to be mutually exclusive
>> with MPU. Hmm, looks like that's already the case, by patch 2 suppressing the
>> "Platform Support" prompt. While that looks fragile to me, I'm sorry for the
>> noise then.
> 
> Are you suggesting to move "default "1" if ARM && MPU” right after “default "256" if X86”?

Yes.

Jan

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Luca Fancellu]

> On 29 Oct 2024, at 09:41, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 29.10.2024 10:30, Luca Fancellu wrote:
>> Hi Jan,
>> 
>>> On 29 Oct 2024, at 08:08, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 28.10.2024 18:38, Ayan Kumar Halder wrote:
>>>> On 28/10/2024 15:01, Jan Beulich wrote:
>>>>> On 28.10.2024 15:39, Ayan Kumar Halder wrote:
>>>>>> On 28/10/2024 12:55, Jan Beulich wrote:
>>>>>>> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>>>>>>>> --- a/xen/arch/Kconfig
>>>>>>>> +++ b/xen/arch/Kconfig
>>>>>>>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>>>>>>> 
>>>>>>>>  config NR_CPUS
>>>>>>>>  	int "Maximum number of CPUs"
>>>>>>>> +	range 1 1 if ARM && MPU
>>>>>>>>  	range 1 16383
>>>>>>>>  	default "256" if X86
>>>>>>>>  	default "8" if ARM && RCAR3
>>>>>>>>  	default "4" if ARM && QEMU
>>>>>>>>  	default "4" if ARM && MPSOC
>>>>>>>> +	default "1" if ARM && MPU
>>>>>>>>  	default "128" if ARM
>>>>>>>>  	help
>>>>>>>>  	  Controls the build-time size of various arrays and bitmaps
>>>>>>> I'm afraid I can't easily tell whether MPU can be used together with any of
>>>>>>> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
>>>>>>> up, as it's the first one that has a match on its condition which is being
>>>>>>> used.
>>>>>> MPU cannot be used with any of the existing platforms.
>>>>> That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
>>>>> don't go together?
>>>> 
>>>> Qemu has support for Aarch32 MPU at EL2 and EL1 (ie R52). As far as I am 
>>>> aware, there is no support for Aarch64 MPU in Qemu (ie R82).
>>>> 
>>>> Even for R52, I could not get the upstream Qemu working (emulating some 
>>>> Arm reference platform).
>>>> 
>>>> I could get the Xilinx fork of Qemu (https://github.com/Xilinx/qemu) 
>>>> working which emulates AMD's SoC using R52.
>>>> 
>>>> However, this should not impact the current patch. There is no Qemu in 
>>>> xen/arch/arm/platforms/*.
>>> 
>>> Aiui that's not relevant. There is a QEMU item in xen/arch/arm/platforms/Kconfig.
>>> I continue to fail to see why that couldn't be selected together with MPU. Yet if
>>> it can be, you'd end up with a default of 4, not 1, if it actually _is_ selected.
>>> Alternatively QEMU (and maybe also RCAR3 and MPSOC) need to be mutually exclusive
>>> with MPU. Hmm, looks like that's already the case, by patch 2 suppressing the
>>> "Platform Support" prompt. While that looks fragile to me, I'm sorry for the
>>> noise then.
>> 
>> Are you suggesting to move "default "1" if ARM && MPU” right after “default "256" if X86”?
> 
> Yes.

Makes sense! 

With that:

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

> 
> Jan

Re: [PATCH v4 0/6] Enable early bootup of AArch64 MPU systems

[Julien Grall]

Hi Ayan,

On 28/10/2024 12:45, Ayan Kumar Halder wrote:
> We have enabled early booting of R82.
> 
> Changes from v2 :-
> 1. Added a new patch "xen/arm: Skip initializing the BSS section when it is empty".
> 2. Split "xen/arm: mpu: Create boot-time MPU protection regions" into 2 patches.
> 
> Changes from v3 :-
> 1. Removed some of the R-b as the patches have been modified.
> 
> Ayan Kumar Halder (6):
>    xen/arm: Skip initializing the BSS section when it is empty
>    xen/arm: mpu: Introduce choice between MMU and MPU
>    xen/arm: mpu: Define Xen start address for MPU systems

I have committed the first 3 patches. I think the others need a respin.

Cheers,

-- 
Julien Grall