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" <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>
>> ---
>> 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
