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

Commit Message

Penny Zheng Jan. 13, 2023, 5:28 a.m. UTC

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 <[e-mail]>
Signed-off-by: Wei Chen <[e-mail]>
---
 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(-)

Comments

Julien Grall Jan. 24, 2023, 7:09 p.m. UTC | #1

Hi Peny,

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 <[e-mail]>
> Signed-off-by: Wei Chen <[e-mail]>
> ---
>   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.

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?

> +     */
> +    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.

> +
> +    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?

> +    /* 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,

Penny Zheng Jan. 29, 2023, 6:17 a.m. UTC | #2

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: Wednesday, January 25, 2023 3:09 AM
> To: Penny Zheng <[e-mail]>; [e-mail]
> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>
> 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 <[e-mail]>
> > Signed-off-by: Wei Chen <[e-mail]>
> > ---
> >   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

Julien Grall Jan. 29, 2023, 7:43 a.m. UTC | #3

Hi Penny,

On 29/01/2023 06:17, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <[e-mail]>
>> Sent: Wednesday, January 25, 2023 3:09 AM
>> To: Penny Zheng <[e-mail]>; [e-mail]
>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Bertrand Marquis <[e-mail]>;
>> Volodymyr Babchuk <[e-mail]>
>> 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 <[e-mail]>
>>> Signed-off-by: Wei Chen <[e-mail]>
>>> ---
>>>    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.


> 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)?

Cheers,

Penny Zheng Jan. 30, 2023, 6:24 a.m. UTC | #4

Hi, Julien

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: Sunday, January 29, 2023 3:43 PM
> To: Penny Zheng <[e-mail]>; [e-mail]
> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>
> 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 <[e-mail]>
> >> Sent: Wednesday, January 25, 2023 3:09 AM
> >> To: Penny Zheng <[e-mail]>; xen-
> [e-mail]
> >> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> >> <[e-mail]>; Bertrand Marquis
> >> <[e-mail]>; Volodymyr Babchuk
> >> <[e-mail]>
> >> 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 <[e-mail]>
> >>> Signed-off-by: Wei Chen <[e-mail]>
> >>> ---
> >>>    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.

> 
> > 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. 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").

That will be already at least 13 MPU regions ;\.

> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng

Julien Grall Jan. 30, 2023, 10 a.m. UTC | #5

On 30/01/2023 06:24, Penny Zheng wrote:
> Hi, Julien

Hi Penny,

>> -----Original Message-----
>> From: Julien Grall <[e-mail]>
>> Sent: Sunday, January 29, 2023 3:43 PM
>> To: Penny Zheng <[e-mail]>; [e-mail]
>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Bertrand Marquis <[e-mail]>;
>> Volodymyr Babchuk <[e-mail]>
>> 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 <[e-mail]>
>>>> Sent: Wednesday, January 25, 2023 3:09 AM
>>>> To: Penny Zheng <[e-mail]>; xen-
>> [e-mail]
>>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>>>> <[e-mail]>; Bertrand Marquis
>>>> <[e-mail]>; Volodymyr Babchuk
>>>> <[e-mail]>
>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Wei Chen <[e-mail]>
>>>>> ---
>>>>>     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?

>  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?

> 
> 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?

On the platform you are describing, what are the devices you are 
expected to be used by Xen?

Cheers,

Penny Zheng Jan. 31, 2023, 5:38 a.m. UTC | #6

Hi Julien

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: Monday, January 30, 2023 6:00 PM
> To: Penny Zheng <[e-mail]>; [e-mail]
> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>
> 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 <[e-mail]>
> >> Sent: Sunday, January 29, 2023 3:43 PM
> >> To: Penny Zheng <[e-mail]>; xen-
> [e-mail]
> >> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> >> <[e-mail]>; Bertrand Marquis
> >> <[e-mail]>; Volodymyr Babchuk
> >> <[e-mail]>
> >> 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 <[e-mail]>
> >>>> Sent: Wednesday, January 25, 2023 3:09 AM
> >>>> To: Penny Zheng <[e-mail]>; xen-
> >> [e-mail]
> >>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> >>>> <[e-mail]>; Bertrand Marquis
> >>>> <[e-mail]>; Volodymyr Babchuk
> >>>> <[e-mail]>
> >>>> 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 <[e-mail]>
> >>>>> Signed-off-by: Wei Chen <[e-mail]>
> >>>>> ---
> >>>>>     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

Julien Grall Jan. 31, 2023, 9:41 a.m. UTC | #7

Hi Penny,

On 31/01/2023 05:38, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <[e-mail]>
>> Sent: Monday, January 30, 2023 6:00 PM
>> To: Penny Zheng <[e-mail]>; [e-mail]
>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Bertrand Marquis <[e-mail]>;
>> Volodymyr Babchuk <[e-mail]>
>> 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 <[e-mail]>
>>>> Sent: Sunday, January 29, 2023 3:43 PM
>>>> To: Penny Zheng <[e-mail]>; xen-
>> [e-mail]
>>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>>>> <[e-mail]>; Bertrand Marquis
>>>> <[e-mail]>; Volodymyr Babchuk
>>>> <[e-mail]>
>>>> 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 <[e-mail]>
>>>>>> Sent: Wednesday, January 25, 2023 3:09 AM
>>>>>> To: Penny Zheng <[e-mail]>; xen-
>>>> [e-mail]
>>>>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>>>>>> <[e-mail]>; Bertrand Marquis
>>>>>> <[e-mail]>; Volodymyr Babchuk
>>>>>> <[e-mail]>
>>>>>> 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 <[e-mail]>
>>>>>>> Signed-off-by: Wei Chen <[e-mail]>
>>>>>>> ---
>>>>>>>      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:

Thanks!

> '''
>   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

In theory 1 and 2 could be merged after boot. But I guess it might be 
complicated?

> xen_mpumap[3] : Xen read-write data
> xen_mpumap[4] : Xen BSS
> xen_mpumap[5] : Xen static heap
> xen_mpumap[6] : Guest memory section

Why do you need to map the "Guest memory section" for the idle vCPU?

> xen_mpumap[7] : Device memory section

I might be missing some context here. But why this section is not also 
mapped in the context of the guest vCPU?

For instance, how would you write to the serial console when the context 
is the guest vCPU?

> 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

I was expected the PL011 to be fully emulated. So why is this necessary?

> 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.

Per above, I am not entirely sure how you could remove the device memory 
section when using the guest vCPU.

Now about the layout between init and runtime. From previous discussion, 
you said you didn't want to have init section to be fixed because of the 
section "Xen static heap".

Furthermore, you also mention that you didn't want a bitmap. So how 
about the following for the assembly part:

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]: Early FDT
xen_mpumap[6]: Xen init data
xen_mpumap[7]: Xen init text
xen_mpumap[8]: Early UART (optional)

Then when you switch to C, you could have:

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]: Early FDT
xen_mpumap[6]: Xen init data
xen_mpumap[7]: Xen init text

xen_mpumap[max_xen_mpumap - 4]: Device memory section
xen_mpumap[max_xen_mpumap - 3]: Guest memory section
xen_mpumap[max_xen_mpumap - 2]: Static shared memory section
xen_mpumap[max_xen_mpumap - 1] : Xen static heap

And at runtime, you would keep the "Xen static heap" right at the end of 
the MPU and keep the middle entries as the switchable one.

There would be not bitmap with this solution and all the entries for the 
assembly code would be fixed.

Cheers,

Penny Zheng Feb. 1, 2023, 5:36 a.m. UTC | #8

Hi Julien

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: Tuesday, January 31, 2023 5:42 PM
> To: Penny Zheng <[e-mail]>; [e-mail]
> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>
> Subject: Re: [PATCH v2 13/40] xen/mpu: introduce unified function
> setup_early_uart to map early UART
> 
> Hi Penny,
> 
> On 31/01/2023 05:38, Penny Zheng wrote:
> >> -----Original Message-----
> >> From: Julien Grall <[e-mail]>
> >> Sent: Monday, January 30, 2023 6:00 PM
> >> To: Penny Zheng <[e-mail]>; xen-
> [e-mail]
> >> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> >> <[e-mail]>; Bertrand Marquis
> >> <[e-mail]>; Volodymyr Babchuk
> >> <[e-mail]>
> >> 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 <[e-mail]>
> >>>> Sent: Sunday, January 29, 2023 3:43 PM
> >>>> To: Penny Zheng <[e-mail]>; xen-
> >> [e-mail]
> >>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> >>>> <[e-mail]>; Bertrand Marquis
> >>>> <[e-mail]>; Volodymyr Babchuk
> >>>> <[e-mail]>
> >>>> 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 <[e-mail]>
> >>>>>> Sent: Wednesday, January 25, 2023 3:09 AM
> >>>>>> To: Penny Zheng <[e-mail]>; xen-
> >>>> [e-mail]
> >>>>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> >>>>>> <[e-mail]>; Bertrand Marquis
> >>>>>> <[e-mail]>; Volodymyr Babchuk
> >>>>>> <[e-mail]>
> >>>>>> 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 <[e-mail]>
> >>>>>>> Signed-off-by: Wei Chen <[e-mail]>
> >>>>>>> ---
> >>>>>>>      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(-)
> >>>>>>>
> > Yes, I'll draw the layout for you:
> 
> Thanks!
> 
> > '''
> >   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
> 
> In theory 1 and 2 could be merged after boot. But I guess it might be
> complicated?
> 

In theory, if in C merging codes, we do not use any read-only data or read-only-after-init
data, then, ig, it will be ok.
Since, In MPU system, when we implement C merging codes, we need to disable region 1 and 2
firstly, and enable the merged region after. The reason is that two MPU regions with address overlapping
is not allowed when MPU on.
 
> > xen_mpumap[3] : Xen read-write data
> > xen_mpumap[4] : Xen BSS
> > xen_mpumap[5] : Xen static heap
> > xen_mpumap[6] : Guest memory section
> 
> Why do you need to map the "Guest memory section" for the idle vCPU?
> 

Hmmm, "Guest memory section" here refers to *ALL* guest RAM address range with only EL2 read/write access.

For guest vcpu, this section will be replaced by guest itself own RAM with both EL1/EL2 access.


> > xen_mpumap[7] : Device memory section
> 
> I might be missing some context here. But why this section is not also
> mapped in the context of the guest vCPU?
> 
> For instance, how would you write to the serial console when the context is
> the guest vCPU?
> 

I think, as Xen itself, it shall have access to all system device memory on EL2.
Ik, it is not accurate in current MMU implementation, only devices with supported driver
will get ioremap.

But like we discussed before, if following the same strategy as MMU does, with limited
MPU regions, we could not afford mapping a MPU region for each device.
For example, On FVPv8R model, we have four uarts, and a GICv3. At most, we may provide
four MPU regions for uarts, and two MPU regions for Distributor and one Redistributor region.
So, I thought up this new device tree property “mpu,device-memory-section = <0x0 0x80000000 0x0 0x7ffff000>;“
to roughly map all system device memory for Xen itself.

For guest, it shall only see vgic, vpl011, and its own passthrough device. And here, to maintain safe and
isolation, we will be mapping a MPU region for each device for guest vcpu.
For example, vgic and vpl011 are emulated and direct-map in MPU. Relevant device
mapping(GFN == MFN with only EL2 access)will be added to its *P2M mapping table*, in vgic_v3_domain_init [1].

Later, on vcpu context switching, when switching from idle vcpu, device memory section gets disabled
and switched out in ctxt_switch_from [2], later when switching into guest vcpu, vgic and vpl011 device mapping
will be switched in along with the whole P2M mapping table [3]. 

Words might be ambiguous, but all related code implementation is on MPU patch serie part II - guest initialization, you may
have to check the gitlab link:
[1] https://gitlab.com/xen-project/people/weic/xen/-/commit/a51d5b25eb17a50a36b27987a2f48e14793ac585 
[2] https://gitlab.com/xen-project/people/weic/xen/-/commit/c6a069d777d9407aeda42b7e5b08a086a1c15976 
[3] https://gitlab.com/xen-project/people/weic/xen/-/commit/d8c6408b6eef1190d75c9bd4e58557d34fc8b4df 

> > 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
> 
> I was expected the PL011 to be fully emulated. So why is this necessary?
> 
> > 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.
> 
> Per above, I am not entirely sure how you could remove the device memory
> section when using the guest vCPU.
> 
> Now about the layout between init and runtime. From previous discussion,
> you said you didn't want to have init section to be fixed because of the
> section "Xen static heap".
> 
> Furthermore, you also mention that you didn't want a bitmap. So how
> about the following for the assembly part:
> 
> 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]: Early FDT
> xen_mpumap[6]: Xen init data
> xen_mpumap[7]: Xen init text
> xen_mpumap[8]: Early UART (optional)
> 
> Then when you switch to C, you could have:
> 
> 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]: Early FDT
> xen_mpumap[6]: Xen init data
> xen_mpumap[7]: Xen init text
> 
> xen_mpumap[max_xen_mpumap - 4]: Device memory section
> xen_mpumap[max_xen_mpumap - 3]: Guest memory section
> xen_mpumap[max_xen_mpumap - 2]: Static shared memory section
> xen_mpumap[max_xen_mpumap - 1] : Xen static heap
> 
> And at runtime, you would keep the "Xen static heap" right at the end of
> the MPU and keep the middle entries as the switchable one.
> 
> There would be not bitmap with this solution and all the entries for the
> assembly code would be fixed.
> 

That's really smart! Thanks!

I've done a little twist on your design: I've put all switchable ones in the middle
after regions defined in assembly. If near Xen heap region, I'm afraid if one day in the future,
someone is introducing another fixed region in C, we will leave holes there.

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
( Fixed MPU region defined in assembly )
--------------------------------------------------------------------------
xen_mpumap[5]: Xen init data
xen_mpumap[6]: Xen init text
xen_mpumap[7]: Early FDT
xen_mpumap[8]: Guest memory section
xen_mpumap[9]: Device memory section
xen_mpumap[10]: Static shared memory section
( boot-only and switching regions defined in C )
--------------------------------------------------------------------------
...
xen_mpumap[max_xen_mpumap - 1] : Xen static heap
( Fixed MPU region defined in C )
--------------------------------------------------------------------------

After re-org:
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
( Fixed MPU region defined in assembly )
--------------------------------------------------------------------------
xen_mpumap[8]: Guest memory section
xen_mpumap[9]: Device memory section
xen_mpumap[10]: Static shared memory section
( Switching region )
--------------------------------------------------------------------------
...
xen_mpumap[max_xen_mpumap - 1] : Xen static heap
( Fixed MPU region defined in C )

If you're fine with it, then next serie, I'll use this layout, to keep both
simple assembly and re-org process.

> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng

Julien Grall Feb. 1, 2023, 7:26 p.m. UTC | #9

On 01/02/2023 05:36, Penny Zheng wrote:
> Hi Julien

Hi Penny,

> 
>> -----Original Message-----
>> From: Julien Grall <[e-mail]>
>> Sent: Tuesday, January 31, 2023 5:42 PM
>> To: Penny Zheng <[e-mail]>; [e-mail]
>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Bertrand Marquis <[e-mail]>;
>> Volodymyr Babchuk <[e-mail]>
>> Subject: Re: [PATCH v2 13/40] xen/mpu: introduce unified function
>> setup_early_uart to map early UART
>>
>> Hi Penny,
>>
>> On 31/01/2023 05:38, Penny Zheng wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall <[e-mail]>
>>>> Sent: Monday, January 30, 2023 6:00 PM
>>>> To: Penny Zheng <[e-mail]>; xen-
>> [e-mail]
>>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>>>> <[e-mail]>; Bertrand Marquis
>>>> <[e-mail]>; Volodymyr Babchuk
>>>> <[e-mail]>
>>>> 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 <[e-mail]>
>>>>>> Sent: Sunday, January 29, 2023 3:43 PM
>>>>>> To: Penny Zheng <[e-mail]>; xen-
>>>> [e-mail]
>>>>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>>>>>> <[e-mail]>; Bertrand Marquis
>>>>>> <[e-mail]>; Volodymyr Babchuk
>>>>>> <[e-mail]>
>>>>>> 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 <[e-mail]>
>>>>>>>> Sent: Wednesday, January 25, 2023 3:09 AM
>>>>>>>> To: Penny Zheng <[e-mail]>; xen-
>>>>>> [e-mail]
>>>>>>>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>>>>>>>> <[e-mail]>; Bertrand Marquis
>>>>>>>> <[e-mail]>; Volodymyr Babchuk
>>>>>>>> <[e-mail]>
>>>>>>>> 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 <[e-mail]>
>>>>>>>>> Signed-off-by: Wei Chen <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>       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(-)
>>>>>>>>>
>>> Yes, I'll draw the layout for you:
>>
>> Thanks!
>>
>>> '''
>>>    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
>>
>> In theory 1 and 2 could be merged after boot. But I guess it might be
>> complicated?
>>
> 
> In theory, if in C merging codes, we do not use any read-only data or read-only-after-init
> data, then, ig, it will be ok.
> Since, In MPU system, when we implement C merging codes, we need to disable region 1 and 2
> firstly, and enable the merged region after. The reason is that two MPU regions with address overlapping
> is not allowed when MPU on.

Good to know! I think it should be feasible to avoid accessing read-only 
variable while doing the merge.

Anyway, this looks more like a potential optimization for the future.

>   
>>> xen_mpumap[3] : Xen read-write data
>>> xen_mpumap[4] : Xen BSS
>>> xen_mpumap[5] : Xen static heap
>>> xen_mpumap[6] : Guest memory section
>>
>> Why do you need to map the "Guest memory section" for the idle vCPU?
>>
> 
> Hmmm, "Guest memory section" here refers to *ALL* guest RAM address range with only EL2 read/write access.

For what purpose? Earlier, you said you had a setup with a limited 
number of MPU entries. So it may not be possible to map all the guests RAM.

Xen should only need to access the guest memory in hypercalls and 
scrubbing. In both cases you could map/unmap on demand.

> 
> For guest vcpu, this section will be replaced by guest itself own RAM with both EL1/EL2 access.
> 
> 
>>> xen_mpumap[7] : Device memory section
>>
>> I might be missing some context here. But why this section is not also
>> mapped in the context of the guest vCPU?
>>
>> For instance, how would you write to the serial console when the context is
>> the guest vCPU?
>>
> 
> I think, as Xen itself, it shall have access to all system device memory on EL2.
> Ik, it is not accurate in current MMU implementation, only devices with supported driver
> will get ioremap.

So in the MMU case, we are not mapping all the devices in Xen because we 
don't exactly know which memory attributes will be used by the guest.

If we are using different attributes, then we are risking to break 
coherency. Could the same issue happen with the MPU?

If so, then you should not mapped those regions in Xen.

> 
> But like we discussed before, if following the same strategy as MMU does, with limited
> MPU regions, we could not afford mapping a MPU region for each device.
> For example, On FVPv8R model, we have four uarts, and a GICv3. At most, we may provide
> four MPU regions for uarts, and two MPU regions for Distributor and one Redistributor region.
> So, I thought up this new device tree property “mpu,device-memory-section = <0x0 0x80000000 0x0 0x7ffff000>;“
> to roughly map all system device memory for Xen itself.

Why do you say "roughly"? Is it possible that you have non-device region 
in the range?

> 
> For guest, it shall only see vgic, vpl011, and its own passthrough device. And here, to maintain safe and
> isolation, we will be mapping a MPU region for each device for guest vcpu.
> For example, vgic and vpl011 are emulated and direct-map in MPU. Relevant device

I am confused. If the vGIC/vPL011 is emulated then why do you need to 
map it in the MPU? IOW, wouldn't you receive a fault in the hypervisor 
if the guest is trying to access a region not present in the MPU?

> mapping(GFN == MFN with only EL2 access)will be added to its *P2M mapping table*, in vgic_v3_domain_init [1].
> 
> Later, on vcpu context switching, when switching from idle vcpu, device memory section gets disabled
> and switched out in ctxt_switch_from [2], later when switching into guest vcpu, vgic and vpl011 device mapping
> will be switched in along with the whole P2M mapping table [3].
> 
> Words might be ambiguous, but all related code implementation is on MPU patch serie part II - guest initialization, you may
> have to check the gitlab link:
> [1] https://gitlab.com/xen-project/people/weic/xen/-/commit/a51d5b25eb17a50a36b27987a2f48e14793ac585
> [2] https://gitlab.com/xen-project/people/weic/xen/-/commit/c6a069d777d9407aeda42b7e5b08a086a1c15976
> [3] https://gitlab.com/xen-project/people/weic/xen/-/commit/d8c6408b6eef1190d75c9bd4e58557d34fc8b4df

I have looked at the code and this doesn't entirely answer my question. 
So let me provide an example.

Xen can print to the serial console at any time. So Xen should be able 
to access the physical UART even when it has context switched to the 
guest vCPU.

But above you said that the physical device would not be accessible and 
instead you map the virtual UART. So how Xen is supported to access the 
physical UART?

Or by vpl011 did you actually mean the physical UART? If so, then if you 
map the device one by one in the MPU context, then it would likely mean 
to have space to map them one by one in the idle context.

> 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
> ( Fixed MPU region defined in assembly )
> --------------------------------------------------------------------------
> xen_mpumap[5]: Xen init data
> xen_mpumap[6]: Xen init text
> xen_mpumap[7]: Early FDT
> xen_mpumap[8]: Guest memory section
> xen_mpumap[9]: Device memory section
> xen_mpumap[10]: Static shared memory section
> ( boot-only and switching regions defined in C )
> --------------------------------------------------------------------------
> ...
> xen_mpumap[max_xen_mpumap - 1] : Xen static heap
> ( Fixed MPU region defined in C )
> --------------------------------------------------------------------------
> 
> After re-org:
> 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
> ( Fixed MPU region defined in assembly )
> --------------------------------------------------------------------------
> xen_mpumap[8]: Guest memory section
> xen_mpumap[9]: Device memory section
> xen_mpumap[10]: Static shared memory section
> ( Switching region )
> --------------------------------------------------------------------------
> ...
> xen_mpumap[max_xen_mpumap - 1] : Xen static heap
> ( Fixed MPU region defined in C )
> 
> If you're fine with it, then next serie, I'll use this layout, to keep both
> simple assembly and re-org process.

I am ok in principle with the layout you propose. My main requirement is 
that the region used in assembly are fixed.

Cheers,

Penny Zheng Feb. 2, 2023, 8:05 a.m. UTC | #10

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: Thursday, February 2, 2023 3:27 AM
> To: Penny Zheng <[e-mail]>; [e-mail]
> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Bertrand Marquis <[e-mail]>;
> Volodymyr Babchuk <[e-mail]>
> Subject: Re: [PATCH v2 13/40] xen/mpu: introduce unified function
> setup_early_uart to map early UART
> 
> 
> 
> On 01/02/2023 05:36, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
>

Hi Julien,
 
> >
[...]
> >>> xen_mpumap[3] : Xen read-write data
> >>> xen_mpumap[4] : Xen BSS
> >>> xen_mpumap[5] : Xen static heap
> >>> xen_mpumap[6] : Guest memory section
> >>
> >> Why do you need to map the "Guest memory section" for the idle vCPU?
> >>
> >
> > Hmmm, "Guest memory section" here refers to *ALL* guest RAM address
> range with only EL2 read/write access.
> 
> For what purpose? Earlier, you said you had a setup with a limited number
> of MPU entries. So it may not be possible to map all the guests RAM.
> 

The "Guest memory section" I referred here is the memory section defined in my new
introducing device tree property, "mpu,guest-memory-section = <...>".  It will include
"ALL" guest memory.

Let me find an example to illustrate why I introduced it and how it shall work:
In MPU system, all guest RAM *MUST* be statically configured through "xen,static-mem" under domain node.
We found that with more and more guests in,  the scattering of  "xen,static-mem" may
exhaust MPU regions very quickly. TBH, at that time, I didn't figure out that I could map/unmap on demand.
And in MMU system, We will never encounter this issue, setup_directmap_mappings will map the whole system
RAM to the directmap area for Xen to access in EL2. 
So instead, "mpu,guest-memory-section" is introduced to limit the scattering and map in advance, we enforce
users to ensure all guest RAM(through "xen,static-mem") must be included within "mpu,guest-memory-section".
e.g.
mpu,guest-memory-section = <0x0 0x20000000 0x0 0x40000000>;
DomU1:
xen,static-mem = <0x0 0x40000000 0x0 0x20000000>;
DomU2:
xen,static-mem = <0x0 0x20000000 0x0 0x20000000>;

> Xen should only need to access the guest memory in hypercalls and
> scrubbing. In both cases you could map/unmap on demand.
> 

thanks for the explanation.
In my understanding, during boot-up, there are two spots where Xen may touch the guest memory:
one is that doing synchronous scrubbing in function unprepare_staticmem_pages.
Another is coping and pasting kernel image to guest memory.
In both cases, we could map/unmap on demand. 
And if you think map/unmap on demand is better than "mpu,guest-memory-section", I'll try to fix it in next serie
 
> >
> > For guest vcpu, this section will be replaced by guest itself own RAM with
> both EL1/EL2 access.
> >
> >
> >>> xen_mpumap[7] : Device memory section
> >>
> >> I might be missing some context here. But why this section is not
> >> also mapped in the context of the guest vCPU?
> >>
> >> For instance, how would you write to the serial console when the
> >> context is the guest vCPU?
> >>
> >
> > I think, as Xen itself, it shall have access to all system device memory on
> EL2.
> > Ik, it is not accurate in current MMU implementation, only devices
> > with supported driver will get ioremap.
> 
> So in the MMU case, we are not mapping all the devices in Xen because we
> don't exactly know which memory attributes will be used by the guest.
> 
> If we are using different attributes, then we are risking to break coherency.
> Could the same issue happen with the MPU?
> 
> If so, then you should not mapped those regions in Xen.
> 
> >
> > But like we discussed before, if following the same strategy as MMU
> > does, with limited MPU regions, we could not afford mapping a MPU
> region for each device.
> > For example, On FVPv8R model, we have four uarts, and a GICv3. At
> > most, we may provide four MPU regions for uarts, and two MPU regions
> for Distributor and one Redistributor region.
> > So, I thought up this new device tree property
> > “mpu,device-memory-section = <0x0 0x80000000 0x0 0x7ffff000>;“ to
> roughly map all system device memory for Xen itself.
> 
> Why do you say "roughly"? Is it possible that you have non-device region in
> the range?
> 
> >
> > For guest, it shall only see vgic, vpl011, and its own passthrough
> > device. And here, to maintain safe and isolation, we will be mapping a
> MPU region for each device for guest vcpu.
> > For example, vgic and vpl011 are emulated and direct-map in MPU.
> > Relevant device
> 
> I am confused. If the vGIC/vPL011 is emulated then why do you need to
> map it in the MPU? IOW, wouldn't you receive a fault in the hypervisor if
> the guest is trying to access a region not present in the MPU?
> 
> > mapping(GFN == MFN with only EL2 access)will be added to its *P2M
> mapping table*, in vgic_v3_domain_init [1].
> >
> > Later, on vcpu context switching, when switching from idle vcpu,
> > device memory section gets disabled and switched out in
> > ctxt_switch_from [2], later when switching into guest vcpu, vgic and
> vpl011 device mapping will be switched in along with the whole P2M
> mapping table [3].
> >
> > Words might be ambiguous, but all related code implementation is on
> > MPU patch serie part II - guest initialization, you may have to check the
> gitlab link:
> > [1]
> > https://gitlab.com/xen-project/people/weic/xen/-
> /commit/a51d5b25eb17a5
> > 0a36b27987a2f48e14793ac585 [2]
> > https://gitlab.com/xen-project/people/weic/xen/-
> /commit/c6a069d777d940
> > 7aeda42b7e5b08a086a1c15976 [3]
> > https://gitlab.com/xen-project/people/weic/xen/-
> /commit/d8c6408b6eef11
> > 90d75c9bd4e58557d34fc8b4df
> 
> I have looked at the code and this doesn't entirely answer my question.
> So let me provide an example.
> 
> Xen can print to the serial console at any time. So Xen should be able to
> access the physical UART even when it has context switched to the guest
> vCPU.
> 

I understand your concern on "device memory section" with your
Example here. True, the current implementation is buggy.

Yes, if vpl011 is not enabled in guest and we instead passthrough a UART to
guest, in current design, Xen is not able to access the physical UART on guest mode.

All guests in MPU are direct-map, So like you said after, the mapping for
vpl011 on guest mode is the same with the physical UART.  And this hides the problem, to
let Xen being able to access to the physical UART.

I'll drop the design on "device memory section", and let device driver map
on demand in boot-time.
  
> But above you said that the physical device would not be accessible and
> instead you map the virtual UART. So how Xen is supported to access the
> physical UART?
> 
> Or by vpl011 did you actually mean the physical UART? If so, then if you
> map the device one by one in the MPU context, then it would likely mean
> to have space to map them one by one in the idle context.
> 
> > 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 ( Fixed MPU region defined in
> > assembly )
> > ----------------------------------------------------------------------
> > ----
> > xen_mpumap[5]: Xen init data
> > xen_mpumap[6]: Xen init text
> > xen_mpumap[7]: Early FDT
> > xen_mpumap[8]: Guest memory section
> > xen_mpumap[9]: Device memory section
> > xen_mpumap[10]: Static shared memory section ( boot-only and switching
> > regions defined in C )
> > ----------------------------------------------------------------------
> > ----
> > ...
> > xen_mpumap[max_xen_mpumap - 1] : Xen static heap ( Fixed MPU region
> > defined in C )
> > ----------------------------------------------------------------------
> > ----
> >
> > After re-org:
> > 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 ( Fixed MPU region defined in
> > assembly )
> > ----------------------------------------------------------------------
> > ----
> > xen_mpumap[8]: Guest memory section
> > xen_mpumap[9]: Device memory section
> > xen_mpumap[10]: Static shared memory section ( Switching region )
> > ----------------------------------------------------------------------
> > ----
> > ...
> > xen_mpumap[max_xen_mpumap - 1] : Xen static heap ( Fixed MPU region
> > defined in C )
> >
> > If you're fine with it, then next serie, I'll use this layout, to keep
> > both simple assembly and re-org process.
> 
> I am ok in principle with the layout you propose. My main requirement is
> that the region used in assembly are fixed.
> 
> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng

Julien Grall Feb. 2, 2023, 11:11 a.m. UTC | #11

Hi Penny,

On 02/02/2023 08:05, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <[e-mail]>
>> Sent: Thursday, February 2, 2023 3:27 AM
>> To: Penny Zheng <[e-mail]>; [e-mail]
>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Bertrand Marquis <[e-mail]>;
>> Volodymyr Babchuk <[e-mail]>
>> Subject: Re: [PATCH v2 13/40] xen/mpu: introduce unified function
>> setup_early_uart to map early UART
>>
>>
>>
>> On 01/02/2023 05:36, Penny Zheng wrote:
>>> Hi Julien
>>
>> Hi Penny,
>>
> 
> Hi Julien,
>   
>>>
> [...]
>>>>> xen_mpumap[3] : Xen read-write data
>>>>> xen_mpumap[4] : Xen BSS
>>>>> xen_mpumap[5] : Xen static heap
>>>>> xen_mpumap[6] : Guest memory section
>>>>
>>>> Why do you need to map the "Guest memory section" for the idle vCPU?
>>>>
>>>
>>> Hmmm, "Guest memory section" here refers to *ALL* guest RAM address
>> range with only EL2 read/write access.
>>
>> For what purpose? Earlier, you said you had a setup with a limited number
>> of MPU entries. So it may not be possible to map all the guests RAM.
>>
> 
> The "Guest memory section" I referred here is the memory section defined in my new
> introducing device tree property, "mpu,guest-memory-section = <...>".  It will include
> "ALL" guest memory.
> 
> Let me find an example to illustrate why I introduced it and how it shall work:
> In MPU system, all guest RAM *MUST* be statically configured through "xen,static-mem" under domain node.
> We found that with more and more guests in,  the scattering of  "xen,static-mem" may
> exhaust MPU regions very quickly. TBH, at that time, I didn't figure out that I could map/unmap on demand.
> And in MMU system, We will never encounter this issue, setup_directmap_mappings will map the whole system
> RAM to the directmap area for Xen to access in EL2.
> So instead, "mpu,guest-memory-section" is introduced to limit the scattering and map in advance, we enforce
> users to ensure all guest RAM(through "xen,static-mem") must be included within "mpu,guest-memory-section".
> e.g.
> mpu,guest-memory-section = <0x0 0x20000000 0x0 0x40000000>;
> DomU1:
> xen,static-mem = <0x0 0x40000000 0x0 0x20000000>;
> DomU2:
> xen,static-mem = <0x0 0x20000000 0x0 0x20000000>;
> 
>> Xen should only need to access the guest memory in hypercalls and
>> scrubbing. In both cases you could map/unmap on demand.
>>
> 
> thanks for the explanation.
> In my understanding, during boot-up, there are two spots where Xen may touch the guest memory:
> one is that doing synchronous scrubbing in function unprepare_staticmem_pages.
> Another is coping and pasting kernel image to guest memory.
> In both cases, we could map/unmap on demand.
> And if you think map/unmap on demand is better than "mpu,guest-memory-section", I'll try to fix it in next serie

I think it would be better for a few reasons:
  1) You are making the assumption that all the RAM for the guests are 
contiguous. This may not be true for various reason (i.e. split bank...).
  2) It reduces the amount of work for the integrator
  3) You increase the defense in the hypervisor but it is more difficult 
to access the guest memory if there is a breakage

I don't expect major rework because you could plug the update to the MPU 
in map_domain_page().

>> I have looked at the code and this doesn't entirely answer my question.
>> So let me provide an example.
>>
>> Xen can print to the serial console at any time. So Xen should be able to
>> access the physical UART even when it has context switched to the guest
>> vCPU.
>>
> 
> I understand your concern on "device memory section" with your
> Example here. True, the current implementation is buggy.
> 
> Yes, if vpl011 is not enabled in guest and we instead passthrough a UART to
> guest, in current design, Xen is not able to access the physical UART on guest mode.

So all the guests are using the same UART?

Cheers,

Patch

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)
 #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.
+ *
+ * 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.
+     */
+    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
+
+    ldr x29, =(CONFIG_EARLY_UART_BASE_ADDRESS + EARLY_UART_SIZE)
+    roundup_section x29
+    /* 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
+    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
 
 #else