| Message ID | 20231009010313.3668423-8-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Split MMU code as the prepration of MPU work | expand |
Hi Henry, On 09/10/2023 02:03, Henry Wang wrote: > From: Penny Zheng <penny.zheng@arm.com> > > 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@arm.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > 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,
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@arm.com> >> >> 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@arm.com> >> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >> --- >> 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. */ I forgot to mention. With that: Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
Hi Julien, > On Oct 14, 2023, at 01:48, Julien Grall <julien@xen.org> 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@arm.com> >>> >>> 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@arm.com> >>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>> --- >>> 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@amazon.com> Thanks! Kind regards, Henry > > Cheers, > > -- > Julien Grall
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. + */ +extern int prepare_secondary_mm(int cpu); /* Map a frame table to cover physical addresses ps through pe */ extern void setup_frametable_mappings(paddr_t ps, paddr_t pe); /* map a physical range in virtual memory */ diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c index eac805c74c..f05c53a087 100644 --- a/xen/arch/arm/mmu/smpboot.c +++ b/xen/arch/arm/mmu/smpboot.c @@ -67,7 +67,7 @@ static void clear_boot_pagetables(void) } #ifdef CONFIG_ARM_64 -int init_secondary_pagetables(int cpu) +int prepare_secondary_mm(int cpu) { clear_boot_pagetables(); @@ -80,7 +80,7 @@ int init_secondary_pagetables(int cpu) return 0; } #else -int init_secondary_pagetables(int cpu) +int prepare_secondary_mm(int cpu) { lpae_t *first; diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index beb137d06e..ac451e9b3e 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -448,7 +448,7 @@ int __cpu_up(unsigned int cpu) printk("Bringing up CPU%d\n", cpu); - rc = init_secondary_pagetables(cpu); + rc = prepare_secondary_mm(cpu); if ( rc < 0 ) return rc;