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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Stefano,

> On Aug 23, 2023, at 08:10, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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
> 
> 
>>> 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 think it is because below call sequence:
init_secondary_mm() -> clear_boot_pagetables() -> clear_table()

We have the suggestion from Julien that:
"File 2: Secondary CPUs MM bringup (mmu/smpboot.c)”, and hence
I suggested the smpboot.c

>> 
>> The placement of all the ones I didn't comment on look fine to me. It would 
>> be
>> good to have a second opinion from either Bertrand or Stefano before you 
>> start
>> moving the functions.
> 
> It looks good in principle and it also looks like a great clean up. It
> is not always super clear to me the distinction between mmu/pt.c,
> mmu/smpboot.c and mmu/setup.c but it doesn't seem important. The
> distinction between mmu/* and arch/arm/mm.c is clear and looks fine to
> me.

I generally followed:
"* 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)"

> 
> I am looking forward to this!

+1, will post the updated patch soon!

Kind regards,
Henry