RE: [PATCH v2 13/40] xen/mpu: introduce unified function setup_early_uart to map early UART

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Wednesday, January 25, 2023 3:09 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2 13/40] xen/mpu: introduce unified function
> setup_early_uart to map early UART
> 
> Hi Peny,

Hi Julien,

> 
> On 13/01/2023 05:28, Penny Zheng wrote:
> > In MMU system, we map the UART in the fixmap (when earlyprintk is used).
> > However in MPU system, we map the UART with a transient MPU memory
> > region.
> >
> > So we introduce a new unified function setup_early_uart to replace the
> > previous setup_fixmap.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/arm64/head.S               |  2 +-
> >   xen/arch/arm/arm64/head_mmu.S           |  4 +-
> >   xen/arch/arm/arm64/head_mpu.S           | 52
> +++++++++++++++++++++++++
> >   xen/arch/arm/include/asm/early_printk.h |  1 +
> >   4 files changed, 56 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 7f3f973468..a92883319d 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -272,7 +272,7 @@ primary_switched:
> >            * afterwards.
> >            */
> >           bl    remove_identity_mapping
> > -        bl    setup_fixmap
> > +        bl    setup_early_uart
> >   #ifdef CONFIG_EARLY_PRINTK
> >           /* Use a virtual address to access the UART. */
> >           ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> > diff --git a/xen/arch/arm/arm64/head_mmu.S
> > b/xen/arch/arm/arm64/head_mmu.S index b59c40495f..a19b7c873d
> 100644
> > --- a/xen/arch/arm/arm64/head_mmu.S
> > +++ b/xen/arch/arm/arm64/head_mmu.S
> > @@ -312,7 +312,7 @@ ENDPROC(remove_identity_mapping)
> >    *
> >    * Clobbers x0 - x3
> >    */
> > -ENTRY(setup_fixmap)
> > +ENTRY(setup_early_uart)
> 
> This function is doing more than enable the early UART. It also setups the
> fixmap even earlyprintk is not configured.

True, true.
I've thoroughly read the MMU implementation of setup_fixmap, and I'll try to 
split
it up.

> 
> I am not entirely sure what could be the name. Maybe this needs to be split
> further.
> 
> >   #ifdef CONFIG_EARLY_PRINTK
> >           /* Add UART to the fixmap table */
> >           ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> > @@ -325,7 +325,7 @@ ENTRY(setup_fixmap)
> >           dsb   nshst
> >
> >           ret
> > -ENDPROC(setup_fixmap)
> > +ENDPROC(setup_early_uart)
> >
> >   /* Fail-stop */
> >   fail:   PRINT("- Boot failed -\r\n")
> > diff --git a/xen/arch/arm/arm64/head_mpu.S
> > b/xen/arch/arm/arm64/head_mpu.S index e2ac69b0cc..72d1e0863d
> 100644
> > --- a/xen/arch/arm/arm64/head_mpu.S
> > +++ b/xen/arch/arm/arm64/head_mpu.S
> > @@ -18,8 +18,10 @@
> >   #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 round up the section address to be PAGE_SIZE aligned @@
> > -334,6 +336,56 @@ ENTRY(enable_mm)
> >       ret
> >   ENDPROC(enable_mm)
> >
> > +/*
> > + * Map the early UART with a new transient MPU memory region.
> > + *
> 
> Missing "Inputs: "
> 
> > + * x27: region selector
> > + * x28: prbar
> > + * x29: prlar
> > + *
> > + * Clobbers x0 - x4
> > + *
> > + */
> > +ENTRY(setup_early_uart)
> > +#ifdef CONFIG_EARLY_PRINTK
> > +    /* stack LR as write_pr will be called later like nested function */
> > +    mov   x3, lr
> > +
> > +    /*
> > +     * MPU region for early UART is a transient region, since it will be
> > +     * replaced by specific device memory layout when FDT gets parsed.
> 
> I would rather not mention "FDT" here because this code is independent to
> the firmware table used.
> 
> However, any reason to use a transient region rather than the one that will
> be used for the UART driver?
> 

We don’t want to define a MPU region for each device driver. It will exhaust
MPU regions very quickly.
In commit " [PATCH v2 28/40] xen/mpu: map boot module section in MPU system", 
A new FDT property `mpu,device-memory-section` will be introduced for users to 
statically
configure the whole system device memory with the least number of memory 
regions in Device Tree.
This section shall cover all devices that will be used in Xen, like `UART`, 
`GIC`, etc.
For FVP_BaseR_AEMv8R, we have the following definition:
```
mpu,device-memory-section = <0x0 0x80000000 0x0 0x7ffff000>;
```

> > +     */
> > +    load_paddr x0, next_transient_region_idx
> > +    ldr   x4, [x0]
> > +
> > +    ldr   x28, =CONFIG_EARLY_UART_BASE_ADDRESS
> > +    and   x28, x28, #MPU_REGION_MASK
> > +    mov   x1, #REGION_DEVICE_PRBAR
> > +    orr   x28, x28, x1
> 
> This needs some documentation to explain the logic. Maybe even a macro.
> 

Do you suggest that I shall explain how we compose PRBAR_EL2 register?

> > +
> > +    ldr x29, =(CONFIG_EARLY_UART_BASE_ADDRESS + EARLY_UART_SIZE)
> > +    roundup_section x29
> 
> Does this mean we could give access to more than necessary? Shouldn't
> instead prevent compilation if the size doesn't align with the section size?
> 

True, we could not treat uart section like we do for the section defined in 
xen.lds.S.
CONFIG_EARLY_UART_BASE_ADDRESS and EARLY_UART_SIZE shall both be checked
if it is aligned with PAGE_SIZE.

> > +    /* Limit address is inclusive */
> > +    sub   x29, x29, #1
> > +    and   x29, x29, #MPU_REGION_MASK
> > +    mov   x2, #REGION_DEVICE_PRLAR
> > +    orr   x29, x29, x2
> > +
> > +    mov   x27, x4
> 
> This needs some documentation like:
> 
> x27: region selector
> 
> See how we documented the existing helpers.
> 
> > +    bl    write_pr
> > +
> > +    /* Create a new entry in xen_mpumap for early UART */
> > +    create_mpu_entry xen_mpumap, x4, x28, x29, x1, x2
> > +
> > +    /* Update next_transient_region_idx */
> > +    sub   x4, x4, #1
> > +    str   x4, [x0]
> > +
> > +    mov   lr, x3
> > +    ret
> > +#endif
> > +ENDPROC(setup_early_uart)
> > +
> >   /*
> >    * Local variables:
> >    * mode: ASM
> > diff --git a/xen/arch/arm/include/asm/early_printk.h
> > b/xen/arch/arm/include/asm/early_printk.h
> > index 44a230853f..d87623e6d5 100644
> > --- a/xen/arch/arm/include/asm/early_printk.h
> > +++ b/xen/arch/arm/include/asm/early_printk.h
> > @@ -22,6 +22,7 @@
> >    * for EARLY_UART_VIRTUAL_ADDRESS.
> >    */
> >   #define EARLY_UART_VIRTUAL_ADDRESS
> CONFIG_EARLY_UART_BASE_ADDRESS
> > +#define EARLY_UART_SIZE            0x1000
> 
> Shouldn't this be PAGE_SIZE? If not, how did you come up with the number?
> 
> Cheers,
> 
> --
> Julien Grall