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

[Julien Grall <julien@xxxxxxx>]

On 22/08/2023 08:44, Henry Wang wrote:
> > On Aug 22, 2023, at 05:31, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi,
> >
> > On 14/08/2023 05:25, Henry Wang wrote:
> > > From: Penny Zheng <Penny.Zheng@xxxxxxx>
> > > Function copy_from_paddr() is defined in asm/setup.h, so it is better
> > > to be implemented in setup.c.
> > I don't agree with this reasoning. We used setup.h to declare prototype for 
> > function that are out of setup.c.
> >
> > Looking at the overall of this series, it is unclear to me what is the 
> > difference between mmu/mm.c and mmu/setup.c. I know this is technically not a 
> > new problem but as we split the code, it would be a good opportunity to have a 
> > better split.
> >
> > For instance, we have setup_mm() defined in setup.c but setup_pagetables() and 
> > mm.c. Both are technically related to the memory management. So having them in 
> > separate file is a bit odd.
> Skimming through the comments, it looks like we have a common problem
> in patch 7, 9, 10, 12 about how to move/split the code. So instead of having
> the discussion in each patch, I would like to propose that we can discuss all
> of these in a common place here.
+1.

> > 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.
> 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
> 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.
> > > diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> > > index e05cca3f86..889ada6b87 100644
> > > --- a/xen/arch/arm/mmu/setup.c
> > > +++ b/xen/arch/arm/mmu/setup.c
> > > @@ -329,6 +329,33 @@ void __init setup_mm(void)
> > >   }
> > >   #endif
> > >   +/*
> > Why did the second '*' disappear?
> According to the CODING_STYLE, we should use something like this:
>
> /*
>   * Example, multi-line comment block.
>   *
>   * Note beginning and end markers on separate lines and leading '*'.
>   */
>
> Instead of "/**” in the beginning. But I think you made a point, I need
> to mention that I took the opportunity to fix the comment style in commit
> message.
We have started to use /** which I think is for Doxygen (see the PDX 
series). So I think the CODING_STYLE needs to be updated rather than 
removing the extra *.
Cheers,

--
Julien Grall