Re: [PATCH v5 09/13] xen/arm: mm: Use generic variable/function names for extendability

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/08/2023 05:25, Henry Wang wrote:
> From: Penny Zheng <penny.zheng@xxxxxxx>
>
> As preparation for MPU support, which will use some variables/functions
> for both MMU and MPU system, We rename the affected variable/function
> to more generic names:
> - init_ttbr -> init_mm,
You moved init_ttbr to mm/mmu.c. So why does this need to be renamed?

And if you really planned to use it for the MPU code. Then init_ttbr 
should not have been moved.
> - mmu_init_secondary_cpu() -> mm_init_secondary_cpu()
> - init_secondary_pagetables() -> init_secondary_mm()
The original naming were not great but the new one are a lot more 
confusing as they seem to just be a reshuffle of word.
mm_init_secondary_cpu() is only setting the WxN bit. For the MMU, I 
think it can be done much earlier. Do you have anything to add in it? If 
not, then I would consider to get rid of it.
For init_secondary_mm(), I would renamed it to prepare_secondary_mm().

> - Add a wrapper update_mm_mapping() for MMU system's
>    update_identity_mapping()
>
> Modify the related in-code comment to reflect above changes, take the
> opportunity to fix the incorrect coding style of the in-code comments.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> ---
> v5:
> - Rebase on top of xen/arm: mm: add missing extern variable declaration
> v4:
> - Extract the renaming part from the original patch:
>    "[v3,13/52] xen/mmu: extract mmu-specific codes from mm.c/mm.h"
> ---
>   xen/arch/arm/arm32/head.S           |  4 ++--
>   xen/arch/arm/arm64/mmu/head.S       |  2 +-
>   xen/arch/arm/arm64/mmu/mm.c         | 11 ++++++++---
>   xen/arch/arm/arm64/smpboot.c        |  6 +++---
>   xen/arch/arm/include/asm/arm64/mm.h |  7 ++++---
>   xen/arch/arm/include/asm/mm.h       | 12 +++++++-----
>   xen/arch/arm/mmu/mm.c               | 20 ++++++++++----------
>   xen/arch/arm/smpboot.c              |  4 ++--
>   8 files changed, 37 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 33b038e7e0..03ab68578a 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -238,11 +238,11 @@ GLOBAL(init_secondary)
>   secondary_switched:
>           /*
>            * Non-boot CPUs need to move on to the proper pagetables, which were
> -         * setup in init_secondary_pagetables.
> +         * setup in init_secondary_mm.
>            *
>            * XXX: This is not compliant with the Arm Arm.
>            */
> -        mov_w r4, init_ttbr          /* VA of HTTBR value stashed by CPU 0 */
> +        mov_w r4, init_mm            /* VA of HTTBR value stashed by CPU 0 */
>           ldrd  r4, r5, [r4]           /* Actual value */
>           dsb
>           mcrr  CP64(r4, r5, HTTBR)
> diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
> index ba2ddd7e67..58d91c9088 100644
> --- a/xen/arch/arm/arm64/mmu/head.S
> +++ b/xen/arch/arm/arm64/mmu/head.S
> @@ -302,7 +302,7 @@ ENDPROC(enable_mmu)
>   ENTRY(enable_secondary_cpu_mm)
>           mov   x5, lr
>   
> -        load_paddr x0, init_ttbr
> +        load_paddr x0, init_mm
>           ldr   x0, [x0]
>   
>           bl    enable_mmu
> diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
> index 78b7c7eb00..ed0fc5ff7b 100644
> --- a/xen/arch/arm/arm64/mmu/mm.c
> +++ b/xen/arch/arm/arm64/mmu/mm.c
> @@ -106,7 +106,7 @@ void __init arch_setup_page_tables(void)
>       prepare_runtime_identity_mapping();
>   }
>   
> -void update_identity_mapping(bool enable)
> +static void update_identity_mapping(bool enable)
Why not simply renaming this function to update_mm_mapping()? But...

>   {
>       paddr_t id_addr = virt_to_maddr(_start);
>       int rc;
> @@ -120,6 +120,11 @@ void update_identity_mapping(bool enable)
>       BUG_ON(rc);
>   }
>   
> +void update_mm_mapping(bool enable)
... the new name it quite confusing. What is the mapping it is referring to?

If you don't want to keep update_identity_mapping(), then I would 
consider to simply wrap...
> +{
> +    update_identity_mapping(enable);
> +}
> +
>   extern void switch_ttbr_id(uint64_t ttbr);
>   
>   typedef void (switch_ttbr_fn)(uint64_t ttbr);
> @@ -131,7 +136,7 @@ void __init switch_ttbr(uint64_t ttbr)
>       lpae_t pte;
>   
>       /* Enable the identity mapping in the boot page tables */
> -    update_identity_mapping(true);
> +    update_mm_mapping(true);
>   
>       /* Enable the identity mapping in the runtime page tables */
>       pte = pte_of_xenaddr((vaddr_t)switch_ttbr_id);
> @@ -148,7 +153,7 @@ void __init switch_ttbr(uint64_t ttbr)
>        * Note it is not necessary to disable it in the boot page tables
>        * because they are not going to be used by this CPU anymore.
>        */
> -    update_identity_mapping(false);
> +    update_mm_mapping(false);
>   }
>   
>   /*
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 9637f42469..2b1d086a1e 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -111,18 +111,18 @@ int arch_cpu_up(int cpu)
>       if ( !smp_enable_ops[cpu].prepare_cpu )
>           return -ENODEV;
>   
> -    update_identity_mapping(true);
> +    update_mm_mapping(true);
... with #ifdef CONFIG_MMU here...

>   
>       rc = smp_enable_ops[cpu].prepare_cpu(cpu);
>       if ( rc )
> -        update_identity_mapping(false);
> +        update_mm_mapping(false);
... here and ...


>   
>       return rc;
>   }
>   
>   void arch_cpu_up_finish(void)
>   {
> -    update_identity_mapping(false);
> +    update_mm_mapping(false);
... here.

>   }
>   
>   /*
> diff --git a/xen/arch/arm/include/asm/arm64/mm.h 
> b/xen/arch/arm/include/asm/arm64/mm.h
> index e0bd23a6ed..7a389c4b21 100644
> --- a/xen/arch/arm/include/asm/arm64/mm.h
> +++ b/xen/arch/arm/include/asm/arm64/mm.h
> @@ -15,13 +15,14 @@ static inline bool arch_mfns_in_directmap(unsigned long 
> mfn, unsigned long nr)
>   void arch_setup_page_tables(void);
>   
>   /*
> - * Enable/disable the identity mapping in the live page-tables (i.e.
> - * the one pointed by TTBR_EL2).
> + * In MMU system, enable/disable the identity mapping in the live
> + * page-tables (i.e. the one pointed by TTBR_EL2) through
> + * update_identity_mapping().
>    *
>    * Note that nested call (e.g. enable=true, enable=true) is not
>    * supported.
>    */
> -void update_identity_mapping(bool enable);
> +void update_mm_mapping(bool enable);
>   
>   #endif /* __ARM_ARM64_MM_H__ */
>   
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index dc1458b047..8084c62c01 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -170,7 +170,7 @@ struct page_info
>   #define PGC_need_scrub    PGC_allocated
>   
>   /* Non-boot CPUs use this to find the correct pagetables. */
> -extern uint64_t init_ttbr;
> +extern uint64_t init_mm;
>   
>   #ifdef CONFIG_ARM_32
>   #define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
> @@ -205,11 +205,13 @@ extern void setup_pagetables(unsigned long 
> boot_phys_offset);
>   extern void *early_fdt_map(paddr_t fdt_paddr);
>   /* Remove early mappings */
>   extern void remove_early_mappings(void);
> -/* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to 
> the
> - * new page table */
> -extern int init_secondary_pagetables(int cpu);
> +/*
> + * Allocate and initialise pagetables for a secondary CPU. Sets init_mm to the
> + * new page table
> + */
> +extern int init_secondary_mm(int cpu);
>   /* Switch secondary CPUS to its own pagetables and finalise MMU setup */
Regardless what I wrote above, this comment is not accurate anymore as 
we don't switch the page tables for the secondary CPUs. We are only 
enable WxN.
In any case, this comment would need to be reworded to be more generic.

Cheers,

--
Julien Grall