| Message ID | 20240918175102.223076-4-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable early bootup of AArch64 MPU systems | expand |
Hi Ayan, On 18/09/2024 19:51, 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 (Read/Write at EL2 as the data is RW during init) > 4. RW data (Read/Write at EL2) > 5. BSS (Read/Write at EL2) > 6. Init Text (Read only at EL2, execution is permitted) > 7. Init data (Read/Write at EL2) > > If the number of MPU regions created is greater than the count defined > in MPUIR_EL2, then the boot fails. > > 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. > > 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). > > xen/arch/arm/Makefile | 1 + > xen/arch/arm/arm64/mpu/Makefile | 1 + > xen/arch/arm/arm64/mpu/head.S | 176 +++++++++++++++++++ > xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 +++ > xen/arch/arm/include/asm/arm64/sysregs.h | 3 + > 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 + > 9 files changed, 253 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/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/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..ef55c8765c > --- /dev/null > +++ b/xen/arch/arm/arm64/mpu/head.S > @@ -0,0 +1,176 @@ > +/* 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> > + > +#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 round up the section address to be PAGE_SIZE aligned > + * Each section(e.g. .text, .data, etc) in xen.lds.S is page-aligned, > + * which is usually guarded with ". = ALIGN(PAGE_SIZE)" in the head, > + * or in the end > + */ > +.macro roundup_section, xb > + add \xb, \xb, #(PAGE_SIZE-1) > + and \xb, \xb, #PAGE_MASK > +.endm Can you explain why we need the region to be page-aligned? Would not 64-bytes alignment (IIRC this is the minimum by the MPU sufficient) And more importantly, if those regions were effectively not aligned, would not this mean we would could configure the MPU with two clashing regions? IOW, this round up looks harmful to me. > + > +/* > + * 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 > + * 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 > + * > + * Clobber \tmp1, \tmp2 > + * > + */ > +.macro prepare_xen_region, sel, base, limit, prbar, prlar, tmp1, tmp2, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR > + /* 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 > + > + /* Prepare value for PRLAR_EL2 reg and preserve it in \prlar.*/ > + /* Round up limit address to be PAGE_SIZE aligned */ > + roundup_section \limit > + /* 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 Please use "dsb sy". This has the same meaning as "dsb" but more explicit for the reader. > + isb > +.endm > + > +/* Load the physical address of a symbol into xb */ > +.macro load_paddr xb, sym > + ldr \xb, =\sym > + add \xb, \xb, x20 /* x20 - Phys offset */ > +.endm > + > +.section .text.header, "ax", %progbits Does the code below actually need to be in .text.header? > + > +/* > + * Enable EL2 MPU and data cache > + * If the Background region is enabled, then the MPU uses the default memory > + * map as the Background region for generating the memory > + * attributes when MPU is disabled. > + * Since the default memory map of the Armv8-R AArch64 architecture is > + * IMPLEMENTATION DEFINED, we intend to turn off the Background region here. Based on this sentence, I was expecting an instruction to clear SCTRL_EL2.BR. What did I miss? > + * > + * Clobbers x0 > + * > + */ > +FUNC_LOCAL(enable_mpu) > + mrs x0, SCTLR_EL2 > + orr x0, x0, #SCTLR_Axx_ELx_M /* Enable MPU */ > + orr x0, x0, #SCTLR_Axx_ELx_C /* Enable D-cache */ > + orr x0, x0, #SCTLR_Axx_ELx_WXN /* Enable WXN */ > + dsb sy There is no memory access in enable_mpu. So what is this dsb for? > + msr SCTLR_EL2, x0 > + isb > + > + ret > +END(enable_mpu) > + > +/* > + * Maps the various sections of Xen (described in xen.lds.S) as different MPU > + * regions. Some of these regions will be marked read only while the others will > + * be read write or execute. And some in the future may need to be memory region (e.g. to use the UART early) :). So how about just dropping it? > + *> + * Inputs: > + * lr : Address to return to. > + * > + * Clobbers x0 - x7 > + * > + */ > +FUNC(enable_boot_cpu_mm) > + mov x7, lr> + > + /* x0: region sel */ > + mov x0, xzr > + /* Xen text section. */ > + load_paddr x1, _stext > + load_paddr x2, _etext > + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, attr_prbar=REGION_TEXT_PRBAR > + > + add x0, x0, #1 > + /* Xen read-only data section. */ > + load_paddr x1, _srodata > + load_paddr x2, _erodata > + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, attr_prbar=REGION_RO_PRBAR > + > + add x0, x0, #1 > + /* Xen read-only after init data section. */ > + load_paddr x1, __ro_after_init_start > + load_paddr x2, __ro_after_init_end > + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 Is it necessary to have a section for the ro-after-init? IOW, could we just fold into... > + > + add x0, x0, #1 > + /* Xen read-write data section. */ > + load_paddr x1, __ro_after_init_end > + load_paddr x2, __init_begin > + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 ... this one during boot and then fold the ro-after-init to the read-only region? This would possibly reduce the number of used by Xen and also avoid reshfulling the MPU section. Another possibility would be to move the ro-after-init MPU region at the end. > +> + add x0, x0, #1 > + /* Xen BSS section. */ > + load_paddr x1, __bss_start > + load_paddr x2, __bss_end > + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 > + > + add x0, x0, #1 > + /* Xen init text section. */ > + load_paddr x1, __init_begin > + load_paddr x2, __init_data_begin > + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, attr_prbar=REGION_TEXT_PRBAR > + > + add x0, x0, #1 > + /* Xen init data section. */ > + load_paddr x1, __init_data_begin > + load_paddr x2, __init_end > + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 > + > + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ > + mrs x1, MPUIR_EL2 > + cmp x0, x1 > + bgt 1f > + bl enable_mpu It seems to be a bit odd to check *after* we wrote. It would be useful to explain why this is fine. > + > + mov lr, x7 > + ret > + > +1: > + PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n") > + wfe > + b 1b > +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/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h > index b593e4028b..8b6b373ce9 100644 > --- a/xen/arch/arm/include/asm/arm64/sysregs.h > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h > @@ -2,6 +2,9 @@ > #define __ASM_ARM_ARM64_SYSREGS_H > > #include <xen/stringify.h> > +#if defined(CONFIG_MPU) > +#include <asm/arm64/mpu/sysregs.h> > +#endif I would expect the number of user for the MPU sysregs to be limited. So can we include the header only when it is necessary? Cheers,
On 19/09/2024 14:16, Julien Grall wrote: > Hi Ayan, Hi Julien, I need some clarifications. > > On 18/09/2024 19:51, 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 (Read/Write at EL2 as the data is RW during init) >> 4. RW data (Read/Write at EL2) >> 5. BSS (Read/Write at EL2) >> 6. Init Text (Read only at EL2, execution is permitted) >> 7. Init data (Read/Write at EL2) >> >> If the number of MPU regions created is greater than the count defined >> in MPUIR_EL2, then the boot fails. >> >> 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. >> >> 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). >> >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/arm64/mpu/Makefile | 1 + >> xen/arch/arm/arm64/mpu/head.S | 176 +++++++++++++++++++ >> xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 +++ >> xen/arch/arm/include/asm/arm64/sysregs.h | 3 + >> 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 + >> 9 files changed, 253 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/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/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..ef55c8765c >> --- /dev/null >> +++ b/xen/arch/arm/arm64/mpu/head.S >> @@ -0,0 +1,176 @@ >> +/* 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> >> + >> +#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 round up the section address to be PAGE_SIZE aligned >> + * Each section(e.g. .text, .data, etc) in xen.lds.S is page-aligned, >> + * which is usually guarded with ". = ALIGN(PAGE_SIZE)" in the head, >> + * or in the end >> + */ >> +.macro roundup_section, xb >> + add \xb, \xb, #(PAGE_SIZE-1) >> + and \xb, \xb, #PAGE_MASK >> +.endm > > Can you explain why we need the region to be page-aligned? Would not > 64-bytes alignment (IIRC this is the minimum by the MPU sufficient) We are aligning the limit address only (not the base address). However ... > > And more importantly, if those regions were effectively not aligned, > would not this mean we would could configure the MPU with two clashing > regions? IOW, this round up looks harmful to me. you are correct that there is chance that limit address might overlap between 2 regions. Also (thinking again), the limit address is 0x3f extended when is programmed into PRLAR. So, we might not need alignment at all for the limit address. > >> + >> +/* >> + * 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 >> + * 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 >> + * >> + * Clobber \tmp1, \tmp2 >> + * >> + */ >> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, tmp1, >> tmp2, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >> + /* 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 >> + >> + /* Prepare value for PRLAR_EL2 reg and preserve it in \prlar.*/ >> + /* Round up limit address to be PAGE_SIZE aligned */ >> + roundup_section \limit >> + /* 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 > > Please use "dsb sy". This has the same meaning as "dsb" but more > explicit for the reader. Ack > >> + isb >> +.endm >> + >> +/* Load the physical address of a symbol into xb */ >> +.macro load_paddr xb, sym >> + ldr \xb, =\sym >> + add \xb, \xb, x20 /* x20 - Phys offset */ >> +.endm >> + >> +.section .text.header, "ax", %progbits > > Does the code below actually need to be in .text.header? I can remove this altogether. As I understand, the code should be in .text section. > >> + >> +/* >> + * Enable EL2 MPU and data cache >> + * If the Background region is enabled, then the MPU uses the >> default memory >> + * map as the Background region for generating the memory >> + * attributes when MPU is disabled. >> + * Since the default memory map of the Armv8-R AArch64 architecture is >> + * IMPLEMENTATION DEFINED, we intend to turn off the Background >> region here. > > Based on this sentence, I was expecting an instruction to clear > SCTRL_EL2.BR. What did I miss? Sorry, based on https://developer.arm.com/documentation/102670/0300/AArch64-registers/AArch64-register-descriptions/AArch64-other-register-description/SCTLR-EL2--System-Control-Register--EL2- , SCTLR_EL2.BR == 0 (reset value). Thus, the background region is disabled by default. Should I still set SCTLR_EL2.BR = 0 ? Or do I update the description with this info. > >> + * >> + * Clobbers x0 >> + * >> + */ >> +FUNC_LOCAL(enable_mpu) >> + mrs x0, SCTLR_EL2 >> + orr x0, x0, #SCTLR_Axx_ELx_M /* Enable MPU */ >> + orr x0, x0, #SCTLR_Axx_ELx_C /* Enable D-cache */ >> + orr x0, x0, #SCTLR_Axx_ELx_WXN /* Enable WXN */ >> + dsb sy > > There is no memory access in enable_mpu. So what is this dsb for? We need to ensure that the outstanding memory accesses are completed before the MPU is enabled. I think the same rationale applies here as enable_mmu() { ... "dsb sy /* Flush PTE writes and finish reads */" .. } In the case of MPU, we need the reads to be completed. > >> + msr SCTLR_EL2, x0 >> + isb >> + >> + ret >> +END(enable_mpu) >> + >> +/* >> + * Maps the various sections of Xen (described in xen.lds.S) as >> different MPU >> + * regions. Some of these regions will be marked read only while the >> others will >> + * be read write or execute. > > And some in the future may need to be memory region (e.g. to use the > UART early) :). So how about just dropping it? Yes. > > > + *> + * Inputs: >> + * lr : Address to return to. >> + * >> + * Clobbers x0 - x7 >> + * >> + */ >> +FUNC(enable_boot_cpu_mm) > > + mov x7, lr> + >> + /* x0: region sel */ >> + mov x0, xzr >> + /* Xen text section. */ >> + load_paddr x1, _stext >> + load_paddr x2, _etext >> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, >> attr_prbar=REGION_TEXT_PRBAR >> + >> + add x0, x0, #1 >> + /* Xen read-only data section. */ >> + load_paddr x1, _srodata >> + load_paddr x2, _erodata >> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, >> attr_prbar=REGION_RO_PRBAR >> + >> + add x0, x0, #1 >> + /* Xen read-only after init data section. */ >> + load_paddr x1, __ro_after_init_start >> + load_paddr x2, __ro_after_init_end >> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 > > Is it necessary to have a section for the ro-after-init? IOW, could we > just fold into... > >> + >> + add x0, x0, #1 >> + /* Xen read-write data section. */ >> + load_paddr x1, __ro_after_init_end >> + load_paddr x2, __init_begin >> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 > > ... this one during boot This makes sense. So what you are saying is that there should be a single RW data region from __ro_after_init_start to _eteemediator (not __init_begin as it would overlap with the next). Followed by a text region from __init_begin to _einittext. However _eteemediator is same as __init_begin, so should we be inserting a dummy page in between ? A RW data region from __init_data_begin to __bss_end. Can we combine the BSS section and init data section into one region ? > and then fold the ro-after-init to the read-only region? This I did not understand. > This would possibly reduce the number of used by Xen and also avoid > reshfulling the MPU section. > > Another possibility would be to move the ro-after-init MPU region at > the end. > > > +> + add x0, x0, #1 >> + /* Xen BSS section. */ >> + load_paddr x1, __bss_start >> + load_paddr x2, __bss_end >> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 >> + >> + add x0, x0, #1 >> + /* Xen init text section. */ >> + load_paddr x1, __init_begin >> + load_paddr x2, __init_data_begin >> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, >> attr_prbar=REGION_TEXT_PRBAR >> + >> + add x0, x0, #1 >> + /* Xen init data section. */ >> + load_paddr x1, __init_data_begin >> + load_paddr x2, __init_end >> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 >> + >> + /* Check if the number of regions exceeded the count specified >> in MPUIR_EL2 */ >> + mrs x1, MPUIR_EL2 >> + cmp x0, x1 >> + bgt 1f >> + bl enable_mpu > > It seems to be a bit odd to check *after* we wrote. It would be useful > to explain why this is fine. Sorry, it needs to be checked each time we create a new region. I will fix this. > >> + >> + mov lr, x7 >> + ret >> + >> +1: >> + PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n") >> + wfe >> + b 1b >> +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/arm64/sysregs.h >> b/xen/arch/arm/include/asm/arm64/sysregs.h >> index b593e4028b..8b6b373ce9 100644 >> --- a/xen/arch/arm/include/asm/arm64/sysregs.h >> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h >> @@ -2,6 +2,9 @@ >> #define __ASM_ARM_ARM64_SYSREGS_H >> #include <xen/stringify.h> >> +#if defined(CONFIG_MPU) >> +#include <asm/arm64/mpu/sysregs.h> >> +#endif > > I would expect the number of user for the MPU sysregs to be limited. > So can we include the header only when it is necessary? ok. - Ayan > > Cheers, >
Hi Ayan, On 24/09/2024 12:32, Ayan Kumar Halder wrote: > > On 19/09/2024 14:16, Julien Grall wrote: >> Hi Ayan, > > Hi Julien, > > I need some clarifications. > >> >> On 18/09/2024 19:51, 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 (Read/Write at EL2 as the data is RW during init) >>> 4. RW data (Read/Write at EL2) >>> 5. BSS (Read/Write at EL2) >>> 6. Init Text (Read only at EL2, execution is permitted) >>> 7. Init data (Read/Write at EL2) >>> >>> If the number of MPU regions created is greater than the count defined >>> in MPUIR_EL2, then the boot fails. >>> >>> 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. >>> >>> 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). >>> >>> xen/arch/arm/Makefile | 1 + >>> xen/arch/arm/arm64/mpu/Makefile | 1 + >>> xen/arch/arm/arm64/mpu/head.S | 176 +++++++++++++++++++ >>> xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 +++ >>> xen/arch/arm/include/asm/arm64/sysregs.h | 3 + >>> 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 + >>> 9 files changed, 253 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/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/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..ef55c8765c >>> --- /dev/null >>> +++ b/xen/arch/arm/arm64/mpu/head.S >>> @@ -0,0 +1,176 @@ >>> +/* 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> >>> + >>> +#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 round up the section address to be PAGE_SIZE aligned >>> + * Each section(e.g. .text, .data, etc) in xen.lds.S is page-aligned, >>> + * which is usually guarded with ". = ALIGN(PAGE_SIZE)" in the head, >>> + * or in the end >>> + */ >>> +.macro roundup_section, xb >>> + add \xb, \xb, #(PAGE_SIZE-1) >>> + and \xb, \xb, #PAGE_MASK >>> +.endm >> >> Can you explain why we need the region to be page-aligned? Would not >> 64-bytes alignment (IIRC this is the minimum by the MPU sufficient) > We are aligning the limit address only (not the base address). However ... >> >> And more importantly, if those regions were effectively not aligned, >> would not this mean we would could configure the MPU with two clashing >> regions? IOW, this round up looks harmful to me. > > you are correct that there is chance that limit address might overlap > between 2 regions. Also (thinking again), the limit address is 0x3f > extended when is programmed into PRLAR. So, we might not need alignment > at all for the limit address. I am not sure I fully understand what you wrote. Can you point me to the code you are referring to? > >> >>> + >>> +/* >>> + * 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 >>> + * 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 >>> + * >>> + * Clobber \tmp1, \tmp2 >>> + * >>> + */ >>> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, tmp1, >>> tmp2, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >>> + /* 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 >>> + >>> + /* Prepare value for PRLAR_EL2 reg and preserve it in \prlar.*/ >>> + /* Round up limit address to be PAGE_SIZE aligned */ >>> + roundup_section \limit >>> + /* 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 >> >> Please use "dsb sy". This has the same meaning as "dsb" but more >> explicit for the reader. > Ack >> >>> + isb >>> +.endm >>> + >>> +/* Load the physical address of a symbol into xb */ >>> +.macro load_paddr xb, sym >>> + ldr \xb, =\sym >>> + add \xb, \xb, x20 /* x20 - Phys offset */ >>> +.endm >>> + >>> +.section .text.header, "ax", %progbits >> >> Does the code below actually need to be in .text.header? > > I can remove this altogether. As I understand, the code should be > in .text section. > >> >>> + >>> +/* >>> + * Enable EL2 MPU and data cache >>> + * If the Background region is enabled, then the MPU uses the >>> default memory >>> + * map as the Background region for generating the memory >>> + * attributes when MPU is disabled. >>> + * Since the default memory map of the Armv8-R AArch64 architecture is >>> + * IMPLEMENTATION DEFINED, we intend to turn off the Background >>> region here. >> >> Based on this sentence, I was expecting an instruction to clear >> SCTRL_EL2.BR. What did I miss? > > Sorry, based on https://developer.arm.com/documentation/102670/0300/ > AArch64-registers/AArch64-register-descriptions/AArch64-other-register- > description/SCTLR-EL2--System-Control-Register--EL2- , SCTLR_EL2.BR == 0 > (reset value). Thus, the background region is disabled by default. > > Should I still set SCTLR_EL2.BR = 0 ? Or do I update the description > with this info. Both the Arm Arm and the TRM will tell us the state when the CPU boot. But I guess that there might be a firmware running before Xen. So we can't assume the values in the registers (unless this is documented in the boot protocol like Image). So I think we need to set SCTLR_EL2.BR to 0. > >> >>> + * >>> + * Clobbers x0 >>> + * >>> + */ >>> +FUNC_LOCAL(enable_mpu) >>> + mrs x0, SCTLR_EL2 >>> + orr x0, x0, #SCTLR_Axx_ELx_M /* Enable MPU */ >>> + orr x0, x0, #SCTLR_Axx_ELx_C /* Enable D-cache */ >>> + orr x0, x0, #SCTLR_Axx_ELx_WXN /* Enable WXN */ >>> + dsb sy >> >> There is no memory access in enable_mpu. So what is this dsb for? > > We need to ensure that the outstanding memory accesses are completed > before the MPU is enabled. I think the same rationale applies here as > > enable_mmu() > > { > > ... > > "dsb sy /* Flush PTE writes and finish reads */" > > .. > > } > > In the case of MPU, we need the reads to be completed. I can't remember why a dsb was added there. But I don't see why we would need with the MPU as: 1/ we have a 1:1 mapping 2/ everytime we touch the MPU sections, we ensure the change will be visible > >> >>> + msr SCTLR_EL2, x0 >>> + isb >>> + >>> + ret >>> +END(enable_mpu) >>> + >>> +/* >>> + * Maps the various sections of Xen (described in xen.lds.S) as >>> different MPU >>> + * regions. Some of these regions will be marked read only while the >>> others will >>> + * be read write or execute. >> >> And some in the future may need to be memory region (e.g. to use the >> UART early) :). So how about just dropping it? > Yes. >> >> > + *> + * Inputs: >>> + * lr : Address to return to. >>> + * >>> + * Clobbers x0 - x7 >>> + * >>> + */ >>> +FUNC(enable_boot_cpu_mm) >> > + mov x7, lr> + >>> + /* x0: region sel */ >>> + mov x0, xzr >>> + /* Xen text section. */ >>> + load_paddr x1, _stext >>> + load_paddr x2, _etext >>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, >>> attr_prbar=REGION_TEXT_PRBAR >>> + >>> + add x0, x0, #1 >>> + /* Xen read-only data section. */ >>> + load_paddr x1, _srodata >>> + load_paddr x2, _erodata >>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, >>> attr_prbar=REGION_RO_PRBAR >>> + >>> + add x0, x0, #1 >>> + /* Xen read-only after init data section. */ >>> + load_paddr x1, __ro_after_init_start >>> + load_paddr x2, __ro_after_init_end >>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 >> >> Is it necessary to have a section for the ro-after-init? IOW, could we >> just fold into... >> >>> + >>> + add x0, x0, #1 >>> + /* Xen read-write data section. */ >>> + load_paddr x1, __ro_after_init_end >>> + load_paddr x2, __init_begin >>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 >> >> ... this one during boot > > This makes sense. So what you are saying is that there should be a > single RW data region from __ro_after_init_start to _eteemediator (not > __init_begin as it would overlap with the next). _eteemediator may not be properly aligned. You will likely need another variable. As a side note, I don't understand why the TEE mediator are part of RW. It is a separate problem though. > > Followed by a text region from __init_begin to _einittext. However > _eteemediator is same as __init_begin, so should we be inserting a dummy > page in between ? I don't understand what you mean. _init_begin should be suitably aligned to 4KB. So why would we need a page in between? > > A RW data region from __init_data_begin to __bss_end. Can we combine the > BSS section and init data section into one region ? If they have the same attribute then yes. > >> and then fold the ro-after-init to the read-only region? > > This I did not understand. I mean that the MPU regions would be updated so that after boot, one region would cover .rodata + .ro_after_init. The other region would cover .data up to .init (not included). Cheers,
On 24/09/2024 18:10, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 24/09/2024 12:32, Ayan Kumar Halder wrote: >> >> On 19/09/2024 14:16, Julien Grall wrote: >>> Hi Ayan, >> >> Hi Julien, >> >> I need some clarifications. >> >>> >>> On 18/09/2024 19:51, 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 (Read/Write at EL2 as the data is RW during >>>> init) >>>> 4. RW data (Read/Write at EL2) >>>> 5. BSS (Read/Write at EL2) >>>> 6. Init Text (Read only at EL2, execution is permitted) >>>> 7. Init data (Read/Write at EL2) >>>> >>>> If the number of MPU regions created is greater than the count defined >>>> in MPUIR_EL2, then the boot fails. >>>> >>>> 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. >>>> >>>> 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). >>>> >>>> xen/arch/arm/Makefile | 1 + >>>> xen/arch/arm/arm64/mpu/Makefile | 1 + >>>> xen/arch/arm/arm64/mpu/head.S | 176 >>>> +++++++++++++++++++ >>>> xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 +++ >>>> xen/arch/arm/include/asm/arm64/sysregs.h | 3 + >>>> 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 + >>>> 9 files changed, 253 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/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/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..ef55c8765c >>>> --- /dev/null >>>> +++ b/xen/arch/arm/arm64/mpu/head.S >>>> @@ -0,0 +1,176 @@ >>>> +/* 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> >>>> + >>>> +#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 round up the section address to be PAGE_SIZE aligned >>>> + * Each section(e.g. .text, .data, etc) in xen.lds.S is page-aligned, >>>> + * which is usually guarded with ". = ALIGN(PAGE_SIZE)" in the head, >>>> + * or in the end >>>> + */ >>>> +.macro roundup_section, xb >>>> + add \xb, \xb, #(PAGE_SIZE-1) >>>> + and \xb, \xb, #PAGE_MASK >>>> +.endm >>> >>> Can you explain why we need the region to be page-aligned? Would not >>> 64-bytes alignment (IIRC this is the minimum by the MPU sufficient) >> We are aligning the limit address only (not the base address). >> However ... >>> >>> And more importantly, if those regions were effectively not aligned, >>> would not this mean we would could configure the MPU with two >>> clashing regions? IOW, this round up looks harmful to me. >> >> you are correct that there is chance that limit address might overlap >> between 2 regions. Also (thinking again), the limit address is 0x3f >> extended when is programmed into PRLAR. So, we might not need >> alignment at all for the limit address. > > I am not sure I fully understand what you wrote. Can you point me to > the code you are referring to? Actually I was referring to the ArmV8-R AArch32 docs ( ARM DDI 0568A.c ID110520) here. In case of HPRBAR "Address[31:6] concatenated with zeroes to form Address[31:0]" , so the base address need to be at least 64 byte aligned. However for HPRLAR, "Address[31:6] concatenated with the value 0x3F to form Address[31:0],". So even if we align the limit address to 4 KB or 64 byte or anything, the actual limit address will always be unaligned. So, I am thinking that aligning limit address might not make sense. > >> >>> >>>> + >>>> +/* >>>> + * 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 >>>> + * 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 >>>> + * >>>> + * Clobber \tmp1, \tmp2 >>>> + * >>>> + */ >>>> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, tmp1, >>>> tmp2, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >>>> + /* 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 >>>> + >>>> + /* Prepare value for PRLAR_EL2 reg and preserve it in \prlar.*/ >>>> + /* Round up limit address to be PAGE_SIZE aligned */ >>>> + roundup_section \limit >>>> + /* 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 >>> >>> Please use "dsb sy". This has the same meaning as "dsb" but more >>> explicit for the reader. >> Ack >>> >>>> + isb >>>> +.endm >>>> + >>>> +/* Load the physical address of a symbol into xb */ >>>> +.macro load_paddr xb, sym >>>> + ldr \xb, =\sym >>>> + add \xb, \xb, x20 /* x20 - Phys offset */ >>>> +.endm >>>> + >>>> +.section .text.header, "ax", %progbits >>> >>> Does the code below actually need to be in .text.header? >> >> I can remove this altogether. As I understand, the code should be in >> .text section. >> >>> >>>> + >>>> +/* >>>> + * Enable EL2 MPU and data cache >>>> + * If the Background region is enabled, then the MPU uses the >>>> default memory >>>> + * map as the Background region for generating the memory >>>> + * attributes when MPU is disabled. >>>> + * Since the default memory map of the Armv8-R AArch64 >>>> architecture is >>>> + * IMPLEMENTATION DEFINED, we intend to turn off the Background >>>> region here. >>> >>> Based on this sentence, I was expecting an instruction to clear >>> SCTRL_EL2.BR. What did I miss? >> >> Sorry, based on https://developer.arm.com/documentation/102670/0300/ >> AArch64-registers/AArch64-register-descriptions/AArch64-other-register- >> description/SCTLR-EL2--System-Control-Register--EL2- , SCTLR_EL2.BR >> == 0 (reset value). Thus, the background region is disabled by default. >> >> Should I still set SCTLR_EL2.BR = 0 ? Or do I update the description >> with this info. > > Both the Arm Arm and the TRM will tell us the state when the CPU boot. > But I guess that there might be a firmware running before Xen. So we > can't assume the values in the registers (unless this is documented in > the boot protocol like Image). > > So I think we need to set SCTLR_EL2.BR to 0. Ack. > >> >>> >>>> + * >>>> + * Clobbers x0 >>>> + * >>>> + */ >>>> +FUNC_LOCAL(enable_mpu) >>>> + mrs x0, SCTLR_EL2 >>>> + orr x0, x0, #SCTLR_Axx_ELx_M /* Enable MPU */ >>>> + orr x0, x0, #SCTLR_Axx_ELx_C /* Enable D-cache */ >>>> + orr x0, x0, #SCTLR_Axx_ELx_WXN /* Enable WXN */ >>>> + dsb sy >>> >>> There is no memory access in enable_mpu. So what is this dsb for? >> >> We need to ensure that the outstanding memory accesses are completed >> before the MPU is enabled. I think the same rationale applies here as >> >> enable_mmu() >> >> { >> >> ... >> >> "dsb sy /* Flush PTE writes and finish reads */" >> >> .. >> >> } >> >> In the case of MPU, we need the reads to be completed. > > I can't remember why a dsb was added there. But I don't see why we > would need with the MPU as: > 1/ we have a 1:1 mapping > 2/ everytime we touch the MPU sections, we ensure the change will be > visible ok, so we don't need any barriers here. > >> >>> >>>> + msr SCTLR_EL2, x0 >>>> + isb >>>> + >>>> + ret >>>> +END(enable_mpu) >>>> + >>>> +/* >>>> + * Maps the various sections of Xen (described in xen.lds.S) as >>>> different MPU >>>> + * regions. Some of these regions will be marked read only while >>>> the others will >>>> + * be read write or execute. >>> >>> And some in the future may need to be memory region (e.g. to use the >>> UART early) :). So how about just dropping it? >> Yes. >>> >>> > + *> + * Inputs: >>>> + * lr : Address to return to. >>>> + * >>>> + * Clobbers x0 - x7 >>>> + * >>>> + */ >>>> +FUNC(enable_boot_cpu_mm) >>> > + mov x7, lr> + >>>> + /* x0: region sel */ >>>> + mov x0, xzr >>>> + /* Xen text section. */ >>>> + load_paddr x1, _stext >>>> + load_paddr x2, _etext >>>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, >>>> attr_prbar=REGION_TEXT_PRBAR >>>> + >>>> + add x0, x0, #1 >>>> + /* Xen read-only data section. */ >>>> + load_paddr x1, _srodata >>>> + load_paddr x2, _erodata >>>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, >>>> attr_prbar=REGION_RO_PRBAR >>>> + >>>> + add x0, x0, #1 >>>> + /* Xen read-only after init data section. */ >>>> + load_paddr x1, __ro_after_init_start >>>> + load_paddr x2, __ro_after_init_end >>>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 >>> >>> Is it necessary to have a section for the ro-after-init? IOW, could >>> we just fold into... >>> >>>> + >>>> + add x0, x0, #1 >>>> + /* Xen read-write data section. */ >>>> + load_paddr x1, __ro_after_init_end >>>> + load_paddr x2, __init_begin >>>> + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 >>> >>> ... this one during boot >> >> This makes sense. So what you are saying is that there should be a >> single RW data region from __ro_after_init_start to _eteemediator (not > > __init_begin as it would overlap with the next). > > _eteemediator may not be properly aligned. You will likely need > another variable. > > As a side note, I don't understand why the TEE mediator are part of > RW. It is a separate problem though. > >> >> Followed by a text region from __init_begin to _einittext. However >> _eteemediator is same as __init_begin, so should we be inserting a >> dummy page in between ? > > I don't understand what you mean. _init_begin should be suitably > aligned to 4KB. So why would we need a page in between? Sorry I didn't explain clearly. In my xen-syms.map 0x202000 D __ro_after_init_start 0x202000 D _eteemediator 0x202000 T __init_begin So since __ro_after_init_start == _eteemediator, so the RW data region is empty. There is nothing to map here. However if _eteemediator and __init_begin were 0x203000 , so I should create a RW data region from 0x202000 - 0x202FFF (ie __init_begin - 1). And then there will be Text region from __init_begin to _einittext - 1. Is my understanding correct ? > >> >> A RW data region from __init_data_begin to __bss_end. Can we combine >> the BSS section and init data section into one region ? > > If they have the same attribute then yes. ok, so my sections look like this (from readelf -DS) [ 0] NULL 0000000000000000 00000000 0000000000000000 0000000000000000 0 0 0 [ 1] .text PROGBITS 0000000000200000 00000100 0000000000000832 0000000000000000 WAX 0 0 32 [ 2] .rodata PROGBITS 0000000000201000 00001100 0000000000000d78 0000000000000000 A 0 0 256 [ 3] .note.gnu.bu[...] NOTE 0000000000201d78 00001e78 0000000000000024 0000000000000000 A 0 0 4 [ 4] .data.ro_aft[...] PROGBITS 0000000000202000 00003170 0000000000000000 0000000000000000 WA 0 0 1 [ 5] .data PROGBITS 0000000000202000 00002100 0000000000000000 0000000000000000 WA 0 0 1 [ 6] .init.text PROGBITS 0000000000202000 00002100 0000000000000048 0000000000000000 AX 0 0 4 [ 7] .init.data PROGBITS 0000000000203000 00003100 0000000000000070 0000000000000000 A 0 0 1 [ 8] .bss NOBITS 0000000000208000 00003170 0000000000000000 0000000000000000 WA 0 0 1 IIUC, [2] and [3] will be combined. [4] and [5] will be combined. So we will be creating 6 MPU regions in total. Is this correct ? - Ayan > >> >>> and then fold the ro-after-init to the read-only region? >> >> This I did not understand. > > I mean that the MPU regions would be updated so that after boot, one > region would cover .rodata + .ro_after_init. The other region would > cover .data up to .init (not included). > > Cheers, >
Hi, On 25/09/2024 14:22, Ayan Kumar Halder wrote: > > On 24/09/2024 18:10, Julien Grall wrote: >> On 24/09/2024 12:32, Ayan Kumar Halder wrote: >>> >>> On 19/09/2024 14:16, Julien Grall wrote: >>>> On 18/09/2024 19:51, 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 (Read/Write at EL2 as the data is RW during >>>>> init) >>>>> 4. RW data (Read/Write at EL2) >>>>> 5. BSS (Read/Write at EL2) >>>>> 6. Init Text (Read only at EL2, execution is permitted) >>>>> 7. Init data (Read/Write at EL2) >>>>> >>>>> If the number of MPU regions created is greater than the count defined >>>>> in MPUIR_EL2, then the boot fails. >>>>> >>>>> 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. >>>>> >>>>> 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). >>>>> >>>>> xen/arch/arm/Makefile | 1 + >>>>> xen/arch/arm/arm64/mpu/Makefile | 1 + >>>>> xen/arch/arm/arm64/mpu/head.S | 176 ++++++++++++++ >>>>> +++++ >>>>> xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 +++ >>>>> xen/arch/arm/include/asm/arm64/sysregs.h | 3 + >>>>> 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 + >>>>> 9 files changed, 253 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/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/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..ef55c8765c >>>>> --- /dev/null >>>>> +++ b/xen/arch/arm/arm64/mpu/head.S >>>>> @@ -0,0 +1,176 @@ >>>>> +/* 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> >>>>> + >>>>> +#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 round up the section address to be PAGE_SIZE aligned >>>>> + * Each section(e.g. .text, .data, etc) in xen.lds.S is page-aligned, >>>>> + * which is usually guarded with ". = ALIGN(PAGE_SIZE)" in the head, >>>>> + * or in the end >>>>> + */ >>>>> +.macro roundup_section, xb >>>>> + add \xb, \xb, #(PAGE_SIZE-1) >>>>> + and \xb, \xb, #PAGE_MASK >>>>> +.endm >>>> >>>> Can you explain why we need the region to be page-aligned? Would not >>>> 64-bytes alignment (IIRC this is the minimum by the MPU sufficient) >>> We are aligning the limit address only (not the base address). >>> However ... >>>> >>>> And more importantly, if those regions were effectively not aligned, >>>> would not this mean we would could configure the MPU with two >>>> clashing regions? IOW, this round up looks harmful to me. >>> >>> you are correct that there is chance that limit address might overlap >>> between 2 regions. Also (thinking again), the limit address is 0x3f >>> extended when is programmed into PRLAR. So, we might not need >>> alignment at all for the limit address. >> >> I am not sure I fully understand what you wrote. Can you point me to >> the code you are referring to? > > Actually I was referring to the ArmV8-R AArch32 docs ( ARM DDI 0568A.c > ID110520) here. The code you are writing is for 64-bit :). So please provide some reference using the ArmV8-R AArch64 docs. > > In case of HPRBAR "Address[31:6] concatenated with zeroes to form > Address[31:0]" , so the base address need to be at least 64 byte aligned. AFAICT, the 64-bit equivalent is PRBAR<n>_EL2. But indeed, we need the address to be 64-byte. That said, I would simply mask the bits rather than rounding up because all the address should be properly aligned (as we do for the MMU). > > However for HPRLAR, "Address[31:6] concatenated with the value 0x3F to > form Address[31:0],". So even if we align the limit address to 4 KB or > 64 byte or anything, the actual limit address will always be unaligned. That's just the limit is inclusive. So it will always be 64-byte aligned - 1. Anyway... > So, I am thinking that aligning limit address might not make sense. ... same as I above. I don't think we need to align anything. We can just mask it. [...] >>> Followed by a text region from __init_begin to _einittext. However >>> _eteemediator is same as __init_begin, so should we be inserting a >>> dummy page in between ? >> >> I don't understand what you mean. _init_begin should be suitably >> aligned to 4KB. So why would we need a page in between? > > Sorry I didn't explain clearly. In my xen-syms.map > > 0x202000 D __ro_after_init_start > > 0x202000 D _eteemediator > > 0x202000 T __init_begin > > > So since __ro_after_init_start == _eteemediator, so the RW data region > is empty. There is nothing to map here. > > However if _eteemediator and __init_begin were 0x203000 , so I should > create a RW data region from 0x202000 - 0x202FFF (ie __init_begin - 1). > > And then there will be Text region from __init_begin to _einittext - 1. > > Is my understanding correct ? Ah I understand your problem now. Technically all the regions can be empty. I don't think add a page between is nice because we would end up to waste some space in memory. Instead, we should check if a region is empty, if it is then skip it. > >> >>> >>> A RW data region from __init_data_begin to __bss_end. Can we combine >>> the BSS section and init data section into one region ? >> >> If they have the same attribute then yes. > > ok, so my sections look like this (from readelf -DS) > > [ 0] NULL 0000000000000000 00000000 > 0000000000000000 0000000000000000 0 0 0 > [ 1] .text PROGBITS 0000000000200000 00000100 > 0000000000000832 0000000000000000 WAX 0 0 32 > [ 2] .rodata PROGBITS 0000000000201000 00001100 > 0000000000000d78 0000000000000000 A 0 0 256 > [ 3] .note.gnu.bu[...] NOTE 0000000000201d78 00001e78 > 0000000000000024 0000000000000000 A 0 0 4 > [ 4] .data.ro_aft[...] PROGBITS 0000000000202000 00003170 > 0000000000000000 0000000000000000 WA 0 0 1 > [ 5] .data PROGBITS 0000000000202000 00002100 > 0000000000000000 0000000000000000 WA 0 0 1 > [ 6] .init.text PROGBITS 0000000000202000 00002100 > 0000000000000048 0000000000000000 AX 0 0 4 > [ 7] .init.data PROGBITS 0000000000203000 00003100 > 0000000000000070 0000000000000000 A 0 0 1 > [ 8] .bss NOBITS 0000000000208000 00003170 > 0000000000000000 0000000000000000 WA 0 0 1 > > IIUC, [2] and [3] will be combined. > > [4] and [5] will be combined. > > So we will be creating 6 MPU regions in total. Is this correct ? I think you could combine [7] and [8] as well. So it would look like: 1. [1] .text 2. [2] .rodata [3] .note.gnu.bu[...] 3. [4] .data.ro_aft[...] [3] .data 4. [6] .init.text 4. [7] .init.data [8] .bss After boot, you would have: 1. [1] .text 2. [2] .rodata [3] .note.gnu.bu[...] [4] .data.ro_aft[...] 3. [3] .data 4. [8] .bss Cheers,
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..ef55c8765c --- /dev/null +++ b/xen/arch/arm/arm64/mpu/head.S @@ -0,0 +1,176 @@ +/* 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> + +#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 round up the section address to be PAGE_SIZE aligned + * Each section(e.g. .text, .data, etc) in xen.lds.S is page-aligned, + * which is usually guarded with ". = ALIGN(PAGE_SIZE)" in the head, + * or in the end + */ +.macro roundup_section, xb + add \xb, \xb, #(PAGE_SIZE-1) + and \xb, \xb, #PAGE_MASK +.endm + +/* + * 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 + * 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 + * + * Clobber \tmp1, \tmp2 + * + */ +.macro prepare_xen_region, sel, base, limit, prbar, prlar, tmp1, tmp2, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR + /* 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 + + /* Prepare value for PRLAR_EL2 reg and preserve it in \prlar.*/ + /* Round up limit address to be PAGE_SIZE aligned */ + roundup_section \limit + /* 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 + isb +.endm + +/* Load the physical address of a symbol into xb */ +.macro load_paddr xb, sym + ldr \xb, =\sym + add \xb, \xb, x20 /* x20 - Phys offset */ +.endm + +.section .text.header, "ax", %progbits + +/* + * Enable EL2 MPU and data cache + * If the Background region is enabled, then the MPU uses the default memory + * map as the Background region for generating the memory + * attributes when MPU is disabled. + * Since the default memory map of the Armv8-R AArch64 architecture is + * IMPLEMENTATION DEFINED, we intend to turn off the Background region here. + * + * Clobbers x0 + * + */ +FUNC_LOCAL(enable_mpu) + mrs x0, SCTLR_EL2 + orr x0, x0, #SCTLR_Axx_ELx_M /* Enable MPU */ + orr x0, x0, #SCTLR_Axx_ELx_C /* Enable D-cache */ + orr x0, x0, #SCTLR_Axx_ELx_WXN /* Enable WXN */ + dsb sy + msr SCTLR_EL2, x0 + isb + + ret +END(enable_mpu) + +/* + * Maps the various sections of Xen (described in xen.lds.S) as different MPU + * regions. Some of these regions will be marked read only while the others will + * be read write or execute. + * + * Inputs: + * lr : Address to return to. + * + * Clobbers x0 - x7 + * + */ +FUNC(enable_boot_cpu_mm) + mov x7, lr + + /* x0: region sel */ + mov x0, xzr + /* Xen text section. */ + load_paddr x1, _stext + load_paddr x2, _etext + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, attr_prbar=REGION_TEXT_PRBAR + + add x0, x0, #1 + /* Xen read-only data section. */ + load_paddr x1, _srodata + load_paddr x2, _erodata + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, attr_prbar=REGION_RO_PRBAR + + add x0, x0, #1 + /* Xen read-only after init data section. */ + load_paddr x1, __ro_after_init_start + load_paddr x2, __ro_after_init_end + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 + + add x0, x0, #1 + /* Xen read-write data section. */ + load_paddr x1, __ro_after_init_end + load_paddr x2, __init_begin + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 + + add x0, x0, #1 + /* Xen BSS section. */ + load_paddr x1, __bss_start + load_paddr x2, __bss_end + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 + + add x0, x0, #1 + /* Xen init text section. */ + load_paddr x1, __init_begin + load_paddr x2, __init_data_begin + prepare_xen_region x0, x1, x2, x3, x4, x5, x6, attr_prbar=REGION_TEXT_PRBAR + + add x0, x0, #1 + /* Xen init data section. */ + load_paddr x1, __init_data_begin + load_paddr x2, __init_end + prepare_xen_region x0, x1, x2, x3, x4, x5, x6 + + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ + mrs x1, MPUIR_EL2 + cmp x0, x1 + bgt 1f + bl enable_mpu + + mov lr, x7 + ret + +1: + PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n") + wfe + b 1b +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/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h index b593e4028b..8b6b373ce9 100644 --- a/xen/arch/arm/include/asm/arm64/sysregs.h +++ b/xen/arch/arm/include/asm/arm64/sysregs.h @@ -2,6 +2,9 @@ #define __ASM_ARM_ARM64_SYSREGS_H #include <xen/stringify.h> +#if defined(CONFIG_MPU) +#include <asm/arm64/mpu/sysregs.h> +#endif /* * GIC System register assembly aliases picked from kernel 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 bd884664ad..0b8956ac3c 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -157,6 +157,7 @@ SECTIONS *(.altinstr_replacement) } :text . = ALIGN(PAGE_SIZE); + __init_data_begin = .; .init.data : { *(.init.rodata) *(.init.rodata.*)
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 (Read/Write at EL2 as the data is RW during init) 4. RW data (Read/Write at EL2) 5. BSS (Read/Write at EL2) 6. Init Text (Read only at EL2, execution is permitted) 7. Init data (Read/Write at EL2) If the number of MPU regions created is greater than the count defined in MPUIR_EL2, then the boot fails. 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. 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). xen/arch/arm/Makefile | 1 + xen/arch/arm/arm64/mpu/Makefile | 1 + xen/arch/arm/arm64/mpu/head.S | 176 +++++++++++++++++++ xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 +++ xen/arch/arm/include/asm/arm64/sysregs.h | 3 + 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 + 9 files changed, 253 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/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