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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien, Stefano, Bertrand,

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

> 
> 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
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?
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
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?
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
"""

> 
> Bertrand, Stefano, any thoughts?
> 
> [...]
> 
>> 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.

Kind regards,
Henry

> 
>> + * copy_from_paddr - copy data from a physical address
>> + * @dst: destination virtual address
>> + * @paddr: source physical address
>> + * @len: length to copy
>> + */
>> +void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
>> +{
>> 
> 
> Cheers,
> 
> -- 
> Julien Grall