Re: [PATCH v1 10/18] x86: introduce the domain builder
On 06.07.2022 23:04, Daniel P. Smith wrote:
> 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.
>
> Signed-off-by: Daniel P. Smith
> Reviewed-by: Christopher Clark
> ---
> xen/arch/x86/setup.c | 9 +
> xen/common/Makefile| 1 +
> xen/common/domain-builder/Makefile | 2 +
> xen/common/domain-builder/core.c | 96 ++
> xen/common/domain-builder/fdt.c| 295 +
> xen/common/domain-builder/fdt.h| 7 +
> xen/include/xen/bootinfo.h | 16 ++
> xen/include/xen/domain_builder.h | 1 +
> 8 files changed, 427 insertions(+)
With this diffstat - why the x86: prefix in the subject?
Also note the naming inconsistency: domain-builder/ (preferred) vs
domain_builder.h (adjustment would require touching earlier patches).
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1,4 +1,6 @@
> +#include
> #include
> +#include
> #include
> #include
> #include
> @@ -826,6 +828,13 @@ static struct domain *__init create_dom0(const struct
> boot_info *bi)
> return d;
> }
>
> +void __init arch_create_dom(
> +const struct boot_info *bi, struct boot_domain *bd)
> +{
> +if ( builder_is_initdom(bd) )
> +create_dom0(bi);
> +}
You're not removing any code in exchange - is Dom0 now being built twice?
Or is the function above effectively dead code?
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -72,6 +72,7 @@ extra-y := symbols-dummy.o
> obj-$(CONFIG_COVERAGE) += coverage/
> obj-y += sched/
> obj-$(CONFIG_UBSAN) += ubsan/
> +obj-y += domain-builder/
At least as long as all of this is still experimental I would really like
to see a way to disable all of it via Kconfig.
> --- /dev/null
> +++ b/xen/common/domain-builder/core.c
> @@ -0,0 +1,96 @@
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include "fdt.h"
> +
> +static struct domain_builder __initdata builder;
> +
> +void __init builder_init(struct boot_info *info)
> +{
> +struct boot_domain *d = NULL;
> +
> +info->builder =
> +
> +if ( IS_ENABLED(CONFIG_BUILDER_FDT) )
> +{
> +/* fdt is required to be module 0 */
> +switch ( check_fdt(info, __va(info->mods[0].start)) )
Besides requiring fixed order looking inflexible to me, what guarantees
there is at least one module? (Perhaps there is, but once again -
without seeing where this function is being called from, how am I to
judge?)
> +{
> +case 0:
> +printk("Domain Builder: initialized from config\n");
> +info->builder->fdt_enabled = true;
> +return;
> +case -EINVAL:
> +info->builder->fdt_enabled = false;
> +break;
Aiui this is the case where no FDT is present. I'd strongly suggest
to use a less common / ambiguous error code to cover that case. Maybe
-ENODEV or -EOPNOTSUPP or ...
> +case -ENODATA:
... -ENODATA, albeit you having that here suggests this has some
other specific meaning already.
> +default:
> +panic("%s: error occured processing DTB\n", __func__);
> +}
> +}
> +
> +/*
> + * 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 = >builder->domains[0];
> +
> +d->mode = opt_dom0_pvh ? 0 : BUILD_MODE_PARAVIRTUALIZED;
> +
> +d->kernel = >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;
Nit: Indentation.
> +d->kernel->arch->headroom = bzimage_headroom(bootstrap_map(d->kernel),
> + d->kernel->size);
bzimage isn't an arch-agnostic concept afaict, so I don't see this
function legitimately being called from here.
And nit again: Indentation. (And at least one more further down.)
> +bootstrap_map(NULL);
> +
> +if ( d->kernel->string.len )
> +d->kernel->string.kind = BOOTSTR_CMDLINE;
> +}
> +
> +uint32_t __init builder_create_domains(struct boot_info *info)
> +{
> +uint32_t build_count = 0, functions_built = 0;
> +int i;
> +
> +for ( i = 0; i < info->builder->nr_doms; i++ )
> +{
> +struct boot_domain *d = >builder->domains[i];
Can variables of this type please not be named "d", but e.g. "bd"?
> +if ( ! IS_ENABLED(CONFIG_MULTIDOM_BUILDER) &&
> + ! builder_is_initdom(d) &&
Nit: Stray blanks after ! .
> --- /dev/null
> +++
Re: [PATCH v1 10/18] x86: introduce the domain builder
On 7/22/22 16:33, Smith, Jackson wrote:
-Original Message-
From: Daniel P. Smith
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 =
+
+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 = >builder->domains[0];
+
+d->mode = opt_dom0_pvh ? 0 : BUILD_MODE_PARAVIRTUALIZED;
+
+d->kernel = >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
RE: [PATCH v1 10/18] x86: introduce the domain builder
> -Original Message-
> From: Daniel P. Smith
>
> 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 =
> >> +
> >> +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 = >builder->domains[0];
> >> +
> >> +d->mode = opt_dom0_pvh ? 0 : BUILD_MODE_PARAVIRTUALIZED;
> >> +
> >> +d->kernel = >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
Re: [PATCH v1 10/18] x86: introduce the domain builder
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 =
>> +
>> +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 = >builder->domains[0];
>> +
>> +d->mode = opt_dom0_pvh ? 0 : BUILD_MODE_PARAVIRTUALIZED;
>> +
>> +d->kernel = >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
RE: [PATCH v1 10/18] x86: introduce the domain builder
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 =
> +
> +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 = >builder->domains[0];
> +
> +d->mode = opt_dom0_pvh ? 0 : BUILD_MODE_PARAVIRTUALIZED;
> +
> +d->kernel = >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.
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;
Thanks,
Jackson
smime.p7s
Description: S/MIME cryptographic signature
