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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 20:35, Alejandro Vallejo wrote:
> On Thu Apr 10, 2025 at 12:49 PM BST, Jan Beulich wrote:
>> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>>> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>> @@ -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);
>>> +                return -EINVAL;
>>> +            }
>>> +            if ( val >= DOMID_FIRST_RESERVED )
>>> +            {
>>> +                printk("  invalid domain id for domain %s\n", name);
>>> +                return -EINVAL;
>>> +            }
>>> +            bd->domid = (domid_t)val;
>>
>> And a conflict with other domains' IDs will not be complained about?
> 
> Hmmm... sure, I can iterate the domlist and check.

Well, just to clarify: The checking doesn't necessarily need to happen here
and now. It may also happen as domains are actually created. Yet then I
think a pointer there (in a code comment) would be helpful here.

>>> @@ -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");
>>
>> I'm not a native speaker, but (or perhaps because of that) "without" feels
>> wrong here.
> 
> It's probably the compound effect of without and "not supported". The
> statement is correct, but it's arguably a bit obtuse.
> 
> I'll replace it with "WARN: Unsupported boot with missing initial domid.".

But that still doesn't fit the check, which compares the given ID (i.e.
there's nothing "missing" here) with the expected one. The "no ID given"
is handled by the plain if() that's first.

> As for the first branch of that clause... I'm not sure we want to
> support running with DTs that are missing the domid property.

This I can't really judge on.

Jan