On Fri Apr 18, 2025 at 11:39 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:28PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsm...@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 <dpsm...@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andr...@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarc...@amd.com>
>> ---
>> v4:
>> * Stop printing on the fallback path of builder_init().
>> It's in fact the most common path and just adds noise.
>> * Add missing XENLOG_X.
>> * Simplified check to log error on nr_domains != 1.
>> * s/XENLOG_ERR/XENLOG_WARNING/ on duplicate kernel.
>> * Turned foo == 0 into !foo in the "multiboot,kernel" check
>> ---
>> xen/arch/x86/setup.c | 5 ---
>> xen/common/domain-builder/core.c | 9 +++++
>> xen/common/domain-builder/fdt.c | 64 ++++++++++++++++++++++++++++++--
>> xen/include/xen/domain-builder.h | 3 --
>> 4 files changed, 70 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index ccc57cc70a..4f669f3c60 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1285,11 +1285,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 */
>> diff --git a/xen/common/domain-builder/core.c
>> b/xen/common/domain-builder/core.c
>> index 3b062e85ec..924cb495a3 100644
>> --- a/xen/common/domain-builder/core.c
>> +++ b/xen/common/domain-builder/core.c
>> @@ -54,6 +54,15 @@ void __init builder_init(struct boot_info *bi)
>>
>> printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
>> }
>> + else
>> + {
>> + /* Find first unknown boot module to use as dom0 kernel */
>> + unsigned int 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/common/domain-builder/fdt.c
>> b/xen/common/domain-builder/fdt.c
>> index d73536fed6..1fae6add3b 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -89,9 +89,9 @@ static int __init read_fdt_prop_as_reg(const struct
>> fdt_property *prop,
>> * @return -EINVAL on malformed nodes, otherwise
>> * index inside `bi->mods`
>> */
>> -int __init fdt_read_multiboot_module(const void *fdt, int node,
>> - int address_cells, int size_cells,
>> - struct boot_info *bi)
>> +static int __init fdt_read_multiboot_module(const void *fdt, int node,
>> + int address_cells, int
>> size_cells,
>> + struct boot_info *bi)
>> {
>> const struct fdt_property *prop;
>> uint64_t addr, size;
>> @@ -175,6 +175,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") )
>
> Suggest to restructure the code to reduce levels of indentation, e.g.:
>
> int idx;
>
> if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
> continue;
>
> if ( bd->kernel )
> ...
>
This hunk checking for "multiboot,kernel" is part of an if-elseif in a
later patch that also checks for "multiboot,ramdisk", so we can't just
do an early continue here without forcing a bigger diff later on.
>
>> + {
>> + int idx;
>> +
>> + if ( bd->kernel )
>> + {
>> + printk(XENLOG_WARNING
>> + " duplicate kernel for domain %s\n", name);
>> + continue;
>> + }
>> +
>> + idx = fdt_read_multiboot_module(fdt, node, address_cells,
>> + size_cells, bi);
>> + if ( idx < 0 )
>> + {
>> + printk(XENLOG_ERR
>> + " failed processing kernel for domain %s\n", name);
>> + return idx;
>> + }
>> +
>> + printk(XENLOG_INFO " kernel: multiboot-index=%d\n", idx);
>> + bi->mods[idx].type = BOOTMOD_KERNEL;
>> + bd->kernel = &bi->mods[idx];
>> + }
>> + }
>> +
>> + if ( !bd->kernel )
>> + {
>> + printk(XENLOG_ERR "error: no kernel assigned to domain\n");
>
> Add domain name printout similar to above logging?
Good point.
Cheers,
Alejandro
