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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 7/22/22 16:33, Smith, Jackson wrote:
> > -----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
Yep, I ran into this one before and thought I had it addressed.

> 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)
Ack

> > > 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.
You are correct, my apologies. Thank you!

> Thanks,
> Jackson