Hi Julien > -----Original Message----- > From: Julien Grall <jul...@xen.org> > Sent: Monday, November 7, 2022 5:02 AM > To: Wei Chen <wei.c...@arm.com>; xen-devel@lists.xenproject.org > Cc: nd <n...@arm.com>; Penny Zheng <penny.zh...@arm.com>; Stefano > Stabellini <sstabell...@kernel.org>; Bertrand Marquis > <bertrand.marq...@arm.com>; Volodymyr Babchuk > <volodymyr_babc...@epam.com> > Subject: Re: [PATCH v6 11/11] xen/arm64: add setup_fixmap and > remove_identity_mapping for MPU > > Hi, > > On 04/11/2022 10:07, Wei Chen wrote: > > From: Penny Zheng <penny.zh...@arm.com> > > > > setup_fixmap and remove_identity_mapping are two functions that are > > used in Xen boot-time code flow. We implement these two functions for > > MPU system, in this case, the code flow in head.S doesn't need to use > > #ifdef to gate MPU/MMU code. > > > > In MMU system, setup_fixmap is used for Xen to map some essentail data > > or devices in boot-time. For MPU system, we still have this > > requirement, we map the early UART to MPU protection region when > > earlyprintk is enabled. This also means PRINT can't be used after > > turning on MPU but before setup_fixmap. This restriction is the same > > as MMU system. > > > > For remove_identity_mapping, we just need an empty function to make > > head.S code flow happy. > > > > Signed-off-by: Wei Chen <wei.c...@arm.com> > > Signed-off-by: Penny Zheng <penny.zh...@arm.com> > > --- > > xen/arch/arm/arm64/head_mpu.S | 49 +++++++++++++++++++ > > .../arm/include/asm/platforms/fvp_baser.h | 4 ++ > > 2 files changed, 53 insertions(+) > > > > diff --git a/xen/arch/arm/arm64/head_mpu.S > > b/xen/arch/arm/arm64/head_mpu.S index 5a1b03e293..336c0a630f > 100644 > > --- a/xen/arch/arm/arm64/head_mpu.S > > +++ b/xen/arch/arm/arm64/head_mpu.S > > @@ -20,13 +20,20 @@ > > /* > > * In boot stage, we will use 1 MPU region: > > * Region#0: Normal memory for Xen text + data + bss (2MB) > > + * Region#1: Device memory for EARLY UART, size is defined > > + * by platform's EARLY_UART_SIZE > > */ > > #define BOOT_NORMAL_REGION_IDX 0x0 > > +#define BOOT_DEVICE_REGION_IDX 0x1 > > > > /* MPU normal memory attributes. */ > > #define PRBAR_NORMAL_MEM 0x30 /* SH=11 AP=00 XN=00 */ > > #define PRLAR_NORMAL_MEM 0x0f /* NS=0 ATTR=111 EN=1 */ > > > > +/* MPU device memory attributes. */ > > +#define PRBAR_DEVICE_MEM 0x20 /* SH=10 AP=00 XN=00 */ > > +#define PRLAR_DEVICE_MEM 0x09 /* NS=0 ATTR=100 EN=1 */ > > + > > .macro write_pr, sel, prbar, prlar > > msr PRSELR_EL2, \sel > > dsb sy > > @@ -69,6 +76,48 @@ ENTRY(prepare_early_mappings) > > ret > > ENDPROC(prepare_early_mappings) > > > > +/* > > + * In MMU system, setup_fixmap is used for Xen to map some essential > > +data > > + * or devices in boot-time. In order to be consistent with MMU > > +system, we > > + * inherit the function name for MPU system. > > + * setup_fixmap of MPU system will: > > + * - Map the early UART to MPU protection region when earlyprintk is > > + * enabled (The PRINT can't be used after turning on MPU but before > > + * setup_fixmap). > > For the MMU, we have this restriction because the fixmap could clash with > the identity mapping. I don't think there are such restrictions for the MPU > and therefore it seems strange to pertain the same behavior. >
Yes, both removing identity mapping and using fixmap virtual memory layout are not applied to the MPU system. And in MMU system, the setup_fixmap function has a behavior to map the UART for early printk. And we are only trying to pertain this behavior on MPU system. > In fact, I have plan to get rid of this restriction even for the MMU. So > better > this restriction is not spread if we can. Hmm, I'm a bit confused here. Which restriction you are trying to remove? The whole fixmap virtual memory layout? > > > + * > > + * Clobbers x0 - x3 > > + */ > > +ENTRY(setup_fixmap) > > +#ifdef CONFIG_EARLY_PRINTK > > + /* Map early uart to MPU device region for early printk. */ > > + mov x0, #BOOT_DEVICE_REGION_IDX > > + ldr x1, =CONFIG_EARLY_UART_BASE_ADDRESS > > + and x1, x1, #MPU_REGION_MASK > > + mov x3, #PRBAR_DEVICE_MEM > > + orr x1, x1, x3 > > + > > + ldr x2, =CONFIG_EARLY_UART_BASE_ADDRESS > > + ldr x3, =(CONFIG_EARLY_UART_BASE_ADDRESS + EARLY_UART_SIZE - 1) > > + add x2, x2, x3 > > + and x2, x2, #MPU_REGION_MASK > > + mov x3, #PRLAR_DEVICE_MEM > > + orr x2, x2, x3 > > + > > + /* > > + * Write to MPU protection region: > > + * x0 for pr_sel, x1 for prbar x2 for prlar > > + */ > > + write_pr x0, x1, x2 > > +#endif > > + > > + ret > > +ENDPROC(setup_fixmap) > > + > > +/* Stub of remove_identity_mapping for MPU systems */ > > +ENTRY(remove_identity_mapping) > > + ret > > +ENDPROC(remove_identity_mapping) > > This stub could be avoided if you move the call to remove_identity_mapping > in enable_mmu() as I did for arm32. See [1]. > > [1] https://lore.kernel.org/all/20221022150422.17707-3-jul...@xen.org/ > Thx! Understood, and I'll use the same logic for enable_mmu. > Cheers, > > -- > Julien Grall
