On 08.04.2025 18:07, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsm...@apertussolutions.com>
>
> 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
