RE: [PATCH v1 10/18] x86: introduce the domain builder

["Smith, Jackson" <rsmith@xxxxxxxxxxxxxxxxxxxxx>]

> -----Original Message-----
> From: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>
> On 7/18/22 09:59, Smith, Jackson wrote:
> > Hi Daniel,
> >
> >> -----Original Message-----
> >> Subject: [PATCH v1 10/18] x86: introduce the domain builder
> >>
> >> This commit introduces the domain builder configuration FDT parser
> >> along with the domain builder core for domain creation. To enable
> >> domain builder to be a cross architecture internal API, a new arch
> >> domain creation call
> > is
> >> introduced for use by the domain builder.
> >
> >> diff --git a/xen/common/domain-builder/core.c
> >
> >> +void __init builder_init(struct boot_info *info) {
> >> +    struct boot_domain *d = NULL;
> >> +
> >> +    info->builder = &builder;
> >> +
> >> +    if ( IS_ENABLED(CONFIG_BUILDER_FDT) )
> >> +    {
> >
> >> +    }
> >> +
> >> +    /*
> >> +     * No FDT config support or an FDT wasn't present, do an initial
> >> +     * domain construction
> >> +     */
> >> +    printk("Domain Builder: falling back to initial domain build\n");
> >> +    info->builder->nr_doms = 1;
> >> +    d = &info->builder->domains[0];
> >> +
> >> +    d->mode = opt_dom0_pvh ? 0 : BUILD_MODE_PARAVIRTUALIZED;
> >> +
> >> +    d->kernel = &info->mods[0];
> >> +    d->kernel->kind = BOOTMOD_KERNEL;
> >> +
> >> +    d->permissions = BUILD_PERMISSION_CONTROL |
> >> BUILD_PERMISSION_HARDWARE;
> >> +    d->functions = BUILD_FUNCTION_CONSOLE |
> >> BUILD_FUNCTION_XENSTORE |
> >> +                     BUILD_FUNCTION_INITIAL_DOM;
> >> +
> >> +    d->kernel->arch->headroom = bzimage_headroom(bootstrap_map(d-
> >>> kernel),
> >> +                                                   d->kernel->size);
> >> +    bootstrap_map(NULL);
> >> +
> >> +    if ( d->kernel->string.len )
> >> +        d->kernel->string.kind = BOOTSTR_CMDLINE; }
> >
> > Forgive me if I'm incorrect, but I believe there is an issue with this
> > fallback logic for the case where no FDT was provided.
>
> IIUC, the issue at hand has to deal with patch #15.
>
> > If dom0_mem is not supplied to the xen cmd line, then d->meminfo is
> > never initialized. (See dom0_compute_nr_pages/dom0_build.c:335)
> > This was giving me trouble because bd->meminfo.mem_max.nr_pages was
> > left at 0, effectivity clamping dom0 to 0 pages of ram.
> >

I realize I never shared the exact panic message I was experiencing. Sorry 
about that.
It's "Domain 0 allocation is too small for kernel image" on 
xen/arch/x86/pv/domain_builder.c:534

I think you should be able to consistently reproduce what I'm seeing as long as 
these two conditions are met:
- the dom0_mem cmdline option is _not_ set
- no domain builder device tree is passed to xen (the fallback case I 
identified above)

> > I'm not sure what the best solution is but one (easy) possibility is
> > just initializing meminfo to the dom0 defaults near the end of this 
> > function:
> >         d->meminfo.mem_size = dom0_size;
> >         d->meminfo.mem_min = dom0_min_size;
> >         d->meminfo.mem_max = dom0_max_size;
>
> I believe the correct fix is to this hunk,
>
> @@ -416,7 +379,12 @@ unsigned long __init dom0_compute_nr_pages(
>          }
>      }
>
> -    d->max_pages = min_t(unsigned long, max_pages, UINT_MAX);
> +    /* Clamp according to min/max limits and available memory (final). */
> +    nr_pages = max(nr_pages, min_pages);
> +    nr_pages = min(nr_pages, max_pages);
> +    nr_pages = min(nr_pages, avail);
> +
> +    bd->domain->max_pages = min_t(unsigned long, max_pages, UINT_MAX);
>
> Before that last line, there should be a clamp up of max_pages, e.g.
>
>     nr_pages = max(nr_pages, min_pages);
>     nr_pages = min(nr_pages, max_pages);
>     nr_pages = min(nr_pages, avail);
>
>     max_pages = max(nr_pages, max_pages);
>
>     bd->domain->max_pages = min_t(unsigned long, max_pages, UINT_MAX);
>
> v/r,
> dps

I don't believe this resolves my issue.

If max_pages is 0 before these 5 lines, then the second line will still clamp 
nr_pages to 0 and the panic on line 534 will be hit.

Before patch 15, this max limit came directly from dom0_max_size, which has a 
default value of { .nr_pages = LONG_MAX }, so no clamping will occur unless 
overridden by the cmd line.

After patch 15, bd->meminfo.mem_max is used as the max limit. (unless 
overridden by the cmdline)
I'm assuming it will eventually be specified in the device tree, but for now, 
the max limit just set to equal to the size 
(xen/common/domain-builder/fdt.c:155) so no down-clamping will occur.

The only exception is the initial domain construction fallback. In this case, 
there is no device tree and bd->meminfo is never initialized.
If bd->meminfo.mem_size is zero, the code will try to compute a reasonable 
default for nr_pages, but there is no such logic max_pages. It remains 0, and 
clamps nr_pages to zero.

Does this help clarify?
The core issue is that without a device tree or command line option to specify 
the max limit, the max limit is left uninitialized, which clamps dom0's memory 
to 0. I think it should be initialized to LONG_MAX in that case, like it was 
before this patch set.

Thanks,
Jackson