From: Denis Mukhin <dmukhin@ford.com>
Currently, hypervisor code has two different non-system domain ID allocation
implementations:
(a) Sequential IDs allocation in dom0less Arm code based on max_init_domid;
(b) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
max_init_domid (both Arm and x86).
It makes sense to have a common helper code for such task across architectures
(Arm and x86) and between dom0less / toolstack domU allocation.
Wrap the domain ID allocation as an arch-independent function domid_alloc() in
common/domain.c based on the bitmap.
Allocation algorithm:
- If an explicit domain ID is provided, verify its availability and use it if
ID is not used;
- If DOMID_INVALID is provided, search the range [0..DOMID_FIRST_RESERVED-1],
starting from the last used ID and wrapping around as needed. It guarantees
that two consecutive calls will never return the same ID.
Also, remove is_free_domid() helper as it is not needed now.
No functional change intended.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v7:
- Use DOMID_FIRST_RESERVED
---
xen/arch/arm/domain_build.c | 17 ++++++---
xen/arch/x86/setup.c | 11 +++---
xen/common/device-tree/dom0less-build.c | 10 +++---
xen/common/domain.c | 47 +++++++++++++++++++++++++
xen/common/domctl.c | 42 +++-------------------
xen/include/xen/domain.h | 3 ++
6 files changed, 80 insertions(+), 50 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b189a7cfae..e9d563c269 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2010,6 +2010,7 @@ void __init create_dom0(void)
.grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
};
unsigned int flags = CDF_privileged | CDF_hardware;
+ domid_t domid;
int rc;
/* The vGIC for DOM0 is exactly emulating the hardware GIC */
@@ -2034,19 +2035,25 @@ void __init create_dom0(void)
if ( !llc_coloring_enabled )
flags |= CDF_directmap;
- dom0 = domain_create(0, &dom0_cfg, flags);
+ domid = domid_alloc(0);
+ if ( domid == DOMID_INVALID )
+ panic("Error allocating domain ID 0\n");
+
+ dom0 = domain_create(domid, &dom0_cfg, flags);
if ( IS_ERR(dom0) )
- panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
+ panic("Error creating domain %d (rc = %ld)\n", domid, PTR_ERR(dom0));
if ( llc_coloring_enabled && (rc = dom0_set_llc_colors(dom0)) )
- panic("Error initializing LLC coloring for domain 0 (rc = %d)\n", rc);
+ panic("Error initializing LLC coloring for domain %pd (rc = %d)\n",
+ dom0, rc);
if ( alloc_dom0_vcpu0(dom0) == NULL )
- panic("Error creating domain 0 vcpu0\n");
+ panic("Error creating domain %pdv0\n", dom0);
rc = construct_dom0(dom0);
if ( rc )
- panic("Could not set up DOM0 guest OS (rc = %d)\n", rc);
+ panic("Could not set up guest OS for domain %pd (rc = %d)\n",
+ dom0, rc);
set_xs_domain(dom0);
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2518954124..ac1c3e669b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1030,8 +1030,11 @@ 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();
+ /* Allocate initial domain ID. Not d0 for pvshim. */
+ bd->domid = domid_alloc(get_initial_domain_id());
+ if ( bd->domid == DOMID_INVALID )
+ panic("Error allocating domain ID %d\n", get_initial_domain_id());
+
d = domain_create(bd->domid, &dom0_cfg,
pv_shim ? 0 : CDF_privileged | CDF_hardware);
if ( IS_ERR(d) )
@@ -1063,7 +1066,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( (strlen(acpi_param) == 0) && acpi_disabled )
{
- printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
+ printk("ACPI is disabled, notifying domain %pd (acpi=off)\n", d);
safe_strcpy(acpi_param, "off");
}
@@ -1078,7 +1081,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
bd->d = d;
if ( construct_dom0(bd) != 0 )
- panic("Could not construct domain 0\n");
+ panic("Could not construct domain %pd\n", d);
bd->cmdline = NULL;
xfree(cmdline);
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 2c56f13771..9236dbae11 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -850,15 +850,13 @@ void __init create_domUs(void)
struct xen_domctl_createdomain d_cfg = {0};
unsigned int flags = 0U;
bool has_dtb = false;
+ domid_t domid;
uint32_t val;
int rc;
if ( !dt_device_is_compatible(node, "xen,domain") )
continue;
- if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
- panic("No more domain IDs available\n");
-
d_cfg.max_evtchn_port = 1023;
d_cfg.max_grant_frames = -1;
d_cfg.max_maptrack_frames = -1;
@@ -981,7 +979,11 @@ void __init create_domUs(void)
* very important to use the pre-increment operator to call
* domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
*/
- d = domain_create(++max_init_domid, &d_cfg, flags);
+ domid = domid_alloc(++max_init_domid);
+ if ( domid == DOMID_INVALID )
+ panic("Error allocating ID for domain %s\n", dt_node_name(node));
+
+ d = domain_create(domid, &d_cfg, flags);
if ( IS_ERR(d) )
panic("Error creating domain %s (rc = %ld)\n",
dt_node_name(node), PTR_ERR(d));
diff --git a/xen/common/domain.c b/xen/common/domain.c
index abf1969e60..9c6932c457 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -66,6 +66,11 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
static struct domain *domain_hash[DOMAIN_HASH_SIZE];
struct domain *domain_list;
+/* Non-system domain ID allocator. */
+static DEFINE_SPINLOCK(domid_lock);
+static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
+static domid_t domid_last;
+
/*
* Insert a domain into the domlist/hash. This allows the domain to be looked
* up by domid, and therefore to be the subject of hypercalls/etc.
@@ -1449,6 +1454,8 @@ void domain_destroy(struct domain *d)
TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
+ domid_free(d->domain_id);
+
/* Remove from the domlist/hash. */
domlist_remove(d);
@@ -2405,6 +2412,46 @@ domid_t get_initial_domain_id(void)
return hardware_domid;
}
+domid_t domid_alloc(domid_t domid)
+{
+ spin_lock(&domid_lock);
+
+ if ( domid < DOMID_FIRST_RESERVED )
+ {
+ if ( __test_and_set_bit(domid, domid_bitmap) )
+ domid = DOMID_INVALID;
+ }
+ else
+ {
+ domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
+ domid_last);
+
+ if ( domid == DOMID_FIRST_RESERVED )
+ domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
+
+ if ( domid == DOMID_FIRST_RESERVED )
+ {
+ domid = DOMID_INVALID;
+ }
+ else
+ {
+ __set_bit(domid, domid_bitmap);
+ domid_last = domid;
+ }
+ }
+
+ spin_unlock(&domid_lock);
+
+ return domid;
+}
+
+void domid_free(domid_t domid)
+{
+ spin_lock(&domid_lock);
+ __clear_bit(domid, domid_bitmap);
+ spin_unlock(&domid_lock);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index bfe2e1f9f0..8ef0c147c9 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -49,20 +49,6 @@ static int xenctl_bitmap_to_nodemask(nodemask_t *nodemask,
MAX_NUMNODES);
}
-static inline int is_free_domid(domid_t dom)
-{
- struct domain *d;
-
- if ( dom >= DOMID_FIRST_RESERVED )
- return 0;
-
- if ( (d = rcu_lock_domain_by_id(dom)) == NULL )
- return 1;
-
- rcu_unlock_domain(d);
- return 0;
-}
-
void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
{
struct vcpu *v;
@@ -421,36 +407,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
case XEN_DOMCTL_createdomain:
{
- domid_t dom;
- static domid_t rover = 0;
+ domid_t domid = domid_alloc(op->domain);
- dom = op->domain;
- if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
+ if ( domid == DOMID_INVALID )
{
ret = -EEXIST;
- if ( !is_free_domid(dom) )
- break;
- }
- else
- {
- for ( dom = rover + 1; dom != rover; dom++ )
- {
- if ( dom == DOMID_FIRST_RESERVED )
- dom = 1;
- if ( is_free_domid(dom) )
- break;
- }
-
- ret = -ENOMEM;
- if ( dom == rover )
- break;
-
- rover = dom;
+ break;
}
- d = domain_create(dom, &op->u.createdomain, false);
+ d = domain_create(domid, &op->u.createdomain, false);
if ( IS_ERR(d) )
{
+ domid_free(domid);
ret = PTR_ERR(d);
d = NULL;
break;
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index e10baf2615..8aab05ae93 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -38,6 +38,9 @@ void arch_get_domain_info(const struct domain *d,
domid_t get_initial_domain_id(void);
+domid_t domid_alloc(domid_t domid);
+void domid_free(domid_t domid);
+
/* CDF_* constant. Internal flags for domain creation. */
/* Is this a privileged domain? */
#define CDF_privileged (1U << 0)
--
2.34.1On 21.05.2025 02:00, dmkhn@proton.me wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -66,6 +66,11 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
> static struct domain *domain_hash[DOMAIN_HASH_SIZE];
> struct domain *domain_list;
>
> +/* Non-system domain ID allocator. */
> +static DEFINE_SPINLOCK(domid_lock);
> +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> +static domid_t domid_last;
Please move this into the narrowest possible scope. No reason to expose
it to the entire CU.
> @@ -2405,6 +2412,46 @@ domid_t get_initial_domain_id(void)
> return hardware_domid;
> }
>
> +domid_t domid_alloc(domid_t domid)
> +{
> + spin_lock(&domid_lock);
> +
> + if ( domid < DOMID_FIRST_RESERVED )
> + {
> + if ( __test_and_set_bit(domid, domid_bitmap) )
> + domid = DOMID_INVALID;
> + }
> + else
> + {
> + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
> + domid_last);
> +
> + if ( domid == DOMID_FIRST_RESERVED )
> + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
Despite what the last sentence of the description says, there is a subtle
behavioral change here: The original code would never return what was
called "rover" there. If the most recently created domain went away, you
would return its ID again here if no other ID is free (which is easier to
run into with patch 3 in place, and MAX_DOMID set to a pretty low value).
(Noticing only later: This could even occur without wrapping, as the last
argument passed is just domid_last, without adding in 1.)
I agree it's debatable whether using the sole available ID instead of
failing wouldn't be better(?) behavior in this specific case, but such
definitely can't go silently.
Furthermore you once again are potentially returning ID 0 here (after
wrapping), when the original code specifically avoided that (irrespective
of there actually being a Dom0, that is; i.e. covering both the dom0less
case and the late-hwdom one).
To be frank, being at v8 I find it problematic that there still are
(unmentioned and potentially unwanted) behavioral changes here. This
kind of supports my earlier raised question of whether we actually want
this sort of a change.
Jan
On Wed, May 21, 2025 at 10:02:52AM +0200, Jan Beulich wrote:
> On 21.05.2025 02:00, dmkhn@proton.me wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -66,6 +66,11 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
> > static struct domain *domain_hash[DOMAIN_HASH_SIZE];
> > struct domain *domain_list;
> >
> > +/* Non-system domain ID allocator. */
> > +static DEFINE_SPINLOCK(domid_lock);
> > +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> > +static domid_t domid_last;
>
> Please move this into the narrowest possible scope. No reason to expose
> it to the entire CU.
Will do.
>
> > @@ -2405,6 +2412,46 @@ domid_t get_initial_domain_id(void)
> > return hardware_domid;
> > }
> >
> > +domid_t domid_alloc(domid_t domid)
> > +{
> > + spin_lock(&domid_lock);
> > +
> > + if ( domid < DOMID_FIRST_RESERVED )
> > + {
> > + if ( __test_and_set_bit(domid, domid_bitmap) )
> > + domid = DOMID_INVALID;
> > + }
> > + else
> > + {
> > + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
> > + domid_last);
> > +
> > + if ( domid == DOMID_FIRST_RESERVED )
> > + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
>
> Despite what the last sentence of the description says, there is a subtle
> behavioral change here: The original code would never return what was
> called "rover" there. If the most recently created domain went away, you
> would return its ID again here if no other ID is free (which is easier to
> run into with patch 3 in place, and MAX_DOMID set to a pretty low value).
> (Noticing only later: This could even occur without wrapping, as the last
> argument passed is just domid_last, without adding in 1.)
Thanks for spotting the problem!
>
> I agree it's debatable whether using the sole available ID instead of
> failing wouldn't be better(?) behavior in this specific case, but such
> definitely can't go silently.
>
> Furthermore you once again are potentially returning ID 0 here (after
> wrapping), when the original code specifically avoided that (irrespective
> of there actually being a Dom0, that is; i.e. covering both the dom0less
> case and the late-hwdom one).
>
> To be frank, being at v8 I find it problematic that there still are
> (unmentioned and potentially unwanted) behavioral changes here. This
> kind of supports my earlier raised question of whether we actually want
> this sort of a change.
I appreciate patience and the feedback for the series!
>
> Jan
© 2016 - 2025 Red Hat, Inc.