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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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?

> @@ -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).

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

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

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

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?

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

Jan