| Message ID | 20250407184443.1790995-2-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable early bootup of Armv8-R AArch32 systems | expand |
On 07/04/2025 20:44, Ayan Kumar Halder wrote: > Added a new file common.inc to hold the common earlyboot MPU regions NIT: Describe your changes in imperative mood Also, my understanding was that this file will contain common constructs not only regions related. If this is just for regions, then it's better to name it regions.inc. > configurations across arm64 and arm32. > > prepare_xen_region, fail_insufficient_regions() will be used by both arm32 and > arm64. Thus, they have been moved to common.inc. > > *_PRBAR are moved to arm64/sysregs.h. > *_PRLAR are moved to common.inc as they are common between arm32 and arm64. > > Introduce WRITE_SYSREG_ASM to write to the system registers from the common asm > file. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > > Changes from > > v1 - > > 1. enable_mpu() now sets HMAIR{0,1} registers. This is similar to what is > being done in enable_mmu(). All the mm related configurations happen in this > function. > > 2. Fixed some typos. > > v2 - > 1. Extracted the arm64 head.S functions/macros in a common file. > > v3 - > 1. Moved *_PRLAR are moved to prepare_xen_region.inc > > 2. enable_boot_cpu_mm() is preserved in mpu/head.S. > > 3. STORE_SYSREG is renamed as WRITE_SYSREG_ASM() > > 4. LOAD_SYSREG is removed. > > 5. No need to save/restore lr in enable_boot_cpu_mm(). IOW, keep it as it was > in the original code. > > v4 - > 1. Rename prepare_xen_region.inc to common.inc > > 2. enable_secondary_cpu_mm() is moved back to mpu/head.S. > > xen/arch/arm/arm64/mpu/head.S | 78 +---------------------- > xen/arch/arm/include/asm/arm64/sysregs.h | 11 ++++ > xen/arch/arm/include/asm/mpu/common.inc | 79 ++++++++++++++++++++++++ > 3 files changed, 91 insertions(+), 77 deletions(-) > create mode 100644 xen/arch/arm/include/asm/mpu/common.inc > > diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S > index ed01993d85..4d76a3166e 100644 > --- a/xen/arch/arm/arm64/mpu/head.S > +++ b/xen/arch/arm/arm64/mpu/head.S > @@ -3,83 +3,7 @@ > * Start-of-day code for an Armv8-R MPU system. > */ > > -#include <asm/early_printk.h> > -#include <asm/mpu.h> > - > -/* Backgroud region enable/disable */ > -#define SCTLR_ELx_BR BIT(17, UL) > - > -#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_DEVICE_PRBAR 0x22 /* SH=10 AP=00 XN=10 */ > - > -#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ > -#define REGION_DEVICE_PRLAR 0x09 /* NS=0 ATTR=100 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. > - * > - * sel: region selector > - * base: reg storing base address > - * 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 > - * > - * Preserves \maxcount > - * Output: > - * \sel: Next available region selector index. > - * Clobbers \base, \limit, \prbar, \prlar > - * > - * Note that all parameters using registers should be distinct. > - */ > -.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR > - /* Check if the region is empty */ > - cmp \base, \limit > - beq 1f > - > - /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ > - cmp \sel, \maxcount > - bge fail_insufficient_regions > - > - /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ > - and \base, \base, #MPU_REGION_MASK > - mov \prbar, #\attr_prbar > - orr \prbar, \prbar, \base > - > - /* Limit address should be inclusive */ > - sub \limit, \limit, #1 > - and \limit, \limit, #MPU_REGION_MASK > - mov \prlar, #\attr_prlar > - orr \prlar, \prlar, \limit > - > - msr PRSELR_EL2, \sel > - isb > - msr PRBAR_EL2, \prbar > - msr PRLAR_EL2, \prlar > - dsb sy > - isb > - > - add \sel, \sel, #1 > - > -1: > -.endm > - > -/* > - * Failure caused due to insufficient MPU regions. > - */ > -FUNC_LOCAL(fail_insufficient_regions) > - PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n") > -1: wfe > - b 1b > -END(fail_insufficient_regions) > +#include <asm/mpu/common.inc> > > /* > * Enable EL2 MPU and data cache > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h > index b593e4028b..3ee3715430 100644 > --- a/xen/arch/arm/include/asm/arm64/sysregs.h > +++ b/xen/arch/arm/include/asm/arm64/sysregs.h > @@ -462,6 +462,15 @@ > #define ZCR_ELx_LEN_SIZE 9 > #define ZCR_ELx_LEN_MASK 0x1ff > > +#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_DEVICE_PRBAR 0x22 /* SH=10 AP=00 XN=10 */ > + > +#define WRITE_SYSREG_ASM(v, name) "msr " __stringify(name,) #v; 1) This macro is ASM only and you don't protect it like the C macros below. Why? 2) Semicolon not needed at the end ~Michal
Hi Ayan, > On 7 Apr 2025, at 19:44, Ayan Kumar Halder <ayan.kumar.halder@amd.com> wrote: > > Added a new file common.inc to hold the common earlyboot MPU regions > configurations across arm64 and arm32. > > prepare_xen_region, fail_insufficient_regions() will be used by both arm32 and > arm64. Thus, they have been moved to common.inc. > > *_PRBAR are moved to arm64/sysregs.h. > *_PRLAR are moved to common.inc as they are common between arm32 and arm64. > > Introduce WRITE_SYSREG_ASM to write to the system registers from the common asm > file. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > — > Provided that you fix Michal’s comment about file name (even if {mpu-}common.inc is more flexible and would not need any renaming in case additional code is added that doesn’t involve region manipulation, but at this stage I can’t foresee any) and protect the asm macro: Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Please keep it if you change the code for the above comment. Cheers, Luca
Hi, On 08/04/2025 08:33, Orzel, Michal wrote: > > On 07/04/2025 20:44, Ayan Kumar Halder wrote: >> Added a new file common.inc to hold the common earlyboot MPU regions > NIT: Describe your changes in imperative mood > > Also, my understanding was that this file will contain common constructs not > only regions related. If this is just for regions, then it's better to name it > regions.inc. > >> configurations across arm64 and arm32. >> >> prepare_xen_region, fail_insufficient_regions() will be used by both arm32 and >> arm64. Thus, they have been moved to common.inc. >> >> *_PRBAR are moved to arm64/sysregs.h. >> *_PRLAR are moved to common.inc as they are common between arm32 and arm64. >> >> Introduce WRITE_SYSREG_ASM to write to the system registers from the common asm >> file. >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> >> Changes from >> >> v1 - >> >> 1. enable_mpu() now sets HMAIR{0,1} registers. This is similar to what is >> being done in enable_mmu(). All the mm related configurations happen in this >> function. >> >> 2. Fixed some typos. >> >> v2 - >> 1. Extracted the arm64 head.S functions/macros in a common file. >> >> v3 - >> 1. Moved *_PRLAR are moved to prepare_xen_region.inc >> >> 2. enable_boot_cpu_mm() is preserved in mpu/head.S. >> >> 3. STORE_SYSREG is renamed as WRITE_SYSREG_ASM() >> >> 4. LOAD_SYSREG is removed. >> >> 5. No need to save/restore lr in enable_boot_cpu_mm(). IOW, keep it as it was >> in the original code. >> >> v4 - >> 1. Rename prepare_xen_region.inc to common.inc >> >> 2. enable_secondary_cpu_mm() is moved back to mpu/head.S. >> >> xen/arch/arm/arm64/mpu/head.S | 78 +---------------------- >> xen/arch/arm/include/asm/arm64/sysregs.h | 11 ++++ >> xen/arch/arm/include/asm/mpu/common.inc | 79 ++++++++++++++++++++++++ >> 3 files changed, 91 insertions(+), 77 deletions(-) >> create mode 100644 xen/arch/arm/include/asm/mpu/common.inc >> >> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S >> index ed01993d85..4d76a3166e 100644 >> --- a/xen/arch/arm/arm64/mpu/head.S >> +++ b/xen/arch/arm/arm64/mpu/head.S >> @@ -3,83 +3,7 @@ >> * Start-of-day code for an Armv8-R MPU system. >> */ >> >> -#include <asm/early_printk.h> >> -#include <asm/mpu.h> >> - >> -/* Backgroud region enable/disable */ >> -#define SCTLR_ELx_BR BIT(17, UL) >> - >> -#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_DEVICE_PRBAR 0x22 /* SH=10 AP=00 XN=10 */ >> - >> -#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ >> -#define REGION_DEVICE_PRLAR 0x09 /* NS=0 ATTR=100 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. >> - * >> - * sel: region selector >> - * base: reg storing base address >> - * 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 >> - * >> - * Preserves \maxcount >> - * Output: >> - * \sel: Next available region selector index. >> - * Clobbers \base, \limit, \prbar, \prlar >> - * >> - * Note that all parameters using registers should be distinct. >> - */ >> -.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR >> - /* Check if the region is empty */ >> - cmp \base, \limit >> - beq 1f >> - >> - /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ >> - cmp \sel, \maxcount >> - bge fail_insufficient_regions >> - >> - /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ >> - and \base, \base, #MPU_REGION_MASK >> - mov \prbar, #\attr_prbar >> - orr \prbar, \prbar, \base >> - >> - /* Limit address should be inclusive */ >> - sub \limit, \limit, #1 >> - and \limit, \limit, #MPU_REGION_MASK >> - mov \prlar, #\attr_prlar >> - orr \prlar, \prlar, \limit >> - >> - msr PRSELR_EL2, \sel >> - isb >> - msr PRBAR_EL2, \prbar >> - msr PRLAR_EL2, \prlar >> - dsb sy >> - isb >> - >> - add \sel, \sel, #1 >> - >> -1: >> -.endm >> - >> -/* >> - * Failure caused due to insufficient MPU regions. >> - */ >> -FUNC_LOCAL(fail_insufficient_regions) >> - PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n") >> -1: wfe >> - b 1b >> -END(fail_insufficient_regions) >> +#include <asm/mpu/common.inc> >> >> /* >> * Enable EL2 MPU and data cache >> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h >> index b593e4028b..3ee3715430 100644 >> --- a/xen/arch/arm/include/asm/arm64/sysregs.h >> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h >> @@ -462,6 +462,15 @@ >> #define ZCR_ELx_LEN_SIZE 9 >> #define ZCR_ELx_LEN_MASK 0x1ff >> >> +#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_DEVICE_PRBAR 0x22 /* SH=10 AP=00 XN=10 */ >> + >> +#define WRITE_SYSREG_ASM(v, name) "msr " __stringify(name,) #v; Agreed with your other points. > 1) This macro is ASM only and you don't protect it like the C macros below. Why? I have a weak argument for this. :) You can write ""msr " __stringify(name,) #v" in the C files. However, you cannot write "uint64_t _r ....." in the assembly files. If you strongly prefer, I can enclose this within #ifdef __ASSEMBLY__ > 2) Semicolon not needed at the end Ack. - Ayan > > ~Michal >
diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S index ed01993d85..4d76a3166e 100644 --- a/xen/arch/arm/arm64/mpu/head.S +++ b/xen/arch/arm/arm64/mpu/head.S @@ -3,83 +3,7 @@ * Start-of-day code for an Armv8-R MPU system. */ -#include <asm/early_printk.h> -#include <asm/mpu.h> - -/* Backgroud region enable/disable */ -#define SCTLR_ELx_BR BIT(17, UL) - -#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_DEVICE_PRBAR 0x22 /* SH=10 AP=00 XN=10 */ - -#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ -#define REGION_DEVICE_PRLAR 0x09 /* NS=0 ATTR=100 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. - * - * sel: region selector - * base: reg storing base address - * 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 - * - * Preserves \maxcount - * Output: - * \sel: Next available region selector index. - * Clobbers \base, \limit, \prbar, \prlar - * - * Note that all parameters using registers should be distinct. - */ -.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR - /* Check if the region is empty */ - cmp \base, \limit - beq 1f - - /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ - cmp \sel, \maxcount - bge fail_insufficient_regions - - /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ - and \base, \base, #MPU_REGION_MASK - mov \prbar, #\attr_prbar - orr \prbar, \prbar, \base - - /* Limit address should be inclusive */ - sub \limit, \limit, #1 - and \limit, \limit, #MPU_REGION_MASK - mov \prlar, #\attr_prlar - orr \prlar, \prlar, \limit - - msr PRSELR_EL2, \sel - isb - msr PRBAR_EL2, \prbar - msr PRLAR_EL2, \prlar - dsb sy - isb - - add \sel, \sel, #1 - -1: -.endm - -/* - * Failure caused due to insufficient MPU regions. - */ -FUNC_LOCAL(fail_insufficient_regions) - PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n") -1: wfe - b 1b -END(fail_insufficient_regions) +#include <asm/mpu/common.inc> /* * Enable EL2 MPU and data cache diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h index b593e4028b..3ee3715430 100644 --- a/xen/arch/arm/include/asm/arm64/sysregs.h +++ b/xen/arch/arm/include/asm/arm64/sysregs.h @@ -462,6 +462,15 @@ #define ZCR_ELx_LEN_SIZE 9 #define ZCR_ELx_LEN_MASK 0x1ff +#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_DEVICE_PRBAR 0x22 /* SH=10 AP=00 XN=10 */ + +#define WRITE_SYSREG_ASM(v, name) "msr " __stringify(name,) #v; + +#ifndef __ASSEMBLY__ + /* Access to system registers */ #define WRITE_SYSREG64(v, name) do { \ @@ -481,6 +490,8 @@ #define WRITE_SYSREG_LR(v, index) WRITE_SYSREG(v, ICH_LR_REG(index)) #define READ_SYSREG_LR(index) READ_SYSREG(ICH_LR_REG(index)) +#endif /* __ASSEMBLY__ */ + #endif /* _ASM_ARM_ARM64_SYSREGS_H */ /* diff --git a/xen/arch/arm/include/asm/mpu/common.inc b/xen/arch/arm/include/asm/mpu/common.inc new file mode 100644 index 0000000000..47868a1526 --- /dev/null +++ b/xen/arch/arm/include/asm/mpu/common.inc @@ -0,0 +1,79 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <asm/mpu.h> +#include <asm/sysregs.h> + +/* Backgroud region enable/disable */ +#define SCTLR_ELx_BR BIT(17, UL) + +#define REGION_NORMAL_PRLAR 0x0f /* NS=0 ATTR=111 EN=1 */ +#define REGION_DEVICE_PRLAR 0x09 /* NS=0 ATTR=100 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. + * + * sel: region selector + * base: reg storing base address + * 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 + * + * Preserves maxcount + * Output: + * sel: Next available region selector index. + * Clobbers base, limit, prbar, prlar + * + * Note that all parameters using registers should be distinct. + */ +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR + /* Check if the region is empty */ + cmp \base, \limit + beq 1f + + /* Check if the number of regions exceeded the count specified in MPUIR_EL2 */ + cmp \sel, \maxcount + bge fail_insufficient_regions + + /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/ + and \base, \base, #MPU_REGION_MASK + mov \prbar, #\attr_prbar + orr \prbar, \prbar, \base + + /* Limit address should be inclusive */ + sub \limit, \limit, #1 + and \limit, \limit, #MPU_REGION_MASK + mov \prlar, #\attr_prlar + orr \prlar, \prlar, \limit + + WRITE_SYSREG_ASM(\sel, PRSELR_EL2) + isb + WRITE_SYSREG_ASM(\prbar, PRBAR_EL2) + WRITE_SYSREG_ASM(\prlar, PRLAR_EL2) + dsb sy + isb + + add \sel, \sel, #1 + +1: +.endm + +/* Failure caused due to insufficient MPU regions. */ +FUNC_LOCAL(fail_insufficient_regions) + PRINT("- Selected MPU region is above the implemented number in MPUIR_EL2 -\r\n") +1: wfe + b 1b +END(fail_insufficient_regions) + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */
Added a new file common.inc to hold the common earlyboot MPU regions configurations across arm64 and arm32. prepare_xen_region, fail_insufficient_regions() will be used by both arm32 and arm64. Thus, they have been moved to common.inc. *_PRBAR are moved to arm64/sysregs.h. *_PRLAR are moved to common.inc as they are common between arm32 and arm64. Introduce WRITE_SYSREG_ASM to write to the system registers from the common asm file. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from v1 - 1. enable_mpu() now sets HMAIR{0,1} registers. This is similar to what is being done in enable_mmu(). All the mm related configurations happen in this function. 2. Fixed some typos. v2 - 1. Extracted the arm64 head.S functions/macros in a common file. v3 - 1. Moved *_PRLAR are moved to prepare_xen_region.inc 2. enable_boot_cpu_mm() is preserved in mpu/head.S. 3. STORE_SYSREG is renamed as WRITE_SYSREG_ASM() 4. LOAD_SYSREG is removed. 5. No need to save/restore lr in enable_boot_cpu_mm(). IOW, keep it as it was in the original code. v4 - 1. Rename prepare_xen_region.inc to common.inc 2. enable_secondary_cpu_mm() is moved back to mpu/head.S. xen/arch/arm/arm64/mpu/head.S | 78 +---------------------- xen/arch/arm/include/asm/arm64/sysregs.h | 11 ++++ xen/arch/arm/include/asm/mpu/common.inc | 79 ++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 77 deletions(-) create mode 100644 xen/arch/arm/include/asm/mpu/common.inc