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

["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'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