Re: [PATCH v3 05/52] xen/arm64: head: Introduce enable_boot_mmu and enable_runtime_mmu

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Julien

On 2023/7/5 05:24, Julien Grall wrote:
> Hi Penny,
>
> On 26/06/2023 04:33, Penny Zheng wrote:
> > From: Wei Chen <wei.chen@xxxxxxx>
> >
> > At the moment, on MMU system, enable_mmu() will return to an
> > address in the 1:1 mapping, then each path is responsible to
> > switch to virtual runtime mapping. Then remove_identity_mapping()
> > is called to remove all 1:1 mapping.
> The identity mapping is only removed for the boot CPU. You mention it 
> correctly below but here it lead to think it may be called on the 
> secondary CPU. So I would add 'on the boot CPU'.
> > Since remove_identity_mapping() is not necessary on Non-MMU system,
> > and we also avoid creating empty function for Non-MMU system, trying
> > to keep only one codeflow in arm64/head.S, we move path switch and
> > remove_identity_mapping() in enable_mmu() on MMU system.
> >
> > As the remove_identity_mapping should only be called for the boot
> > CPU only, so we introduce enable_boot_mmu for boot CPU and
> > enable_runtime_mmu for secondary CPUs in this patch.
> NIT: Add () after enable_runtime_mmu to be consistent with the rest of 
> commit message. Same for the title.
sure.

> Also, I would prefer if we name the functions properly from the start. 
> So we don't have to rename them when they are moved in patch 7.
ok. change them to enable_runtime_mm() and enable_boot_mm() at the beginning

> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> > v3:
> > - new patch
> > ---
> >   xen/arch/arm/arm64/head.S | 87 +++++++++++++++++++++++++++++++--------
> >   1 file changed, 70 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 10a07db428..4dfbe0bc6f 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -314,21 +314,12 @@ real_start_efi:
> >           bl    check_cpu_mode
> >           bl    cpu_init
> > -        bl    create_page_tables
> > -        load_paddr x0, boot_pgtable
> > -        bl    enable_mmu
> >           /* We are still in the 1:1 mapping. Jump to the runtime 
> > Virtual Address. */
> This comment is not accurate anymore given that the MMU is off.
I'll remove.

> > -        ldr   x0, =primary_switched
> > -        br    x0
> > +        ldr   lr, =primary_switched
> > +        b     enable_boot_mmu
> > +
> >   primary_switched:
> > -        /*
> > -         * The 1:1 map may clash with other parts of the Xen virtual 
> > memory
> > -         * layout. As it is not used anymore, remove it completely to
> > -         * avoid having to worry about replacing existing mapping
> > -         * afterwards.
> > -         */
> > -        bl    remove_identity_mapping
> >           bl    setup_fixmap
> >   #ifdef CONFIG_EARLY_PRINTK
> >           /* Use a virtual address to access the UART. */
> > @@ -373,13 +364,11 @@ GLOBAL(init_secondary)
> >   #endif
> >           bl    check_cpu_mode
> >           bl    cpu_init
> > -        load_paddr x0, init_ttbr
> > -        ldr   x0, [x0]
> > -        bl    enable_mmu
> >           /* We are still in the 1:1 mapping. Jump to the runtime 
> > Virtual Address. */
Note: Remove this too.

> > -        ldr   x0, =secondary_switched
> > -        br    x0
> > +        ldr   lr, =secondary_switched
> > +        b     enable_runtime_mmu
> > +
> >   secondary_switched:
> >   #ifdef CONFIG_EARLY_PRINTK
> >           /* Use a virtual address to access the UART. */
> > @@ -694,6 +683,70 @@ enable_mmu:
> >           ret
> >   ENDPROC(enable_mmu)
> > +/*
> > + * Turn on the Data Cache and the MMU. The function will return
> > + * to the virtual address provided in LR (e.g. the runtime mapping).
> The description here is exactly the same as below. However, there is a 
> different between the two functions. One is to deal with the secondary 
> CPUs whilst the second is for the boot CPUs.
I'll add-on: This function deals with the secondary CPUs.

> > + *
> > + * Inputs:
> > + *   lr : Virtual address to return to.
> > + *
> > + * Clobbers x0 - x5
> > + */
> > +enable_runtime_mmu:
> I find "runtime" confusing in this case. How about 
> "enable_secondary_cpu_mm"?
Sure, will do.

> > +        mov   x5, lr
> > +
> > +        load_paddr x0, init_ttbr
> > +        ldr   x0, [x0]
> > +
> > +        bl    enable_mmu
> > +        mov   lr, x5
> > +
> > +        /* return to secondary_switched */
> > +        ret
> > +ENDPROC(enable_runtime_mmu)
> > +
> > +/*
> > + * Turn on the Data Cache and the MMU. The function will return
> > + * to the virtual address provided in LR (e.g. the runtime mapping).
> Similar remark as for the comment above.
I'll add-on: This function deals with the boot CPUs.

> > + *
> > + * Inputs:
> > + *   lr : Virtual address to return to.
> > + *
> > + * Clobbers x0 - x5
> > + */
> > +enable_boot_mmu:
> Based on my comment above, I would sugesst to call it "enable_boot_cpu_mm"
sure,

> > +        mov   x5, lr
> > +
> > +        bl    create_page_tables
> > +        load_paddr x0, boot_pgtable
> > +
> > +        bl    enable_mmu
> > +        mov   lr, x5
> > +
> > +        /*
> > +         * The MMU is turned on and we are in the 1:1 mapping. Switch
> > +         * to the runtime mapping.
> > +         */
> > +        ldr   x0, =1f
> > +        br    x0
> > +1:
> > +        /*
> > +         * The 1:1 map may clash with other parts of the Xen virtual 
> > memory
> > +         * layout. As it is not used anymore, remove it completely to
> > +         * avoid having to worry about replacing existing mapping
> > +         * afterwards. Function will return to primary_switched.
> > +         */
> > +        b     remove_identity_mapping
> > +
> > +        /*
> > +         * Here might not be reached, as "ret" in 
> > remove_identity_mapping
> > +         * will use the return address in LR in advance. But keep ret 
> > here
> > +         * might be more safe if "ret" in remove_identity_mapping is 
> > removed
> > +         * in future.
> > +         */
> > +        ret
> Given this path is meant to be unreachable, I would prefer if we call 
> "fail".
sure,

> > +ENDPROC(enable_boot_mmu)
> > +
> >   /*
> >    * Remove the 1:1 map from the page-tables. It is not easy to keep 
> > track
> >    * where the 1:1 map was mapped, so we will look for the top-level 
> > entry
> Cheers,