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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Michal,

Thanks for the review.

On Fri, Jun 5, 2026 at 10:32 AM Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>
> We are in the RC phase, so please CC Oleksii (release manager) as we need his
> tags for fixes. Adding him now.

Ack, I will keep Oleksii Cc'ed for this series.

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

Got it.

>
> > +    }
> > +    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>

Ack on the reverse pointer in xg_dom_arm.c. It should point at
place_dtb_initrd() now. I am fine with this being fixed on commit, but
since I will send a new revision for the comments on patch 2, I will
fold this update into patch 1 as well.

Best regards,
Mykola