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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Oct 14, 2023, at 01:48, Julien Grall <julien@xxxxxxx> wrote:
> 
> On 13/10/2023 18:47, Julien Grall wrote:
>> 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. */

Good suggestion! I will follow this in v8.

> 
> I forgot to mention. With that:
> 
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall