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