Re: [PATCH v2 11/15] x86/hyperlaunch: add domain id parsing to domain config

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.12.2024 17:57, Daniel P. Smith wrote:
> 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.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>

(Not going to repeat style remarks already made on earlier patches. Please
apply throughout the series.)

> @@ -61,10 +62,40 @@ static int __init dom0less_module_index(
>  static int __init process_domain_node(
>      struct boot_info *bi, 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";
>  
> +    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 )

Isn't this going to (wrongly) match when e.g. the property has just "d" (and
hence name_len is 1).

> +        {
> +            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;
> +            printk("  domid: %d\n", bd->domid);
> +        }
> +    }

Perhaps the question comes too early (will be taken care of in later
patches), but still: What if multiple domains have the same ID specified?

> @@ -125,7 +156,29 @@ static int __init process_domain_node(
>          else if (
>              fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
>          {
> -            int idx = dom0less_module_node(fdt, node, size_size, 
> address_size);
> +            unsigned int idx;
> +            int ret = 0;
> +
> +            if ( bd->ramdisk )
> +            {
> +                printk(XENLOG_ERR "Duplicate ramdisk module for domain 
> %s)\n",
> +                       name);
> +                continue;
> +            }
> +
> +            /* Try hyperlaunch property, fall back to dom0less property. */
> +            if ( hl_module_index(fdt, node, &idx) < 0 )
> +            {
> +                int address_size = fdt_address_cells(fdt, dom_node);
> +                int size_size = fdt_size_cells(fdt, dom_node);
> +
> +                if ( address_size < 0 || size_size < 0 )
> +                    ret = -EINVAL;
> +                else
> +                    ret = dom0less_module_index(
> +                            fdt, node, size_size, address_size, &idx);
> +            }

Doesn't this belong into the earlier patch?

> @@ -154,6 +207,12 @@ static int __init process_domain_node(
>          return -EFAULT;
>      }
>  
> +    if ( bd->domid == DOMID_INVALID )
> +        bd->domid = get_initial_domain_id();

Isn't this redundant with ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1029,8 +1029,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();

... this?

Jan