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

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall