| Message ID | 20241010140351.309922-5-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable early bootup of AArch64 MPU systems | expand |
On 10/10/2024 15:03, Ayan Kumar Halder wrote: > Define enable_boot_cpu_mm() for the AArch64-V8R system. > > Like boot-time page table in MMU system, we need a boot-time MPU protection > region configuration in MPU system so Xen can fetch code and data from normal > memory. > > To do this, Xen maps the following sections of the binary as separate regions > (with permissions) :- > 1. Text (Read only at EL2, execution is permitted) > 2. RO data (Read only at EL2) > 3. RO after init data and RW data (Read/Write at EL2) > 4. Init Text (Read only at EL2, execution is permitted) > 5. Init data and BSS (Read/Write at EL2) > > Before creating a region, we check if the count exceeds the number defined in > MPUIR_EL2. If so, then the boot fails. > > Also we check if the region is empty or not. IOW, if the start and end address > of a section is the same, we skip mapping the region. > > To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers. > One can refer to ARM DDI 0600B.a ID062922 G1.3 "General System Control > Registers", to get the definitions of these registers. Also, refer to G1.2 > "Accessing MPU memory region registers", the following > > ``` > The MPU provides two register interfaces to program the MPU regions: > - Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and > PRLAR<n>_ELx. > ``` > > We use the above mechanism for mapping sections to MPU memory regions. > > MPU specific registers are defined in > xen/arch/arm/include/asm/arm64/mpu/sysregs.h. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > Changes from :- > > v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region, > we have separate MPU regions for different parts of the Xen binary. The reason > being different regions will nned different permissions (as mentioned in the > linker script). > > 2. Introduced a label (__init_data_begin) to mark the beginning of the init data > section. > > 3. Moved MPU specific register definitions to mpu/sysregs.h. > > 4. Fixed coding style issues. > > 5. Included page.h in mpu/head.S as page.h includes sysregs.h. > I haven't seen sysregs.h included directly from head.S or mmu/head.S. > (Outstanding comment not addressed). > > v2 - 1. Extracted "enable_mpu()" in a separate patch. > > 2. Removed alignment for limit address. > > 3. Merged some of the sections for preparing the early boot regions. > > 4. Checked for the max limit of MPU regions before creating a new region. > > 5. Checked for empty regions. > > xen/arch/arm/Makefile | 1 + > xen/arch/arm/arm64/mpu/Makefile | 1 + > xen/arch/arm/arm64/mpu/head.S | 130 +++++++++++++++++++ > xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 ++++ > xen/arch/arm/include/asm/mm.h | 2 + > xen/arch/arm/include/asm/mpu/arm64/mm.h | 22 ++++ > xen/arch/arm/include/asm/mpu/mm.h | 20 +++ > xen/arch/arm/xen.lds.S | 1 + > 8 files changed, 204 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/ This change is incorrect. The correct change should have been in :- --- a/xen/arch/arm/arm64/Makefile +++ b/xen/arch/arm/arm64/Makefile @@ -1,5 +1,6 @@ obj-y += lib/ obj-$(CONFIG_MMU) += mmu/ +obj-$(CONFIG_MPU) += mpu/ obj-y += cache.o I will wait for comments from reviewers before re-spinning the patch. - Ayan
Hi Ayan, > diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S > new file mode 100644 > index 0000000000..4a21bc815c > --- /dev/null > +++ b/xen/arch/arm/arm64/mpu/head.S > @@ -0,0 +1,130 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Start-of-day code for an Armv8-R MPU system. > + */ > + > +#include <asm/mm.h> > +#include <asm/arm64/mpu/sysregs.h> > + > +#define REGION_TEXT_PRBAR 0x38 /* SH=11 AP=10 XN=00 */ > +#define REGION_RO_PRBAR 0x3A /* SH=11 AP=10 XN=10 */ NIT alignment > +#define REGION_DATA_PRBAR 0x32 /* SH=11 AP=00 XN=10 */ > + > +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ > + > +/* > + * Macro to prepare and set a EL2 MPU memory region. > + * We will also create an according MPU memory region entry, which > + * is a structure of pr_t, in table \prmap. > + * > + * Inputs: > + * sel: region selector > + * base: reg storing base address (should be page-aligned) > + * limit: reg storing limit address > + * prbar: store computed PRBAR_EL2 value > + * prlar: store computed PRLAR_EL2 value > + * maxcount: maximum number of EL2 regions supported > + * attr_prbar: PRBAR_EL2-related memory attributes. If not specified it will be > + * REGION_DATA_PRBAR > + * attr_prlar: PRLAR_EL2-related memory attributes. If not specified it will be > + * REGION_NORMAL_PRLAR > + */ > +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR > + > + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ > + add \sel, \sel, #1 I think there is an issue adding 1 here, because the very first region we are going to fill will be the 1st even if we intended the 0th. Probably moving this one at the end will fix the issue > + cmp \sel, \maxcount > + bgt fail > + > + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ > + and \base, \base, #MPU_REGION_MASK > + mov \prbar, #\attr_prbar > + orr \prbar, \prbar, \base > + > + /* Limit address should be inclusive */ > + sub \limit, \limit, #1 > + and \limit, \limit, #MPU_REGION_MASK > + mov \prlar, #\attr_prlar > + orr \prlar, \prlar, \limit > + > + msr PRSELR_EL2, \sel > + isb > + msr PRBAR_EL2, \prbar > + msr PRLAR_EL2, \prlar > + dsb sy > + isb > +.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 > + > +/* > + * Maps the various sections of Xen (described in xen.lds.S) as different MPU > + * regions. > + * > + * Inputs: > + * lr : Address to return to. > + * > + * Clobbers x0 - x5 > + * > + */ > +FUNC(enable_boot_cpu_mm) > + > + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ > + mrs x5, MPUIR_EL2 > + > + /* x0: region sel */ > + mov x0, xzr > + /* Xen text section. */ > + load_paddr x1, _stext > + load_paddr x2, _etext > + cmp x1, x2 > + beq 1f > + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR > + > +1: /* Xen read-only data section. */ > + load_paddr x1, _srodata > + load_paddr x2, _erodata > + cmp x1, x2 > + beq 2f > + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR > + > +2: /* Xen read-only after init and data section. (RW data) */ > + load_paddr x1, __ro_after_init_start > + load_paddr x2, __init_begin > + cmp x1, x2 > + beq 3f > + prepare_xen_region x0, x1, x2, x3, x4, x5 > + > +3: /* Xen code section. */ > + load_paddr x1, __init_begin > + load_paddr x2, __init_data_begin > + cmp x1, x2 > + beq 4f > + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR > + > +4: /* Xen data and BSS section. */ > + load_paddr x1, __init_data_begin > + load_paddr x2, __bss_end > + cmp x1, x2 > + beq 5f > + prepare_xen_region x0, x1, x2, x3, x4, x5 > + > +5: > + ret > + > +fail: > + 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 Same comment about define name as in patch 3, here and in every new file of this patch Cheers, Luca
On 14/10/2024 20:03, Luca Fancellu wrote: > Hi Ayan, Hi Luca, > > >> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S >> new file mode 100644 >> index 0000000000..4a21bc815c >> --- /dev/null >> +++ b/xen/arch/arm/arm64/mpu/head.S >> @@ -0,0 +1,130 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Start-of-day code for an Armv8-R MPU system. >> + */ >> + >> +#include <asm/mm.h> >> +#include <asm/arm64/mpu/sysregs.h> >> + >> +#define REGION_TEXT_PRBAR 0x38 /* SH=11 AP=10 XN=00 */ >> +#define REGION_RO_PRBAR 0x3A /* SH=11 AP=10 XN=10 */ > NIT alignment Ack > >> +#define REGION_DATA_PRBAR 0x32 /* SH=11 AP=00 XN=10 */ >> + >> +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ >> + >> +/* >> + * Macro to prepare and set a EL2 MPU memory region. >> + * We will also create an according MPU memory region entry, which >> + * is a structure of pr_t, in table \prmap. >> + * >> + * Inputs: >> + * sel: region selector >> + * base: reg storing base address (should be page-aligned) >> + * limit: reg storing limit address >> + * prbar: store computed PRBAR_EL2 value >> + * prlar: store computed PRLAR_EL2 value >> + * maxcount: maximum number of EL2 regions supported >> + * attr_prbar: PRBAR_EL2-related memory attributes. If not specified it will be >> + * REGION_DATA_PRBAR >> + * attr_prlar: PRLAR_EL2-related memory attributes. If not specified it will be >> + * REGION_NORMAL_PRLAR >> + */ >> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >> + >> + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ >> + add \sel, \sel, #1 > I think there is an issue adding 1 here, because the very first region we are going to fill will be the 1st even if we intended the 0th. > Probably moving this one at the end will fix the issue We are also using 'sel' to compare against the maximum number of regions supported. So, for the first region it needs to be 1 otherwise there is a risk of comparing 0 (ie first region) with 0 (max supported regions). May be what I can do is ... > >> + cmp \sel, \maxcount >> + bgt fail >> + >> + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ >> + and \base, \base, #MPU_REGION_MASK >> + mov \prbar, #\attr_prbar >> + orr \prbar, \prbar, \base >> + >> + /* Limit address should be inclusive */ >> + sub \limit, \limit, #1 >> + and \limit, \limit, #MPU_REGION_MASK >> + mov \prlar, #\attr_prlar >> + orr \prlar, \prlar, \limit >> + /* Regions should start from 0 */ sub \sel, \sel, #1 - Ayan
Hi Ayan, On 15/10/2024 16:56, Ayan Kumar Halder wrote: > > On 14/10/2024 20:03, Luca Fancellu wrote: >> Hi Ayan, > Hi Luca, >> >> >>> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/ >>> head.S >>> new file mode 100644 >>> index 0000000000..4a21bc815c >>> --- /dev/null >>> +++ b/xen/arch/arm/arm64/mpu/head.S >>> @@ -0,0 +1,130 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/* >>> + * Start-of-day code for an Armv8-R MPU system. >>> + */ >>> + >>> +#include <asm/mm.h> >>> +#include <asm/arm64/mpu/sysregs.h> >>> + >>> +#define REGION_TEXT_PRBAR 0x38 /* SH=11 AP=10 XN=00 */ >>> +#define REGION_RO_PRBAR 0x3A /* SH=11 AP=10 XN=10 */ >> NIT alignment > Ack >> >>> +#define REGION_DATA_PRBAR 0x32 /* SH=11 AP=00 XN=10 */ >>> + >>> +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ >>> + >>> +/* >>> + * Macro to prepare and set a EL2 MPU memory region. >>> + * We will also create an according MPU memory region entry, which >>> + * is a structure of pr_t, in table \prmap. >>> + * >>> + * Inputs: >>> + * sel: region selector >>> + * base: reg storing base address (should be page-aligned) >>> + * limit: reg storing limit address >>> + * prbar: store computed PRBAR_EL2 value >>> + * prlar: store computed PRLAR_EL2 value >>> + * maxcount: maximum number of EL2 regions supported >>> + * attr_prbar: PRBAR_EL2-related memory attributes. If not >>> specified it will be >>> + * REGION_DATA_PRBAR >>> + * attr_prlar: PRLAR_EL2-related memory attributes. If not >>> specified it will be >>> + * REGION_NORMAL_PRLAR >>> + */ >>> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, >>> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >>> + >>> + /* Check if the number of regions exceeded the count specified >>> in MPUIR_EL2 */ >>> + add \sel, \sel, #1 >> I think there is an issue adding 1 here, because the very first region >> we are going to fill will be the 1st even if we intended the 0th. >> Probably moving this one at the end will fix the issue > > We are also using 'sel' to compare against the maximum number of regions > supported. So, for the first region it needs to be 1 otherwise there is > a risk of comparing 0 (ie first region) with 0 (max supported regions). > > May be what I can do is ... > >> >>> + cmp \sel, \maxcount >>> + bgt fail >>> + >>> + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ >>> + and \base, \base, #MPU_REGION_MASK >>> + mov \prbar, #\attr_prbar >>> + orr \prbar, \prbar, \base >>> + >>> + /* Limit address should be inclusive */ >>> + sub \limit, \limit, #1 >>> + and \limit, \limit, #MPU_REGION_MASK >>> + mov \prlar, #\attr_prlar >>> + orr \prlar, \prlar, \limit >>> + > > /* Regions should start from 0 */ > > sub \sel, \sel, #1 I didn't review the full patch yet. But couldn't we use ``bge``? This would cover "maxcount == 0" and avoid to increment and then decrement \sel. Cheers,
On 15/10/2024 17:51, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 15/10/2024 16:56, Ayan Kumar Halder wrote: >> >> On 14/10/2024 20:03, Luca Fancellu wrote: >>> Hi Ayan, >> Hi Luca, >>> >>> >>>> diff --git a/xen/arch/arm/arm64/mpu/head.S >>>> b/xen/arch/arm/arm64/mpu/ head.S >>>> new file mode 100644 >>>> index 0000000000..4a21bc815c >>>> --- /dev/null >>>> +++ b/xen/arch/arm/arm64/mpu/head.S >>>> @@ -0,0 +1,130 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>> +/* >>>> + * Start-of-day code for an Armv8-R MPU system. >>>> + */ >>>> + >>>> +#include <asm/mm.h> >>>> +#include <asm/arm64/mpu/sysregs.h> >>>> + >>>> +#define REGION_TEXT_PRBAR 0x38 /* SH=11 AP=10 XN=00 */ >>>> +#define REGION_RO_PRBAR 0x3A /* SH=11 AP=10 XN=10 */ >>> NIT alignment >> Ack >>> >>>> +#define REGION_DATA_PRBAR 0x32 /* SH=11 AP=00 XN=10 */ >>>> + >>>> +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ >>>> + >>>> +/* >>>> + * Macro to prepare and set a EL2 MPU memory region. >>>> + * We will also create an according MPU memory region entry, which >>>> + * is a structure of pr_t, in table \prmap. >>>> + * >>>> + * Inputs: >>>> + * sel: region selector >>>> + * base: reg storing base address (should be page-aligned) >>>> + * limit: reg storing limit address >>>> + * prbar: store computed PRBAR_EL2 value >>>> + * prlar: store computed PRLAR_EL2 value >>>> + * maxcount: maximum number of EL2 regions supported >>>> + * attr_prbar: PRBAR_EL2-related memory attributes. If not >>>> specified it will be >>>> + * REGION_DATA_PRBAR >>>> + * attr_prlar: PRLAR_EL2-related memory attributes. If not >>>> specified it will be >>>> + * REGION_NORMAL_PRLAR >>>> + */ >>>> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, >>>> maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >>>> + >>>> + /* Check if the number of regions exceeded the count specified >>>> in MPUIR_EL2 */ >>>> + add \sel, \sel, #1 >>> I think there is an issue adding 1 here, because the very first >>> region we are going to fill will be the 1st even if we intended the >>> 0th. >>> Probably moving this one at the end will fix the issue >> >> We are also using 'sel' to compare against the maximum number of >> regions supported. So, for the first region it needs to be 1 >> otherwise there is a risk of comparing 0 (ie first region) with 0 >> (max supported regions). >> >> May be what I can do is ... >> >>> >>>> + cmp \sel, \maxcount >>>> + bgt fail >>>> + >>>> + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ >>>> + and \base, \base, #MPU_REGION_MASK >>>> + mov \prbar, #\attr_prbar >>>> + orr \prbar, \prbar, \base >>>> + >>>> + /* Limit address should be inclusive */ >>>> + sub \limit, \limit, #1 >>>> + and \limit, \limit, #MPU_REGION_MASK >>>> + mov \prlar, #\attr_prlar >>>> + orr \prlar, \prlar, \limit >>>> + >> >> /* Regions should start from 0 */ >> >> sub \sel, \sel, #1 > > I didn't review the full patch yet. But couldn't we use ``bge``? This > would cover "maxcount == 0" and avoid to increment and then decrement > \sel. Oh yes. My bad , I missed this. - Ayan
Hi Ayan, On 10/10/2024 15:03, Ayan Kumar Halder wrote: > 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..4a21bc815c > --- /dev/null > +++ b/xen/arch/arm/arm64/mpu/head.S > @@ -0,0 +1,130 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Start-of-day code for an Armv8-R MPU system. > + */ > + > +#include <asm/mm.h> > +#include <asm/arm64/mpu/sysregs.h> > + > +#define REGION_TEXT_PRBAR 0x38 /* SH=11 AP=10 XN=00 */ > +#define REGION_RO_PRBAR 0x3A /* SH=11 AP=10 XN=10 */ > +#define REGION_DATA_PRBAR 0x32 /* SH=11 AP=00 XN=10 */ > + > +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ > + > +/* > + * Macro to prepare and set a EL2 MPU memory region. > + * We will also create an according MPU memory region entry, which > + * is a structure of pr_t, in table \prmap. > + * > + * Inputs: > + * sel: region selector > + * base: reg storing base address (should be page-aligned) > + * limit: reg storing limit address > + * prbar: store computed PRBAR_EL2 value > + * prlar: store computed PRLAR_EL2 value > + * maxcount: maximum number of EL2 regions supported > + * attr_prbar: PRBAR_EL2-related memory attributes. If not specified it will be > + * REGION_DATA_PRBAR > + * attr_prlar: PRLAR_EL2-related memory attributes. If not specified it will be > + * REGION_NORMAL_PRLAR > + */ > +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR > + > + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ > + add \sel, \sel, #1 > + cmp \sel, \maxcount > + bgt fail > + > + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ > + and \base, \base, #MPU_REGION_MASK > + mov \prbar, #\attr_prbar > + orr \prbar, \prbar, \base > + > + /* Limit address should be inclusive */ > + sub \limit, \limit, #1 > + and \limit, \limit, #MPU_REGION_MASK > + mov \prlar, #\attr_prlar > + orr \prlar, \prlar, \limit > + > + msr PRSELR_EL2, \sel > + isb > + msr PRBAR_EL2, \prbar > + msr PRLAR_EL2, \prlar > + dsb sy > + isb > +.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 */ Sorry I didn't spot this until now. Xen will be linked to a specific physical address, so why do we need to add an offset? > +.endm > + > +/* > + * Maps the various sections of Xen (described in xen.lds.S) as different MPU > + * regions. > + * > + * Inputs: > + * lr : Address to return to. > + * > + * Clobbers x0 - x5 > + * > + */ > +FUNC(enable_boot_cpu_mm) > + > + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ AFAIU, this doesn't match what the instruction is doing below. > + mrs x5, MPUIR_EL2 > + > + /* x0: region sel */ > + mov x0, xzr > + /* Xen text section. */ > + load_paddr x1, _stext > + load_paddr x2, _etext > + cmp x1, x2 > + beq 1f This check seems to be excessive... I can't think of a reason why there would be no text at all... Same for a lot of the checks below. > + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR > + > +1: /* Xen read-only data section. */ > + load_paddr x1, _srodata > + load_paddr x2, _erodata > + cmp x1, x2 > + beq 2f > + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR > + > +2: /* Xen read-only after init and data section. (RW data) */ > + load_paddr x1, __ro_after_init_start > + load_paddr x2, __init_begin > + cmp x1, x2 > + beq 3f > + prepare_xen_region x0, x1, x2, x3, x4, x5 > + > +3: /* Xen code section. */ > + load_paddr x1, __init_begin > + load_paddr x2, __init_data_begin > + cmp x1, x2 > + beq 4f > + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR > + > +4: /* Xen data and BSS section. */ > + load_paddr x1, __init_data_begin > + load_paddr x2, __bss_end > + cmp x1, x2 > + beq 5f > + prepare_xen_region x0, x1, x2, x3, x4, x5 > + > +5: > + ret > + > +fail: This name is a bit too generic given you use within a macro. Also, I think it should be its own local function because the macro can be used anywhere. Cheers,
On 18/10/2024 23:13, Julien Grall wrote: > Hi Ayan, Hi Julien, Just one clarification. > > On 10/10/2024 15:03, Ayan Kumar Halder wrote: >> 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..4a21bc815c >> --- /dev/null >> +++ b/xen/arch/arm/arm64/mpu/head.S >> @@ -0,0 +1,130 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Start-of-day code for an Armv8-R MPU system. >> + */ >> + >> +#include <asm/mm.h> >> +#include <asm/arm64/mpu/sysregs.h> >> + >> +#define REGION_TEXT_PRBAR 0x38 /* SH=11 AP=10 XN=00 */ >> +#define REGION_RO_PRBAR 0x3A /* SH=11 AP=10 XN=10 */ >> +#define REGION_DATA_PRBAR 0x32 /* SH=11 AP=00 XN=10 */ >> + >> +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ >> + >> +/* >> + * Macro to prepare and set a EL2 MPU memory region. >> + * We will also create an according MPU memory region entry, which >> + * is a structure of pr_t, in table \prmap. >> + * >> + * Inputs: >> + * sel: region selector >> + * base: reg storing base address (should be page-aligned) >> + * limit: reg storing limit address >> + * prbar: store computed PRBAR_EL2 value >> + * prlar: store computed PRLAR_EL2 value >> + * maxcount: maximum number of EL2 regions supported >> + * attr_prbar: PRBAR_EL2-related memory attributes. If not >> specified it will be >> + * REGION_DATA_PRBAR >> + * attr_prlar: PRLAR_EL2-related memory attributes. If not >> specified it will be >> + * REGION_NORMAL_PRLAR >> + */ >> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, >> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >> + >> + /* Check if the number of regions exceeded the count specified >> in MPUIR_EL2 */ >> + add \sel, \sel, #1 >> + cmp \sel, \maxcount >> + bgt fail >> + >> + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ >> + and \base, \base, #MPU_REGION_MASK >> + mov \prbar, #\attr_prbar >> + orr \prbar, \prbar, \base >> + >> + /* Limit address should be inclusive */ >> + sub \limit, \limit, #1 >> + and \limit, \limit, #MPU_REGION_MASK >> + mov \prlar, #\attr_prlar >> + orr \prlar, \prlar, \limit >> + >> + msr PRSELR_EL2, \sel >> + isb >> + msr PRBAR_EL2, \prbar >> + msr PRLAR_EL2, \prlar >> + dsb sy >> + isb >> +.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 */ > > Sorry I didn't spot this until now. Xen will be linked to a specific > physical address, so why do we need to add an offset? Yes, this needs to be removed. x20 contains 0. > >> +.endm >> + >> +/* >> + * Maps the various sections of Xen (described in xen.lds.S) as >> different MPU >> + * regions. >> + * >> + * Inputs: >> + * lr : Address to return to. >> + * >> + * Clobbers x0 - x5 >> + * >> + */ >> +FUNC(enable_boot_cpu_mm) >> + >> + /* Check if the number of regions exceeded the count specified >> in MPUIR_EL2 */ > > AFAIU, this doesn't match what the instruction is doing below. Sorry, this needs to be removed. > >> + mrs x5, MPUIR_EL2 >> + >> + /* x0: region sel */ >> + mov x0, xzr >> + /* Xen text section. */ >> + load_paddr x1, _stext >> + load_paddr x2, _etext >> + cmp x1, x2 >> + beq 1f > > This check seems to be excessive... I can't think of a reason why > there would be no text at all... Same for a lot of the checks below. Is it ok if we have this excess check ? The downsides are only a small increase in code size and boot time. Otherwise, I need to justify why we have this checks in some places, but not in others. > >> + prepare_xen_region x0, x1, x2, x3, x4, x5, >> attr_prbar=REGION_TEXT_PRBAR >> + >> +1: /* Xen read-only data section. */ >> + load_paddr x1, _srodata >> + load_paddr x2, _erodata >> + cmp x1, x2 >> + beq 2f >> + prepare_xen_region x0, x1, x2, x3, x4, x5, >> attr_prbar=REGION_RO_PRBAR >> + >> +2: /* Xen read-only after init and data section. (RW data) */ >> + load_paddr x1, __ro_after_init_start >> + load_paddr x2, __init_begin >> + cmp x1, x2 >> + beq 3f >> + prepare_xen_region x0, x1, x2, x3, x4, x5 >> + >> +3: /* Xen code section. */ >> + load_paddr x1, __init_begin >> + load_paddr x2, __init_data_begin >> + cmp x1, x2 >> + beq 4f >> + prepare_xen_region x0, x1, x2, x3, x4, x5, >> attr_prbar=REGION_TEXT_PRBAR >> + >> +4: /* Xen data and BSS section. */ >> + load_paddr x1, __init_data_begin >> + load_paddr x2, __bss_end >> + cmp x1, x2 >> + beq 5f >> + prepare_xen_region x0, x1, x2, x3, x4, x5 >> + >> +5: >> + ret >> + >> +fail: > > This name is a bit too generic given you use within a macro. Also, I > think it should be its own local function because the macro can be > used anywhere. Ack. I will convert this to a function. - Ayan
Hi Ayan, On 21/10/2024 16:07, Ayan Kumar Halder wrote: > > On 18/10/2024 23:13, Julien Grall wrote: >> Hi Ayan, >>> +.endm >>> + >>> +/* >>> + * Maps the various sections of Xen (described in xen.lds.S) as >>> different MPU >>> + * regions. >>> + * >>> + * Inputs: >>> + * lr : Address to return to. >>> + * >>> + * Clobbers x0 - x5 >>> + * >>> + */ >>> +FUNC(enable_boot_cpu_mm) >>> + >>> + /* Check if the number of regions exceeded the count specified >>> in MPUIR_EL2 */ >> >> AFAIU, this doesn't match what the instruction is doing below. > Sorry, this needs to be removed. >> >>> + mrs x5, MPUIR_EL2 >>> + >>> + /* x0: region sel */ >>> + mov x0, xzr >>> + /* Xen text section. */ >>> + load_paddr x1, _stext >>> + load_paddr x2, _etext >>> + cmp x1, x2 >>> + beq 1f >> >> This check seems to be excessive... I can't think of a reason why >> there would be no text at all... Same for a lot of the checks below. > Is it ok if we have this excess check ? The downsides are only a small > increase in code size and boot time. Otherwise, I need to justify why we > have this checks in some places, but not in others. How about moving the check in prepare_xen_region? This should solve the concerns on both the current approach and my proposed approach. 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..4a21bc815c --- /dev/null +++ b/xen/arch/arm/arm64/mpu/head.S @@ -0,0 +1,130 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Start-of-day code for an Armv8-R MPU system. + */ + +#include <asm/mm.h> +#include <asm/arm64/mpu/sysregs.h> + +#define REGION_TEXT_PRBAR 0x38 /* SH=11 AP=10 XN=00 */ +#define REGION_RO_PRBAR 0x3A /* SH=11 AP=10 XN=10 */ +#define REGION_DATA_PRBAR 0x32 /* SH=11 AP=00 XN=10 */ + +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ + +/* + * Macro to prepare and set a EL2 MPU memory region. + * We will also create an according MPU memory region entry, which + * is a structure of pr_t, in table \prmap. + * + * Inputs: + * sel: region selector + * base: reg storing base address (should be page-aligned) + * limit: reg storing limit address + * prbar: store computed PRBAR_EL2 value + * prlar: store computed PRLAR_EL2 value + * maxcount: maximum number of EL2 regions supported + * attr_prbar: PRBAR_EL2-related memory attributes. If not specified it will be + * REGION_DATA_PRBAR + * attr_prlar: PRLAR_EL2-related memory attributes. If not specified it will be + * REGION_NORMAL_PRLAR + */ +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR + + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ + add \sel, \sel, #1 + cmp \sel, \maxcount + bgt fail + + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ + and \base, \base, #MPU_REGION_MASK + mov \prbar, #\attr_prbar + orr \prbar, \prbar, \base + + /* Limit address should be inclusive */ + sub \limit, \limit, #1 + and \limit, \limit, #MPU_REGION_MASK + mov \prlar, #\attr_prlar + orr \prlar, \prlar, \limit + + msr PRSELR_EL2, \sel + isb + msr PRBAR_EL2, \prbar + msr PRLAR_EL2, \prlar + dsb sy + isb +.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 + +/* + * Maps the various sections of Xen (described in xen.lds.S) as different MPU + * regions. + * + * Inputs: + * lr : Address to return to. + * + * Clobbers x0 - x5 + * + */ +FUNC(enable_boot_cpu_mm) + + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ + mrs x5, MPUIR_EL2 + + /* x0: region sel */ + mov x0, xzr + /* Xen text section. */ + load_paddr x1, _stext + load_paddr x2, _etext + cmp x1, x2 + beq 1f + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR + +1: /* Xen read-only data section. */ + load_paddr x1, _srodata + load_paddr x2, _erodata + cmp x1, x2 + beq 2f + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR + +2: /* Xen read-only after init and data section. (RW data) */ + load_paddr x1, __ro_after_init_start + load_paddr x2, __init_begin + cmp x1, x2 + beq 3f + prepare_xen_region x0, x1, x2, x3, x4, x5 + +3: /* Xen code section. */ + load_paddr x1, __init_begin + load_paddr x2, __init_data_begin + cmp x1, x2 + beq 4f + prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR + +4: /* Xen data and BSS section. */ + load_paddr x1, __init_data_begin + load_paddr x2, __bss_end + cmp x1, x2 + beq 5f + prepare_xen_region x0, x1, x2, x3, x4, x5 + +5: + ret + +fail: + 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/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 fe4b468cca..2c9b5ee238 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 and RW data (Read/Write at EL2) 4. Init Text (Read only at EL2, execution is permitted) 5. Init data and BSS (Read/Write at EL2) Before creating a region, we check if the count exceeds the number defined in MPUIR_EL2. If so, then the boot fails. Also we check if the region is empty or not. IOW, if the start and end address of a section is the same, we skip mapping the region. To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers. One can refer to ARM DDI 0600B.a ID062922 G1.3 "General System Control Registers", to get the definitions of these registers. Also, refer to G1.2 "Accessing MPU memory region registers", the following ``` The MPU provides two register interfaces to program the MPU regions: - Access to any of the MPU regions via PRSELR_ELx, PRBAR<n>_ELx, and PRLAR<n>_ELx. ``` We use the above mechanism for mapping sections to MPU memory regions. MPU specific registers are defined in xen/arch/arm/include/asm/arm64/mpu/sysregs.h. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from :- v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region, we have separate MPU regions for different parts of the Xen binary. The reason being different regions will nned different permissions (as mentioned in the linker script). 2. Introduced a label (__init_data_begin) to mark the beginning of the init data section. 3. Moved MPU specific register definitions to mpu/sysregs.h. 4. Fixed coding style issues. 5. Included page.h in mpu/head.S as page.h includes sysregs.h. I haven't seen sysregs.h included directly from head.S or mmu/head.S. (Outstanding comment not addressed). v2 - 1. Extracted "enable_mpu()" in a separate patch. 2. Removed alignment for limit address. 3. Merged some of the sections for preparing the early boot regions. 4. Checked for the max limit of MPU regions before creating a new region. 5. Checked for empty regions. xen/arch/arm/Makefile | 1 + xen/arch/arm/arm64/mpu/Makefile | 1 + xen/arch/arm/arm64/mpu/head.S | 130 +++++++++++++++++++ xen/arch/arm/include/asm/arm64/mpu/sysregs.h | 27 ++++ xen/arch/arm/include/asm/mm.h | 2 + xen/arch/arm/include/asm/mpu/arm64/mm.h | 22 ++++ xen/arch/arm/include/asm/mpu/mm.h | 20 +++ xen/arch/arm/xen.lds.S | 1 + 8 files changed, 204 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