Re: [PATCH for-4.22 v3 2/2] device-tree: validate hwdom bank 0 boot placement

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

On 08-Jun-26 07:39, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> 
> With LLC coloring enabled, the hardware domain memory is allocated by
> allocate_hwdom_memory() rather than by using the fixed direct-map layout.
> 
> Commit de99f3263555 ("device-tree: Improve hwdom memory allocation for
> DMA") made that allocator prefer lower host regions. The first-bank
> filter, however, still only checked the old 128MB heuristic. A low region
> can satisfy that heuristic but still be too small, or otherwise
> unsuitable, for the hardware-domain kernel and the DTB/initrd area to fit
> in bank 0 according to the Arm placement rules.
> 
> Keep the existing first-bank size policy and add an architecture-specific
> candidate check. On Arm, compute the kernel load address for the
> candidate bank using the same logic as kernel_zimage_place(), verify that
> the kernel range is covered by that bank, and then reuse the same
> DTB/initrd placement helper as place_dtb_initrd(). The FDT is generated
> later, so use the hardware-domain FDT allocation size as a conservative
> upper bound for the final DTB size.
> 
> Check the candidate after capping the host region by the remaining
> unassigned hardware-domain memory, so the validation is performed against
> the size that would actually become bank 0.
> 
> This keeps the DMA-oriented allocation policy from de99f3263555 while
> preventing a too-small bank 0 from reaching place_dtb_initrd().
> 
> Make kernel_zimage_place_in_bank() return INVALID_PADDR when a
> position-independent zImage cannot be placed in the supplied bank; the
> real load path turns this into a panic, while the hwdom candidate check
> uses it to reject the bank.
> 
> Fixes: de99f3263555 ("device-tree: Improve hwdom memory allocation for DMA")
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in v3:
> - Rename the architecture hook to
>   arch_hwdom_first_bank_can_fit_modules() and document its contract.
> - Drop the redundant post-alignment zImage load-address check.
> - Drop the dead kernel range overflow check.
> - Make the candidate-bank coverage condition explicit.
> 
> Changes in v2:
> - Split the behavior-preserving placement refactoring into the previous
>   patch.
> - Reuse the refactored Arm kernel and DTB/initrd placement helpers for
>   the first-bank candidate check.
> 
> Link to v1:
>   
> https://patchew.org/Xen/4f862bb2dc323914b8120b0f16af7516140cf42b.1780065103.git.mykola._5Fkvach@xxxxxxxx/
> 
> Changes since RFC:
> - Do not keep the RFC scalar minimum-size check.  It can both reject valid
>   layouts and accept layouts which still fail later.  Instead, validate
>   the candidate bank using the same kernel and DTB/initrd placement rules
>   as the load path.
> - Replace the scalar minimum-size check with arch_hwdom_first_bank_ok().
> - Validate fixed-address and AArch32 start == 0 kernel placement against
>   the candidate bank.
> - Check the candidate after capping the host region by the remaining
>   unassigned hardware-domain memory.
> - Treat the hardware-domain FDT allocation size as a conservative upper
>   bound because the final FDT is generated later.
> 
> Link to RFC: 
> https://patchew.org/Xen/9ae4f7dd49f5b1f761193adae573c2675c92e883.1779051035.git.mykola._5Fkvach@xxxxxxxx/
> 
> Why the RFC scalar approach was not kept:
> 
> A simple minimum-size check is not sufficient here because the validity
> of the first bank depends on the actual Arm placement rules, not only on
> the aggregate size of the kernel, DTB and initrd. The DTB/initrd area may
> fit before a 64-bit Image loaded with a text offset, while an AArch32
> position-independent kernel may leave no valid module location even when
> the aggregate size appears to fit. Fixed-address kernels also need the
> candidate bank start to be considered.
> ---
>  xen/arch/arm/acpi/domain_build.c        |  2 -
>  xen/arch/arm/domain_build.c             |  8 ++++
>  xen/arch/arm/include/asm/domain_build.h |  4 ++
>  xen/arch/arm/include/asm/kernel.h       | 10 +++++
>  xen/arch/arm/kernel.c                   | 53 ++++++++++++++++++++++++-
>  xen/common/device-tree/domain-build.c   | 25 ++++++++----
>  xen/include/xen/fdt-kernel.h            | 14 +++++++
>  7 files changed, 105 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/domain_build.c 
> b/xen/arch/arm/acpi/domain_build.c
> index 249d899c33..db16f7fa94 100644
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -26,8 +26,6 @@
>  #undef virt_to_mfn
>  #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
>  
> -#define ACPI_DOM0_FDT_MIN_SIZE 4096
> -
>  static int __init acpi_iomem_deny_access(struct domain *d)
>  {
>      acpi_status status;
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1efddc60ef..550617f152 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -115,6 +115,14 @@ int __init parse_arch_dom0_param(const char *s, const 
> char *e)
>                               (IS_ENABLED(CONFIG_STATIC_SHM) ?         \
>                                (NR_SHMEM_BANKS * (160 + 16)) : 0))
>  
> +paddr_t __init hwdom_get_fdt_alloc_size(void)
> +{
> +    if ( acpi_disabled )
> +        return fdt_totalsize(device_tree_flattened) + DOM0_FDT_EXTRA_SIZE;
> +
> +    return ACPI_DOM0_FDT_MIN_SIZE;
> +}
> +
>  unsigned int __init dom0_max_vcpus(void)
>  {
>      if ( opt_dom0_max_vcpus == 0 )
> diff --git a/xen/arch/arm/include/asm/domain_build.h 
> b/xen/arch/arm/include/asm/domain_build.h
> index df8b361b3d..85cf46a958 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -19,6 +19,10 @@ int prepare_acpi(struct domain *d, struct kernel_info 
> *kinfo);
>  
>  int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
>  
> +#define ACPI_DOM0_FDT_MIN_SIZE 4096
> +
> +paddr_t hwdom_get_fdt_alloc_size(void);
> +
>  #if defined(CONFIG_MPU) && defined(CONFIG_ARM_64)
>  /* Utility function to determine if an Armv8-R processor supports VMSA. */
>  bool has_v8r_vmsa_support(void);
> diff --git a/xen/arch/arm/include/asm/kernel.h 
> b/xen/arch/arm/include/asm/kernel.h
> index 21f4273fa1..b86c7337fe 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -8,12 +8,22 @@
>  
>  #include <asm/domain.h>
>  
> +#include <xen/types.h>
> +
> +struct kernel_info;
> +
>  struct arch_kernel_info
>  {
>      /* Enable pl011 emulation */
>      bool vpl011;
>  };
>  
> +#define arch_hwdom_first_bank_can_fit_modules \
> +        arch_hwdom_first_bank_can_fit_modules
> +bool arch_hwdom_first_bank_can_fit_modules(const struct kernel_info *info,
> +                                           paddr_t bank_start,
> +                                           paddr_t bank_size);
> +
>  #endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index d1be4d8074..47229644b2 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -64,6 +64,9 @@ kernel_zimage_place_in_bank(const struct kernel_info *info,
>          load_end = bank_start + bank_size;
>          load_end = MIN(bank_start + MB(128), load_end);
>  
> +        if ( load_end - bank_start < info->image.len )
> +            return INVALID_PADDR;
> +
>          load_addr = load_end - info->image.len;
>          /* Align to 2MB */
>          load_addr &= ~(MB(2) - 1);
> @@ -164,9 +167,55 @@ static void __init place_dtb_initrd(struct kernel_info 
> *info,
>  static paddr_t __init kernel_zimage_place(struct kernel_info *info)
>  {
>      const struct membanks *mem = kernel_info_get_mem(info);
> +    paddr_t load_addr;
> +
> +    load_addr = kernel_zimage_place_in_bank(info, mem->bank[0].start,
> +                                            mem->bank[0].size);
> +    if ( load_addr == INVALID_PADDR )
> +        panic("Unable to find suitable location for the kernel\n");
> +
> +    return load_addr;
> +}
> +
> +bool __init arch_hwdom_first_bank_can_fit_modules(const struct kernel_info 
> *info,
> +                                                  paddr_t bank_start,
> +                                                  paddr_t bank_size)
> +{
> +    const struct boot_module *initrd = info->bd.initrd;
> +    /*
> +     * place_dtb_initrd() rounds the DTB and initrd placement to 2MB 
> boundaries;
> +     * use the same granularity when checking whether the first bank can hold
> +     * them.
> +     */
> +    const paddr_t initrd_len = ROUNDUP(initrd ? initrd->size : 0, MB(2));
> +    /*
> +     * The hardware domain FDT has not been generated yet. Use the allocation
> +     * size as a conservative upper bound for the final DTB size.
> +     */
> +    const paddr_t dtb_len = ROUNDUP(hwdom_get_fdt_alloc_size(), MB(2));
> +    const paddr_t rambase = bank_start;
> +    const paddr_t ramsize = bank_size;
> +    const paddr_t dtb_initrd_size = initrd_len + dtb_len;
> +    const paddr_t ramend = rambase + ramsize;
> +    paddr_t kernbase;
> +    paddr_t kernend;
> +    paddr_t dtb_base;
> +
> +    kernbase = kernel_zimage_place_in_bank(info, bank_start, bank_size);
> +    if ( kernbase == INVALID_PADDR )
> +        return false;
> +
> +    kernend = kernbase + info->image.len;
> +
> +    if ( (kernbase < rambase) || (kernend > ramend) )
> +        return false;
> +
> +    if ( !first_bank_can_fit_modules(ramsize, kernbase, kernend,
> +                                     dtb_initrd_size) )
> +        return false;
>  
> -    return kernel_zimage_place_in_bank(info, mem->bank[0].start,
> -                                       mem->bank[0].size);
> +    return find_dtb_initrd_placement(rambase, ramend, kernbase, kernend,
> +                                     dtb_initrd_size, &dtb_base);
>  }
>  
>  static void __init kernel_zimage_load(struct kernel_info *info)
> diff --git a/xen/common/device-tree/domain-build.c 
> b/xen/common/device-tree/domain-build.c
> index f3ba496f1e..30a59abfa7 100644
> --- a/xen/common/device-tree/domain-build.c
> +++ b/xen/common/device-tree/domain-build.c
> @@ -299,20 +299,31 @@ static bool __init allocate_hwdom_memory(struct 
> kernel_info *kinfo)
>  
>      for ( i = 0; (kinfo->unassigned_mem > 0) && (i < nr_banks); i++ )
>      {
> -        paddr_t bank_size;
> +        const paddr_t bank_start = hwdom_free_mem->bank[i].start;
> +        paddr_t bank_size = hwdom_free_mem->bank[i].size;
> +
> +        /*
> +         * Check the size that would actually be assigned, not just the size
> +         * of the host region.
> +         */
> +        bank_size = min(bank_size, kinfo->unassigned_mem);
>  
>          /*
>           * The first bank must be large enough for place_dtb_initrd() to
>           * fit the kernel, DTB and initrd.  Skip small regions to avoid
>           * ending up with a tiny first bank.
>           */
> -        if ( !mem->nr_banks && (hwdom_free_mem->bank[i].size < 
> min_bank_size) )
> -            continue;
> +        if ( !mem->nr_banks )
> +        {
> +            if ( bank_size < min_bank_size )
> +                continue;
> +
> +            if ( !arch_hwdom_first_bank_can_fit_modules(kinfo, bank_start,
> +                                                        bank_size) )
> +                continue;
> +        }
>  
> -        bank_size = MIN(hwdom_free_mem->bank[i].size, kinfo->unassigned_mem);
> -        if ( !allocate_bank_memory(kinfo,
> -                                   
> gaddr_to_gfn(hwdom_free_mem->bank[i].start),
> -                                   bank_size) )
> +        if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(bank_start), 
> bank_size) )
>          {
>              xfree(hwdom_free_mem);
>              return false;
> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
> index 00c37be101..61721d22a2 100644
> --- a/xen/include/xen/fdt-kernel.h
> +++ b/xen/include/xen/fdt-kernel.h
> @@ -93,6 +93,20 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
>      return container_of(&kinfo->mem.common, const struct membanks, common);
>  }
>  
> +/*
> + * Return whether the proposed hardware-domain first RAM bank can contain the
To `contain a placement` reads weird. I think `satisfy` would be a better word
that clearly denotes the purpose i.e. check if the bank satsifies the placement
requirements (we can also add `requirements` word to the end of the comment).

Can be done on commit (it looks like there are some ECLAIR issues, so we need to
wait a bit to merge the series).

Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal