| Message ID | 20250408160802.49870-10-agarciav@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Hyperlaunch device tree for dom0 | expand |
On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: > > > From: "Daniel P. Smith" dpsmith@apertussolutions.com > > > Look for a subnode of type `multiboot,kernel` within a domain node. If > found, locate it using the multiboot module helper to generically ensure > it lives in the module list. If the bootargs property is present and > there was not an MB1 string, then use the command line from the device > tree definition. > > Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com > > Signed-off-by: Jason Andryuk jason.andryuk@amd.com > > Signed-off-by: Alejandro Vallejo agarciav@amd.com > > --- > v3: > * Add const to fdt > * Remove idx == NULL checks > * Add BUILD_BUG_ON for MAX_NR_BOOTMODS fitting in a uint32_t > * Remove trailing ) from printks > * Return ENODATA for missing kernel > * Re-work "max domains" warning and print limit > * fdt_cell_as_u32/directly return values > * Remove "pairs" looping from fdt_get_reg_prop() and only grab 1. > * Use addr_cells and size_cells > --- > xen/arch/x86/domain-builder/core.c | 11 ++++++ > xen/arch/x86/domain-builder/fdt.c | 57 ++++++++++++++++++++++++++++++ > xen/arch/x86/setup.c | 5 --- > 3 files changed, 68 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c > index c50eff34fb..eda7fa7a8f 100644 > --- a/xen/arch/x86/domain-builder/core.c > +++ b/xen/arch/x86/domain-builder/core.c > @@ -59,6 +59,17 @@ void __init builder_init(struct boot_info *bi) > > printk(XENLOG_INFO " Number of domains: %d\n", bi->nr_domains); > > } > + else > + { > + unsigned int i; > + > + /* Find first unknown boot module to use as Dom0 kernel */ > + printk("Falling back to using first boot module as dom0\n"); > + i = first_boot_module_index(bi, BOOTMOD_UNKNOWN); > + bi->mods[i].type = BOOTMOD_KERNEL; > > + bi->domains[0].kernel = &bi->mods[i]; > > + bi->nr_domains = 1; > > + } > } > > /* > diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c > index 9ebc8fd0e4..a037c8b6cb 100644 > --- a/xen/arch/x86/domain-builder/fdt.c > +++ b/xen/arch/x86/domain-builder/fdt.c > @@ -155,6 +155,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, > return idx; > } > > +static int __init process_domain_node( > + struct boot_info *bi, const void *fdt, int dom_node) > +{ > + int node; > + struct boot_domain *bd = &bi->domains[bi->nr_domains]; > > + const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; > + int address_cells = fdt_address_cells(fdt, dom_node); > + int size_cells = fdt_size_cells(fdt, dom_node); > + > + fdt_for_each_subnode(node, fdt, dom_node) > + { > + if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) > + { > + int idx; > + > + if ( bd->kernel ) > > + { > + printk(XENLOG_ERR "Duplicate kernel module for domain %s\n", Looks like it should be XENLOG_WARNING since the loop continues. Also, I would use either Capitalized or lower case messages everywhere for consistency. > + name); > + continue; > + } > + > + idx = fdt_read_multiboot_module(fdt, node, address_cells, > + size_cells, bi); > + if ( idx < 0 ) > + { > + printk(" failed processing kernel module for domain %s\n", I think this printout should have XENLOG_ERR in it since it's on the error code path. > + name); > + return idx; > + } > + > + printk(" kernel: boot module %d\n", idx); > + bi->mods[idx].type = BOOTMOD_KERNEL; > > + bd->kernel = &bi->mods[idx]; > > + } > + } > + > + if ( !bd->kernel ) > > + { > + printk(XENLOG_ERR "ERR: no kernel assigned to domain\n"); Drop "ERR" since it is already XENLOG_ERR level? > + return -ENODATA; > + } > + > + return 0; > +} > + > static int __init find_hyperlaunch_node(const void *fdt) > { > int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor"); > @@ -217,9 +263,20 @@ int __init walk_hyperlaunch_fdt(struct boot_info *bi) > > fdt_for_each_subnode(node, fdt, hv_node) > { > + if ( bi->nr_domains >= MAX_NR_BOOTDOMS ) > > + { > + printk(XENLOG_WARNING > + "WARN: only creating first %u domains\n", MAX_NR_BOOTDOMS); Drop "WARN" since it is already XENLOG_WARNING level? > + break; > + } > + > ret = fdt_node_check_compatible(fdt, node, "xen,domain"); > if ( ret == 0 ) > + { > + if ( (ret = process_domain_node(bi, fdt, node)) < 0 ) > + break; > bi->nr_domains++; > > + } > } > > /* Until multi-domain construction is added, throw an error / > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index e5d78bcb48..00e8c8a2a3 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1284,11 +1284,6 @@ void asmlinkage __init noreturn __start_xen(void) > > builder_init(bi); > > - / Find first unknown boot module to use as Dom0 kernel */ > - i = first_boot_module_index(bi, BOOTMOD_UNKNOWN); > - bi->mods[i].type = BOOTMOD_KERNEL; > > - bi->domains[0].kernel = &bi->mods[i]; > > - > if ( pvh_boot ) > { > /* pvh_init() already filled in e820_raw */ > -- > 2.43.0
On 08.04.2025 18:07, Alejandro Vallejo wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > Look for a subnode of type `multiboot,kernel` within a domain node. If > found, locate it using the multiboot module helper to generically ensure > it lives in the module list. If the bootargs property is present and > there was not an MB1 string, then use the command line from the device > tree definition. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > Signed-off-by: Alejandro Vallejo <agarciav@amd.com> > --- > v3: > * Add const to fdt > * Remove idx == NULL checks > * Add BUILD_BUG_ON for MAX_NR_BOOTMODS fitting in a uint32_t At least this one looks to rather belong into patch 08? > --- a/xen/arch/x86/domain-builder/fdt.c > +++ b/xen/arch/x86/domain-builder/fdt.c > @@ -155,6 +155,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, > return idx; > } > > +static int __init process_domain_node( > + struct boot_info *bi, const void *fdt, int dom_node) > +{ > + int node; > + struct boot_domain *bd = &bi->domains[bi->nr_domains]; > + const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; > + int address_cells = fdt_address_cells(fdt, dom_node); > + int size_cells = fdt_size_cells(fdt, dom_node); Oh, okay - regarding my earlier comment elsewhere: If the sizes come from DT, then of course ASSERT_UNREACHABLE() can't be used at the place where bogus ones are rejected. > + fdt_for_each_subnode(node, fdt, dom_node) > + { > + if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) > + { When the loop body is merely an if() with a non-negligible body, inverting the condition and using "continue" is usually better. Much like you do ... > + int idx; > + > + if ( bd->kernel ) > + { > + printk(XENLOG_ERR "Duplicate kernel module for domain %s\n", > + name); > + continue; ... here already. Jan
On Wed Apr 9, 2025 at 10:24 PM BST, Denis Mukhin wrote: > On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: > >> >> >> From: "Daniel P. Smith" dpsmith@apertussolutions.com >> >> >> Look for a subnode of type `multiboot,kernel` within a domain node. If >> found, locate it using the multiboot module helper to generically ensure >> it lives in the module list. If the bootargs property is present and >> there was not an MB1 string, then use the command line from the device >> tree definition. >> >> Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com >> >> Signed-off-by: Jason Andryuk jason.andryuk@amd.com >> >> Signed-off-by: Alejandro Vallejo agarciav@amd.com >> >> --- >> v3: >> * Add const to fdt >> * Remove idx == NULL checks >> * Add BUILD_BUG_ON for MAX_NR_BOOTMODS fitting in a uint32_t >> * Remove trailing ) from printks >> * Return ENODATA for missing kernel >> * Re-work "max domains" warning and print limit >> * fdt_cell_as_u32/directly return values >> * Remove "pairs" looping from fdt_get_reg_prop() and only grab 1. >> * Use addr_cells and size_cells >> --- >> xen/arch/x86/domain-builder/core.c | 11 ++++++ >> xen/arch/x86/domain-builder/fdt.c | 57 ++++++++++++++++++++++++++++++ >> xen/arch/x86/setup.c | 5 --- >> 3 files changed, 68 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c >> index c50eff34fb..eda7fa7a8f 100644 >> --- a/xen/arch/x86/domain-builder/core.c >> +++ b/xen/arch/x86/domain-builder/core.c >> @@ -59,6 +59,17 @@ void __init builder_init(struct boot_info *bi) >> >> printk(XENLOG_INFO " Number of domains: %d\n", bi->nr_domains); >> >> } >> + else >> + { >> + unsigned int i; >> + >> + /* Find first unknown boot module to use as Dom0 kernel */ >> + printk("Falling back to using first boot module as dom0\n"); >> + i = first_boot_module_index(bi, BOOTMOD_UNKNOWN); >> + bi->mods[i].type = BOOTMOD_KERNEL; >> >> + bi->domains[0].kernel = &bi->mods[i]; >> >> + bi->nr_domains = 1; >> >> + } >> } >> >> /* >> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c >> index 9ebc8fd0e4..a037c8b6cb 100644 >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -155,6 +155,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, >> return idx; >> } >> >> +static int __init process_domain_node( >> + struct boot_info *bi, const void *fdt, int dom_node) >> +{ >> + int node; >> + struct boot_domain *bd = &bi->domains[bi->nr_domains]; >> >> + const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; >> + int address_cells = fdt_address_cells(fdt, dom_node); >> + int size_cells = fdt_size_cells(fdt, dom_node); >> + >> + fdt_for_each_subnode(node, fdt, dom_node) >> + { >> + if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) >> + { >> + int idx; >> + >> + if ( bd->kernel ) >> >> + { >> + printk(XENLOG_ERR "Duplicate kernel module for domain %s\n", > > Looks like it should be XENLOG_WARNING since the loop continues. Fair point. > > Also, I would use either Capitalized or lower case messages everywhere > for consistency. That's related to those leading spaces. The lowercases end up immediately under the configuration message so it's easier to bind them visually as "hyperlaunch-related". (XEN) Hyperlaunch configuration: (XEN) something (XEN) failed processing kernel module for domain %s > >> + name); >> + continue; >> + } >> + >> + idx = fdt_read_multiboot_module(fdt, node, address_cells, >> + size_cells, bi); >> + if ( idx < 0 ) >> + { >> + printk(" failed processing kernel module for domain %s\n", > > I think this printout should have XENLOG_ERR in it since it's on the > error code path. All of these should have a XENLOG_X so they can be skipped when _INFO is itself filtered out. > >> + name); >> + return idx; >> + } >> + >> + printk(" kernel: boot module %d\n", idx); >> + bi->mods[idx].type = BOOTMOD_KERNEL; >> >> + bd->kernel = &bi->mods[idx]; >> >> + } >> + } >> + >> + if ( !bd->kernel ) >> >> + { >> + printk(XENLOG_ERR "ERR: no kernel assigned to domain\n"); > > Drop "ERR" since it is already XENLOG_ERR level? ERR: is printed though, whereas XENLOG_ERR is not. That's meant to make it visually clear that's _really_ not meant to happen. > >> + return -ENODATA; >> + } >> + >> + return 0; >> +} >> + >> static int __init find_hyperlaunch_node(const void *fdt) >> { >> int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor"); >> @@ -217,9 +263,20 @@ int __init walk_hyperlaunch_fdt(struct boot_info *bi) >> >> fdt_for_each_subnode(node, fdt, hv_node) >> { >> + if ( bi->nr_domains >= MAX_NR_BOOTDOMS ) >> >> + { >> + printk(XENLOG_WARNING >> + "WARN: only creating first %u domains\n", MAX_NR_BOOTDOMS); > > Drop "WARN" since it is already XENLOG_WARNING level? Same rationale as above. > >> + break; >> + } >> + >> ret = fdt_node_check_compatible(fdt, node, "xen,domain"); >> if ( ret == 0 ) >> + { >> + if ( (ret = process_domain_node(bi, fdt, node)) < 0 ) >> + break; >> bi->nr_domains++; >> >> + } >> } >> >> /* Until multi-domain construction is added, throw an error / >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index e5d78bcb48..00e8c8a2a3 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1284,11 +1284,6 @@ void asmlinkage __init noreturn __start_xen(void) >> >> builder_init(bi); >> >> - / Find first unknown boot module to use as Dom0 kernel */ >> - i = first_boot_module_index(bi, BOOTMOD_UNKNOWN); >> - bi->mods[i].type = BOOTMOD_KERNEL; >> >> - bi->domains[0].kernel = &bi->mods[i]; >> >> - >> if ( pvh_boot ) >> { >> /* pvh_init() already filled in e820_raw */ >> -- >> 2.43.0 Cheers, Alejandro
On Thu Apr 10, 2025 at 11:58 AM BST, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> Look for a subnode of type `multiboot,kernel` within a domain node. If >> found, locate it using the multiboot module helper to generically ensure >> it lives in the module list. If the bootargs property is present and >> there was not an MB1 string, then use the command line from the device >> tree definition. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> >> Signed-off-by: Alejandro Vallejo <agarciav@amd.com> >> --- >> v3: >> * Add const to fdt >> * Remove idx == NULL checks >> * Add BUILD_BUG_ON for MAX_NR_BOOTMODS fitting in a uint32_t > > At least this one looks to rather belong into patch 08? Urg, yes. Sorry. There was a lot of code motion when I factored out the helpers. > >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -155,6 +155,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, >> return idx; >> } >> >> +static int __init process_domain_node( >> + struct boot_info *bi, const void *fdt, int dom_node) >> +{ >> + int node; >> + struct boot_domain *bd = &bi->domains[bi->nr_domains]; >> + const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; >> + int address_cells = fdt_address_cells(fdt, dom_node); >> + int size_cells = fdt_size_cells(fdt, dom_node); > > Oh, okay - regarding my earlier comment elsewhere: If the sizes come from DT, > then of course ASSERT_UNREACHABLE() can't be used at the place where bogus > ones are rejected. > >> + fdt_for_each_subnode(node, fdt, dom_node) >> + { >> + if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) >> + { > > When the loop body is merely an if() with a non-negligible body, inverting > the condition and using "continue" is usually better. Much like you do ... This becomes a chain of if conditions later on, one per property. > >> + int idx; >> + >> + if ( bd->kernel ) >> + { >> + printk(XENLOG_ERR "Duplicate kernel module for domain %s\n", >> + name); >> + continue; > > ... here already. > > Jan Cheers, Alejandro
diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c index c50eff34fb..eda7fa7a8f 100644 --- a/xen/arch/x86/domain-builder/core.c +++ b/xen/arch/x86/domain-builder/core.c @@ -59,6 +59,17 @@ void __init builder_init(struct boot_info *bi) printk(XENLOG_INFO " Number of domains: %d\n", bi->nr_domains); } + else + { + unsigned int i; + + /* Find first unknown boot module to use as Dom0 kernel */ + printk("Falling back to using first boot module as dom0\n"); + i = first_boot_module_index(bi, BOOTMOD_UNKNOWN); + bi->mods[i].type = BOOTMOD_KERNEL; + bi->domains[0].kernel = &bi->mods[i]; + bi->nr_domains = 1; + } } /* diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c index 9ebc8fd0e4..a037c8b6cb 100644 --- a/xen/arch/x86/domain-builder/fdt.c +++ b/xen/arch/x86/domain-builder/fdt.c @@ -155,6 +155,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, return idx; } +static int __init process_domain_node( + struct boot_info *bi, const void *fdt, int dom_node) +{ + int node; + struct boot_domain *bd = &bi->domains[bi->nr_domains]; + const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; + int address_cells = fdt_address_cells(fdt, dom_node); + int size_cells = fdt_size_cells(fdt, dom_node); + + fdt_for_each_subnode(node, fdt, dom_node) + { + if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) + { + int idx; + + if ( bd->kernel ) + { + printk(XENLOG_ERR "Duplicate kernel module for domain %s\n", + name); + continue; + } + + idx = fdt_read_multiboot_module(fdt, node, address_cells, + size_cells, bi); + if ( idx < 0 ) + { + printk(" failed processing kernel module for domain %s\n", + name); + return idx; + } + + printk(" kernel: boot module %d\n", idx); + bi->mods[idx].type = BOOTMOD_KERNEL; + bd->kernel = &bi->mods[idx]; + } + } + + if ( !bd->kernel ) + { + printk(XENLOG_ERR "ERR: no kernel assigned to domain\n"); + return -ENODATA; + } + + return 0; +} + static int __init find_hyperlaunch_node(const void *fdt) { int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor"); @@ -217,9 +263,20 @@ int __init walk_hyperlaunch_fdt(struct boot_info *bi) fdt_for_each_subnode(node, fdt, hv_node) { + if ( bi->nr_domains >= MAX_NR_BOOTDOMS ) + { + printk(XENLOG_WARNING + "WARN: only creating first %u domains\n", MAX_NR_BOOTDOMS); + break; + } + ret = fdt_node_check_compatible(fdt, node, "xen,domain"); if ( ret == 0 ) + { + if ( (ret = process_domain_node(bi, fdt, node)) < 0 ) + break; bi->nr_domains++; + } } /* Until multi-domain construction is added, throw an error */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index e5d78bcb48..00e8c8a2a3 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1284,11 +1284,6 @@ void asmlinkage __init noreturn __start_xen(void) builder_init(bi); - /* Find first unknown boot module to use as Dom0 kernel */ - i = first_boot_module_index(bi, BOOTMOD_UNKNOWN); - bi->mods[i].type = BOOTMOD_KERNEL; - bi->domains[0].kernel = &bi->mods[i]; - if ( pvh_boot ) { /* pvh_init() already filled in e820_raw */