RE: [PATCH v6 11/11] xen/arm64: add setup_fixmap and remove_identity_mapping for MPU

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, November 7, 2022 5:02 AM
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Penny Zheng <Penny.Zheng@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> 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.zheng@xxxxxxx>
> >
> > 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.chen@xxxxxxx>
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >   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-julien@xxxxxxx/
> 

Thx! Understood, and I'll use the same logic for enable_mmu.

> Cheers,
> 
> --
> Julien Grall