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

Commit Message

Henry Wang Aug. 14, 2023, 4:25 a.m. UTC

From: Penny Zheng <[e-mail]>

Function copy_from_paddr() is defined in asm/setup.h, so it is better
to be implemented in setup.c.

Current copy_from_paddr() implementation is mmu-specific, so this
commit moves copy_from_paddr() into mmu/setup.c, and it is also
benefical for us to implement MPU version of copy_from_paddr() in
later commit.

Signed-off-by: Penny Zheng <[e-mail]>
Signed-off-by: Wei Chen <[e-mail]>
Signed-off-by: Henry Wang <[e-mail]>
---
v5:
- No change
v4:
- No change
v3:
- new commit
---
 xen/arch/arm/kernel.c    | 27 ---------------------------
 xen/arch/arm/mmu/setup.c | 27 +++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 27 deletions(-)

Comments

Julien Grall Aug. 21, 2023, 9:31 p.m. UTC | #1

Hi,

On 14/08/2023 05:25, Henry Wang wrote:
> From: Penny Zheng <[e-mail]>
> 
> 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.

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.

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?

> + * 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)
> +{
> +    void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
> +
> +    while (len) {
> +        unsigned long l, s;
> +
> +        s = paddr & (PAGE_SIZE-1);
> +        l = min(PAGE_SIZE - s, len);
> +
> +        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
> +        memcpy(dst, src + s, l);
> +        clean_dcache_va_range(dst, l);
> +        clear_fixmap(FIXMAP_MISC);
> +
> +        paddr += l;
> +        dst += l;
> +        len -= l;
> +    }
> +}
> +
>   /*
>    * Local variables:
>    * mode: C

Cheers,

Henry Wang Aug. 22, 2023, 7:44 a.m. UTC | #2

Hi Julien, Stefano, Bertrand,

> On Aug 22, 2023, at 05:31, Julien Grall <[e-mail]> wrote:
> 
> Hi,
> 
> On 14/08/2023 05:25, Henry Wang wrote:
>> From: Penny Zheng <[e-mail]>
>> 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

Julien Grall Aug. 22, 2023, 8:42 a.m. UTC | #3

On 22/08/2023 08:44, Henry Wang wrote:
>> On Aug 22, 2023, at 05:31, Julien Grall <[e-mail]> wrote:
>>
>> Hi,
>>
>> On 14/08/2023 05:25, Henry Wang wrote:
>>> From: Penny Zheng <[e-mail]>
>>> 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,

Henry Wang Aug. 22, 2023, 8:54 a.m. UTC | #4

Hi Julien,

> On Aug 22, 2023, at 16:42, Julien Grall <[e-mail]> wrote:
> 
> On 22/08/2023 08:44, Henry Wang wrote:
>>> On Aug 22, 2023, at 05:31, Julien Grall <[e-mail]> wrote:
>>> 
>>> Hi,
>>> 
>>> On 14/08/2023 05:25, Henry Wang wrote:
>>>> From: Penny Zheng <[e-mail]>
>>>> 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

Stefano Stabellini Aug. 23, 2023, 12:10 a.m. UTC | #5

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!

Henry Wang Aug. 23, 2023, 12:58 a.m. UTC | #6

Hi Stefano,

> On Aug 23, 2023, at 08:10, Stefano Stabellini <[e-mail]> 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

Julien Grall Aug. 23, 2023, 5:59 p.m. UTC | #7

Hi Stefano,

On 23/08/2023 01:10, Stefano Stabellini 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

That's correct.

> 
> 
>>> 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 initially wondered the same. But then I didn't comment because 
clear_boot_pagetables() is only used in order to prepare the page-tables 
for the secondary boot CPU.

Also, even if today we don't support CPU hotplug, there is nothing 
preventing us to do (in fact there was a series on the ML for that). 
This means clear_table() & co would need to be outside of the init 
section and we would need to remove the check that all 
variables/functions defined in setup.c are residing in it.

Saying that, we should not need to clear the boot page tables anymore 
for arm64 because secondary CPUs will directly jump to the runtime 
page-tables. So the code could be cleaned up a bit. Anyway, this is not 
a request for this series and could be done afterwards by someone else.

Cheers,

Patch

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 508c54824d..0d433a32e7 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -41,33 +41,6 @@  struct minimal_dtb_header {
 
 #define DTB_MAGIC 0xd00dfeedU
 
-/**
- * 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)
-{
-    void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
-
-    while (len) {
-        unsigned long l, s;
-
-        s = paddr & (PAGE_SIZE-1);
-        l = min(PAGE_SIZE - s, len);
-
-        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
-        memcpy(dst, src + s, l);
-        clean_dcache_va_range(dst, l);
-        clear_fixmap(FIXMAP_MISC);
-
-        paddr += l;
-        dst += l;
-        len -= l;
-    }
-}
-
 static void __init place_modules(struct kernel_info *info,
                                  paddr_t kernbase, paddr_t kernend)
 {
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
 
+/*
+ * 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)
+{
+    void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
+
+    while (len) {
+        unsigned long l, s;
+
+        s = paddr & (PAGE_SIZE-1);
+        l = min(PAGE_SIZE - s, len);
+
+        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
+        memcpy(dst, src + s, l);
+        clean_dcache_va_range(dst, l);
+        clear_fixmap(FIXMAP_MISC);
+
+        paddr += l;
+        dst += l;
+        len -= l;
+    }
+}
+
 /*
  * Local variables:
  * mode: C