Re: [PATCH v5 12/13] xen/arm: mmu: relocate copy_from_paddr() to setup.c

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 23/08/2023 01:10, Stefano Stabellini wrote:
> On Tue, 22 Aug 2023, Julien Grall wrote:
> > > > I also don't like the idea of having again a massive mm.c files. So maybe
> > > > we need a split like:
> > > >    * File 1: Boot CPU0 MM bringup (mmu/setup.c)
> > > >    * File 2: Secondary CPUs MM bringup (mmu/smpboot.c)
> > > >    * File 3: Page tables update. (mmu/pt.c)
> > > >
> > > > Ideally file 1 should contain only init code/data so it can be marked as
> > > > .init. So the static pagetables may want to be defined in mmu/pt.c.
> > > So based on Julien’s suggestion, Penny and I worked a bit on the current
> > > functions in “arch/arm/mm.c” and we would like to propose below split
> > > scheme, would you please comment on if below makes sense to you,
> > > thanks!
> > >
> > > """
> > > static void __init __maybe_unused build_assertions()      -> arch/arm/mm.c
> > All the existing build assertions seems to be MMU specific. So shouldn't they
> > be moved to mmu/mm.c.
> >
> > > static lpae_t *xen_map_table()                            -> mmu/pt.c
> > > static void xen_unmap_table()                             -> mmu/pt.c
> > > void dump_pt_walk()                                       -> mmu/pt.c
> > > void dump_hyp_walk()                                      -> mmu/pt.c
> > > lpae_t mfn_to_xen_entry()                                 -> mmu/pt.c
> > > void set_fixmap()                                         -> mmu/pt.c
> > > void clear_fixmap()                                       -> mmu/pt.c
> > > void flush_page_to_ram()                                  -> arch/arm/mm.c?
> > I think it should stay in arch/arm/mm.c because you will probably need to
> > clean a page even on MPU systems.
> I take you are referring to flush_page_to_ram() only, and not the other
> functions above
That's correct.

> > > lpae_t pte_of_xenaddr()                                   -> mmu/pt.c
> > > void * __init early_fdt_map()                             -> mmu/setup.c
> > > void __init remove_early_mappings()                       -> mmu/setup.c
> > > static void xen_pt_enforce_wnx()                          -> mmu/pt.c,
> > > export it
> > AFAIU, it would be called from smpboot.c and setup.c. For the former, the
> > caller is mmu_init_secondary_cpu() which I think can be folded in head.S.
> >
> > If we do that, then xen_pt_enforce_wnx() can be moved in setup.c and doesn't
> > need to be exported.
> >
> > > static void clear_table()                                 -> mmu/smpboot.c
> > > void __init setup_pagetables()                            -> mmu/setup.c
> > > static void clear_boot_pagetables()                       -> mmu/smpboot.c
> Why clear_table() and clear_boot_pagetables() in mmu/smpboot.c rather
> than mmu/setup.c ? It is OK either way as it does seem to make much of a
> difference but I am curious.
I initially wondered the same. But then I didn't comment because 
clear_boot_pagetables() is only used in order to prepare the page-tables 
for the secondary boot CPU.
Also, even if today we don't support CPU hotplug, there is nothing 
preventing us to do (in fact there was a series on the ML for that). 
This means clear_table() & co would need to be outside of the init 
section and we would need to remove the check that all 
variables/functions defined in setup.c are residing in it.
Saying that, we should not need to clear the boot page tables anymore 
for arm64 because secondary CPUs will directly jump to the runtime 
page-tables. So the code could be cleaned up a bit. Anyway, this is not 
a request for this series and could be done afterwards by someone else.
Cheers,

--
Julien Grall