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

[Alejandro Vallejo <agarciav@xxxxxxx>]

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.

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

Cheers,
Alejandro