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" <dpsm...@apertussolutions.com>
>>>> @@ -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
