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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, January 30, 2023 6:00 PM
> 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
> 
> 
> 
> On 30/01/2023 06:24, Penny Zheng wrote:
> > Hi, Julien
> 
> Hi Penny,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: Sunday, January 29, 2023 3:43 PM
> >> 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 Penny,
> >>
> >> On 29/01/2023 06:17, Penny Zheng wrote:
> >>>> -----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.
> >> What the usual size of an MPU?
> >>
> >> However, even if you don't want to define one for every device, it
> >> still seem to be sensible to define a fixed temporary one for the
> >> early UART as this would simplify the assembly code.
> >>
> >
> > We will add fixed MPU regions for Xen static heap in function setup_mm.
> > If we put early uart region in front(fixed region place), it will
> > leave holes later after removing it.
> 
> Why? The entry could be re-used to map the devices entry.
> 
> >
> >>
> >>> In commit " [PATCH v2 28/40] xen/mpu: map boot module section in
> MPU
> >>> system",
> >>
> >> Did you mean patch #27?
> >>
> >>> 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>; ```
> >>
> >> I am a bit worry this will be a recipe for mistake. Do you have an
> >> example where the MPU will be exhausted if we reserve some entries
> >> for each device (or some)?
> >>
> >
> > Yes, we have internal platform where MPU regions are only 16.
> 
> Internal is in silicon (e.g. real) or virtual platform?
> 

Sorry, we met this kind of type platform is all I'm allowed to say.
Due to NDA, I couldn’t tell more.

> >  It will almost eat up
> > all MPU regions based on current implementation, when launching two
> guests in platform.
> >
> > Let's calculate the most simple scenario:
> > The following is MPU-related static configuration in device tree:
> > ```
> >          mpu,boot-module-section = <0x0 0x10000000 0x0 0x10000000>;
> >          mpu,guest-memory-section = <0x0 0x20000000 0x0 0x40000000>;
> >          mpu,device-memory-section = <0x0 0x80000000 0x0 0x7ffff000>;
> >          mpu,shared-memory-section = <0x0 0x7a000000 0x0 0x02000000>;
> >
> >          xen,static-heap = <0x0 0x60000000 0x0 0x1a000000>; ``` At the
> > end of the boot, before reshuffling, the MPU region usage will be as
> follows:
> > 7 (defined in assembly) + FDT(early_fdt_map) + 5 (at least one region for
> each "mpu,xxx-memory-section").
> 
> Can you list the 7 sections? Is it including the init section?
> 

Yes, I'll draw the layout for you:
'''
 Xen MPU Map before reorg:

xen_mpumap[0] : Xen text
xen_mpumap[1] : Xen read-only data
xen_mpumap[2] : Xen read-only after init data
xen_mpumap[3] : Xen read-write data
xen_mpumap[4] : Xen BSS
xen_mpumap[5] : Xen static heap
......
xen_mpumap[max_xen_mpumap - 7]: Static shared memory section
xen_mpumap[max_xen_mpumap - 6]: Boot Module memory section(kernel, initramfs, 
etc)
xen_mpumap[max_xen_mpumap - 5]: Device memory section
xen_mpumap[max_xen_mpumap - 4]: Guest memory section
xen_mpumap[max_xen_mpumap - 3]: Early FDT
xen_mpumap[max_xen_mpumap - 2]: Xen init data
xen_mpumap[max_xen_mpumap - 1]: Xen init text

In the end of boot, function init_done will do the reorg and boot-only region 
clean-up:

Xen MPU Map after reorg(idle vcpu):

xen_mpumap[0] : Xen text
xen_mpumap[1] : Xen read-only data
xen_mpumap[2] : Xen read-only after init data
xen_mpumap[3] : Xen read-write data
xen_mpumap[4] : Xen BSS
xen_mpumap[5] : Xen static heap
xen_mpumap[6] : Guest memory section
xen_mpumap[7] : Device memory section
xen_mpumap[6] : Static shared memory section

Xen MPU Map on runtime(guest vcpu):

xen_mpumap[0] : Xen text
xen_mpumap[1] : Xen read-only data
xen_mpumap[2] : Xen read-only after init data
xen_mpumap[3] : Xen read-write data
xen_mpumap[4] : Xen BSS
xen_mpumap[5] : Xen static heap
xen_mpumap[6] : Guest memory
xen_mpumap[7] : vGIC map
xen_mpumap[8] : vPL011 map
xen_mpumap[9] : Passthrough device map(UART, etc)
xen_mpumap[10] : Static shared memory section

> >
> > That will be already at least 13 MPU regions ;\.
> 
> The section I am the most concern of is mpu,device-memory-section
> because it would likely mean that all the devices will be mapped in Xen.
> Is there any risk that the guest may use different memory attribute?
> 

Yes, on current implementation, per-domain vgic, vpl011, and passthrough device 
map
will be individually added into per-domain P2M mapping, then when switching 
into guest
vcpu from xen idle vcpu, device memory section will be replaced by vgic, 
vpl011, passthrough
device map.

> On the platform you are describing, what are the devices you are expected
> to be used by Xen?
> 
> Cheers,
> 
> --
> Julien Grall