Re: [PATCH v5 08/12] x86/hyperlaunch: add domain id parsing to domain config

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Thu Apr 24, 2025 at 6:41 PM BST, Jason Andryuk wrote:
> On 2025-04-24 12:10, Alejandro Vallejo 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>
>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>> ---
>
>> diff --git a/xen/common/domain-builder/fdt.c 
>> b/xen/common/domain-builder/fdt.c
>> index 144fcc75b5..5a5b3c8806 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>
>> @@ -188,12 +189,54 @@ static 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, rc;
>> +
>> +        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);
>> +
>
> Stray blank line.
>
>> +        if ( !strncmp(prop_name, "domid", name_len) )
>> +        {
>> +            uint32_t val = DOMID_INVALID;
>> +
>> +            if ( (rc = fdt_prop_as_u32(prop, &val)) )
>> +            {
>> +                printk(XENLOG_ERR
>> +                       "  failed processing domain id for domain %s\n", 
>> name);
>> +                return rc;
>> +            }
>
> Maybe add a blank line here?
>
>> +            if ( val >= DOMID_FIRST_RESERVED )
>> +            {
>> +                printk(XENLOG_ERR "  invalid domain id for domain %s\n", 
>> name);
>> +                return -EINVAL;
>> +            }
>> +
>
>> @@ -258,6 +301,13 @@ 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
>> +               "warning: unsupported boot. d%d is not the initial 
>> domain.\n",
>
> Maybe:
> "warning: d%u is not the expected initial domid (%u)\n" ?
>
> It's a strange message.  The domid property is added, but it's not 
> expected to actually be used?

It's just a transient message until multidomain boot is added in a later
series. It's merely informative that you're booting something you
probably didn't mean to. Or didn't mean to on this hypervisor.

>
> With the newlines addressed (and optionally the message change):

I'm happy to make those adjustments.

>
> Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>

Thanks

>
> Thanks,
> Jason

Cheers,
Alejandro