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

["Daniel P. Smith" <dpsmith@apertussolutions.com>]

On 7/22/22 16:33, Smith, Jackson wrote:
>> -----Original Message-----
>> From: Daniel P. Smith <dpsmith@apertussolutions.com>
>>
>> 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