Re: [PATCH v3 09/16] x86/hyperlaunch: locate dom0 kernel with hyperlaunch

[Alejandro Vallejo <agarciav@xxxxxxx>]

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@xxxxxxx> 
> wrote:
>
>> 
>> 
>> From: "Daniel P. Smith" dpsmith@xxxxxxxxxxxxxxxxxxxx
>> 
>> 
>> 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@xxxxxxxxxxxxxxxxxxxx
>> 
>> Signed-off-by: Jason Andryuk jason.andryuk@xxxxxxx
>> 
>> Signed-off-by: Alejandro Vallejo agarciav@xxxxxxx
>> 
>> ---
>> 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