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.1Hi 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
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 |
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
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
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,
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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.