Re: [PATCH 26/36] xen/arm: add argument to remove_early_mappings

[Julien Grall <julien@xxxxxxx>]

Hi Marco,

On 04/03/2022 17:46, Marco Solieri wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> Upcoming patches will need to remove temporary mappings created during
> Xen coloring process. The function remove_early_mappings does what we
> need but it is case-specific. Parametrize the function to avoid code
> replication.
>
> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
>   xen/arch/arm/include/asm/mm.h | 2 +-
>   xen/arch/arm/mm.c             | 8 ++++----
>   xen/arch/arm/setup.c          | 3 ++-
>   3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 9ac1767595..041ec4ee70 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -184,7 +184,7 @@ extern void setup_pagetables(unsigned long 
> boot_phys_offset, paddr_t xen_paddr);
>   /* Map FDT in boot pagetable */
>   extern void *early_fdt_map(paddr_t fdt_paddr);
>   /* Remove early mappings */
> -extern void remove_early_mappings(void);
> +extern void remove_early_mappings(unsigned long va, unsigned long size);
>   /* Allocate and initialise pagetables for a secondary CPU. Sets init_ttbr to 
> the
>    * new page table */
>   extern int init_secondary_pagetables(int cpu);
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fd7a313d88..d69f18b5d2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -597,13 +597,13 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>       return fdt_virt;
>   }
>   
> -void __init remove_early_mappings(void)
> +void __init remove_early_mappings(unsigned long va, unsigned long size)
>   {
>       lpae_t pte = {0};
> -    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
> -    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + SZ_2M),
> +    write_pte(xen_second + second_table_offset(va), pte);
> +    write_pte(xen_second + second_table_offset(va + size),
>                 pte);
The original goal of this code was to remove 2 entries. Each entry 
covering 2MB.
Anyone calling with size == 2MB will expect a single mapping to be 
removed. But 4MB worth of memory will be removed.
Effectively, remove_early_mappings() is not generic enough to be 
parametrized. I also don't think this function should be parametrized.
The goal is to remove any mappings that was created during early boot.

I will have a look at how you use it before making any suggestions.

Cheers,

--
Julien Grall