Re: [PATCH for-4.22 v2 1/2] xen/arm: split DTB/initrd placement helpers

["Orzel, Michal" <michal.orzel@xxxxxxx>]

We are in the RC phase, so please CC Oleksii (release manager) as we need his
tags for fixes. Adding him now.

On 05-Jun-26 07:19, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> 
> The Arm zImage loader currently computes the kernel load address and
> places the DTB/initrd in one local flow. The hardware-domain memory
> allocator needs to reuse those placement rules before it chooses bank 0,
> but open-coding the same calculations there would make the fix harder to
> audit.
> 
> Split the existing logic into small helpers:
> - kernel_zimage_place_in_bank() computes the zImage load address for a
>   given bank.
> - first_bank_can_fit_modules() checks the aggregate first-bank
>   footprint.
> - find_dtb_initrd_placement() chooses the DTB/initrd location within a
>   known bank and kernel range.
> 
> Rename place_modules() to place_dtb_initrd() so the code distinguishes
> the kernel image from the DTB/initrd placement area. Also update the
> stale xg_dom_arm.c path in the placement comment.
> 
> The caller still panics in the same cases as before, so this is intended
> to be behavior preserving.
> 
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
> Changes in v2:
> - New patch split out from the hardware-domain first-bank fix.
> - Rename the DTB/initrd placement helpers to avoid treating the kernel
>   and DTB/initrd as the same kind of module.
> - Pass the RAM end address to find_dtb_initrd_placement() instead of
>   recomputing it from the RAM size.
> - Update the stale xg_dom_arm.c reference in the placement comment.
> ---
>  xen/arch/arm/kernel.c                 | 147 ++++++++++++++++----------
>  xen/common/device-tree/domain-build.c |   6 +-
>  2 files changed, 97 insertions(+), 56 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index b72585b7fe..d1be4d8074 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -40,27 +40,59 @@ struct minimal_dtb_header {
>      /* There are other fields but we don't use them yet. */
>  };
>  
> -static void __init place_modules(struct kernel_info *info,
> -                                 paddr_t kernbase, paddr_t kernend)
> +static paddr_t __init
> +kernel_zimage_place_in_bank(const struct kernel_info *info,
> +                            paddr_t bank_start, paddr_t bank_size)
>  {
> -    /* Align DTB and initrd size to 2Mb. Linux only requires 4 byte 
> alignment */
> -    const struct boot_module *mod = info->bd.initrd;
> -    const struct membanks *mem = kernel_info_get_mem(info);
> -    const paddr_t initrd_len = ROUNDUP(mod ? mod->size : 0, MB(2));
> -    const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
> -    const paddr_t modsize = initrd_len + dtb_len;
> +    paddr_t load_addr;
>  
> -    /* Convenient */
> -    const paddr_t rambase = mem->bank[0].start;
> -    const paddr_t ramsize = mem->bank[0].size;
> -    const paddr_t ramend = rambase + ramsize;
> +#ifdef CONFIG_HAS_DOMAIN_TYPE
> +    if ( (info->type == DOMAIN_64BIT) && (info->image.start == 0) )
> +        return bank_start + info->image.text_offset;
> +#endif
> +
> +    /*
> +     * If start is zero, the zImage is position independent, in this
> +     * case Documentation/arm/Booting recommends loading below 128MiB
> +     * and above 32MiB. Load it as high as possible within these
> +     * constraints, while also avoiding the DTB.
> +     */
> +    if ( info->image.start == 0 )
> +    {
> +        paddr_t load_end;
> +
> +        load_end = bank_start + bank_size;
> +        load_end = MIN(bank_start + MB(128), load_end);
> +
> +        load_addr = load_end - info->image.len;
> +        /* Align to 2MB */
> +        load_addr &= ~(MB(2) - 1);
For the future mechanical changes, you should not be making even such tiny
changes like s/(2 << 20)/MB(2) without mentioning them in commit msg. For today,
it's ok.

> +    }
> +    else
> +        load_addr = info->image.start;
> +
> +    return load_addr;
> +}
> +
> +static bool __init first_bank_can_fit_modules(paddr_t ramsize,
> +                                              paddr_t kernbase, paddr_t 
> kernend,
> +                                              paddr_t dtb_initrd_size)
> +{
>      const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
> -    const paddr_t ram128mb = rambase + MB(128);
>  
> -    paddr_t modbase;
> +    /*
> +     * Check only the aggregate kernel + DTB/initrd footprint. The actual
> +     * DTB/initrd location is selected by find_dtb_initrd_placement().
> +     */
> +    return dtb_initrd_size + kernsize <= ramsize;
> +}
>  
> -    if ( modsize + kernsize > ramsize )
> -        panic("Not enough memory in the first bank for the 
> kernel+dtb+initrd\n");
> +static bool __init find_dtb_initrd_placement(paddr_t rambase, paddr_t ramend,
> +                                             paddr_t kernbase, paddr_t 
> kernend,
> +                                             paddr_t dtb_initrd_size,
> +                                             paddr_t *dtb_base)
> +{
> +    const paddr_t ram128mb = rambase + MB(128);
>  
>      /*
>       * DTB must be loaded such that it does not conflict with the
> @@ -77,55 +109,64 @@ static void __init place_modules(struct kernel_info 
> *info,
>       * just before the kernel.
>       *
>       * If changing this then consider
> -     * tools/libxc/xc_dom_arm.c:arch_setup_meminit as well.
> +     * tools/libs/guest/xg_dom_arm.c:meminit as well.
This fixes the kernel.c -> tools pointer, but the reverse comment in
xg_dom_arm.c:meminit still points at place_modules. This needs to be fixed.

I think this can be done on commit, therefore:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal