Re: [PATCH v3 12/16] x86/hyperlaunch: add domain id parsing to domain config

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Apr 9, 2025 at 11:15 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
>> 
>> 
>> Introduce the ability to specify the desired domain id for the domain
>> definition. The domain id will be populated in the domid property of the
>> domain
>> node in the device tree configuration.
>> 
>> Signed-off-by: Daniel P. Smith dpsmith@xxxxxxxxxxxxxxxxxxxx
>> 
>> ---
>> v3:
>> * Remove ramdisk parsing
>> * Add missing xen/errno.h include
>> ---
>> xen/arch/x86/domain-builder/fdt.c | 39 ++++++++++++++++++++++++++++-
>> xen/arch/x86/setup.c | 5 ++--
>> xen/include/xen/libfdt/libfdt-xen.h | 11 ++++++++
>> 3 files changed, 52 insertions(+), 3 deletions(-)
>> 
>> diff --git a/xen/arch/x86/domain-builder/fdt.c 
>> b/xen/arch/x86/domain-builder/fdt.c
>> index 0f5fd01557..4c6aafe195 100644
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -8,6 +8,7 @@
>> #include <xen/libfdt/libfdt.h>
>> 
>> 
>> #include <asm/bootinfo.h>
>> 
>> +#include <asm/guest.h>
>> 
>> #include <asm/page.h>
>> 
>> #include <asm/setup.h>
>> 
>> 
>> @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, 
>> int node,
>> static int __init process_domain_node(
>> struct boot_info *bi, const void *fdt, int dom_node)
>> {
>> - int node;
>> + int node, property;
>> 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_property_offset(property, fdt, dom_node)
>> + {
>> + const struct fdt_property *prop;
>> + const char prop_name;
>> + int name_len;
>> +
>> + prop = fdt_get_property_by_offset(fdt, property, NULL);
>> + if ( !prop )
>> + continue; / silently skip */
>> +
>> + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len);
>> 
>> +
>> + if ( strncmp(prop_name, "domid", name_len) == 0 )
>> + {
>> + uint32_t val = DOMID_INVALID;
>> + if ( fdt_prop_as_u32(prop, &val) != 0 )
>> + {
>> + printk(" failed processing domain id for domain %s\n", name);
>
> Add XENLOG_ERR ?

Yes, and...

>
>> + return -EINVAL;
>> + }
>> + if ( val >= DOMID_FIRST_RESERVED )
>> 
>> + {
>> + printk(" invalid domain id for domain %s\n", name);
>
> Add XENLOG_ERR ?

... yes.

>
>> + return -EINVAL;
>> + }
>> + bd->domid = (domid_t)val;
>> 
>> + printk(" domid: %d\n", bd->domid);
>> 
>> + }
>> + }
>> +
>> fdt_for_each_subnode(node, fdt, dom_node)
>> {
>> if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
>> @@ -233,6 +264,12 @@ static int __init process_domain_node(
>> return -ENODATA;
>> }
>> 
>> + if ( bd->domid == DOMID_INVALID )
>> 
>> + bd->domid = get_initial_domain_id();
>> 
>> + else if ( bd->domid != get_initial_domain_id() )
>> 
>> + printk(XENLOG_WARNING
>> + "WARN: Booting without initial domid not supported.\n");
>
> Drop WARN since the log message is XENLOG_WARNING level already?

As mentioned elsewhere, the point of those prefixes are to be readable.

Though I'm starting to get urges to rewrite many of this error handlers
as asserts, on the basis that "why do we think it's ok to boot with
malformed DTBs"? A safe system that doesn't boot is more helpful than an
unsafe one that boots everything except a critical component for you to
find later on.

Cheers,
Alejandro