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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


> > int init_secondary_pagetables()                           -> mmu/smpboot.c
> > void mmu_init_secondary_cpu()                             -> mmu/smpboot.c
> > void __init setup_directmap_mappings()                    -> mmu/setup.c
> > void __init setup_frametable_mappings()                   -> mmu/setup.c
> > void *__init arch_vmap_virt_end()                         -> arch/arm/mm.c
> > or mmu/setup.c?
> 
> AFAIU, the VMAP will not be used for MPU systems. So this would want to go in
> mmu/. I am ok with setup.c.
> 
> > void *ioremap_attr()                                      -> mmu/pt.c
> > void *ioremap()                                           -> mmu/pt.c
> > static int create_xen_table()                             -> mmu/pt.c
> > static int xen_pt_next_level()                            -> mmu/pt.c
> > static bool xen_pt_check_entry()                          -> mmu/pt.c
> > static int xen_pt_update_entry()                          -> mmu/pt.c
> > static int xen_pt_mapping_level()                         -> mmu/pt.c
> > static unsigned int xen_pt_check_contig()                 -> mmu/pt.c
> > static int xen_pt_update()                                -> mmu/pt.c
> > int map_pages_to_xen()                                    -> mmu/pt.c
> > int __init populate_pt_range()                            -> mmu/pt.c
> > int destroy_xen_mappings()                                -> mmu/pt.c
> > int modify_xen_mappings()                                 -> mmu/pt.c
> > void free_init_memory()                                   -> mmu/setup.c
> > void arch_dump_shared_mem_info()                          -> arch/arm/mm.c
> > int steal_page()                                          -> arch/arm/mm.c
> > int page_is_ram_type()                                    -> arch/arm/mm.c
> > unsigned long domain_get_maximum_gpfn()                   -> arch/arm/mm.c
> > void share_xen_page_with_guest()                          -> arch/arm/mm.c
> > int xenmem_add_to_physmap_one()                           -> arch/arm/mm.c
> > long arch_memory_op()                                     -> arch/arm/mm.c
> > static struct domain *page_get_owner_and_nr_reference()   -> arch/arm/mm.c
> > struct domain *page_get_owner_and_reference()             -> arch/arm/mm.c
> > void put_page_nr()                                        -> arch/arm/mm.c
> > void put_page()                                           -> arch/arm/mm.c
> > bool get_page_nr()                                        -> arch/arm/mm.c
> > bool get_page()                                           -> arch/arm/mm.c
> > int get_page_type()                                       -> arch/arm/mm.c
> > void put_page_type()                                      -> arch/arm/mm.c
> > int create_grant_host_mapping()                           -> arch/arm/mm.c
> > int replace_grant_host_mapping()                          -> arch/arm/mm.c
> > bool is_iomem_page()                                      -> arch/arm/mm.c
> > void clear_and_clean_page()                               -> arch/arm/mm.c
> > unsigned long get_upper_mfn_bound()                       -> arch/arm/mm.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 am looking forward to this!