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