Re: [PATCH 04/15] x86/setup: special treatment of initrd for PVH Dom0

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Mon, Oct 12, 2026 at 9:20 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> Dealing with large initrd-s is problematic in two ways: We needlessly
> copy around a large chunk of data, and our calculation of how much
> memory to give Dom0 assumes the space taken by the initrd is actually
> available at the time of populating Dom0's P2M.
>
> As a first step, make effort during early boot to try to place the initrd
> in a chunk of memory that's a multiple of 1Gb (or, if not available / not
> possibly going to fit, in a chunk a multiple of 2Mb in size). This may
> (but generally won't) result in more copying, which we simply need to
> accept. Record (and use) the extra space allocated in (as) headroom, which
> then needs accouting for in pvh_load_kernel().
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Not being able to check bm->kind is of course not very nice. The field is
> only set quite a bit later, and that setting can - afaict - not be moved
> ahead of the call to xsm_multiboot_init().
>
> The bumping of mask to L3's in __start_xen() transiently breaks booting of
> Dom0 with overly much memory assigned to it. That'll be resolved when the
> headroom space is properly dealt with (consumed into Dom0, or freed).
> Therefore this patch and everything up to the one adding the consuming
> code want committing together, or addition of that conditional wants
> delaying until that later change.

I'm generally opposed to breaking bisection.  I'd personally prefer an
extra conditional until then, but I'd defer to the other maintainers
on that (don't consider that a nack).

> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -68,4 +68,6 @@ extern bool opt_dom0_verbose;
>  extern bool opt_dom0_cpuid_faulting;
>  extern bool opt_dom0_msr_relaxed;
>
> +unsigned long get_dom0_size(unsigned long avail);
> +
>  #endif
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1608,6 +1608,40 @@ void asmlinkage __init noreturn __start_
>              struct boot_module *bm = &bi->mods[j];
>              unsigned long size = PAGE_ALIGN(bm->arch.headroom + bm->size);
>
> +            /*
> +             * For PVH Dom0 try to place the initrd in a full multi-Gb (or
> +             * multi-2Mb) chunk, such that assigning the pages directly can
> +             * be done while still using superpage mappings.
> +             */
> +            if ( opt_dom0_pvh && j == 1 /* bm->kind == BOOTMOD_RAMDISK */ )

What if there's a microcode blob or an XSM policy here instead?

> +            {
> +                if ( get_dom0_size(nr_ram_pages()) >= PFN_DOWN(GB(2)) )
> +                    mask = (1UL << L3_PAGETABLE_SHIFT) - 1;
> +
> +                for ( ; mask > PAGE_SIZE; mask >>= PAGETABLE_ORDER )
> +                {
> +                    if ( !(bm->start & mask) && bm->arch.relocated )
> +                        break;
> +
> +                    size = PAGE_ALIGN(bm->size - bm->arch.headroom);
> +                    end = e;
> +                    do {
> +                        end = consider_modules((s + mask) & ~mask, end & 
> ~mask,
> +                                               (size + mask) & ~mask, 
> bi->mods,
> +                                               bi->nr_modules + relocated, 
> j);
> +                    } while ( (end & mask) && end > s );
> +
> +                    if ( highmem_start && end > highmem_start )
> +                        break;
> +
> +                    size = (size + mask) & ~mask;
> +                    bm->start += bm->arch.headroom;
> +                    bm->size  -= bm->arch.headroom;
> +                    bm->arch.headroom = size - PAGE_ALIGN(bm->size);
> +                    goto move_module;
> +                }

Every path through this block either does a 'break' or a 'goto'; so
this isn't actually a for loop at all -- the 2MiB fallback can never
execute.

Did you forget an `if (end) { }` somewhere?

As it stands, if the 1GiB pass doesn't work, it looks like we'll jump
to move_module with 'end' at 0, and likely fault the copy.

Also, looks like consider_modules() takes a uint32_t for size, and we
round up to the nearest GiB; so if the dom0 initrd is between 3GiB and
4GiB, won't we pass 0 as a size argument, which will always fit?

 -George