|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 11/15] x86/PVH: try to map initrd directly into Dom0
On Mon, Oct 12, 2026 at 10:25:11AM +0200, Jan Beulich wrote:
> Our present handling of initrd is problematic in particular when the
> initrd is huge, and when at the same time "dom0_mem=" isn't in use to
> significantly shrink Dom0's memory (from the default). In that case,
> insufficient memory may exist for pvh_populate_p2m() to succeed (because
> initrd memory can be freed only after copying).
>
> Make an attempt to map the blob (including headroom) into Dom0 instead,
I would use "dom0 p2m" instead of plain p2m, it's clearer IMO.
> thus also saving the copying operation. With __start_xen() having arranged
> for a larger than necessary blob, that mapping can typically be done with
> superpage mappings.
>
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Is there a need to prune (scrub) the headroom space?
We already populate the p2m with memory allocated with MEMF_no_scrub,
so there's no reason to scrub the headroom unless we also remove the
MEMF_no_scrub for p2m population.
>
> Why does find_memory() go upwards, while find_kernel_memory() goes
> downwards? And why do we use the lowest part of the region found by
> find_memory() rather than the highest part?
find_memory() should probably start downwards from 4G.
>
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -450,7 +450,9 @@ static void __init pvh_init_p2m(struct d
> static int __init pvh_populate_p2m(
> struct domain *d,
> const struct e820entry *e820,
> - unsigned int nr_e820)
> + unsigned int nr_e820,
> + unsigned long initrd_start,
> + unsigned long initrd_end)
We already have a rangeset in find_memory() that contains the RAM
regions, I think it would be better if we propagated that rangeset
here as it would make the logic easier overall. We would need to
initialize it earlier (as find_memory() is used after pvh_init_p2m()),
but would avoid the extra logic.
> {
> struct vcpu *v = d->vcpu[0];
> unsigned int i;
> @@ -468,7 +470,12 @@ static int __init pvh_populate_p2m(
> addr = PFN_DOWN(e820[i].addr);
> size = PFN_DOWN(e820[i].size);
>
> - rc = pvh_populate_memory_range(d, addr, size);
> + if ( addr + size <= initrd_start || addr >= initrd_end )
> + rc = pvh_populate_memory_range(d, addr, size);
> + else
> + rc = pvh_populate_memory_range(d, addr, initrd_start - addr)
> + ?: pvh_populate_memory_range(d, initrd_end,
> + addr + size - initrd_end);
> if ( rc )
> return rc;
>
> @@ -518,6 +525,43 @@ static int __init pvh_populate_p2m(
> #undef MB1_PAGES
> }
>
> +static int __init pvh_map_initrd(
> + struct domain *d, const struct boot_module *initrd,
> + paddr_t initrd_start, unsigned int order)
> +{
> + for ( paddr_t ga = initrd_start, ma = initrd->start;
> + ga < initrd_start + initrd->size;
> + ga += PAGE_SIZE << order, ma += PAGE_SIZE << order )
> + {
> + int rc = p2m_add_page(d, gaddr_to_gfn(ga), maddr_to_mfn(ma),
> + order, p2m_ram_rw);
> +
> + if ( rc )
> + {
> + printk("Failed to map initrd: [%#lx,%#lx): %d\n",
> + PFN_DOWN(ga), PFN_DOWN(ga) + (1UL << order), rc);
I would print the domain prefix.
> + /*
> + * Best effort rollback: The range will be replaced anyway by the
> + * subsequent pvh_populate_p2m().
> + */
> + while ( ga > initrd_start )
> + {
> + ga -= PAGE_SIZE << order;
> + ma -= PAGE_SIZE << order;
> + p2m_remove_page(d, gaddr_to_gfn(ga), maddr_to_mfn(ma),
> order);
> + order_stats[order]--;
> + }
> +
> + return rc;
> + }
> +
> + order_stats[order]++;
> + }
> +
> + return 0;
> +}
> +
> struct mem_info {
> size_t size;
> paddr_t addr;
> @@ -538,7 +582,10 @@ static int cf_check __init _find_memory(
> }
>
> static paddr_t __init find_memory(
> - const struct domain *d, const struct elf_binary *elf, size_t size)
> + const struct domain *d,
> + const struct elf_binary *elf,
> + const struct hvm_modlist_entry *initrd,
> + size_t size)
> {
> struct rangeset *ram = rangeset_new(NULL, NULL, 0);
> struct mem_info info = {
> @@ -577,6 +624,13 @@ static paddr_t __init find_memory(
> PAGE_SIZE) - 1) != 0 )
> return INVALID_PADDR;
>
> + /* Remove the mapped initrd range (if any), rounded to page boundaries.
> */
> + if ( initrd->paddr &&
> + rangeset_remove_range(ram, initrd->paddr,
> + ROUNDUP(initrd->paddr + initrd->size,
> + PAGE_SIZE) - 1) != 0 )
> + return INVALID_PADDR;
> +
> rangeset_report_ranges(ram, MB(1), -1, _find_memory, &info);
>
> rangeset_destroy(ram);
> @@ -689,6 +743,70 @@ static bool __init check_and_adjust_load
> return true;
> }
>
> +static paddr_t __init find_initrd_memory(
> + const struct domain *d,
> + const struct elf_binary *elf,
> + const struct boot_module *initrd,
> + size_t align,
> + paddr_t limit)
> +{
> + paddr_t kernel_start = (paddr_t)elf->dest_base & PAGE_MASK;
> + paddr_t kernel_end = ROUNDUP((paddr_t)elf->dest_base + elf->dest_size,
> + PAGE_SIZE);
> + unsigned long initrd_size = ROUNDUP(initrd->size, PAGE_SIZE);
> + unsigned long initrd_len = initrd_size - initrd->arch.headroom;
> +
> + if ( !IS_ALIGNED(initrd->start | initrd_size, align) )
> + return INVALID_PADDR;
> +
> + /*
> + * The memory map is sorted. Search backwards to find the highest
> + * suitable address.
> + */
> + for ( unsigned int i = d->arch.nr_e820; i--; )
> + {
> + paddr_t start = d->arch.e820[i].addr;
> + paddr_t end = start + d->arch.e820[i].size;
> +
> + if ( d->arch.e820[i].type != E820_RAM )
> + continue;
> +
> + if ( end > limit )
> + continue;
Don't you want to clamp end to limit here, ie:
end = min(end, limit);
Like we clamp start to be above MB(1)?
We possibly want to continue if start > limit, as then there's nothing
to clamp?
> +
> + start = MAX(ROUNDUP(start, align), MB(1));
You need to roundup after the max, ie:
ROUNDUP(MAX(start, MB(1)), align);
Otherwise the result might be unaligned if start > 0 and < MB(1)
depending on the requested alignment.
> + end = ROUNDDOWN(end, align);
> +
> + /* Don't use memory below 1MB, as it could overwrite BDA/EBDA/IBFT.
> */
> + if ( end <= MB(1) )
> + break;
> +
> + if ( end <= initrd_size )
> + break;
> +
> + if ( end - initrd_size <= start )
This should be <, otherwise an exact fit is rejected.
> + continue;
> +
> + /*
> + * NB: Even better would be to use rangesets to determine a suitable
> + * range, in particular in case a kernel requests multiple heavily
> + * discontiguous regions (which right now we fold all into one big
> + * region).
> + */
I understand that you switched find_memory() to use a rangeset because
it will need to deal with two discontinuous region because of the
initrd plus the kernel. However the code here could be adjusted to
possibly use a rangeset from the start?
I think we should find a way to pass a rangeset around with the
available memory regions for the allocation functions to use. Maybe
that way we could even remove the need to add a separate
find_initrd_memory()? Which TBH looks like it wants to share its code
with plain find_memory().
> + if ( end <= kernel_start || end - initrd_len >= kernel_end )
> + return end - initrd_size; /* No overlap. */
> +
> + /* Deal with the kernel already being loaded in the region. */
> + if ( kernel_end < end && end - kernel_end >= initrd_len )
The condition in this second branch is never reached, as the if above
already has the same condition: "end - initrd_len >= kernel_end".
> + return end - initrd_size;
> + if ( kernel_start > start &&
> + ROUNDDOWN(kernel_start, align) - start >= initrd_size )
> + return ROUNDDOWN(kernel_start, align) - initrd_size;
> + }
> +
> + return INVALID_PADDR;
> +}
> +
> static int __init pvh_place_kernel(
> const struct boot_domain *bd,
> struct elf_binary *elf,
> @@ -744,9 +862,93 @@ static int __init pvh_place_kernel(
> return 0;
> }
>
> +static void __init pvh_place_initrd(
> + const struct boot_domain *bd,
> + const struct elf_binary *elf,
> + struct hvm_modlist_entry *mod,
> + paddr_t *blob_start,
> + paddr_t *blob_end)
> +{
> + struct domain *d = bd->d;
> + struct boot_module *initrd = bd->initrd;
> + struct boot_module try = *initrd;
Looking at the line below initrd can be NULL here, and hence this
should be:
struct boot_module try = initrd ? *initrd : {};
Or possibly only initialize 'try' after the initrd_len check?
> + unsigned long initrd_len =
> + initrd ? initrd->size - initrd->arch.headroom : 0;
> + size_t align = 1UL << L3_PAGETABLE_SHIFT;
> + int rc;
> +
> + *blob_start = INVALID_PADDR;
> + *blob_end = 0;
> +
> + if ( !initrd_len )
> + return;
> +
> + mod->size = initrd_len;
> +
> + /*
> + * As we promise to place below 4G whatever fits there, make a first pass
> + * limiting permitted addresses. If we can't fit it there, fall back to
> + * placing above 4G.
> + */
> + for ( paddr_t limit = GB(4); ; )
> + {
> + paddr_t start;
> +
> + *blob_start = find_initrd_memory(d, elf, &try, align, limit);
> + if ( *blob_start != INVALID_PADDR )
> + break;
> +
> + if ( align == PAGE_SIZE )
> + {
> + if ( !(limit + 1) )
> + return;
> +
> + limit = -1;
> + align = 1UL << L3_PAGETABLE_SHIFT;
Aren't you possibly asking for an alignment better than what's
required by the actual initrd memory? Maybe it's using 2M pages yet
you request 1G alignment here.
Thanks, Roger.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |