Re: [PATCH v10 11/12] xen/arm: make consider_modules() available for xen relocation

[Michal Orzel <michal.orzel@xxxxxxx>]

On 19/11/2024 15:13, Carlo Nonato wrote:
> 
> 
> Cache coloring must physically relocate Xen in order to color the hypervisor
> and consider_modules() is a key function that is needed to find a new
> available physical address.
> 
> 672d67f339c0 ("xen/arm: Split MMU-specific setup_mm() and related code out")
> moved consider_modules() under arm32. Move it to mmu/setup.c and make it
> non-static so that it can be used outside.
> 
> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> ---
> v10:
> - no changes
> v9:
> - no changes
> v8:
> - patch adapted to new changes to consider_modules()
> v7:
> - moved consider_modules() to arm/mmu/setup.c
> v6:
> - new patch
> ---
>  xen/arch/arm/arm32/mmu/mm.c      | 95 +------------------------------
>  xen/arch/arm/include/asm/setup.h |  3 +
>  xen/arch/arm/mmu/setup.c         | 97 ++++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+), 94 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
> index 063611412b..c5fcd19291 100644
> --- a/xen/arch/arm/arm32/mmu/mm.c
> +++ b/xen/arch/arm/arm32/mmu/mm.c
> @@ -9,6 +9,7 @@
>  #include <asm/fixmap.h>
>  #include <asm/static-memory.h>
>  #include <asm/static-shmem.h>
> +#include <asm/setup.h>
Sort alphabetically.

> 
>  static unsigned long opt_xenheap_megabytes __initdata;
>  integer_param("xenheap_megabytes", opt_xenheap_megabytes);
> @@ -31,100 +32,6 @@ static void __init setup_directmap_mappings(unsigned long 
> base_mfn,
>      directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
>  }
> 
> -/*
> - * Returns the end address of the highest region in the range s..e
> - * with required size and alignment that does not conflict with the
> - * modules from first_mod to nr_modules.
> - *
> - * For non-recursive callers first_mod should normally be 0 (all
> - * modules and Xen itself) or 1 (all modules but not Xen).
> - */
> -static paddr_t __init consider_modules(paddr_t s, paddr_t e,
> -                                       uint32_t size, paddr_t align,
> -                                       int first_mod)
> -{
> -    const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
> -#ifdef CONFIG_STATIC_SHM
> -    const struct membanks *shmem = bootinfo_get_shmem();
> -#endif
> -    const struct bootmodules *mi = &bootinfo.modules;
> -    int i;
> -    int nr;
> -
> -    s = (s+align-1) & ~(align-1);
> -    e = e & ~(align-1);
> -
> -    if ( s > e ||  e - s < size )
> -        return 0;
> -
> -    /* First check the boot modules */
> -    for ( i = first_mod; i < mi->nr_mods; i++ )
> -    {
> -        paddr_t mod_s = mi->module[i].start;
> -        paddr_t mod_e = mod_s + mi->module[i].size;
> -
> -        if ( s < mod_e && mod_s < e )
> -        {
> -            mod_e = consider_modules(mod_e, e, size, align, i+1);
> -            if ( mod_e )
> -                return mod_e;
> -
> -            return consider_modules(s, mod_s, size, align, i+1);
> -        }
> -    }
> -
> -    /*
> -     * i is the current bootmodule we are evaluating, across all
> -     * possible kinds of bootmodules.
> -     *
> -     * When retrieving the corresponding reserved-memory addresses, we
> -     * need to index the reserved_mem bank starting from 0, and only counting
> -     * the reserved-memory modules. Hence, we need to use i - nr.
> -     */
> -    nr = mi->nr_mods;
> -    for ( ; i - nr < reserved_mem->nr_banks; i++ )
> -    {
> -        paddr_t r_s = reserved_mem->bank[i - nr].start;
> -        paddr_t r_e = r_s + reserved_mem->bank[i - nr].size;
> -
> -        if ( s < r_e && r_s < e )
> -        {
> -            r_e = consider_modules(r_e, e, size, align, i + 1);
> -            if ( r_e )
> -                return r_e;
> -
> -            return consider_modules(s, r_s, size, align, i + 1);
> -        }
> -    }
> -
> -#ifdef CONFIG_STATIC_SHM
> -    nr += reserved_mem->nr_banks;
> -    for ( ; i - nr < shmem->nr_banks; i++ )
> -    {
> -        paddr_t r_s, r_e;
> -
> -        r_s = shmem->bank[i - nr].start;
> -
> -        /* Shared memory banks can contain INVALID_PADDR as start */
> -        if ( INVALID_PADDR == r_s )
> -            continue;
> -
> -        r_e = r_s + shmem->bank[i - nr].size;
> -
> -        if ( s < r_e && r_s < e )
> -        {
> -            r_e = consider_modules(r_e, e, size, align, i + 1);
> -            if ( r_e )
> -                return r_e;
> -
> -            return consider_modules(s, r_s, size, align, i + 1);
> -        }
> -    }
> -#endif
> -
> -    return e;
> -}
> -
>  /*
>   * Find a contiguous region that fits in the static heap region with
>   * required size and alignment, and return the end address of the region
> diff --git a/xen/arch/arm/include/asm/setup.h 
> b/xen/arch/arm/include/asm/setup.h
> index 64c227d171..0c560d141f 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -89,6 +89,9 @@ struct init_info
>      unsigned int cpuid;
>  };
> 
> +paddr_t consider_modules(paddr_t s, paddr_t e, uint32_t size, paddr_t align,
> +                         int first_mod);
> +
>  #endif
>  /*
>   * Local variables:
> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> index 9664e85ee6..1cf62390e3 100644
> --- a/xen/arch/arm/mmu/setup.c
> +++ b/xen/arch/arm/mmu/setup.c
> @@ -6,7 +6,10 @@
>   */
> 
>  #include <xen/init.h>
> +#include <xen/lib.h>
What's in lib.h that consider_modules() requires?

>  #include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt-xen.h>
Why? AFAICS, this header defines fdt_get_mem_rsv_paddr() which is not used in 
this file.

> +#include <xen/llc-coloring.h>
Why? There's nothing LLC related here at this point.

With the header inclusions sorted out:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal