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

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 07/11/2022 08:13, Penny Zheng wrote:
> > -----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 fact that the fixmap entry is not created in create_page_tables() 
(now prepare_early_mappings()).
Cheers,

--
Julien Grall