Re: [PATCH v7 7/8] xen/arm: Rename init_secondary_pagetables() to prepare_secondary_mm()

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 09/10/2023 02:03, Henry Wang wrote:
> From: Penny Zheng <penny.zheng@xxxxxxx>
>
> init_secondary_pagetables() is a function in the common code path
> of both MMU and future MPU support. Since "page table" is a MMU
> specific concept, rename init_secondary_pagetables() to a generic
> name prepare_secondary_mm() as the preparation for MPU support.
>
> 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>
> ---
> v7:
> - No change.
> v6:
> - Only rename init_secondary_pagetables() to prepare_secondary_mm().
> ---
>   xen/arch/arm/arm32/head.S     | 2 +-
>   xen/arch/arm/include/asm/mm.h | 8 +++++---
>   xen/arch/arm/mmu/smpboot.c    | 4 ++--
>   xen/arch/arm/smpboot.c        | 2 +-
>   4 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 39218cf15f..c7b2efb8f0 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -257,7 +257,7 @@ 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 prepare_secondary_mm.
>            *
>            * XXX: This is not compliant with the Arm Arm.
>            */
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index d23ebc7df6..db6d889826 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -204,9 +204,11 @@ 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_ttbr to the new page table.
> + */
AFAIU, with the renaming, you are trying to make the call MMU/MPU 
agnostic. But the comment is still very tailored to the MPU. I would 
consider to move the comment to mmu/smpboot.c and replace this one with 
a generic comment. Something like:
/* Prepare the memory subystem to bring-up the given secondary CPU. */

Cheers,

--
Julien Grall