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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Aug 22, 2023, at 16:42, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.

I agree to all your above comments, and sure I can wait for a few days for other
maintainers opinion before start moving the code.

> 
>>>> 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 *.

Ahhh thanks for the context, I totally forgot the Doxygen…Then I will use
"/**” in v6.

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall