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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2025 14:05, Alejandro Vallejo wrote:
> On Tue Apr 15, 2025 at 7:27 AM BST, Jan Beulich wrote:
>> 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.
> 
> That'd be fairly unsafe. In the case of parallel boot it'd be
> indeterminate which VMs end up running if they happen to have a domid
> clash. It's better to detect the error earlier and crash before any get
> to start up.

What's the unsafe aspect here? We'd crash either way; the domain(s) that
may be successfully launched wouldn't make it very far.

Yet irrespective - my request is _that_ collisions are checked for. I
don't mind much _where_ that checking lives.

>>>>> @@ -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.
> 
> It's not that the domid is missing from the node, but that the domid in
> the node doesn't match the initial domid. Maybe s/domid/domain, then?
> 
>   "Warning: Unsupported boot with missing initial domain"

I must be missing something: When it's "don't match" why would the message
say "missing"?

Jan