RE: [PATCH v6 10/11] xen/arm64: introduce helpers for MPU enable/disable

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Julien Grall
> Sent: Monday, November 7, 2022 4:56 AM
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Penny Zheng <Penny.Zheng@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v6 10/11] xen/arm64: introduce helpers for MPU
> enable/disable
> 
> Hi Wei,
> 
> On 04/11/2022 10:07, Wei Chen wrote:
> > From: Penny Zheng <penny.zheng@xxxxxxx>
> >
> > We need some helpers for Xen to enable/disable MPU in boot-time and
> > runtime. For MPU enable helper, we know that it's an essential
> > requirement of MPU system. But for MPU disable, we need to use it for
> > some special situations. For example, in the progress of tranferring
> > from boot-time to runtime, we need to update the MPU protection
> > regions configuration, but we can't modify an MPU protection region if
> > there is some data accessed by Xen. But in boot-time all of Xen text,
> > data and BSS are in one MPU protection region, if Xen want to update
> > this protection region, above restriction will be triggered.
> 
> This raises the following question: Why can't we create the split regions 
> right
> now?
> 

The reason why we are not creating the split regions right now is that we
are trying to go the same path MMU goes. Then we could reuse as much
same interfaces as we could, in order to not leave #ifdef CONFIG_HAS_MPU
all over the place.

> In particular, disabling the MMU/Cache is fairly risky because you need to
> ensure that anything in the cache you care about have been written back to
> the RAM).
>

Hope I could understand your concern totally, you are worrying about stale info 
left in
the cache, even if it's always 1:1 mapping on the MPU system, memory attributes 
could
be different before and after? 
So it is never enough that we only flush the variables which we will use during 
the disabling
time, it should be everything in the cache...:/

Since in current design, there are two time points in boot time where we will 
disable
MPU/Cache to configure MPU.

One is in setup_mm, here, we will map XEN components by components, each MPU 
memory
region for a different component.
The other is near the end of boot time, we will reorg the whole MPU memory 
region layout
before going runtime, and we will keep unchanging regions in the front and 
flexible ones in the rear.
Otherwise it is hard and complex to maintain on runtime, especially during 
context switch.

Unchanging regions like xen text will not change during vcpu context switch. 
Flexible regions, like
guest memory region, will display different contents during vcpu context switch.

> > So in this situation, we need to disable the whole MPU to update the
> > protection regions.
> >
> > In these helper, enable/disable MPU will also enable/disable the
> > D-cache. There are two reasons for it:
> > 1. Make the function semantic be consistent with enable_mmu.
> >     For MMU systems, enable_mmu will turn MMU and D-Cache at
> >     the same time.
> > 2. When MPU is disabled, the MPU background attributes will
> >     be used. On some platforms, the background will treat all
> >     memory as device memory. The access to device memory will
> >     bypass the cache, even if the C bit is enabled in SCTLR.
> >     To avoid this implicit behavior, we disable cache with MPU
> >     explicitly to tell user that when MPU is disabled, the
> >     memory access is uncacheable.
> >
> > In this patch, we also introduce a neutral name enable_mm for Xen to
> > enable MMU/MPU. This can help us to keep one code flow in head.S
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >   xen/arch/arm/arm64/head.S     |  5 +++--
> >   xen/arch/arm/arm64/head_mmu.S |  4 ++--
> >   xen/arch/arm/arm64/head_mpu.S | 35
> +++++++++++++++++++++++++++++++++++
> >   3 files changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 6c1a5f74a1..228f01db69 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -255,7 +255,8 @@ real_start_efi:
> >            * and protection regions for MPU systems.
> >            */
> >           bl    prepare_early_mappings
> > -        bl    enable_mmu
> > +        /* Turn on MMU or MPU */
> > +        bl    enable_mm
> >
> >           /* We are still in the 1:1 mapping. Jump to the runtime Virtual
> Address. */
> >           ldr   x0, =primary_switched
> > @@ -313,7 +314,7 @@ GLOBAL(init_secondary)
> >           bl    check_cpu_mode
> >           bl    cpu_init
> >           bl    prepare_early_mappings
> > -        bl    enable_mmu
> > +        bl    enable_mm
> >
> >           /* We are still in the 1:1 mapping. Jump to the runtime Virtual
> Address. */
> >           ldr   x0, =secondary_switched
> > diff --git a/xen/arch/arm/arm64/head_mmu.S
> > b/xen/arch/arm/arm64/head_mmu.S index fc64819a98..b542755bd2
> 100644
> > --- a/xen/arch/arm/arm64/head_mmu.S
> > +++ b/xen/arch/arm/arm64/head_mmu.S
> > @@ -217,7 +217,7 @@ ENDPROC(prepare_early_mappings)
> >    *
> >    * Clobbers x0 - x3
> >    */
> > -ENTRY(enable_mmu)
> > +ENTRY(enable_mm)
> >           PRINT("- Turning on paging -\r\n")
> >
> >           /*
> > @@ -239,7 +239,7 @@ ENTRY(enable_mmu)
> >           msr   SCTLR_EL2, x0          /* now paging is enabled */
> >           isb                          /* Now, flush the icache */
> >           ret
> > -ENDPROC(enable_mmu)
> > +ENDPROC(enable_mm)
> >
> >   /*
> >    * Remove the 1:1 map from the page-tables. It is not easy to keep
> > track diff --git a/xen/arch/arm/arm64/head_mpu.S
> > b/xen/arch/arm/arm64/head_mpu.S index f60611b556..5a1b03e293
> 100644
> > --- a/xen/arch/arm/arm64/head_mpu.S
> > +++ b/xen/arch/arm/arm64/head_mpu.S
> > @@ -68,3 +68,38 @@ ENTRY(prepare_early_mappings)
> >
> >       ret
> >   ENDPROC(prepare_early_mappings)
> > +
> > +/*
> > + * Enable EL2 MPU and data cache. Because we will disable cache
> > + * with MPU at the same time, in accordance with that, we have
> > + * to enable cache with MPU at the same time in this function.
> > + * When MPU is disabled, the MPU background attributes will
> > + * be used. On some platform, the background will treat all
> > + * memory as IO memory.
> 
> I was under the impression that all access would be treated as Device
> Memory when the MMU is off. Isn't it the case for the MPU?
> 

Yes, without background region. Right now, background region is disabled
here.
But if the Background region is enabled, then the MPU uses the default memory
map as the Background region for generating the memory attributes when MPU is 
disabled.
And it is also IMPLENMENTATION DEFINED~

> Also, I think the correct wording is "device memory" rather than "IO
> memory".
> 
> > The access to IO memory will bypass
> 
> Ditto.
> 
> > + * the cache, even you have enabled the C bit in SCTLR.
> > + * To avoid this implicit behavior, we disable cache with MPU
> > + * explicitly to tell user that when MPU is disabled, the memory
> > + * access is uncacheable.
> > + */
> > +ENTRY(enable_mm)
> > +    mrs   x0, SCTLR_EL2
> > +    mov   x1, #(SCTLR_Axx_ELx_M | SCTLR_Axx_ELx_C)
> > +    /* Enable EL2 MPU and D-cache */
> > +    orr   x0, x0, x1
> > +    dsb   sy
> > +    msr   SCTLR_EL2, x0
> > +    isb
> > +    ret
> > +ENDPROC(enable_mm)
> > +
> > +/* Disable MPU system, including data cache. */
> > +ENTRY(disable_mm)
> 
> I would rather not introduce this function until there is a caller. This is
> because, I believe, there are some assumptions on the state of the cache
> before we can turn off the MMU. So I would like to see the caller in order to
> assess whether this function makes sense.
> 
> Cheers,
> 
> --
> Julien Grall