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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.10.2026 13:23, George Dunlap wrote:
> 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).

Noted. If no-one else voices a different opinion, I'll wrap that piece of
code in "#if 0".

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

Especially for the XSM policy - very good question (hence also the
respective post-commit-message remark). The ucode blob we can tell,
if necessary, but aiui the XSM policy we can't. I have absolutely no
idea how to deal with this, and it's only because of this that I
left things in this shape.

>> +            {
>> +                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?

"Forget" as in "wrongly purge when purging other debugging code", as I
had temporary logic there to actually also exercise the falling back to
2M and 4k. So yes,

                    if ( end <= s )
                        continue;

is supposed to live right after the inner loop.

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

Hmm, ugly. Since imo at some point we will want to lift the 4G
restriction on initrd and kernel anyway, I think I'll slip in a prereq
patch widening the function's parameter.

Thank you very much for spotting both of these flaws.

Jan