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

[Alejandro Vallejo <agarciav@xxxxxxx>]

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>
>> 
>> Introduce the ability to specify the desired domain id for the domain
>> definition. The domain id will be populated in the domid property of the
>> domain
>> node in the device tree configuration.
>
> Nit: Odd splitting of lines.

Fixed

>
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -8,6 +8,7 @@
>>  #include <xen/libfdt/libfdt.h>
>>  
>>  #include <asm/bootinfo.h>
>> +#include <asm/guest.h>
>
> What is this needed for?

get_initial_domain_id(), but that ought to come from xen/domain.h instead.

Fixed.

>
>> @@ -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 )
>
> Nit: Blank line please between declaration(s) and statement(s).

Ack.

>
>> +            {
>> +                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.

>
>> +            printk("  domid: %d\n", bd->domid);
>
> If the error messages log "name" for (I suppose) disambiguation, why would
> the success message here not also do so?
>
>> @@ -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.".

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.

>
> Also nit: No full stops please at the end of log messages, at least in the
> common case.

Ack

>
> Is the resolving of DOMID_INVALID invalid really needed both here and ...
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1033,8 +1033,9 @@ static struct domain *__init create_dom0(struct 
>> boot_info *bi)
>>      if ( iommu_enabled )
>>          dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>  
>> -    /* Create initial domain.  Not d0 for pvshim. */
>> -    bd->domid = get_initial_domain_id();
>> +    if ( bd->domid == DOMID_INVALID )
>> +        /* Create initial domain.  Not d0 for pvshim. */
>> +        bd->domid = get_initial_domain_id();
>
> ... here?

I'll rationatise all that on v4.

>
>> @@ -23,6 +24,16 @@ static inline uint64_t  __init fdt_cell_as_u64(const 
>> fdt32_t *cell)
>>      return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]);
>>  }
>>  
>> +static inline int __init fdt_prop_as_u32(
>> +    const struct fdt_property *prop, uint32_t *val)
>> +{
>> +    if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) )
>> +        return -EINVAL;
>> +
>> +    *val = fdt_cell_as_u32((fdt32_t *)prop->data);
>> +    return 0;
>> +}
>
> Path 08 looks to (partly) open-code this. Perhaps better to introduce already
> there?

Already done.

Cheers,
Alejandro