Do the arm32 equivalent initialization for commit id ca5df936c4.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
xen/arch/arm/arm32/asm-offsets.c | 6 +++
xen/arch/arm/arm32/mpu/head.S | 57 ++++++++++++++++++++++++
xen/arch/arm/include/asm/mpu/regions.inc | 8 +++-
3 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index 8bbb0f938e..c203ce269d 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -75,6 +75,12 @@ void __dummy__(void)
OFFSET(INITINFO_stack, struct init_info, stack);
BLANK();
+
+#ifdef CONFIG_MPU
+ DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
+ DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
+ BLANK();
+#endif
}
/*
diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
index b2c5245e51..1f9eec6e68 100644
--- a/xen/arch/arm/arm32/mpu/head.S
+++ b/xen/arch/arm/arm32/mpu/head.S
@@ -10,6 +10,38 @@
#include <asm/mpu/regions.inc>
#include <asm/page.h>
+/*
+ * dcache_line_size - get the minimum D-cache line size from the CTR register.
+ */
+ .macro dcache_line_size, reg, tmp1, tmp2
+ mrc CP32(\reg, CTR) // read CTR
+ ubfx \tmp1, \reg, #16, #4 // Extract DminLine (bits 19:16) into tmp1
+ mov \tmp2, #1
+ lsl \tmp2, \tmp2, \tmp1 // tmp2 = 2^DminLine
+ lsl \tmp2, \tmp2, #2 // tmp2 = 4 * 2^DminLine = cache line size in bytes
+ .endm
+
+/*
+ * __invalidate_dcache_area(addr, size)
+ *
+ * Ensure that the data held in the cache for the buffer is invalidated.
+ *
+ * - addr - start address of the buffer
+ * - size - size of the buffer
+ */
+FUNC(__invalidate_dcache_area)
+ dcache_line_size r2, r3, r4
+ add r1, r0, r1
+ sub r4, r2, #1
+ bic r0, r0, r4
+1: mcr CP32(r0, DCIMVAC) /* invalidate D line / unified line */
+ add r0, r0, r2
+ cmp r0, r1
+ blo 1b
+ dsb sy
+ ret
+END(__invalidate_dcache_area)
+
/*
* Set up the memory attribute type tables and enable EL2 MPU and data cache.
* If the Background region is enabled, then the MPU uses the default memory
@@ -49,6 +81,10 @@ FUNC(enable_boot_cpu_mm)
mrc CP32(r5, MPUIR_EL2)
and r5, r5, #NUM_MPU_REGIONS_MASK
+ ldr r0, =max_mpu_regions
+ str r5, [r0]
+ mcr CP32(r0, DCIMVAC) /* Invalidate cache for max_mpu_regions addr */
+
/* x0: region sel */
mov r0, #0
/* Xen text section. */
@@ -83,6 +119,27 @@ FUNC(enable_boot_cpu_mm)
prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
#endif
+zero_mpu:
+ /* Reset remaining MPU regions */
+ cmp r0, r5
+ beq out_zero_mpu
+ mov r1, #0
+ mov r2, #1
+ prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prlar=REGION_DISABLED_PRLAR
+ b zero_mpu
+
+out_zero_mpu:
+ /* Invalidate data cache for MPU data structures */
+ mov r5, lr
+ ldr r0, =xen_mpumap_mask
+ mov r1, #XEN_MPUMAP_MASK_sizeof
+ bl __invalidate_dcache_area
+
+ ldr r0, =xen_mpumap
+ mov r1, #XEN_MPUMAP_sizeof
+ bl __invalidate_dcache_area
+ mov lr, r5
+
b enable_mpu
END(enable_boot_cpu_mm)
diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
index 6b8c233e6c..943bcda346 100644
--- a/xen/arch/arm/include/asm/mpu/regions.inc
+++ b/xen/arch/arm/include/asm/mpu/regions.inc
@@ -24,7 +24,13 @@
#define XEN_MPUMAP_ENTRY_SHIFT 0x3 /* 8 byte structure */
.macro store_pair reg1, reg2, dst
- .word 0xe7f000f0 /* unimplemented */
+ str \reg1, [\dst]
+ add \dst, \dst, #4
+ str \reg2, [\dst]
+.endm
+
+.macro invalidate_dcache_one reg
+ mcr CP32(\reg, DCIMVAC)
.endm
#endif
--
2.25.1On 04/06/2025 19:43, Ayan Kumar Halder wrote: > Do the arm32 equivalent initialization for commit id ca5df936c4. This is not a good commit msg. Also, we somewhat require passing 12 char long IDs. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > xen/arch/arm/arm32/asm-offsets.c | 6 +++ > xen/arch/arm/arm32/mpu/head.S | 57 ++++++++++++++++++++++++ > xen/arch/arm/include/asm/mpu/regions.inc | 8 +++- > 3 files changed, 70 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c > index 8bbb0f938e..c203ce269d 100644 > --- a/xen/arch/arm/arm32/asm-offsets.c > +++ b/xen/arch/arm/arm32/asm-offsets.c > @@ -75,6 +75,12 @@ void __dummy__(void) > > OFFSET(INITINFO_stack, struct init_info, stack); > BLANK(); > + > +#ifdef CONFIG_MPU > + DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask)); > + DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap)); > + BLANK(); > +#endif > } > > /* > diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S > index b2c5245e51..1f9eec6e68 100644 > --- a/xen/arch/arm/arm32/mpu/head.S > +++ b/xen/arch/arm/arm32/mpu/head.S > @@ -10,6 +10,38 @@ > #include <asm/mpu/regions.inc> > #include <asm/page.h> > > +/* > + * dcache_line_size - get the minimum D-cache line size from the CTR register. > + */ I do think we should have a cache.S file to store cache related ops just like for Arm64. Also, no need for multiline comment. > + .macro dcache_line_size, reg, tmp1, tmp2 I would prefer to use the macro from Linux that uses one temporary register > + mrc CP32(\reg, CTR) // read CTR NIT: wrong comment style + wrong alignment > + ubfx \tmp1, \reg, #16, #4 // Extract DminLine (bits 19:16) into tmp1 > + mov \tmp2, #1 > + lsl \tmp2, \tmp2, \tmp1 // tmp2 = 2^DminLine > + lsl \tmp2, \tmp2, #2 // tmp2 = 4 * 2^DminLine = cache line size in bytes > + .endm > + > +/* > + * __invalidate_dcache_area(addr, size) > + * > + * Ensure that the data held in the cache for the buffer is invalidated. > + * > + * - addr - start address of the buffer > + * - size - size of the buffer > + */ > +FUNC(__invalidate_dcache_area) > + dcache_line_size r2, r3, r4 > + add r1, r0, r1 > + sub r4, r2, #1 > + bic r0, r0, r4 > +1: mcr CP32(r0, DCIMVAC) /* invalidate D line / unified line */ > + add r0, r0, r2 > + cmp r0, r1 > + blo 1b > + dsb sy > + ret > +END(__invalidate_dcache_area) > + > /* > * Set up the memory attribute type tables and enable EL2 MPU and data cache. > * If the Background region is enabled, then the MPU uses the default memory > @@ -49,6 +81,10 @@ FUNC(enable_boot_cpu_mm) > mrc CP32(r5, MPUIR_EL2) > and r5, r5, #NUM_MPU_REGIONS_MASK > > + ldr r0, =max_mpu_regions Why ldr and not mov_w? > + str r5, [r0] > + mcr CP32(r0, DCIMVAC) /* Invalidate cache for max_mpu_regions addr */ > + > /* x0: region sel */ > mov r0, #0 > /* Xen text section. */ > @@ -83,6 +119,27 @@ FUNC(enable_boot_cpu_mm) > prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR > #endif > > +zero_mpu: > + /* Reset remaining MPU regions */ > + cmp r0, r5 > + beq out_zero_mpu > + mov r1, #0 > + mov r2, #1 > + prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prlar=REGION_DISABLED_PRLAR > + b zero_mpu > + > +out_zero_mpu: > + /* Invalidate data cache for MPU data structures */ > + mov r5, lr > + ldr r0, =xen_mpumap_mask Why not mov_w? > + mov r1, #XEN_MPUMAP_MASK_sizeof > + bl __invalidate_dcache_area > + > + ldr r0, =xen_mpumap > + mov r1, #XEN_MPUMAP_sizeof > + bl __invalidate_dcache_area > + mov lr, r5 > + > b enable_mpu > END(enable_boot_cpu_mm) > > diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc > index 6b8c233e6c..943bcda346 100644 > --- a/xen/arch/arm/include/asm/mpu/regions.inc > +++ b/xen/arch/arm/include/asm/mpu/regions.inc > @@ -24,7 +24,13 @@ > #define XEN_MPUMAP_ENTRY_SHIFT 0x3 /* 8 byte structure */ > > .macro store_pair reg1, reg2, dst > - .word 0xe7f000f0 /* unimplemented */ > + str \reg1, [\dst] > + add \dst, \dst, #4 > + str \reg2, [\dst] AFAIR there is STM instruction to do the same > +.endm > + > +.macro invalidate_dcache_one reg > + mcr CP32(\reg, DCIMVAC) Why? You don't seem to use this macro > .endm > > #endif ~Michal
Hi Michal/Julien, On 05/06/2025 08:44, Orzel, Michal wrote: > > On 04/06/2025 19:43, Ayan Kumar Halder wrote: >> Do the arm32 equivalent initialization for commit id ca5df936c4. > This is not a good commit msg. > Also, we somewhat require passing 12 char long IDs. Modify Arm32 assembly boot code to reset any unused MPU region, initialise 'max_mpu_regions' with the number of supported MPU regions and set/clear the bitmap 'xen_mpumap_mask' used to track the enabled regions. Use the macro definition for "dcache_line_size" from linux. Does ^^^ read fine ? > >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> xen/arch/arm/arm32/asm-offsets.c | 6 +++ >> xen/arch/arm/arm32/mpu/head.S | 57 ++++++++++++++++++++++++ >> xen/arch/arm/include/asm/mpu/regions.inc | 8 +++- >> 3 files changed, 70 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c >> index 8bbb0f938e..c203ce269d 100644 >> --- a/xen/arch/arm/arm32/asm-offsets.c >> +++ b/xen/arch/arm/arm32/asm-offsets.c >> @@ -75,6 +75,12 @@ void __dummy__(void) >> >> OFFSET(INITINFO_stack, struct init_info, stack); >> BLANK(); >> + >> +#ifdef CONFIG_MPU >> + DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask)); >> + DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap)); >> + BLANK(); >> +#endif >> } >> >> /* >> diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S >> index b2c5245e51..1f9eec6e68 100644 >> --- a/xen/arch/arm/arm32/mpu/head.S >> +++ b/xen/arch/arm/arm32/mpu/head.S >> @@ -10,6 +10,38 @@ >> #include <asm/mpu/regions.inc> >> #include <asm/page.h> >> >> +/* >> + * dcache_line_size - get the minimum D-cache line size from the CTR register. >> + */ > I do think we should have a cache.S file to store cache related ops just like > for Arm64. ok, I will introduce a new file. > Also, no need for multiline comment. ack. > >> + .macro dcache_line_size, reg, tmp1, tmp2 > I would prefer to use the macro from Linux that uses one temporary register /* * dcache_line_size - get the minimum D-cache line size from the CTR register * on ARMv7. */ .macro dcache_line_size, reg, tmp mrc p15, 0, \tmp, c0, c0, 1 /* read ctr */ lsr \tmp, \tmp, #16 and \tmp, \tmp, #0xf /* cache line size encoding */ mov \reg, #4 /* bytes per word */ mov \reg, \reg, lsl \tmp /* actual cache line size */ .endm > >> + mrc CP32(\reg, CTR) // read CTR > NIT: wrong comment style + wrong alignment yes, I should use /* ... */ > >> + ubfx \tmp1, \reg, #16, #4 // Extract DminLine (bits 19:16) into tmp1 >> + mov \tmp2, #1 >> + lsl \tmp2, \tmp2, \tmp1 // tmp2 = 2^DminLine >> + lsl \tmp2, \tmp2, #2 // tmp2 = 4 * 2^DminLine = cache line size in bytes >> + .endm >> + >> +/* >> + * __invalidate_dcache_area(addr, size) >> + * >> + * Ensure that the data held in the cache for the buffer is invalidated. >> + * >> + * - addr - start address of the buffer >> + * - size - size of the buffer >> + */ >> +FUNC(__invalidate_dcache_area) >> + dcache_line_size r2, r3, r4 >> + add r1, r0, r1 >> + sub r4, r2, #1 >> + bic r0, r0, r4 >> +1: mcr CP32(r0, DCIMVAC) /* invalidate D line / unified line */ >> + add r0, r0, r2 >> + cmp r0, r1 >> + blo 1b >> + dsb sy >> + ret >> +END(__invalidate_dcache_area) >> + >> /* >> * Set up the memory attribute type tables and enable EL2 MPU and data cache. >> * If the Background region is enabled, then the MPU uses the default memory >> @@ -49,6 +81,10 @@ FUNC(enable_boot_cpu_mm) >> mrc CP32(r5, MPUIR_EL2) >> and r5, r5, #NUM_MPU_REGIONS_MASK >> >> + ldr r0, =max_mpu_regions > Why ldr and not mov_w? mov_w r0, max_mpu_regions > >> + str r5, [r0] >> + mcr CP32(r0, DCIMVAC) /* Invalidate cache for max_mpu_regions addr */ >> + >> /* x0: region sel */ >> mov r0, #0 >> /* Xen text section. */ >> @@ -83,6 +119,27 @@ FUNC(enable_boot_cpu_mm) >> prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR >> #endif >> >> +zero_mpu: >> + /* Reset remaining MPU regions */ >> + cmp r0, r5 >> + beq out_zero_mpu >> + mov r1, #0 >> + mov r2, #1 >> + prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prlar=REGION_DISABLED_PRLAR >> + b zero_mpu >> + >> +out_zero_mpu: >> + /* Invalidate data cache for MPU data structures */ >> + mov r5, lr >> + ldr r0, =xen_mpumap_mask > Why not mov_w? mov_w r0, xen_mpumap_mask > >> + mov r1, #XEN_MPUMAP_MASK_sizeof >> + bl __invalidate_dcache_area >> + >> + ldr r0, =xen_mpumap >> + mov r1, #XEN_MPUMAP_sizeof >> + bl __invalidate_dcache_area >> + mov lr, r5 >> + >> b enable_mpu >> END(enable_boot_cpu_mm) >> >> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc >> index 6b8c233e6c..943bcda346 100644 >> --- a/xen/arch/arm/include/asm/mpu/regions.inc >> +++ b/xen/arch/arm/include/asm/mpu/regions.inc >> @@ -24,7 +24,13 @@ >> #define XEN_MPUMAP_ENTRY_SHIFT 0x3 /* 8 byte structure */ >> >> .macro store_pair reg1, reg2, dst >> - .word 0xe7f000f0 /* unimplemented */ >> + str \reg1, [\dst] >> + add \dst, \dst, #4 >> + str \reg2, [\dst] > AFAIR there is STM instruction to do the same strd \reg1, \reg2, [\dst] > >> +.endm >> + >> +.macro invalidate_dcache_one reg >> + mcr CP32(\reg, DCIMVAC) > Why? You don't seem to use this macro oh, this can be removed. - Ayan > >> .endm >> >> #endif > ~Michal >
On 05/06/2025 16:27, Ayan Kumar Halder wrote: > Hi Michal/Julien, > > On 05/06/2025 08:44, Orzel, Michal wrote: >> >> On 04/06/2025 19:43, Ayan Kumar Halder wrote: >>> Do the arm32 equivalent initialization for commit id ca5df936c4. >> This is not a good commit msg. >> Also, we somewhat require passing 12 char long IDs. > > Modify Arm32 assembly boot code to reset any unused MPU region, > initialise 'max_mpu_regions' with the number of supported MPU regions > and set/clear the bitmap 'xen_mpumap_mask' used to track the enabled > regions. > > Use the macro definition for "dcache_line_size" from linux. > > Does ^^^ read fine ? Yes, it certainly reads better. ~Michal > >> >>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> --- >>> xen/arch/arm/arm32/asm-offsets.c | 6 +++ >>> xen/arch/arm/arm32/mpu/head.S | 57 ++++++++++++++++++++++++ >>> xen/arch/arm/include/asm/mpu/regions.inc | 8 +++- >>> 3 files changed, 70 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c >>> index 8bbb0f938e..c203ce269d 100644 >>> --- a/xen/arch/arm/arm32/asm-offsets.c >>> +++ b/xen/arch/arm/arm32/asm-offsets.c >>> @@ -75,6 +75,12 @@ void __dummy__(void) >>> >>> OFFSET(INITINFO_stack, struct init_info, stack); >>> BLANK(); >>> + >>> +#ifdef CONFIG_MPU >>> + DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask)); >>> + DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap)); >>> + BLANK(); >>> +#endif >>> } >>> >>> /* >>> diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S >>> index b2c5245e51..1f9eec6e68 100644 >>> --- a/xen/arch/arm/arm32/mpu/head.S >>> +++ b/xen/arch/arm/arm32/mpu/head.S >>> @@ -10,6 +10,38 @@ >>> #include <asm/mpu/regions.inc> >>> #include <asm/page.h> >>> >>> +/* >>> + * dcache_line_size - get the minimum D-cache line size from the CTR register. >>> + */ >> I do think we should have a cache.S file to store cache related ops just like >> for Arm64. > ok, I will introduce a new file. >> Also, no need for multiline comment. > ack. >> >>> + .macro dcache_line_size, reg, tmp1, tmp2 >> I would prefer to use the macro from Linux that uses one temporary register > /* > * dcache_line_size - get the minimum D-cache line size from the CTR > register > * on ARMv7. > */ > .macro dcache_line_size, reg, tmp > mrc p15, 0, \tmp, c0, c0, 1 /* read ctr */ > lsr \tmp, \tmp, #16 > and \tmp, \tmp, #0xf /* cache line size encoding */ > mov \reg, #4 /* bytes per word */ > mov \reg, \reg, lsl \tmp /* actual cache line size */ > .endm > >> >>> + mrc CP32(\reg, CTR) // read CTR >> NIT: wrong comment style + wrong alignment > yes, I should use /* ... */ >> >>> + ubfx \tmp1, \reg, #16, #4 // Extract DminLine (bits 19:16) into tmp1 >>> + mov \tmp2, #1 >>> + lsl \tmp2, \tmp2, \tmp1 // tmp2 = 2^DminLine >>> + lsl \tmp2, \tmp2, #2 // tmp2 = 4 * 2^DminLine = cache line size in bytes >>> + .endm >>> + >>> +/* >>> + * __invalidate_dcache_area(addr, size) >>> + * >>> + * Ensure that the data held in the cache for the buffer is invalidated. >>> + * >>> + * - addr - start address of the buffer >>> + * - size - size of the buffer >>> + */ >>> +FUNC(__invalidate_dcache_area) >>> + dcache_line_size r2, r3, r4 >>> + add r1, r0, r1 >>> + sub r4, r2, #1 >>> + bic r0, r0, r4 >>> +1: mcr CP32(r0, DCIMVAC) /* invalidate D line / unified line */ >>> + add r0, r0, r2 >>> + cmp r0, r1 >>> + blo 1b >>> + dsb sy >>> + ret >>> +END(__invalidate_dcache_area) >>> + >>> /* >>> * Set up the memory attribute type tables and enable EL2 MPU and data cache. >>> * If the Background region is enabled, then the MPU uses the default memory >>> @@ -49,6 +81,10 @@ FUNC(enable_boot_cpu_mm) >>> mrc CP32(r5, MPUIR_EL2) >>> and r5, r5, #NUM_MPU_REGIONS_MASK >>> >>> + ldr r0, =max_mpu_regions >> Why ldr and not mov_w? > mov_w r0, max_mpu_regions >> >>> + str r5, [r0] >>> + mcr CP32(r0, DCIMVAC) /* Invalidate cache for max_mpu_regions addr */ >>> + >>> /* x0: region sel */ >>> mov r0, #0 >>> /* Xen text section. */ >>> @@ -83,6 +119,27 @@ FUNC(enable_boot_cpu_mm) >>> prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR >>> #endif >>> >>> +zero_mpu: >>> + /* Reset remaining MPU regions */ >>> + cmp r0, r5 >>> + beq out_zero_mpu >>> + mov r1, #0 >>> + mov r2, #1 >>> + prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prlar=REGION_DISABLED_PRLAR >>> + b zero_mpu >>> + >>> +out_zero_mpu: >>> + /* Invalidate data cache for MPU data structures */ >>> + mov r5, lr >>> + ldr r0, =xen_mpumap_mask >> Why not mov_w? > mov_w r0, xen_mpumap_mask >> >>> + mov r1, #XEN_MPUMAP_MASK_sizeof >>> + bl __invalidate_dcache_area >>> + >>> + ldr r0, =xen_mpumap >>> + mov r1, #XEN_MPUMAP_sizeof >>> + bl __invalidate_dcache_area >>> + mov lr, r5 >>> + >>> b enable_mpu >>> END(enable_boot_cpu_mm) >>> >>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc >>> index 6b8c233e6c..943bcda346 100644 >>> --- a/xen/arch/arm/include/asm/mpu/regions.inc >>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc >>> @@ -24,7 +24,13 @@ >>> #define XEN_MPUMAP_ENTRY_SHIFT 0x3 /* 8 byte structure */ >>> >>> .macro store_pair reg1, reg2, dst >>> - .word 0xe7f000f0 /* unimplemented */ >>> + str \reg1, [\dst] >>> + add \dst, \dst, #4 >>> + str \reg2, [\dst] >> AFAIR there is STM instruction to do the same > strd \reg1, \reg2, [\dst] >> >>> +.endm >>> + >>> +.macro invalidate_dcache_one reg >>> + mcr CP32(\reg, DCIMVAC) >> Why? You don't seem to use this macro > > oh, this can be removed. > > - Ayan > >> >>> .endm >>> >>> #endif >> ~Michal >>
Hi Michal, On 05/06/2025 08:44, Orzel, Michal wrote: > > > On 04/06/2025 19:43, Ayan Kumar Halder wrote: >> Do the arm32 equivalent initialization for commit id ca5df936c4. > This is not a good commit msg. > Also, we somewhat require passing 12 char long IDs. We are following the same convention as Linux. IIRC this was updated because there was some collision with 10 characters in Linux (not sure if we have seen it in Xen yet). [...] >> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc >> index 6b8c233e6c..943bcda346 100644 >> --- a/xen/arch/arm/include/asm/mpu/regions.inc >> +++ b/xen/arch/arm/include/asm/mpu/regions.inc >> @@ -24,7 +24,13 @@ >> #define XEN_MPUMAP_ENTRY_SHIFT 0x3 /* 8 byte structure */ >> >> .macro store_pair reg1, reg2, dst >> - .word 0xe7f000f0 /* unimplemented */ >> + str \reg1, [\dst] >> + add \dst, \dst, #4 >> + str \reg2, [\dst] > AFAIR there is STM instruction to do the same AFAICT, one issue with stm is the ordering is forced by the instruction rather than the user. So \reg1 could be stored first. I think it would be better to use "strd". It still has restriction (the two registers need to have contiguous index). But I think that would be better if we want to reduce the number of instructions. Cheers, > >> +.endm >> + >> +.macro invalidate_dcache_one reg >> + mcr CP32(\reg, DCIMVAC) > Why? You don't seem to use this macro > >> .endm >> >> #endif > > ~Michal > -- Julien Grall
On 05/06/2025 10:58, Julien Grall wrote:
> Hi Michal,
>
> On 05/06/2025 08:44, Orzel, Michal wrote:
>>
>>
>> On 04/06/2025 19:43, Ayan Kumar Halder wrote:
>>> Do the arm32 equivalent initialization for commit id ca5df936c4.
>> This is not a good commit msg.
>> Also, we somewhat require passing 12 char long IDs.
>
> We are following the same convention as Linux. IIRC this was updated
> because there was some collision with 10 characters in Linux (not sure
> if we have seen it in Xen yet).
>
> [...]
>
>>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
>>> index 6b8c233e6c..943bcda346 100644
>>> --- a/xen/arch/arm/include/asm/mpu/regions.inc
>>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
>>> @@ -24,7 +24,13 @@
>>> #define XEN_MPUMAP_ENTRY_SHIFT 0x3 /* 8 byte structure */
>>>
>>> .macro store_pair reg1, reg2, dst
>>> - .word 0xe7f000f0 /* unimplemented */
>>> + str \reg1, [\dst]
>>> + add \dst, \dst, #4
>>> + str \reg2, [\dst]
>> AFAIR there is STM instruction to do the same
>
> AFAICT, one issue with stm is the ordering is forced by the instruction
> rather than the user. So \reg1 could be stored first.
Yes, I think it stores based on register number. So if you do {r2, r1} it will
still store r1 first.
>
> I think it would be better to use "strd". It still has restriction
> (the two registers need to have contiguous index). But I think that
> would be better if we want to reduce the number of instructions.
Ok
~Michal
On 05/06/2025 09:58, Julien Grall wrote: > Hi Michal, > > On 05/06/2025 08:44, Orzel, Michal wrote: >> >> >> On 04/06/2025 19:43, Ayan Kumar Halder wrote: >>> Do the arm32 equivalent initialization for commit id ca5df936c4. >> This is not a good commit msg. >> Also, we somewhat require passing 12 char long IDs. > > We are following the same convention as Linux. IIRC this was updated > because there was some collision with 10 characters in Linux (not sure > if we have seen it in Xen yet). > > [...] > >>> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/ >>> include/asm/mpu/regions.inc >>> index 6b8c233e6c..943bcda346 100644 >>> --- a/xen/arch/arm/include/asm/mpu/regions.inc >>> +++ b/xen/arch/arm/include/asm/mpu/regions.inc >>> @@ -24,7 +24,13 @@ >>> #define XEN_MPUMAP_ENTRY_SHIFT 0x3 /* 8 byte structure */ >>> .macro store_pair reg1, reg2, dst >>> - .word 0xe7f000f0 /* unimplemented */ >>> + str \reg1, [\dst] >>> + add \dst, \dst, #4 >>> + str \reg2, [\dst] >> AFAIR there is STM instruction to do the same > > AFAICT, one issue with stm is the ordering is forced by the instruction > rather than the user. So \reg1 could be stored first. Sorry I meant \reg2. Cheers, -- Julien Grall
© 2016 - 2025 Red Hat, Inc.