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.
START_ADDRESS + 2MB memory is mapped to contain the text and data
required for early boot of Xen.
One can refer to ARM DDI 0600B.a ID062922 G1.3 "General System Control
Registers", to get the definitions of PRBAR_EL2, PRLAR_EL2 and
PRSELR_EL2 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 configure the MPU memory regions.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
xen/arch/arm/Makefile | 1 +
xen/arch/arm/arm64/mpu/Makefile | 1 +
xen/arch/arm/arm64/mpu/head.S | 70 ++++++++++++++++++++++++
xen/arch/arm/include/asm/arm64/sysregs.h | 50 +++++++++++++++++
xen/arch/arm/include/asm/mpu/arm64/mm.h | 13 +++++
xen/arch/arm/include/asm/mpu/mm.h | 18 ++++++
6 files changed, 153 insertions(+)
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/include/asm/mpu/arm64/mm.h
create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7792bff597..aebccec63a 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -1,6 +1,7 @@
obj-$(CONFIG_ARM_32) += arm32/
obj-$(CONFIG_ARM_64) += arm64/
obj-$(CONFIG_MMU) += mmu/
+obj-$(CONFIG_MPU) += mpu/
obj-$(CONFIG_ACPI) += acpi/
obj-$(CONFIG_HAS_PCI) += pci/
ifneq ($(CONFIG_NO_PLAT),y)
diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
new file mode 100644
index 0000000000..3340058c08
--- /dev/null
+++ b/xen/arch/arm/arm64/mpu/Makefile
@@ -0,0 +1 @@
+obj-y += head.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..2b023c346a
--- /dev/null
+++ b/xen/arch/arm/arm64/mpu/head.S
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Start-of-day code for an Armv8-R MPU system.
+ */
+
+#include <asm/mm.h>
+#include <asm/page.h>
+#include <asm/early_printk.h>
+
+/*
+ * From the requirements of head.S we know that Xen image should
+ * be linked at XEN_START_ADDRESS, and all of text + data + bss
+ * must fit in 2MB. On MPU systems, XEN_START_ADDRESS is also the
+ * address that Xen image should be loaded at. So for initial MPU
+ * regions setup, we use 2MB for Xen data memory to setup boot
+ * region, or the create boot regions code below will need adjustment.
+ */
+#define XEN_START_MEM_SIZE 0x200000
+
+/*
+ * In boot stage, we will use 1 MPU region:
+ * Region#0: Normal memory for Xen text + data + bss (2MB)
+ */
+#define BOOT_NORMAL_REGION_IDX 0x0
+
+/* MPU normal memory attributes. */
+#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */
+#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */
+
+.macro write_pr, sel, prbar, prlar
+ msr PRSELR_EL2, \sel
+ dsb sy
+ msr PRBAR_EL2, \prbar
+ msr PRLAR_EL2, \prlar
+ dsb sy
+ isb
+.endm
+
+.section .text.header, "ax", %progbits
+
+/*
+ * Static start-of-day EL2 MPU memory layout.
+ *
+ * It has a very simple structure, including:
+ * - 2MB normal memory mappings of xen at XEN_START_ADDRESS, which
+ * is the address where Xen was loaded by the bootloader.
+ */
+ENTRY(enable_boot_cpu_mm)
+ /* Map Xen start memory to a normal memory region. */
+ mov x0, #BOOT_NORMAL_REGION_IDX
+ ldr x1, =XEN_START_ADDRESS
+ and x1, x1, #MPU_REGION_MASK
+ mov x3, #PRBAR_NORMAL_MEM
+ orr x1, x1, x3
+
+ ldr x2, =XEN_START_ADDRESS
+ mov x3, #(XEN_START_MEM_SIZE - 1)
+ add x2, x2, x3
+ and x2, x2, #MPU_REGION_MASK
+ mov x3, #PRLAR_NORMAL_MEM
+ orr x2, x2, x3
+
+ /*
+ * Write to MPU protection region:
+ * x0 for pr_sel, x1 for prbar x2 for prlar
+ */
+ write_pr x0, x1, x2
+
+ ret
+ENDPROC(enable_boot_cpu_mm)
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index b593e4028b..0d122e1fa6 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -462,6 +462,56 @@
#define ZCR_ELx_LEN_SIZE 9
#define ZCR_ELx_LEN_MASK 0x1ff
+/* System registers for AArch64 with PMSA */
+#ifdef CONFIG_MPU
+
+/* EL2 MPU Protection Region Base Address Register encode */
+#define PRBAR_EL2 S3_4_C6_C8_0
+#define PRBAR1_EL2 S3_4_C6_C8_4
+#define PRBAR2_EL2 S3_4_C6_C9_0
+#define PRBAR3_EL2 S3_4_C6_C9_4
+#define PRBAR4_EL2 S3_4_C6_C10_0
+#define PRBAR5_EL2 S3_4_C6_C10_4
+#define PRBAR6_EL2 S3_4_C6_C11_0
+#define PRBAR7_EL2 S3_4_C6_C11_4
+#define PRBAR8_EL2 S3_4_C6_C12_0
+#define PRBAR9_EL2 S3_4_C6_C12_4
+#define PRBAR10_EL2 S3_4_C6_C13_0
+#define PRBAR11_EL2 S3_4_C6_C13_4
+#define PRBAR12_EL2 S3_4_C6_C14_0
+#define PRBAR13_EL2 S3_4_C6_C14_4
+#define PRBAR14_EL2 S3_4_C6_C15_0
+#define PRBAR15_EL2 S3_4_C6_C15_4
+
+/* EL2 MPU Protection Region Limit Address Register encode */
+#define PRLAR_EL2 S3_4_C6_C8_1
+#define PRLAR1_EL2 S3_4_C6_C8_5
+#define PRLAR2_EL2 S3_4_C6_C9_1
+#define PRLAR3_EL2 S3_4_C6_C9_5
+#define PRLAR4_EL2 S3_4_C6_C10_1
+#define PRLAR5_EL2 S3_4_C6_C10_5
+#define PRLAR6_EL2 S3_4_C6_C11_1
+#define PRLAR7_EL2 S3_4_C6_C11_5
+#define PRLAR8_EL2 S3_4_C6_C12_1
+#define PRLAR9_EL2 S3_4_C6_C12_5
+#define PRLAR10_EL2 S3_4_C6_C13_1
+#define PRLAR11_EL2 S3_4_C6_C13_5
+#define PRLAR12_EL2 S3_4_C6_C14_1
+#define PRLAR13_EL2 S3_4_C6_C14_5
+#define PRLAR14_EL2 S3_4_C6_C15_1
+#define PRLAR15_EL2 S3_4_C6_C15_5
+
+/* MPU Protection Region Enable Register encode */
+#define PRENR_EL2 S3_4_C6_C1_1
+
+/* MPU Protection Region Selection Register encode */
+#define PRSELR_EL2 S3_4_C6_C2_1
+
+/* MPU Type registers encode */
+#define MPUIR_EL2 S3_4_C0_C0_4
+
+#endif
+
/* Access to system registers */
#define WRITE_SYSREG64(v, name) do { \
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..d209eef6db
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/arm64/mm.h
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * mpu.h: Arm Memory Protection Unit definitions.
+ */
+
+#ifndef __ARM64_MPU_H__
+#define __ARM64_MPU_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_H__ */
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..f5ebca8261
--- /dev/null
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -0,0 +1,18 @@
+#ifndef __ARCH_ARM_MPU__
+#define __ARCH_ARM_MPU__
+
+#if defined(CONFIG_ARM_64)
+# include <asm/mpu/arm64/mm.h>
+#else
+# error "unknown ARM variant"
+#endif
+
+#endif /* __ARCH_ARM_MPU__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.25.1Hi,
On 23/08/2024 17:31, 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.
>
> START_ADDRESS + 2MB memory is mapped to contain the text and data
> required for early boot of Xen.
>
> One can refer to ARM DDI 0600B.a ID062922 G1.3 "General System Control
> Registers", to get the definitions of PRBAR_EL2, PRLAR_EL2 and
> PRSELR_EL2 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 configure the MPU memory regions.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/arm64/mpu/Makefile | 1 +
> xen/arch/arm/arm64/mpu/head.S | 70 ++++++++++++++++++++++++
> xen/arch/arm/include/asm/arm64/sysregs.h | 50 +++++++++++++++++
> xen/arch/arm/include/asm/mpu/arm64/mm.h | 13 +++++
> xen/arch/arm/include/asm/mpu/mm.h | 18 ++++++
> 6 files changed, 153 insertions(+)
> 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/include/asm/mpu/arm64/mm.h
> create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7792bff597..aebccec63a 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -1,6 +1,7 @@
> obj-$(CONFIG_ARM_32) += arm32/
> obj-$(CONFIG_ARM_64) += arm64/
> obj-$(CONFIG_MMU) += mmu/
> +obj-$(CONFIG_MPU) += mpu/
> obj-$(CONFIG_ACPI) += acpi/
> obj-$(CONFIG_HAS_PCI) += pci/
> ifneq ($(CONFIG_NO_PLAT),y)
> diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
> new file mode 100644
> index 0000000000..3340058c08
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/Makefile
> @@ -0,0 +1 @@
> +obj-y += head.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..2b023c346a
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Coding style: /* ... */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include <asm/mm.h>
> +#include <asm/page.h>
I can't see any use of the definition of page.h. Is it necessary?
> +#include <asm/early_printk.h>
You include early_printk.h but don't use it.
> +
> +/*
> + * From the requirements of head.S we know that Xen image should
> + * be linked at XEN_START_ADDRESS, and all of text + data + bss
> + * must fit in 2MB.
Xen can be bigger than 2MB. But rather than hardcoding the limit, you
want to use _end.
> On MPU systems, XEN_START_ADDRESS is also the
> + * address that Xen image should be loaded at. So for initial MPU
> + * regions setup, we use 2MB for Xen data memory to setup boot
> + * region, or the create boot regions code below will need adjustment.
> + */
> +#define XEN_START_MEM_SIZE 0x200000
See above.
> +
> +/*
> + * In boot stage, we will use 1 MPU region:
> + * Region#0: Normal memory for Xen text + data + bss (2MB)
> + */
> +#define BOOT_NORMAL_REGION_IDX 0x0
> +
> +/* MPU normal memory attributes. */
> +#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */
> +#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */
> +
> +.macro write_pr, sel, prbar, prlar
> + msr PRSELR_EL2, \sel
> + dsb sy
I am not sure I understand why this is a dsb rather than isb. Can you
clarify?
If a "dsb" is necessary, "sy" seems to be quite strict.
> + msr PRBAR_EL2, \prbar
> + msr PRLAR_EL2, \prlar
> + dsb sy
> + isb
> +.endm
> +
> +.section .text.header, "ax", %progbits
> +
> +/*
> + * Static start-of-day EL2 MPU memory layout.
> + *
> + * It has a very simple structure, including:
> + * - 2MB normal memory mappings of xen at XEN_START_ADDRESS, which
> + * is the address where Xen was loaded by the bootloader.
> + */
Please document a bit more the code and how the registers are used. For
an example see the mmu/head.S code.
> +ENTRY(enable_boot_cpu_mm)
> + /* Map Xen start memory to a normal memory region. */
> + mov x0, #BOOT_NORMAL_REGION_IDX
> + ldr x1, =XEN_START_ADDRESS
> + and x1, x1, #MPU_REGION_MASK
> + mov x3, #PRBAR_NORMAL_MEM
> + orr x1, x1, x3
> +
> + ldr x2, =XEN_START_ADDRESS
> + mov x3, #(XEN_START_MEM_SIZE - 1)
> + add x2, x2, x3
> + and x2, x2, #MPU_REGION_MASK
> + mov x3, #PRLAR_NORMAL_MEM
> + orr x2, x2, x3
> +
> + /*
> + * Write to MPU protection region:
> + * x0 for pr_sel, x1 for prbar x2 for prlar
> + */
> + write_pr x0, x1, x2
> +
> + ret
> +ENDPROC(enable_boot_cpu_mm)
Missing emacs magic.
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index b593e4028b..0d122e1fa6 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -462,6 +462,56 @@
> #define ZCR_ELx_LEN_SIZE 9
> #define ZCR_ELx_LEN_MASK 0x1ff
>
> +/* System registers for AArch64 with PMSA */
> +#ifdef CONFIG_MPU
Can we define the registers in mpu/sysregs.h? This can then be included
only where it is needed.
> +
> +/* EL2 MPU Protection Region Base Address Register encode */
> +#define PRBAR_EL2 S3_4_C6_C8_0
> +#define PRBAR1_EL2 S3_4_C6_C8_4
> +#define PRBAR2_EL2 S3_4_C6_C9_0
> +#define PRBAR3_EL2 S3_4_C6_C9_4
> +#define PRBAR4_EL2 S3_4_C6_C10_0
> +#define PRBAR5_EL2 S3_4_C6_C10_4
> +#define PRBAR6_EL2 S3_4_C6_C11_0
> +#define PRBAR7_EL2 S3_4_C6_C11_4
> +#define PRBAR8_EL2 S3_4_C6_C12_0
> +#define PRBAR9_EL2 S3_4_C6_C12_4
> +#define PRBAR10_EL2 S3_4_C6_C13_0
> +#define PRBAR11_EL2 S3_4_C6_C13_4
> +#define PRBAR12_EL2 S3_4_C6_C14_0
> +#define PRBAR13_EL2 S3_4_C6_C14_4
> +#define PRBAR14_EL2 S3_4_C6_C15_0
> +#define PRBAR15_EL2 S3_4_C6_C15_4
> +
> +/* EL2 MPU Protection Region Limit Address Register encode */
> +#define PRLAR_EL2 S3_4_C6_C8_1
> +#define PRLAR1_EL2 S3_4_C6_C8_5
> +#define PRLAR2_EL2 S3_4_C6_C9_1
> +#define PRLAR3_EL2 S3_4_C6_C9_5
> +#define PRLAR4_EL2 S3_4_C6_C10_1
> +#define PRLAR5_EL2 S3_4_C6_C10_5
> +#define PRLAR6_EL2 S3_4_C6_C11_1
> +#define PRLAR7_EL2 S3_4_C6_C11_5
> +#define PRLAR8_EL2 S3_4_C6_C12_1
> +#define PRLAR9_EL2 S3_4_C6_C12_5
> +#define PRLAR10_EL2 S3_4_C6_C13_1
> +#define PRLAR11_EL2 S3_4_C6_C13_5
> +#define PRLAR12_EL2 S3_4_C6_C14_1
> +#define PRLAR13_EL2 S3_4_C6_C14_5
> +#define PRLAR14_EL2 S3_4_C6_C15_1
> +#define PRLAR15_EL2 S3_4_C6_C15_5
> +
> +/* MPU Protection Region Enable Register encode */
> +#define PRENR_EL2 S3_4_C6_C1_1
> +
> +/* MPU Protection Region Selection Register encode */
> +#define PRSELR_EL2 S3_4_C6_C2_1
> +
> +/* MPU Type registers encode */
> +#define MPUIR_EL2 S3_4_C0_C0_4
> +
> +#endif
> +
> /* Access to system registers */
>
> #define WRITE_SYSREG64(v, name) do { \
> 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..d209eef6db
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/arm64/mm.h
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Coding style: /* ... */
> +/*
> + * mpu.h: Arm Memory Protection Unit definitions.
The file is name mm.h.
> + */
> +
> +#ifndef __ARM64_MPU_H__
> +#define __ARM64_MPU_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_H__ */
Missing emacs magic.
> 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..f5ebca8261
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
Do we need this file?
> @@ -0,0 +1,18 @@
> +#ifndef __ARCH_ARM_MPU__
> +#define __ARCH_ARM_MPU__
> +
> +#if defined(CONFIG_ARM_64)
> +# include <asm/mpu/arm64/mm.h>
> +#else
> +# error "unknown ARM variant"
> +#endif
> +
> +#endif /* __ARCH_ARM_MPU__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
Cheers,
--
Julien Grall
On 28/08/2024 16:01, Julien Grall wrote:
> Hi,
Hi Julien,
I need some clarifications.
>
> On 23/08/2024 17:31, 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.
>>
>> START_ADDRESS + 2MB memory is mapped to contain the text and data
>> required for early boot of Xen.
>>
>> One can refer to ARM DDI 0600B.a ID062922 G1.3 "General System Control
>> Registers", to get the definitions of PRBAR_EL2, PRLAR_EL2 and
>> PRSELR_EL2 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 configure the MPU memory regions.
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
>> ---
>> xen/arch/arm/Makefile | 1 +
>> xen/arch/arm/arm64/mpu/Makefile | 1 +
>> xen/arch/arm/arm64/mpu/head.S | 70 ++++++++++++++++++++++++
>> xen/arch/arm/include/asm/arm64/sysregs.h | 50 +++++++++++++++++
>> xen/arch/arm/include/asm/mpu/arm64/mm.h | 13 +++++
>> xen/arch/arm/include/asm/mpu/mm.h | 18 ++++++
>> 6 files changed, 153 insertions(+)
>> 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/include/asm/mpu/arm64/mm.h
>> create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7792bff597..aebccec63a 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -1,6 +1,7 @@
>> obj-$(CONFIG_ARM_32) += arm32/
>> obj-$(CONFIG_ARM_64) += arm64/
>> obj-$(CONFIG_MMU) += mmu/
>> +obj-$(CONFIG_MPU) += mpu/
>> obj-$(CONFIG_ACPI) += acpi/
>> obj-$(CONFIG_HAS_PCI) += pci/
>> ifneq ($(CONFIG_NO_PLAT),y)
>> diff --git a/xen/arch/arm/arm64/mpu/Makefile
>> b/xen/arch/arm/arm64/mpu/Makefile
>> new file mode 100644
>> index 0000000000..3340058c08
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/mpu/Makefile
>> @@ -0,0 +1 @@
>> +obj-y += head.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..2b023c346a
>> --- /dev/null
>> +++ b/xen/arch/arm/arm64/mpu/head.S
>> @@ -0,0 +1,70 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>
> Coding style: /* ... */
Ack
>
>> +/*
>> + * Start-of-day code for an Armv8-R MPU system.
>> + */
>> +
>> +#include <asm/mm.h>
>> +#include <asm/page.h>
>
> I can't see any use of the definition of page.h. Is it necessary?
I will remove this and ..
>
>> +#include <asm/early_printk.h>
>
> You include early_printk.h but don't use it.
this as well.
>
>> +
>> +/*
>> + * From the requirements of head.S we know that Xen image should
>> + * be linked at XEN_START_ADDRESS, and all of text + data + bss
>> + * must fit in 2MB.
>
> Xen can be bigger than 2MB. But rather than hardcoding the limit, you
> want to use _end.
I was wondering if we can XEN_VIRT_SIZE here , but that would lead to
mapping of a large memory region.
So I guess _end should be used here.
>
>> On MPU systems, XEN_START_ADDRESS is also the
>> + * address that Xen image should be loaded at. So for initial MPU
>> + * regions setup, we use 2MB for Xen data memory to setup boot
>> + * region, or the create boot regions code below will need adjustment.
>> + */
>> +#define XEN_START_MEM_SIZE 0x200000
>
> See above.
Ack.
>
>> +
>> +/*
>> + * In boot stage, we will use 1 MPU region:
>> + * Region#0: Normal memory for Xen text + data + bss (2MB)
>> + */
>> +#define BOOT_NORMAL_REGION_IDX 0x0
>> +
>> +/* MPU normal memory attributes. */
>> +#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */
>> +#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */
>> +
>> +.macro write_pr, sel, prbar, prlar
>> + msr PRSELR_EL2, \sel
>> + dsb sy
>
> I am not sure I understand why this is a dsb rather than isb. Can you
> clarify?
ISB is not needed here as the memory protection hasn't been activated
yet. The above instruction just selects the memory region and the below
two instructions sets the base address and limit for that memory region.
After the three instructions, we need an ISB so that the memory
protection takes into affect for further instruction fetches.
However, a DSB is needed here as the below two instructions depend on
this. So, we definitely want this instruction to complete.
Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1
"Protection region attributes"
0.
```Writes to MPU registers are only guaranteed to be visible
following a Context synchronization event and DSB operation.```
Thus, I infer that DSB is necessary here.
>
> If a "dsb" is necessary, "sy" seems to be quite strict.
I can use "st" here as I am only interested in stores (ie MSR) to complete.
Now the whether we want to restrict it to inner shareable/outer
shareable/POU/full system is a difficult decision to make.
May be here we can use ishst (stores to complete to inner shareable
region). However ....
>
>> + msr PRBAR_EL2, \prbar
>> + msr PRLAR_EL2, \prlar
>> + dsb sy
This should be visible to outer shareable domain atleast. The reason
being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to
outer shareable.
Thus, the writes to these registers should be visible to outer shareable
domain as well.
>> + isb
>> +.endm
>> +
>> +.section .text.header, "ax", %progbits
>> +
>> +/*
>> + * Static start-of-day EL2 MPU memory layout.
>> + *
>> + * It has a very simple structure, including:
>> + * - 2MB normal memory mappings of xen at XEN_START_ADDRESS, which
>> + * is the address where Xen was loaded by the bootloader.
>> + */
>
> Please document a bit more the code and how the registers are used.
> For an example see the mmu/head.S code.
Ack
>
>> +ENTRY(enable_boot_cpu_mm)
>> + /* Map Xen start memory to a normal memory region. */
>> + mov x0, #BOOT_NORMAL_REGION_IDX
>> + ldr x1, =XEN_START_ADDRESS
>> + and x1, x1, #MPU_REGION_MASK
>> + mov x3, #PRBAR_NORMAL_MEM
>> + orr x1, x1, x3
>> +
>> + ldr x2, =XEN_START_ADDRESS
>> + mov x3, #(XEN_START_MEM_SIZE - 1)
>> + add x2, x2, x3
>> + and x2, x2, #MPU_REGION_MASK
>> + mov x3, #PRLAR_NORMAL_MEM
>> + orr x2, x2, x3
>> +
>> + /*
>> + * Write to MPU protection region:
>> + * x0 for pr_sel, x1 for prbar x2 for prlar
>> + */
>> + write_pr x0, x1, x2
>> +
>> + ret
>> +ENDPROC(enable_boot_cpu_mm)
>
> Missing emacs magic.
You mean it should be
END(enable_boot_cpu_mm) .
/*
* Local variables:
* mode: ASM
* indent-tabs-mode: nil
* End:
*/
>
>> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
>> b/xen/arch/arm/include/asm/arm64/sysregs.h
>> index b593e4028b..0d122e1fa6 100644
>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>> @@ -462,6 +462,56 @@
>> #define ZCR_ELx_LEN_SIZE 9
>> #define ZCR_ELx_LEN_MASK 0x1ff
>> +/* System registers for AArch64 with PMSA */
>> +#ifdef CONFIG_MPU
>
> Can we define the registers in mpu/sysregs.h? This can then be
> included only where it is needed.
Ack
>
>> +
>> +/* EL2 MPU Protection Region Base Address Register encode */
>> +#define PRBAR_EL2 S3_4_C6_C8_0
>> +#define PRBAR1_EL2 S3_4_C6_C8_4
>> +#define PRBAR2_EL2 S3_4_C6_C9_0
>> +#define PRBAR3_EL2 S3_4_C6_C9_4
>> +#define PRBAR4_EL2 S3_4_C6_C10_0
>> +#define PRBAR5_EL2 S3_4_C6_C10_4
>> +#define PRBAR6_EL2 S3_4_C6_C11_0
>> +#define PRBAR7_EL2 S3_4_C6_C11_4
>> +#define PRBAR8_EL2 S3_4_C6_C12_0
>> +#define PRBAR9_EL2 S3_4_C6_C12_4
>> +#define PRBAR10_EL2 S3_4_C6_C13_0
>> +#define PRBAR11_EL2 S3_4_C6_C13_4
>> +#define PRBAR12_EL2 S3_4_C6_C14_0
>> +#define PRBAR13_EL2 S3_4_C6_C14_4
>> +#define PRBAR14_EL2 S3_4_C6_C15_0
>> +#define PRBAR15_EL2 S3_4_C6_C15_4
>> +
>> +/* EL2 MPU Protection Region Limit Address Register encode */
>> +#define PRLAR_EL2 S3_4_C6_C8_1
>> +#define PRLAR1_EL2 S3_4_C6_C8_5
>> +#define PRLAR2_EL2 S3_4_C6_C9_1
>> +#define PRLAR3_EL2 S3_4_C6_C9_5
>> +#define PRLAR4_EL2 S3_4_C6_C10_1
>> +#define PRLAR5_EL2 S3_4_C6_C10_5
>> +#define PRLAR6_EL2 S3_4_C6_C11_1
>> +#define PRLAR7_EL2 S3_4_C6_C11_5
>> +#define PRLAR8_EL2 S3_4_C6_C12_1
>> +#define PRLAR9_EL2 S3_4_C6_C12_5
>> +#define PRLAR10_EL2 S3_4_C6_C13_1
>> +#define PRLAR11_EL2 S3_4_C6_C13_5
>> +#define PRLAR12_EL2 S3_4_C6_C14_1
>> +#define PRLAR13_EL2 S3_4_C6_C14_5
>> +#define PRLAR14_EL2 S3_4_C6_C15_1
>> +#define PRLAR15_EL2 S3_4_C6_C15_5
>> +
>> +/* MPU Protection Region Enable Register encode */
>> +#define PRENR_EL2 S3_4_C6_C1_1
>> +
>> +/* MPU Protection Region Selection Register encode */
>> +#define PRSELR_EL2 S3_4_C6_C2_1
>> +
>> +/* MPU Type registers encode */
>> +#define MPUIR_EL2 S3_4_C0_C0_4
>> +
>> +#endif
>> +
>> /* Access to system registers */
>> #define WRITE_SYSREG64(v, name) do { \
>> 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..d209eef6db
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/mpu/arm64/mm.h
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>
> Coding style: /* ... */
Ack
>
>> +/*
>> + * mpu.h: Arm Memory Protection Unit definitions.
>
> The file is name mm.h.
Ack
>
>> + */
>> +
>> +#ifndef __ARM64_MPU_H__
>> +#define __ARM64_MPU_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_H__ */
>
> Missing emacs magic.
/*
* Local variables:
* mode: C
* c-file-style: "BSD"
* c-basic-offset: 4
* tab-width: 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..f5ebca8261
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/mpu/mm.h
>
> Do we need this file?
In xen/arch/arm/arm64/mpu/head.S, we have
#include <asm/mm.h>
So, it should pick up this file.
- Ayan
>
>> @@ -0,0 +1,18 @@
>> +#ifndef __ARCH_ARM_MPU__
>> +#define __ARCH_ARM_MPU__
>> +
>> +#if defined(CONFIG_ARM_64)
>> +# include <asm/mpu/arm64/mm.h>
>> +#else
>> +# error "unknown ARM variant"
>> +#endif
>> +
>> +#endif /* __ARCH_ARM_MPU__ */
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>
> Cheers,
>
Hi, On 04/09/2024 13:21, Ayan Kumar Halder wrote: > > On 28/08/2024 16:01, Julien Grall wrote: >>> On MPU systems, XEN_START_ADDRESS is also the >>> + * address that Xen image should be loaded at. So for initial MPU >>> + * regions setup, we use 2MB for Xen data memory to setup boot >>> + * region, or the create boot regions code below will need adjustment. >>> + */ >>> +#define XEN_START_MEM_SIZE 0x200000 >> >> See above. > Ack. >> >>> + >>> +/* >>> + * In boot stage, we will use 1 MPU region: >>> + * Region#0: Normal memory for Xen text + data + bss (2MB) >>> + */ >>> +#define BOOT_NORMAL_REGION_IDX 0x0 >>> + >>> +/* MPU normal memory attributes. */ >>> +#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */ >>> +#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */ >>> + >>> +.macro write_pr, sel, prbar, prlar >>> + msr PRSELR_EL2, \sel >>> + dsb sy >> >> I am not sure I understand why this is a dsb rather than isb. Can you >> clarify? > > ISB is not needed here as the memory protection hasn't been activated > yet. The above instruction just selects the memory region and the below > two instructions sets the base address and limit for that memory region. > After the three instructions, we need an ISB so that the memory > protection takes into affect for further instruction fetches. We discussed this on Matrix, I have also had a read of the Arm Arm again (D19.1.2 General behavior of accesses to the AArch64 System registers in ARM DDI 0487J.a). Writing down mainly for keeping track of the decision. In general you need an a context synchronization event (e.g. isb) before the other system registers can start to rely on the change. There are some exceptions though, but AFAICT PRSELR_EL2 doesn't have any. IIUC, the behavior of PRBAR_EL2 and PRLAR_EL2 will change depending on PRSELR_EL2.REGION. Therefore, we need to make sure the new value of PRSELR_EL2 is seen before we write to the two other registers. > > However, a DSB is needed here as the below two instructions depend on > this. So, we definitely want this instruction to complete. > > Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1 > "Protection region attributes" > > 0. > > ```Writes to MPU registers are only guaranteed to be visible > following a Context synchronization event and DSB operation.``` > > Thus, I infer that DSB is necessary here I read it differently. To me it was more the "dsb" is necessary before the MPU can start using the new region values. But here an 'isb' should be fine. That said, from my experience, certain section of the Arm Arm can be interpreted differently whic means that someone could implement your way. This is also not a hot path, so best to use the most restrictive approach. Therefore I am ok with adding a 'dsb'. > >> >> If a "dsb" is necessary, "sy" seems to be quite strict. > > I can use "st" here as I am only interested in stores (ie MSR) to complete. > > Now the whether we want to restrict it to inner shareable/outer > shareable/POU/full system is a difficult decision to make. > > May be here we can use ishst (stores to complete to inner shareable > region). However .... > >> >>> + msr PRBAR_EL2, \prbar >>> + msr PRLAR_EL2, \prlar >>> + dsb sy > > This should be visible to outer shareable domain atleast. The reason > being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to > outer shareable. > > Thus, the writes to these registers should be visible to outer shareable > domain as well. I am a bit confused. SH[1:0] is about how the region will be accessed not up to where should registers are visible. I was expecting that the MPU registers only need to be visible to the MPU itself. For instance, when using the MMU, the translation unit is in the non-shareable domain. So a 'nsh' is sufficient regardless of the shareability of the page/block. This is explicitely written in the Arm Arm (see D5-4929 in ARM DDI 0487H.a) but I can't find a similar section for the MPU yet. Although, I would be a bit surprised if the MPU is not in the non-shareable domain... Maybe this could be clarified with Arm? Anyway, for now, I am open to use 'dsb sy' with a TODO to revisit it. > >>> + isb Re-quoting the spec from you previous answer: ``` Writes to MPU registers are only guaranteed to be visible following a Context synchronization event and DSB operation. ``` So this suggests that it should be first an 'isb' and then a 'dsb'. Any reason you wrote it the other way around? >>> +.endm >>> + >>> +.section .text.header, "ax", %progbits >>> + >>> +/* >>> + * Static start-of-day EL2 MPU memory layout. >>> + * >>> + * It has a very simple structure, including: >>> + * - 2MB normal memory mappings of xen at XEN_START_ADDRESS, which >>> + * is the address where Xen was loaded by the bootloader. >>> + */ >> >> Please document a bit more the code and how the registers are used. >> For an example see the mmu/head.S code. > Ack >> >>> +ENTRY(enable_boot_cpu_mm) >>> + /* Map Xen start memory to a normal memory region. */ >>> + mov x0, #BOOT_NORMAL_REGION_IDX >>> + ldr x1, =XEN_START_ADDRESS >>> + and x1, x1, #MPU_REGION_MASK >>> + mov x3, #PRBAR_NORMAL_MEM >>> + orr x1, x1, x3 >>> + >>> + ldr x2, =XEN_START_ADDRESS >>> + mov x3, #(XEN_START_MEM_SIZE - 1) >>> + add x2, x2, x3 >>> + and x2, x2, #MPU_REGION_MASK >>> + mov x3, #PRLAR_NORMAL_MEM >>> + orr x2, x2, x3 >>> + >>> + /* >>> + * Write to MPU protection region: >>> + * x0 for pr_sel, x1 for prbar x2 for prlar >>> + */ >>> + write_pr x0, x1, x2 >>> + >>> + ret >>> +ENDPROC(enable_boot_cpu_mm) >> >> Missing emacs magic. > > You mean it should be > > END(enable_boot_cpu_mm) . > > /* > * Local variables: > * mode: ASM > * indent-tabs-mode: nil > * End: > */ Correct. >> >>> 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..f5ebca8261 >>> --- /dev/null >>> +++ b/xen/arch/arm/include/asm/mpu/mm.h >> >> Do we need this file? > > In xen/arch/arm/arm64/mpu/head.S, we have > > #include <asm/mm.h> > > So, it should pick up this file. OOI, why can't you include asm/arm64/mm.h? Cheers, -- Julien Grall
Hi Julien, Ayan, >>> >>>> + msr PRBAR_EL2, \prbar >>>> + msr PRLAR_EL2, \prlar >>>> + dsb sy >> This should be visible to outer shareable domain atleast. The reason being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to outer shareable. >> Thus, the writes to these registers should be visible to outer shareable domain as well. > > I am a bit confused. SH[1:0] is about how the region will be accessed not up to where should registers are visible. I was expecting that the MPU registers only need to be visible to the MPU itself. > > For instance, when using the MMU, the translation unit is in the non-shareable domain. So a 'nsh' is sufficient regardless of the shareability of the page/block. > > This is explicitely written in the Arm Arm (see D5-4929 in ARM DDI 0487H.a) but I can't find a similar section for the MPU yet. Although, I would be a bit surprised if the MPU is not in the non-shareable domain... Maybe this could be clarified with Arm? I got the feedback that DSB SY is ok here > > Anyway, for now, I am open to use 'dsb sy' with a TODO to revisit it. > >>>> + isb > > Re-quoting the spec from you previous answer: > > ``` > Writes to MPU registers are only guaranteed to be visible > following a Context synchronization event and DSB operation. > ``` > > So this suggests that it should be first an 'isb' and then a 'dsb'. Any reason you wrote it the other way around? I chased this internally and it was suggested the current order, dsb followed by the isb: DSB ensures the completion of prior instructions before the next executes, and then ISB ensures subsequent instruction fetch observes the updated MPU state. Probably I will raise something to make awareness around the misleading order of that phrase. Cheers, Luca
On 09/09/2024 09:48, Luca Fancellu wrote: > Hi Julien, Ayan, > >>>> >>>>> + msr PRBAR_EL2, \prbar >>>>> + msr PRLAR_EL2, \prlar >>>>> + dsb sy >>> This should be visible to outer shareable domain atleast. The reason being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to outer shareable. >>> Thus, the writes to these registers should be visible to outer shareable domain as well. >> >> I am a bit confused. SH[1:0] is about how the region will be accessed not up to where should registers are visible. I was expecting that the MPU registers only need to be visible to the MPU itself. >> >> For instance, when using the MMU, the translation unit is in the non-shareable domain. So a 'nsh' is sufficient regardless of the shareability of the page/block. >> >> This is explicitely written in the Arm Arm (see D5-4929 in ARM DDI 0487H.a) but I can't find a similar section for the MPU yet. Although, I would be a bit surprised if the MPU is not in the non-shareable domain... Maybe this could be clarified with Arm? > > I got the feedback that DSB SY is ok here Thanks for asking. Does this mean that a "dsb nsh" would not be sufficient? > >> >> Anyway, for now, I am open to use 'dsb sy' with a TODO to revisit it. >> >>>>> + isb >> >> Re-quoting the spec from you previous answer: >> >> ``` >> Writes to MPU registers are only guaranteed to be visible >> following a Context synchronization event and DSB operation. >> ``` >> >> So this suggests that it should be first an 'isb' and then a 'dsb'. Any reason you wrote it the other way around? > > I chased this internally and it was suggested the current order, dsb followed by the isb: DSB ensures the completion of prior > instructions before the next executes, and then ISB ensures subsequent instruction fetch observes the updated MPU state. I am confused. "DSB" doesn't ensure any completion of instructions. It just ensures memory access completion. Can you clarify? > > Probably I will raise something to make awareness around the misleading order of that phrase. > > Cheers, > Luca -- Julien Grall
> On 9 Sep 2024, at 10:14, Julien Grall <julien@xen.org> wrote: > > > > On 09/09/2024 09:48, Luca Fancellu wrote: >> Hi Julien, Ayan, >>>>> >>>>>> + msr PRBAR_EL2, \prbar >>>>>> + msr PRLAR_EL2, \prlar >>>>>> + dsb sy >>>> This should be visible to outer shareable domain atleast. The reason being one can use the SH[1:0] bits in PRBAR_EL2 to set the region to outer shareable. >>>> Thus, the writes to these registers should be visible to outer shareable domain as well. >>> >>> I am a bit confused. SH[1:0] is about how the region will be accessed not up to where should registers are visible. I was expecting that the MPU registers only need to be visible to the MPU itself. >>> >>> For instance, when using the MMU, the translation unit is in the non-shareable domain. So a 'nsh' is sufficient regardless of the shareability of the page/block. >>> >>> This is explicitely written in the Arm Arm (see D5-4929 in ARM DDI 0487H.a) but I can't find a similar section for the MPU yet. Although, I would be a bit surprised if the MPU is not in the non-shareable domain... Maybe this could be clarified with Arm? >> I got the feedback that DSB SY is ok here > > Thanks for asking. Does this mean that a "dsb nsh" would not be sufficient? Unfortunately no one gave a straight answer on that, I was under the impression that nsh was sufficient, but didn’t have a confirmation. I will try to chase more in deep. > >>> >>> Anyway, for now, I am open to use 'dsb sy' with a TODO to revisit it. >>> >>>>>> + isb >>> >>> Re-quoting the spec from you previous answer: >>> >>> ``` >>> Writes to MPU registers are only guaranteed to be visible >>> following a Context synchronization event and DSB operation. >>> ``` >>> >>> So this suggests that it should be first an 'isb' and then a 'dsb'. Any reason you wrote it the other way around? >> I chased this internally and it was suggested the current order, dsb followed by the isb: DSB ensures the completion of prior >> instructions before the next executes, and then ISB ensures subsequent instruction fetch observes the updated MPU state. > > I am confused. "DSB" doesn't ensure any completion of instructions. It just ensures memory access completion. Can you clarify? Sorry, I meant memory access completion. Cheers, Luca
Hi Ayan,
Apologies but I can’t do a full review yet,
>>> +
>>> +/* MPU normal memory attributes. */
>>> +#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */
>>> +#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */
>>> +
>>> +.macro write_pr, sel, prbar, prlar
>>> + msr PRSELR_EL2, \sel
>>> + dsb sy
>>
>> I am not sure I understand why this is a dsb rather than isb. Can you clarify?
>
> ISB is not needed here as the memory protection hasn't been activated yet. The above instruction just selects the memory region and the below two instructions sets the base address and limit for that memory region. After the three instructions, we need an ISB so that the memory protection takes into affect for further instruction fetches.
>
> However, a DSB is needed here as the below two instructions depend on this. So, we definitely want this instruction to complete.
>
> Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1 "Protection region attributes"
>
> 0.
>
> ```Writes to MPU registers are only guaranteed to be visible
> following a Context synchronization event and DSB operation.```
>
> Thus, I infer that DSB is necessary here.
I think this was a mistake from the author of this patch, in my opinion there should be an ISB
after setting PRSELR_ELx, to enforce a synchronisation before writing PR{B,L}AR_ELx which
depends on the value written on PRSELR.
Cheers,
Luca
On 04/09/2024 19:14, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
>
> Apologies but I can’t do a full review yet,
No worries. :)
>
>
>>>> +
>>>> +/* MPU normal memory attributes. */
>>>> +#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */
>>>> +#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */
>>>> +
>>>> +.macro write_pr, sel, prbar, prlar
>>>> + msr PRSELR_EL2, \sel
>>>> + dsb sy
>>> I am not sure I understand why this is a dsb rather than isb. Can you clarify?
>> ISB is not needed here as the memory protection hasn't been activated yet. The above instruction just selects the memory region and the below two instructions sets the base address and limit for that memory region. After the three instructions, we need an ISB so that the memory protection takes into affect for further instruction fetches.
>>
>> However, a DSB is needed here as the below two instructions depend on this. So, we definitely want this instruction to complete.
>>
>> Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1 "Protection region attributes"
>>
>> 0.
>>
>> ```Writes to MPU registers are only guaranteed to be visible
>> following a Context synchronization event and DSB operation.```
>>
>> Thus, I infer that DSB is necessary here.
> I think this was a mistake from the author of this patch, in my opinion there should be an ISB
> after setting PRSELR_ELx, to enforce a synchronisation before writing PR{B,L}AR_ELx which
> depends on the value written on PRSELR.
That synchronisation is enforced by DSB.
From
https://developer.arm.com/documentation/dui0489/c/arm-and-thumb-instructions/miscellaneous-instructions/dmb--dsb--and-isb
,
```Data Synchronization Barrier acts as a special kind of memory
barrier. No instruction in program order after this instruction executes
until this instruction completes.
...
Instruction Synchronization Barrier flushes the pipeline in the
processor, ```
Why should we flush the instruction pipeline after setting PRSELR_ELx ?
It does not have any impact on instruction fetch.
- Ayan
Hi Julien/Luca/all,
On 04/09/2024 19:38, Ayan Kumar Halder wrote:
>
> On 04/09/2024 19:14, Luca Fancellu wrote:
>> Hi Ayan,
> Hi Luca,
>>
>> Apologies but I can’t do a full review yet,
> No worries. :)
>>
>>
>>>>> +
>>>>> +/* MPU normal memory attributes. */
>>>>> +#define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */
>>>>> +#define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */
>>>>> +
>>>>> +.macro write_pr, sel, prbar, prlar
>>>>> + msr PRSELR_EL2, \sel
>>>>> + dsb sy
>>>> I am not sure I understand why this is a dsb rather than isb. Can
>>>> you clarify?
>>> ISB is not needed here as the memory protection hasn't been
>>> activated yet. The above instruction just selects the memory region
>>> and the below two instructions sets the base address and limit for
>>> that memory region. After the three instructions, we need an ISB so
>>> that the memory protection takes into affect for further instruction
>>> fetches.
>>>
>>> However, a DSB is needed here as the below two instructions depend
>>> on this. So, we definitely want this instruction to complete.
>>>
>>> Further, refer to the note in ARM DDI 0600A.d ID120821, C1.7.1
>>> "Protection region attributes"
>>>
>>> 0.
>>>
>>> ```Writes to MPU registers are only guaranteed to be visible
>>> following a Context synchronization event and DSB operation.```
>>>
>>> Thus, I infer that DSB is necessary here.
>> I think this was a mistake from the author of this patch, in my
>> opinion there should be an ISB
>> after setting PRSELR_ELx, to enforce a synchronisation before writing
>> PR{B,L}AR_ELx which
>> depends on the value written on PRSELR.
>
> That synchronisation is enforced by DSB.
>
> From
> https://developer.arm.com/documentation/dui0489/c/arm-and-thumb-instructions/miscellaneous-instructions/dmb--dsb--and-isb
> ,
>
> ```Data Synchronization Barrier acts as a special kind of memory
> barrier. No instruction in program order after this instruction
> executes until this instruction completes.
>
> ...
>
> Instruction Synchronization Barrier flushes the pipeline in the
> processor, ```
>
>
> Why should we flush the instruction pipeline after setting PRSELR_ELx
> ? It does not have any impact on instruction fetch.
After the discussion on matrix, I agree that we need an 'isb' here to
ensure that there is no instruction reordering.
Thanks for the explanation.
- Ayan
© 2016 - 2024 Red Hat, Inc.