* [PATCH v13 0/3] xen/domain: domain ID allocation
@ 2025-07-30 17:40 dmkhn
2025-07-30 17:40 ` [PATCH v13 1/3] xen/domain: unify " dmkhn
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: dmkhn @ 2025-07-30 17:40 UTC (permalink / raw)
To: xen-devel
Cc: alejandro.garciavallejo, andrew.cooper3, anthony.perard, jbeulich,
julien, michal.orzel, roger.pau, sstabellini, dmukhin
Patch 1 introduces new domid_{alloc,free} calls.
Patch 2 introduces some basic testing for domain ID allocator.
Patch 3 adjusts create_dom0() messages (use %pd).
Link to v12: https://lore.kernel.org/xen-devel/20250730033414.1614441-1-dmukhin@ford.com/
Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1957695581
Denis Mukhin (3):
xen/domain: unify domain ID allocation
tools/tests: introduce unit tests for domain ID allocator
xen/domain: update create_dom0() messages
tools/tests/Makefile | 2 +-
tools/tests/domid/.gitignore | 2 +
tools/tests/domid/Makefile | 48 +++++++++
tools/tests/domid/include/xen/domain.h | 126 ++++++++++++++++++++++++
tools/tests/domid/test-domid.c | 78 +++++++++++++++
xen/arch/arm/domain_build.c | 13 ++-
xen/arch/x86/setup.c | 11 ++-
xen/common/Makefile | 1 +
xen/common/device-tree/dom0less-build.c | 15 +--
xen/common/domain.c | 2 +
xen/common/domctl.c | 42 +-------
xen/common/domid.c | 94 ++++++++++++++++++
xen/include/xen/domain.h | 3 +
13 files changed, 384 insertions(+), 53 deletions(-)
create mode 100644 tools/tests/domid/.gitignore
create mode 100644 tools/tests/domid/Makefile
create mode 100644 tools/tests/domid/include/xen/domain.h
create mode 100644 tools/tests/domid/test-domid.c
create mode 100644 xen/common/domid.c
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v13 1/3] xen/domain: unify domain ID allocation
2025-07-30 17:40 [PATCH v13 0/3] xen/domain: domain ID allocation dmkhn
@ 2025-07-30 17:40 ` dmkhn
2025-08-05 13:38 ` Roger Pau Monné
2025-07-30 17:41 ` [PATCH v13 2/3] tools/tests: introduce unit tests for domain ID allocator dmkhn
2025-07-30 17:41 ` [PATCH v13 3/3] xen/domain: update create_dom0() messages dmkhn
2 siblings, 1 reply; 8+ messages in thread
From: dmkhn @ 2025-07-30 17:40 UTC (permalink / raw)
To: xen-devel
Cc: alejandro.garciavallejo, andrew.cooper3, anthony.perard, jbeulich,
julien, michal.orzel, roger.pau, sstabellini, dmukhin
From: Denis Mukhin <dmukhin@ford.com>
Currently, there are two different domain ID allocation implementations:
1) Sequential IDs allocation in dom0less Arm code based on max_init_domid;
2) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
max_init_domid (both Arm and x86).
The domain ID allocation covers dom0 or late hwdom, predefined domains,
post-boot domains, excluding Xen system domains (domid >=
DOMID_FIRST_RESERVED).
It makes sense to have a common helper code for such task across architectures
(Arm and x86) and between dom0less / toolstack domU allocation.
Note, fixing dependency on max_init_domid is out of scope of this patch.
Wrap the domain ID allocation as an arch-independent function domid_alloc() in
new common/domid.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 [1..DOMID_FIRST_RESERVED-1],
starting from the last used ID.
Implementation guarantees that two consecutive calls will never return the
same ID. ID#0 is reserved for the first boot domain (currently, dom0) and
excluded from the allocation range.
Remove is_free_domid() helper as it is not needed now.
No functional change intended.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
Changes since v12:
- updated comment for domid_alloc() and commit message
- added Alejandro's R-b
---
xen/arch/arm/domain_build.c | 7 +-
xen/arch/x86/setup.c | 7 +-
xen/common/Makefile | 1 +
xen/common/device-tree/dom0less-build.c | 15 ++--
xen/common/domain.c | 2 +
xen/common/domctl.c | 42 ++---------
xen/common/domid.c | 94 +++++++++++++++++++++++++
xen/include/xen/domain.h | 3 +
8 files changed, 124 insertions(+), 47 deletions(-)
create mode 100644 xen/common/domid.c
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 463ae4474d30..789f2b9d3ce7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2050,6 +2050,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 */
@@ -2074,7 +2075,11 @@ 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));
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1543dd251cc6..2ff7c28c277b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1047,8 +1047,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) )
diff --git a/xen/common/Makefile b/xen/common/Makefile
index c316957fcb36..0c7d0f5d46e1 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
obj-$(CONFIG_IOREQ_SERVER) += dm.o
obj-y += domain.o
+obj-y += domid.o
obj-y += event_2l.o
obj-y += event_channel.o
obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 6bb038111de9..f4b6b515d2d2 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -833,6 +833,7 @@ void __init create_domUs(void)
{
struct kernel_info ki = KERNEL_INFO_INIT;
int rc = parse_dom0less_node(node, &ki.bd);
+ domid_t domid;
if ( rc == -ENOENT )
continue;
@@ -842,13 +843,13 @@ void __init create_domUs(void)
if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
panic("No more domain IDs available\n");
- /*
- * The variable max_init_domid is initialized with zero, so here it's
- * very important to use the pre-increment operator to call
- * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
- */
- ki.bd.d = domain_create(++max_init_domid,
- &ki.bd.create_cfg, ki.bd.create_flags);
+ domid = domid_alloc(DOMID_INVALID);
+ if ( domid == DOMID_INVALID )
+ panic("Error allocating ID for domain %s\n", dt_node_name(node));
+
+ max_init_domid = max(max_init_domid, domid);
+
+ ki.bd.d = domain_create(domid, &ki.bd.create_cfg, ki.bd.create_flags);
if ( IS_ERR(ki.bd.d) )
panic("Error creating domain %s (rc = %ld)\n",
dt_node_name(node), PTR_ERR(ki.bd.d));
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 5241a1629eeb..12fbab01cd8e 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1473,6 +1473,8 @@ void domain_destroy(struct domain *d)
/* Remove from the domlist/hash. */
domlist_remove(d);
+ domid_free(d->domain_id);
+
/* Schedule RCU asynchronous completion of domain destroy. */
call_rcu(&d->rcu, complete_domain_destroy);
}
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f2a7caaf853c..5509998aa139 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -51,20 +51,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;
@@ -423,36 +409,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/common/domid.c b/xen/common/domid.c
new file mode 100644
index 000000000000..e727dcaf0793
--- /dev/null
+++ b/xen/common/domid.c
@@ -0,0 +1,94 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Domain ID allocator.
+ *
+ * Covers dom0 or late hwdom, predefined domains, post-boot domains.
+ * Excludes Xen system domains (ID >= DOMID_FIRST_RESERVED).
+ *
+ * Copyright 2025 Ford Motor Company
+ */
+
+#include <xen/domain.h>
+
+static DEFINE_SPINLOCK(domid_lock);
+static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
+
+/*
+ * Allocate domain ID.
+ *
+ * @param domid Domain ID hint:
+ * - If an explicit domain ID is provided, verify its availability and use it
+ * if ID is not used;
+ * - If DOMID_INVALID is provided, search [1..DOMID_FIRST_RESERVED-1] range,
+ * starting from the last used ID. Implementation guarantees that two
+ * consecutive calls will never return the same ID. ID#0 is reserved for
+ * the first boot domain (currently, dom0) and excluded from the allocation
+ * range.
+ * @return Valid domain ID in case of successful allocation,
+ * DOMID_INVALID - otherwise.
+ */
+domid_t domid_alloc(domid_t domid)
+{
+ static domid_t domid_last;
+
+ spin_lock(&domid_lock);
+
+ /* Exact match. */
+ if ( domid < DOMID_FIRST_RESERVED )
+ {
+ if ( __test_and_set_bit(domid, domid_bitmap) )
+ domid = DOMID_INVALID;
+ }
+ /*
+ * Exhaustive search.
+ *
+ * Domain ID#0 is reserved for the first boot domain (e.g. control domain)
+ * and excluded from allocation.
+ */
+ else
+ {
+ domid = find_next_zero_bit(domid_bitmap,
+ DOMID_FIRST_RESERVED,
+ domid_last + 1);
+ if ( domid == DOMID_FIRST_RESERVED )
+ domid = find_next_zero_bit(domid_bitmap,
+ DOMID_FIRST_RESERVED,
+ 1);
+
+ ASSERT(domid <= DOMID_FIRST_RESERVED);
+ if ( domid < DOMID_FIRST_RESERVED )
+ {
+ __set_bit(domid, domid_bitmap);
+ domid_last = domid;
+ }
+ else
+ domid = DOMID_INVALID;
+ }
+
+ spin_unlock(&domid_lock);
+
+ return domid;
+}
+
+void domid_free(domid_t domid)
+{
+ int rc;
+
+ ASSERT(domid <= DOMID_FIRST_RESERVED);
+
+ spin_lock(&domid_lock);
+ rc = __test_and_clear_bit(domid, domid_bitmap);
+ spin_unlock(&domid_lock);
+
+ ASSERT(rc);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index e10baf2615fd..8aab05ae93c8 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.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v13 2/3] tools/tests: introduce unit tests for domain ID allocator
2025-07-30 17:40 [PATCH v13 0/3] xen/domain: domain ID allocation dmkhn
2025-07-30 17:40 ` [PATCH v13 1/3] xen/domain: unify " dmkhn
@ 2025-07-30 17:41 ` dmkhn
2025-08-05 14:15 ` Roger Pau Monné
2025-07-30 17:41 ` [PATCH v13 3/3] xen/domain: update create_dom0() messages dmkhn
2 siblings, 1 reply; 8+ messages in thread
From: dmkhn @ 2025-07-30 17:41 UTC (permalink / raw)
To: xen-devel
Cc: alejandro.garciavallejo, andrew.cooper3, anthony.perard, jbeulich,
julien, michal.orzel, roger.pau, sstabellini, dmukhin
From: Denis Mukhin <dmukhin@ford.com>
Introduce some basic infrastructure for doing domain ID allocation unit tests,
and add a few tests that ensure correctness of the domain ID allocator.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v12:
- fixed Makefile
- dropped unused symbols/includes from the test harness header
- s/printk/printf/g in the test code
---
tools/tests/Makefile | 2 +-
tools/tests/domid/.gitignore | 2 +
tools/tests/domid/Makefile | 48 ++++++++++
tools/tests/domid/include/xen/domain.h | 126 +++++++++++++++++++++++++
tools/tests/domid/test-domid.c | 78 +++++++++++++++
5 files changed, 255 insertions(+), 1 deletion(-)
create mode 100644 tools/tests/domid/.gitignore
create mode 100644 tools/tests/domid/Makefile
create mode 100644 tools/tests/domid/include/xen/domain.h
create mode 100644 tools/tests/domid/test-domid.c
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index 36928676a666..ff1666425436 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -1,7 +1,7 @@
XEN_ROOT = $(CURDIR)/../..
include $(XEN_ROOT)/tools/Rules.mk
-SUBDIRS-y :=
+SUBDIRS-y := domid
SUBDIRS-y += resource
SUBDIRS-$(CONFIG_X86) += cpu-policy
SUBDIRS-$(CONFIG_X86) += tsx
diff --git a/tools/tests/domid/.gitignore b/tools/tests/domid/.gitignore
new file mode 100644
index 000000000000..70e306b3c074
--- /dev/null
+++ b/tools/tests/domid/.gitignore
@@ -0,0 +1,2 @@
+*.o
+test-domid
diff --git a/tools/tests/domid/Makefile b/tools/tests/domid/Makefile
new file mode 100644
index 000000000000..08fbad096aec
--- /dev/null
+++ b/tools/tests/domid/Makefile
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Unit tests for domain ID allocator.
+#
+# Copyright 2025 Ford Motor Company
+
+XEN_ROOT=$(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+TESTS := test-domid
+
+vpath domid.c $(XEN_ROOT)/xen/common/
+
+.PHONY: all
+all: $(TESTS)
+
+.PHONY: run
+run: $(TESTS)
+ $(foreach t,$(TESTS),./$(t);)
+
+.PHONY: clean
+clean:
+ $(RM) -- *.o $(TESTS) $(DEPS_RM)
+
+.PHONY: distclean
+distclean: clean
+ $(RM) -- *~
+
+.PHONY: install
+install: all
+ $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+ $(INSTALL_PROG) test-domid $(DESTDIR)$(LIBEXEC)/tests
+
+.PHONY: uninstall
+uninstall:
+ $(RM) -- $(DESTDIR)$(LIBEXEC)/tests/test-domid
+
+CFLAGS += -D__XEN_TOOLS__
+CFLAGS += $(APPEND_CFLAGS)
+CFLAGS += $(CFLAGS_xeninclude)
+CFLAGS += -I./include/
+
+LDFLAGS += $(APPEND_LDFLAGS)
+
+test-domid: domid.o test-domid.o
+ $(CC) $^ -o $@ $(LDFLAGS)
+
+-include $(DEPS_INCLUDE)
diff --git a/tools/tests/domid/include/xen/domain.h b/tools/tests/domid/include/xen/domain.h
new file mode 100644
index 000000000000..e5db0235445e
--- /dev/null
+++ b/tools/tests/domid/include/xen/domain.h
@@ -0,0 +1,126 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Unit test harness for domain ID allocator.
+ *
+ * Copyright 2025 Ford Motor Company
+ */
+
+#ifndef _TEST_HARNESS_
+#define _TEST_HARNESS_
+
+#include <assert.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+
+#include <xen-tools/common-macros.h>
+
+#define BUG_ON(x) assert(!(x))
+#define ASSERT(x) assert(x)
+
+#define DOMID_FIRST_RESERVED (10)
+#define DOMID_INVALID (11)
+
+#define DEFINE_SPINLOCK(x) unsigned long *(x)
+#define spin_lock(x) ((*(x))++)
+#define spin_unlock(x) ((*(x))--)
+
+#define printk printf
+
+#define BITS_PER_LONG sizeof(unsigned long)
+#define BITS_PER_WORD (8U * BITS_PER_LONG)
+#define BITS_TO_LONGS(bits) \
+ (((bits) + BITS_PER_LONG - 1) / BITS_PER_LONG)
+#define DECLARE_BITMAP(name, bits) \
+ unsigned long name[BITS_TO_LONGS(bits)]
+
+static inline int __test_and_set_bit(unsigned int nr, unsigned long *addr)
+{
+ unsigned long mask = 1UL << (nr % BITS_PER_WORD);
+ unsigned long *p = addr + (nr / BITS_PER_WORD);
+ int old = (*p & mask) != 0;
+
+ *p |= mask;
+
+ return old;
+}
+
+static inline int __test_and_clear_bit(unsigned int nr, unsigned long *addr)
+{
+ unsigned long mask = 1UL << (nr % BITS_PER_WORD);
+ unsigned long *p = addr + (nr / BITS_PER_WORD);
+ int old = (*p & mask) != 0;
+
+ *p &= ~mask;
+
+ return old;
+}
+
+static inline void __set_bit(unsigned int nr, volatile unsigned long *addr)
+{
+ unsigned long mask = 1UL << (nr % BITS_PER_WORD);
+ unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
+
+ *p |= mask;
+}
+
+static inline void __clear_bit(unsigned int nr, volatile unsigned long *addr)
+{
+ unsigned long mask = 1UL << (nr % BITS_PER_WORD);
+ unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
+
+ *p &= ~mask;
+}
+
+static inline unsigned long find_next_zero_bit(const unsigned long *addr,
+ unsigned long size,
+ unsigned long offset)
+{
+ unsigned long idx = offset / BITS_PER_WORD;
+ unsigned long bit = offset % BITS_PER_WORD;
+
+ if (offset >= size)
+ return size;
+
+ while (offset < size)
+ {
+ unsigned long val = addr[idx] | (~0UL >> (BITS_PER_WORD - bit));
+
+ if (~val)
+ {
+ unsigned long pos = __builtin_ffsl(~val);
+
+ if (pos > 0)
+ {
+ unsigned long rc = idx * BITS_PER_WORD + (pos - 1);
+
+ if (rc < size)
+ return rc;
+ }
+ }
+
+ offset = (idx + 1) * BITS_PER_WORD;
+ idx++;
+ bit = 0;
+ }
+
+ return size;
+}
+
+typedef bool spinlock_t;
+typedef uint16_t domid_t;
+
+/* See include/xen/domain.h */
+extern domid_t domid_alloc(domid_t domid);
+extern void domid_free(domid_t domid);
+
+#endif /* _TEST_HARNESS_ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/tests/domid/test-domid.c b/tools/tests/domid/test-domid.c
new file mode 100644
index 000000000000..d52eaf5f1f55
--- /dev/null
+++ b/tools/tests/domid/test-domid.c
@@ -0,0 +1,78 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Unit tests for domain ID allocator.
+ *
+ * Copyright 2025 Ford Motor Company
+ */
+
+/* Local test include replicating hypervisor includes. */
+#include <xen/domain.h>
+
+int main(int argc, char **argv)
+{
+ domid_t expected, allocated;
+
+ printf("DOMID_FIRST_RESERVED=%u DOMID_INVALID=%u\n",
+ DOMID_FIRST_RESERVED, DOMID_INVALID);
+
+ /* Test ID#0 cannot be allocated twice. */
+ allocated = domid_alloc(0);
+ printf("TEST 1: expected %u allocated %u\n", 0, allocated);
+ ASSERT(allocated == 0);
+ allocated = domid_alloc(0);
+ printf("TEST 1: expected %u allocated %u\n", DOMID_INVALID, allocated);
+ ASSERT(allocated == DOMID_INVALID);
+
+ /* Ensure ID is not allocated. */
+ domid_free(0);
+
+ /*
+ * Test that that two consecutive calls of domid_alloc(DOMID_INVALID)
+ * will never return the same ID.
+ * NB: ID#0 is reserved and shall not be allocated by
+ * domid_alloc(DOMID_INVALID).
+ */
+ for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
+ {
+ allocated = domid_alloc(DOMID_INVALID);
+ printf("TEST 2: expected %u allocated %u\n", expected, allocated);
+ ASSERT(allocated == expected);
+ }
+ for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
+ {
+ allocated = domid_alloc(DOMID_INVALID);
+ printf("TEST 3: expected %u allocated %u\n", DOMID_INVALID, allocated);
+ ASSERT(allocated == DOMID_INVALID);
+ }
+
+ /* Re-allocate first ID from [1..DOMID_FIRST_RESERVED - 1]. */
+ expected = 1;
+ domid_free(1);
+ allocated = domid_alloc(DOMID_INVALID);
+ printf("TEST 4: expected %u allocated %u\n", expected, allocated);
+ ASSERT(allocated == expected);
+
+ /* Re-allocate last ID from [1..DOMID_FIRST_RESERVED - 1]. */
+ expected = DOMID_FIRST_RESERVED - 1;
+ domid_free(DOMID_FIRST_RESERVED - 1);
+ allocated = domid_alloc(DOMID_INVALID);
+ printf("TEST 5: expected %u allocated %u\n", expected, allocated);
+ ASSERT(allocated == expected);
+
+ /* Allocate an invalid ID. */
+ expected = DOMID_INVALID;
+ allocated = domid_alloc(DOMID_FIRST_RESERVED);
+ printf("TEST 6: expected %u allocated %u\n", expected, allocated);
+ ASSERT(allocated == expected);
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v13 3/3] xen/domain: update create_dom0() messages
2025-07-30 17:40 [PATCH v13 0/3] xen/domain: domain ID allocation dmkhn
2025-07-30 17:40 ` [PATCH v13 1/3] xen/domain: unify " dmkhn
2025-07-30 17:41 ` [PATCH v13 2/3] tools/tests: introduce unit tests for domain ID allocator dmkhn
@ 2025-07-30 17:41 ` dmkhn
2 siblings, 0 replies; 8+ messages in thread
From: dmkhn @ 2025-07-30 17:41 UTC (permalink / raw)
To: xen-devel
Cc: alejandro.garciavallejo, andrew.cooper3, anthony.perard, jbeulich,
julien, michal.orzel, roger.pau, sstabellini, dmukhin
From: Denis Mukhin <dmukhin@ford.com>
Use %pd for domain identification in error/panic messages in create_dom0().
No functional change.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v12:
- n/a
---
xen/arch/arm/domain_build.c | 6 +++---
xen/arch/x86/setup.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 789f2b9d3ce7..02a15d160962 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2084,14 +2084,14 @@ void __init create_dom0(void)
panic("Error creating domain 0 (rc = %ld)\n", 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 %pd (rc = %d)\n", dom0, rc);
if ( vcpu_create(dom0, 0) == NULL )
- panic("Error creating domain 0 vcpu0\n");
+ panic("Error creating %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 %pd guest OS (rc = %d)\n", dom0, rc);
set_xs_domain(dom0);
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2ff7c28c277b..a740c6f60c63 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1084,7 +1084,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 %pd (acpi=off)\n", d);
safe_strcpy(acpi_param, "off");
}
@@ -1099,7 +1099,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 %pd\n", d);
bd->cmdline = NULL;
xfree(cmdline);
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v13 1/3] xen/domain: unify domain ID allocation
2025-07-30 17:40 ` [PATCH v13 1/3] xen/domain: unify " dmkhn
@ 2025-08-05 13:38 ` Roger Pau Monné
2025-08-07 1:16 ` dmkhn
0 siblings, 1 reply; 8+ messages in thread
From: Roger Pau Monné @ 2025-08-05 13:38 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, alejandro.garciavallejo, andrew.cooper3,
anthony.perard, jbeulich, julien, michal.orzel, sstabellini,
dmukhin
On Wed, Jul 30, 2025 at 05:40:54PM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
>
> Currently, there are two different domain ID allocation implementations:
>
> 1) Sequential IDs allocation in dom0less Arm code based on max_init_domid;
>
> 2) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
> max_init_domid (both Arm and x86).
>
> The domain ID allocation covers dom0 or late hwdom, predefined domains,
> post-boot domains, excluding Xen system domains (domid >=
> DOMID_FIRST_RESERVED).
>
> It makes sense to have a common helper code for such task across architectures
> (Arm and x86) and between dom0less / toolstack domU allocation.
>
> Note, fixing dependency on max_init_domid is out of scope of this patch.
>
> Wrap the domain ID allocation as an arch-independent function domid_alloc() in
> new common/domid.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 [1..DOMID_FIRST_RESERVED-1],
> starting from the last used ID.
> Implementation guarantees that two consecutive calls will never return the
> same ID. ID#0 is reserved for the first boot domain (currently, dom0) and
> excluded from the allocation range.
>
> Remove is_free_domid() helper as it is not needed now.
>
> No functional change intended.
>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> ---
> Changes since v12:
> - updated comment for domid_alloc() and commit message
> - added Alejandro's R-b
> ---
> xen/arch/arm/domain_build.c | 7 +-
> xen/arch/x86/setup.c | 7 +-
> xen/common/Makefile | 1 +
> xen/common/device-tree/dom0less-build.c | 15 ++--
> xen/common/domain.c | 2 +
> xen/common/domctl.c | 42 ++---------
> xen/common/domid.c | 94 +++++++++++++++++++++++++
> xen/include/xen/domain.h | 3 +
> 8 files changed, 124 insertions(+), 47 deletions(-)
> create mode 100644 xen/common/domid.c
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 463ae4474d30..789f2b9d3ce7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2050,6 +2050,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 */
> @@ -2074,7 +2075,11 @@ 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));
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 1543dd251cc6..2ff7c28c277b 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1047,8 +1047,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());
Nit: in other error messages in the same function we handle the domid
as an unsigned integer, so %u probably wants using here. Unless you
have an explicit intention to print IDs >= DOMID_FIRST_RESERVED as
negative integers?
> +
> d = domain_create(bd->domid, &dom0_cfg,
> pv_shim ? 0 : CDF_privileged | CDF_hardware);
> if ( IS_ERR(d) )
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index c316957fcb36..0c7d0f5d46e1 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
> obj-$(CONFIG_IOREQ_SERVER) += dm.o
> obj-y += domain.o
> +obj-y += domid.o
> obj-y += event_2l.o
> obj-y += event_channel.o
> obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 6bb038111de9..f4b6b515d2d2 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -833,6 +833,7 @@ void __init create_domUs(void)
> {
> struct kernel_info ki = KERNEL_INFO_INIT;
> int rc = parse_dom0less_node(node, &ki.bd);
> + domid_t domid;
>
> if ( rc == -ENOENT )
> continue;
> @@ -842,13 +843,13 @@ void __init create_domUs(void)
> if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
> panic("No more domain IDs available\n");
>
> - /*
> - * The variable max_init_domid is initialized with zero, so here it's
> - * very important to use the pre-increment operator to call
> - * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
> - */
> - ki.bd.d = domain_create(++max_init_domid,
> - &ki.bd.create_cfg, ki.bd.create_flags);
> + domid = domid_alloc(DOMID_INVALID);
> + if ( domid == DOMID_INVALID )
> + panic("Error allocating ID for domain %s\n", dt_node_name(node));
> +
> + max_init_domid = max(max_init_domid, domid);
> +
> + ki.bd.d = domain_create(domid, &ki.bd.create_cfg, ki.bd.create_flags);
> if ( IS_ERR(ki.bd.d) )
> panic("Error creating domain %s (rc = %ld)\n",
> dt_node_name(node), PTR_ERR(ki.bd.d));
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 5241a1629eeb..12fbab01cd8e 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1473,6 +1473,8 @@ void domain_destroy(struct domain *d)
> /* Remove from the domlist/hash. */
> domlist_remove(d);
>
> + domid_free(d->domain_id);
The domlist removal above still allows current users to continue
"operating" on the domain until the next RCU. Should for safety the
freeing of the domid be deferred to _domain_destroy(), which is
executed in RCU context, and thus ensures there are no current users
of the removed domain?
I cannot think of a specific scenario where this could be dangerous
right now, but deferring to RCU context together with the final
cleanup seems safer overall.
> +
> /* Schedule RCU asynchronous completion of domain destroy. */
> call_rcu(&d->rcu, complete_domain_destroy);
> }
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index f2a7caaf853c..5509998aa139 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -51,20 +51,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;
> @@ -423,36 +409,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 )
This is a change in behavior AFAICT, as you now allow
XEN_DOMCTL_createdomain to possibly create a domain with domid 0 (if
it's available). Currently op->domain == 0 is handled as op->domain
== DOMID_FIRST_RESERVED. You either need to adjust the code here, so
that you do:
domid_t domid = domid_alloc(op->domain ?: DOMID_FIRST_RESERVED);
Or domid_alloc() needs to be adjusted to handle an input domid == 0 as
it handles DOMID_FIRST_RESERVED.
> {
> 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/common/domid.c b/xen/common/domid.c
> new file mode 100644
> index 000000000000..e727dcaf0793
> --- /dev/null
> +++ b/xen/common/domid.c
> @@ -0,0 +1,94 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Domain ID allocator.
> + *
> + * Covers dom0 or late hwdom, predefined domains, post-boot domains.
> + * Excludes Xen system domains (ID >= DOMID_FIRST_RESERVED).
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +#include <xen/domain.h>
> +
> +static DEFINE_SPINLOCK(domid_lock);
> +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> +
> +/*
> + * Allocate domain ID.
> + *
> + * @param domid Domain ID hint:
> + * - If an explicit domain ID is provided, verify its availability and use it
> + * if ID is not used;
> + * - If DOMID_INVALID is provided, search [1..DOMID_FIRST_RESERVED-1] range,
> + * starting from the last used ID. Implementation guarantees that two
> + * consecutive calls will never return the same ID. ID#0 is reserved for
> + * the first boot domain (currently, dom0) and excluded from the allocation
> + * range.
> + * @return Valid domain ID in case of successful allocation,
> + * DOMID_INVALID - otherwise.
> + */
> +domid_t domid_alloc(domid_t domid)
> +{
> + static domid_t domid_last;
> +
> + spin_lock(&domid_lock);
> +
> + /* Exact match. */
> + if ( domid < DOMID_FIRST_RESERVED )
> + {
> + if ( __test_and_set_bit(domid, domid_bitmap) )
> + domid = DOMID_INVALID;
> + }
> + /*
> + * Exhaustive search.
> + *
> + * Domain ID#0 is reserved for the first boot domain (e.g. control domain)
> + * and excluded from allocation.
> + */
> + else
> + {
> + domid = find_next_zero_bit(domid_bitmap,
> + DOMID_FIRST_RESERVED,
> + domid_last + 1);
> + if ( domid == DOMID_FIRST_RESERVED )
Nit: you could further gate this second search to domid_last != 0, as
otherwise the first search has already scanned the whole bitmap.
> + domid = find_next_zero_bit(domid_bitmap,
> + DOMID_FIRST_RESERVED,
> + 1);
Nit: you could possibly limit this second search to (domid_last + 1)
size, as you have already searched from [domid_last + 1,
DOMID_FIRST_RESERVED], and the bitmap couldn't have changed as the
lock is being held.
Thanks, Roger.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v13 2/3] tools/tests: introduce unit tests for domain ID allocator
2025-07-30 17:41 ` [PATCH v13 2/3] tools/tests: introduce unit tests for domain ID allocator dmkhn
@ 2025-08-05 14:15 ` Roger Pau Monné
2025-08-07 2:12 ` dmkhn
0 siblings, 1 reply; 8+ messages in thread
From: Roger Pau Monné @ 2025-08-05 14:15 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, alejandro.garciavallejo, andrew.cooper3,
anthony.perard, jbeulich, julien, michal.orzel, sstabellini,
dmukhin
On Wed, Jul 30, 2025 at 05:41:00PM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
>
> Introduce some basic infrastructure for doing domain ID allocation unit tests,
> and add a few tests that ensure correctness of the domain ID allocator.
>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v12:
> - fixed Makefile
> - dropped unused symbols/includes from the test harness header
> - s/printk/printf/g in the test code
> ---
> tools/tests/Makefile | 2 +-
> tools/tests/domid/.gitignore | 2 +
> tools/tests/domid/Makefile | 48 ++++++++++
> tools/tests/domid/include/xen/domain.h | 126 +++++++++++++++++++++++++
> tools/tests/domid/test-domid.c | 78 +++++++++++++++
> 5 files changed, 255 insertions(+), 1 deletion(-)
> create mode 100644 tools/tests/domid/.gitignore
> create mode 100644 tools/tests/domid/Makefile
> create mode 100644 tools/tests/domid/include/xen/domain.h
> create mode 100644 tools/tests/domid/test-domid.c
>
> diff --git a/tools/tests/Makefile b/tools/tests/Makefile
> index 36928676a666..ff1666425436 100644
> --- a/tools/tests/Makefile
> +++ b/tools/tests/Makefile
> @@ -1,7 +1,7 @@
> XEN_ROOT = $(CURDIR)/../..
> include $(XEN_ROOT)/tools/Rules.mk
>
> -SUBDIRS-y :=
> +SUBDIRS-y := domid
> SUBDIRS-y += resource
> SUBDIRS-$(CONFIG_X86) += cpu-policy
> SUBDIRS-$(CONFIG_X86) += tsx
> diff --git a/tools/tests/domid/.gitignore b/tools/tests/domid/.gitignore
> new file mode 100644
> index 000000000000..70e306b3c074
> --- /dev/null
> +++ b/tools/tests/domid/.gitignore
> @@ -0,0 +1,2 @@
> +*.o
> +test-domid
> diff --git a/tools/tests/domid/Makefile b/tools/tests/domid/Makefile
> new file mode 100644
> index 000000000000..08fbad096aec
> --- /dev/null
> +++ b/tools/tests/domid/Makefile
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Unit tests for domain ID allocator.
> +#
> +# Copyright 2025 Ford Motor Company
> +
> +XEN_ROOT=$(CURDIR)/../../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +TESTS := test-domid
> +
> +vpath domid.c $(XEN_ROOT)/xen/common/
> +
> +.PHONY: all
> +all: $(TESTS)
> +
> +.PHONY: run
> +run: $(TESTS)
> + $(foreach t,$(TESTS),./$(t);)
> +
> +.PHONY: clean
> +clean:
> + $(RM) -- *.o $(TESTS) $(DEPS_RM)
> +
> +.PHONY: distclean
> +distclean: clean
> + $(RM) -- *~
> +
> +.PHONY: install
> +install: all
> + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
> + $(INSTALL_PROG) test-domid $(DESTDIR)$(LIBEXEC)/tests
> +
> +.PHONY: uninstall
> +uninstall:
> + $(RM) -- $(DESTDIR)$(LIBEXEC)/tests/test-domid
> +
> +CFLAGS += -D__XEN_TOOLS__
> +CFLAGS += $(APPEND_CFLAGS)
> +CFLAGS += $(CFLAGS_xeninclude)
> +CFLAGS += -I./include/
> +
> +LDFLAGS += $(APPEND_LDFLAGS)
> +
> +test-domid: domid.o test-domid.o
> + $(CC) $^ -o $@ $(LDFLAGS)
> +
> +-include $(DEPS_INCLUDE)
> diff --git a/tools/tests/domid/include/xen/domain.h b/tools/tests/domid/include/xen/domain.h
> new file mode 100644
> index 000000000000..e5db0235445e
> --- /dev/null
> +++ b/tools/tests/domid/include/xen/domain.h
> @@ -0,0 +1,126 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit test harness for domain ID allocator.
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +#ifndef _TEST_HARNESS_
> +#define _TEST_HARNESS_
> +
> +#include <assert.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +
> +#include <xen-tools/common-macros.h>
> +
> +#define BUG_ON(x) assert(!(x))
> +#define ASSERT(x) assert(x)
> +
> +#define DOMID_FIRST_RESERVED (10)
> +#define DOMID_INVALID (11)
> +
> +#define DEFINE_SPINLOCK(x) unsigned long *(x)
I think this shouldn't be a pointer? As you otherwise trigger a NULL
pointer dereference in the increases and decreases done below?
> +#define spin_lock(x) ((*(x))++)
> +#define spin_unlock(x) ((*(x))--)
FWIW, I would use a plain bool:
#define DEFINE_SPINLOCK(l) bool l
#define spin_lock(l) (*(l) = true)
#define spin_unlock(l) (*(l) = false)
As you don't expect concurrency tests, you could even assert the lock
is in the expected state before taking/releasing it.
> +
> +#define printk printf
> +
> +#define BITS_PER_LONG sizeof(unsigned long)
That's BYTES_PER_LONG, BITS_PER_LONG would be (sizeof(unsigned long) * 8).
> +#define BITS_PER_WORD (8U * BITS_PER_LONG)
> +#define BITS_TO_LONGS(bits) \
> + (((bits) + BITS_PER_LONG - 1) / BITS_PER_LONG)
> +#define DECLARE_BITMAP(name, bits) \
> + unsigned long name[BITS_TO_LONGS(bits)]
> +
> +static inline int __test_and_set_bit(unsigned int nr, unsigned long *addr)
> +{
> + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> + unsigned long *p = addr + (nr / BITS_PER_WORD);
> + int old = (*p & mask) != 0;
> +
> + *p |= mask;
> +
> + return old;
> +}
> +
> +static inline int __test_and_clear_bit(unsigned int nr, unsigned long *addr)
> +{
> + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> + unsigned long *p = addr + (nr / BITS_PER_WORD);
> + int old = (*p & mask) != 0;
> +
> + *p &= ~mask;
> +
> + return old;
> +}
Could you somehow use the generic__test_and_set_bit() and
generic__test_and_clear_bit() implementations in bitops.h?
> +
> +static inline void __set_bit(unsigned int nr, volatile unsigned long *addr)
> +{
> + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> + unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
Why do you need the cast to drop the volatile here?
> +
> + *p |= mask;
> +}
I think you could possibly simplify to a single line:
((unsigned int *)addr)[nr >> 5] |= (1u << (nr & 31));
That's the implementation of constant_set_bit() in x86.
> +
> +static inline void __clear_bit(unsigned int nr, volatile unsigned long *addr)
> +{
> + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> + unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
> +
> + *p &= ~mask;
> +}
I don't think you need __clear_bit()? It's not used by domid.c AFAICT.
> +
> +static inline unsigned long find_next_zero_bit(const unsigned long *addr,
> + unsigned long size,
> + unsigned long offset)
> +{
> + unsigned long idx = offset / BITS_PER_WORD;
> + unsigned long bit = offset % BITS_PER_WORD;
> +
> + if (offset >= size)
> + return size;
> +
> + while (offset < size)
> + {
> + unsigned long val = addr[idx] | (~0UL >> (BITS_PER_WORD - bit));
> +
> + if (~val)
> + {
> + unsigned long pos = __builtin_ffsl(~val);
> +
> + if (pos > 0)
> + {
> + unsigned long rc = idx * BITS_PER_WORD + (pos - 1);
> +
> + if (rc < size)
> + return rc;
> + }
> + }
> +
> + offset = (idx + 1) * BITS_PER_WORD;
> + idx++;
> + bit = 0;
> + }
> +
> + return size;
> +}
Hm, you need a full find_next_zero_bit() implementation here because
addr can be arbitrarily long. Could you somehow include
xen/lib/find-next-bit.c and set the right defines so only the
implementation of find_next_bit() is included?
> +
> +typedef bool spinlock_t;
You want to put this ahead, so that DEFINE_SPINLOCK can be:
#define DEFINE_SPINLOCK(l) spinlock_t l
> +typedef uint16_t domid_t;
> +
> +/* See include/xen/domain.h */
> +extern domid_t domid_alloc(domid_t domid);
> +extern void domid_free(domid_t domid);
> +
> +#endif /* _TEST_HARNESS_ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/tests/domid/test-domid.c b/tools/tests/domid/test-domid.c
> new file mode 100644
> index 000000000000..d52eaf5f1f55
> --- /dev/null
> +++ b/tools/tests/domid/test-domid.c
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for domain ID allocator.
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +/* Local test include replicating hypervisor includes. */
> +#include <xen/domain.h>
I think this is a difficult to maintain position. Right now domid.c
only includes xen/domain.h, so you can easily replace this in
user-space. However if/when domid.c starts including more headers,
replicating this in user-space will be cumbersome IMO.
I would just guard the includes in domid.c with #ifdef __XEN__ for the
preprocessor to remove them when domid.c is compiled as part of the
unit-tests harness.
I usually include a harness.h that contains the glue to make the
imported code build (much like what you have placed in the test
harness xen/domain.h header.
> +
> +int main(int argc, char **argv)
> +{
> + domid_t expected, allocated;
> +
> + printf("DOMID_FIRST_RESERVED=%u DOMID_INVALID=%u\n",
> + DOMID_FIRST_RESERVED, DOMID_INVALID);
> +
> + /* Test ID#0 cannot be allocated twice. */
> + allocated = domid_alloc(0);
> + printf("TEST 1: expected %u allocated %u\n", 0, allocated);
> + ASSERT(allocated == 0);
> + allocated = domid_alloc(0);
> + printf("TEST 1: expected %u allocated %u\n", DOMID_INVALID, allocated);
> + ASSERT(allocated == DOMID_INVALID);
> +
> + /* Ensure ID is not allocated. */
> + domid_free(0);
> +
> + /*
> + * Test that that two consecutive calls of domid_alloc(DOMID_INVALID)
> + * will never return the same ID.
> + * NB: ID#0 is reserved and shall not be allocated by
> + * domid_alloc(DOMID_INVALID).
> + */
> + for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
> + {
> + allocated = domid_alloc(DOMID_INVALID);
> + printf("TEST 2: expected %u allocated %u\n", expected, allocated);
> + ASSERT(allocated == expected);
> + }
> + for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
> + {
> + allocated = domid_alloc(DOMID_INVALID);
> + printf("TEST 3: expected %u allocated %u\n", DOMID_INVALID, allocated);
> + ASSERT(allocated == DOMID_INVALID);
> + }
> +
> + /* Re-allocate first ID from [1..DOMID_FIRST_RESERVED - 1]. */
> + expected = 1;
> + domid_free(1);
> + allocated = domid_alloc(DOMID_INVALID);
> + printf("TEST 4: expected %u allocated %u\n", expected, allocated);
> + ASSERT(allocated == expected);
> +
> + /* Re-allocate last ID from [1..DOMID_FIRST_RESERVED - 1]. */
> + expected = DOMID_FIRST_RESERVED - 1;
> + domid_free(DOMID_FIRST_RESERVED - 1);
> + allocated = domid_alloc(DOMID_INVALID);
> + printf("TEST 5: expected %u allocated %u\n", expected, allocated);
> + ASSERT(allocated == expected);
> +
> + /* Allocate an invalid ID. */
> + expected = DOMID_INVALID;
> + allocated = domid_alloc(DOMID_FIRST_RESERVED);
> + printf("TEST 6: expected %u allocated %u\n", expected, allocated);
> + ASSERT(allocated == expected);
I would make this a bit less chatty maybe?
I think you only need to print on errors, and you probably don't want
to ASSERT() on failure, and rather try to finish all the tests in
order to report multiple failures in a single run.
Thanks, Roger.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v13 1/3] xen/domain: unify domain ID allocation
2025-08-05 13:38 ` Roger Pau Monné
@ 2025-08-07 1:16 ` dmkhn
0 siblings, 0 replies; 8+ messages in thread
From: dmkhn @ 2025-08-07 1:16 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, alejandro.garciavallejo, andrew.cooper3,
anthony.perard, jbeulich, julien, michal.orzel, sstabellini,
dmukhin
On Tue, Aug 05, 2025 at 03:38:37PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 30, 2025 at 05:40:54PM +0000, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Currently, there are two different domain ID allocation implementations:
> >
> > 1) Sequential IDs allocation in dom0less Arm code based on max_init_domid;
> >
> > 2) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
> > max_init_domid (both Arm and x86).
> >
> > The domain ID allocation covers dom0 or late hwdom, predefined domains,
> > post-boot domains, excluding Xen system domains (domid >=
> > DOMID_FIRST_RESERVED).
> >
> > It makes sense to have a common helper code for such task across architectures
> > (Arm and x86) and between dom0less / toolstack domU allocation.
> >
> > Note, fixing dependency on max_init_domid is out of scope of this patch.
> >
> > Wrap the domain ID allocation as an arch-independent function domid_alloc() in
> > new common/domid.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 [1..DOMID_FIRST_RESERVED-1],
> > starting from the last used ID.
> > Implementation guarantees that two consecutive calls will never return the
> > same ID. ID#0 is reserved for the first boot domain (currently, dom0) and
> > excluded from the allocation range.
> >
> > Remove is_free_domid() helper as it is not needed now.
> >
> > No functional change intended.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> > ---
> > Changes since v12:
> > - updated comment for domid_alloc() and commit message
> > - added Alejandro's R-b
> > ---
> > xen/arch/arm/domain_build.c | 7 +-
> > xen/arch/x86/setup.c | 7 +-
> > xen/common/Makefile | 1 +
> > xen/common/device-tree/dom0less-build.c | 15 ++--
> > xen/common/domain.c | 2 +
> > xen/common/domctl.c | 42 ++---------
> > xen/common/domid.c | 94 +++++++++++++++++++++++++
> > xen/include/xen/domain.h | 3 +
> > 8 files changed, 124 insertions(+), 47 deletions(-)
> > create mode 100644 xen/common/domid.c
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 463ae4474d30..789f2b9d3ce7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2050,6 +2050,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 */
> > @@ -2074,7 +2075,11 @@ 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));
> >
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 1543dd251cc6..2ff7c28c277b 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1047,8 +1047,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());
>
> Nit: in other error messages in the same function we handle the domid
> as an unsigned integer, so %u probably wants using here. Unless you
> have an explicit intention to print IDs >= DOMID_FIRST_RESERVED as
> negative integers?
No negative integers, that should be %u
Thanks!
>
> > +
> > d = domain_create(bd->domid, &dom0_cfg,
> > pv_shim ? 0 : CDF_privileged | CDF_hardware);
> > if ( IS_ERR(d) )
> > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > index c316957fcb36..0c7d0f5d46e1 100644
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> > obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
> > obj-$(CONFIG_IOREQ_SERVER) += dm.o
> > obj-y += domain.o
> > +obj-y += domid.o
> > obj-y += event_2l.o
> > obj-y += event_channel.o
> > obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
> > diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> > index 6bb038111de9..f4b6b515d2d2 100644
> > --- a/xen/common/device-tree/dom0less-build.c
> > +++ b/xen/common/device-tree/dom0less-build.c
> > @@ -833,6 +833,7 @@ void __init create_domUs(void)
> > {
> > struct kernel_info ki = KERNEL_INFO_INIT;
> > int rc = parse_dom0less_node(node, &ki.bd);
> > + domid_t domid;
> >
> > if ( rc == -ENOENT )
> > continue;
> > @@ -842,13 +843,13 @@ void __init create_domUs(void)
> > if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
> > panic("No more domain IDs available\n");
> >
> > - /*
> > - * The variable max_init_domid is initialized with zero, so here it's
> > - * very important to use the pre-increment operator to call
> > - * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
> > - */
> > - ki.bd.d = domain_create(++max_init_domid,
> > - &ki.bd.create_cfg, ki.bd.create_flags);
> > + domid = domid_alloc(DOMID_INVALID);
> > + if ( domid == DOMID_INVALID )
> > + panic("Error allocating ID for domain %s\n", dt_node_name(node));
> > +
> > + max_init_domid = max(max_init_domid, domid);
> > +
> > + ki.bd.d = domain_create(domid, &ki.bd.create_cfg, ki.bd.create_flags);
> > if ( IS_ERR(ki.bd.d) )
> > panic("Error creating domain %s (rc = %ld)\n",
> > dt_node_name(node), PTR_ERR(ki.bd.d));
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 5241a1629eeb..12fbab01cd8e 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1473,6 +1473,8 @@ void domain_destroy(struct domain *d)
> > /* Remove from the domlist/hash. */
> > domlist_remove(d);
> >
> > + domid_free(d->domain_id);
>
> The domlist removal above still allows current users to continue
> "operating" on the domain until the next RCU. Should for safety the
> freeing of the domid be deferred to _domain_destroy(), which is
> executed in RCU context, and thus ensures there are no current users
> of the removed domain?
>
> I cannot think of a specific scenario where this could be dangerous
> right now, but deferring to RCU context together with the final
> cleanup seems safer overall.
I agree, _domain_destroy() is the place for domid_free().
>
> > +
> > /* Schedule RCU asynchronous completion of domain destroy. */
> > call_rcu(&d->rcu, complete_domain_destroy);
> > }
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index f2a7caaf853c..5509998aa139 100644
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -51,20 +51,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;
> > @@ -423,36 +409,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 )
>
> This is a change in behavior AFAICT, as you now allow
> XEN_DOMCTL_createdomain to possibly create a domain with domid 0 (if
> it's available). Currently op->domain == 0 is handled as op->domain
> == DOMID_FIRST_RESERVED. You either need to adjust the code here, so
> that you do:
>
> domid_t domid = domid_alloc(op->domain ?: DOMID_FIRST_RESERVED);
>
> Or domid_alloc() needs to be adjusted to handle an input domid == 0 as
> it handles DOMID_FIRST_RESERVED.
Thanks!
I will adjust code in do_domctl().
>
> > {
> > 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/common/domid.c b/xen/common/domid.c
> > new file mode 100644
> > index 000000000000..e727dcaf0793
> > --- /dev/null
> > +++ b/xen/common/domid.c
> > @@ -0,0 +1,94 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Domain ID allocator.
> > + *
> > + * Covers dom0 or late hwdom, predefined domains, post-boot domains.
> > + * Excludes Xen system domains (ID >= DOMID_FIRST_RESERVED).
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +#include <xen/domain.h>
> > +
> > +static DEFINE_SPINLOCK(domid_lock);
> > +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> > +
> > +/*
> > + * Allocate domain ID.
> > + *
> > + * @param domid Domain ID hint:
> > + * - If an explicit domain ID is provided, verify its availability and use it
> > + * if ID is not used;
> > + * - If DOMID_INVALID is provided, search [1..DOMID_FIRST_RESERVED-1] range,
> > + * starting from the last used ID. Implementation guarantees that two
> > + * consecutive calls will never return the same ID. ID#0 is reserved for
> > + * the first boot domain (currently, dom0) and excluded from the allocation
> > + * range.
> > + * @return Valid domain ID in case of successful allocation,
> > + * DOMID_INVALID - otherwise.
> > + */
> > +domid_t domid_alloc(domid_t domid)
> > +{
> > + static domid_t domid_last;
> > +
> > + spin_lock(&domid_lock);
> > +
> > + /* Exact match. */
> > + if ( domid < DOMID_FIRST_RESERVED )
> > + {
> > + if ( __test_and_set_bit(domid, domid_bitmap) )
> > + domid = DOMID_INVALID;
> > + }
> > + /*
> > + * Exhaustive search.
> > + *
> > + * Domain ID#0 is reserved for the first boot domain (e.g. control domain)
> > + * and excluded from allocation.
> > + */
> > + else
> > + {
> > + domid = find_next_zero_bit(domid_bitmap,
> > + DOMID_FIRST_RESERVED,
> > + domid_last + 1);
> > + if ( domid == DOMID_FIRST_RESERVED )
>
> Nit: you could further gate this second search to domid_last != 0, as
> otherwise the first search has already scanned the whole bitmap.
Ack.
>
> > + domid = find_next_zero_bit(domid_bitmap,
> > + DOMID_FIRST_RESERVED,
> > + 1);
>
> Nit: you could possibly limit this second search to (domid_last + 1)
> size, as you have already searched from [domid_last + 1,
> DOMID_FIRST_RESERVED], and the bitmap couldn't have changed as the
> lock is being held.
Ack.
>
> Thanks, Roger.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v13 2/3] tools/tests: introduce unit tests for domain ID allocator
2025-08-05 14:15 ` Roger Pau Monné
@ 2025-08-07 2:12 ` dmkhn
0 siblings, 0 replies; 8+ messages in thread
From: dmkhn @ 2025-08-07 2:12 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, alejandro.garciavallejo, andrew.cooper3,
anthony.perard, jbeulich, julien, michal.orzel, sstabellini,
dmukhin
On Tue, Aug 05, 2025 at 04:15:28PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 30, 2025 at 05:41:00PM +0000, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Introduce some basic infrastructure for doing domain ID allocation unit tests,
> > and add a few tests that ensure correctness of the domain ID allocator.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v12:
> > - fixed Makefile
> > - dropped unused symbols/includes from the test harness header
> > - s/printk/printf/g in the test code
> > ---
> > tools/tests/Makefile | 2 +-
> > tools/tests/domid/.gitignore | 2 +
> > tools/tests/domid/Makefile | 48 ++++++++++
> > tools/tests/domid/include/xen/domain.h | 126 +++++++++++++++++++++++++
> > tools/tests/domid/test-domid.c | 78 +++++++++++++++
> > 5 files changed, 255 insertions(+), 1 deletion(-)
> > create mode 100644 tools/tests/domid/.gitignore
> > create mode 100644 tools/tests/domid/Makefile
> > create mode 100644 tools/tests/domid/include/xen/domain.h
> > create mode 100644 tools/tests/domid/test-domid.c
> >
> > diff --git a/tools/tests/Makefile b/tools/tests/Makefile
> > index 36928676a666..ff1666425436 100644
> > --- a/tools/tests/Makefile
> > +++ b/tools/tests/Makefile
> > @@ -1,7 +1,7 @@
> > XEN_ROOT = $(CURDIR)/../..
> > include $(XEN_ROOT)/tools/Rules.mk
> >
> > -SUBDIRS-y :=
> > +SUBDIRS-y := domid
> > SUBDIRS-y += resource
> > SUBDIRS-$(CONFIG_X86) += cpu-policy
> > SUBDIRS-$(CONFIG_X86) += tsx
> > diff --git a/tools/tests/domid/.gitignore b/tools/tests/domid/.gitignore
> > new file mode 100644
> > index 000000000000..70e306b3c074
> > --- /dev/null
> > +++ b/tools/tests/domid/.gitignore
> > @@ -0,0 +1,2 @@
> > +*.o
> > +test-domid
> > diff --git a/tools/tests/domid/Makefile b/tools/tests/domid/Makefile
> > new file mode 100644
> > index 000000000000..08fbad096aec
> > --- /dev/null
> > +++ b/tools/tests/domid/Makefile
> > @@ -0,0 +1,48 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +#
> > +# Unit tests for domain ID allocator.
> > +#
> > +# Copyright 2025 Ford Motor Company
> > +
> > +XEN_ROOT=$(CURDIR)/../../..
> > +include $(XEN_ROOT)/tools/Rules.mk
> > +
> > +TESTS := test-domid
> > +
> > +vpath domid.c $(XEN_ROOT)/xen/common/
> > +
> > +.PHONY: all
> > +all: $(TESTS)
> > +
> > +.PHONY: run
> > +run: $(TESTS)
> > + $(foreach t,$(TESTS),./$(t);)
> > +
> > +.PHONY: clean
> > +clean:
> > + $(RM) -- *.o $(TESTS) $(DEPS_RM)
> > +
> > +.PHONY: distclean
> > +distclean: clean
> > + $(RM) -- *~
> > +
> > +.PHONY: install
> > +install: all
> > + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
> > + $(INSTALL_PROG) test-domid $(DESTDIR)$(LIBEXEC)/tests
> > +
> > +.PHONY: uninstall
> > +uninstall:
> > + $(RM) -- $(DESTDIR)$(LIBEXEC)/tests/test-domid
> > +
> > +CFLAGS += -D__XEN_TOOLS__
> > +CFLAGS += $(APPEND_CFLAGS)
> > +CFLAGS += $(CFLAGS_xeninclude)
> > +CFLAGS += -I./include/
> > +
> > +LDFLAGS += $(APPEND_LDFLAGS)
> > +
> > +test-domid: domid.o test-domid.o
> > + $(CC) $^ -o $@ $(LDFLAGS)
> > +
> > +-include $(DEPS_INCLUDE)
> > diff --git a/tools/tests/domid/include/xen/domain.h b/tools/tests/domid/include/xen/domain.h
> > new file mode 100644
> > index 000000000000..e5db0235445e
> > --- /dev/null
> > +++ b/tools/tests/domid/include/xen/domain.h
> > @@ -0,0 +1,126 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Unit test harness for domain ID allocator.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +#ifndef _TEST_HARNESS_
> > +#define _TEST_HARNESS_
> > +
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +
> > +#include <xen-tools/common-macros.h>
> > +
> > +#define BUG_ON(x) assert(!(x))
> > +#define ASSERT(x) assert(x)
> > +
> > +#define DOMID_FIRST_RESERVED (10)
> > +#define DOMID_INVALID (11)
> > +
> > +#define DEFINE_SPINLOCK(x) unsigned long *(x)
>
> I think this shouldn't be a pointer? As you otherwise trigger a NULL
> pointer dereference in the increases and decreases done below?
Sorry, this bitops integration is very raw.
Thanks for all your suggestions, I reworked it.
>
> > +#define spin_lock(x) ((*(x))++)
> > +#define spin_unlock(x) ((*(x))--)
>
> FWIW, I would use a plain bool:
>
> #define DEFINE_SPINLOCK(l) bool l
> #define spin_lock(l) (*(l) = true)
> #define spin_unlock(l) (*(l) = false)
>
> As you don't expect concurrency tests, you could even assert the lock
> is in the expected state before taking/releasing it.
>
> > +
> > +#define printk printf
> > +
> > +#define BITS_PER_LONG sizeof(unsigned long)
>
> That's BYTES_PER_LONG, BITS_PER_LONG would be (sizeof(unsigned long) * 8).
>
> > +#define BITS_PER_WORD (8U * BITS_PER_LONG)
> > +#define BITS_TO_LONGS(bits) \
> > + (((bits) + BITS_PER_LONG - 1) / BITS_PER_LONG)
> > +#define DECLARE_BITMAP(name, bits) \
> > + unsigned long name[BITS_TO_LONGS(bits)]
> > +
> > +static inline int __test_and_set_bit(unsigned int nr, unsigned long *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = addr + (nr / BITS_PER_WORD);
> > + int old = (*p & mask) != 0;
> > +
> > + *p |= mask;
> > +
> > + return old;
> > +}
> > +
> > +static inline int __test_and_clear_bit(unsigned int nr, unsigned long *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = addr + (nr / BITS_PER_WORD);
> > + int old = (*p & mask) != 0;
> > +
> > + *p &= ~mask;
> > +
> > + return old;
> > +}
>
> Could you somehow use the generic__test_and_set_bit() and
> generic__test_and_clear_bit() implementations in bitops.h?
I tried that originally, and it pulls a lot of dependencies from xen/bitops.h;
that will be a mini project to compile xen/bitops.h for the host, which I
think I can skip doing for the purpose of this test.
I followed another approach as discussed offline in matrix: re-purpose
tools/libs/ctrl/xc_bitops.h which seems to be working nice!
>
> > +
> > +static inline void __set_bit(unsigned int nr, volatile unsigned long *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
>
> Why do you need the cast to drop the volatile here?
>
> > +
> > + *p |= mask;
> > +}
>
> I think you could possibly simplify to a single line:
>
> ((unsigned int *)addr)[nr >> 5] |= (1u << (nr & 31));
>
> That's the implementation of constant_set_bit() in x86.
>
> > +
> > +static inline void __clear_bit(unsigned int nr, volatile unsigned long *addr)
> > +{
> > + unsigned long mask = 1UL << (nr % BITS_PER_WORD);
> > + unsigned long *p = (unsigned long *)addr + (nr / BITS_PER_WORD);
> > +
> > + *p &= ~mask;
> > +}
>
> I don't think you need __clear_bit()? It's not used by domid.c AFAICT.
Overlooked, thanks.
>
> > +
> > +static inline unsigned long find_next_zero_bit(const unsigned long *addr,
> > + unsigned long size,
> > + unsigned long offset)
> > +{
> > + unsigned long idx = offset / BITS_PER_WORD;
> > + unsigned long bit = offset % BITS_PER_WORD;
> > +
> > + if (offset >= size)
> > + return size;
> > +
> > + while (offset < size)
> > + {
> > + unsigned long val = addr[idx] | (~0UL >> (BITS_PER_WORD - bit));
> > +
> > + if (~val)
> > + {
> > + unsigned long pos = __builtin_ffsl(~val);
> > +
> > + if (pos > 0)
> > + {
> > + unsigned long rc = idx * BITS_PER_WORD + (pos - 1);
> > +
> > + if (rc < size)
> > + return rc;
> > + }
> > + }
> > +
> > + offset = (idx + 1) * BITS_PER_WORD;
> > + idx++;
> > + bit = 0;
> > + }
> > +
> > + return size;
> > +}
>
> Hm, you need a full find_next_zero_bit() implementation here because
> addr can be arbitrarily long. Could you somehow include
> xen/lib/find-next-bit.c and set the right defines so only the
> implementation of find_next_bit() is included?
That's a good idea!
xen/lib/find-next-bit.c seems to be integrating pretty simple.
Thanks for the hint!
>
> > +
> > +typedef bool spinlock_t;
>
> You want to put this ahead, so that DEFINE_SPINLOCK can be:
>
> #define DEFINE_SPINLOCK(l) spinlock_t l
>
> > +typedef uint16_t domid_t;
> > +
> > +/* See include/xen/domain.h */
> > +extern domid_t domid_alloc(domid_t domid);
> > +extern void domid_free(domid_t domid);
> > +
> > +#endif /* _TEST_HARNESS_ */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/tools/tests/domid/test-domid.c b/tools/tests/domid/test-domid.c
> > new file mode 100644
> > index 000000000000..d52eaf5f1f55
> > --- /dev/null
> > +++ b/tools/tests/domid/test-domid.c
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Unit tests for domain ID allocator.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +/* Local test include replicating hypervisor includes. */
> > +#include <xen/domain.h>
>
> I think this is a difficult to maintain position. Right now domid.c
> only includes xen/domain.h, so you can easily replace this in
> user-space. However if/when domid.c starts including more headers,
> replicating this in user-space will be cumbersome IMO.
>
> I would just guard the includes in domid.c with #ifdef __XEN__ for the
> preprocessor to remove them when domid.c is compiled as part of the
> unit-tests harness.
>
> I usually include a harness.h that contains the glue to make the
> imported code build (much like what you have placed in the test
> harness xen/domain.h header.
I like that there's no need to modify the tested code.
I reworked this slightly differently: local include/xen/domain.h is a symlink
to local harness.h, and all future dependendent files will be symlinks to
harness.h as well.
>
> > +
> > +int main(int argc, char **argv)
> > +{
> > + domid_t expected, allocated;
> > +
> > + printf("DOMID_FIRST_RESERVED=%u DOMID_INVALID=%u\n",
> > + DOMID_FIRST_RESERVED, DOMID_INVALID);
> > +
> > + /* Test ID#0 cannot be allocated twice. */
> > + allocated = domid_alloc(0);
> > + printf("TEST 1: expected %u allocated %u\n", 0, allocated);
> > + ASSERT(allocated == 0);
> > + allocated = domid_alloc(0);
> > + printf("TEST 1: expected %u allocated %u\n", DOMID_INVALID, allocated);
> > + ASSERT(allocated == DOMID_INVALID);
> > +
> > + /* Ensure ID is not allocated. */
> > + domid_free(0);
> > +
> > + /*
> > + * Test that that two consecutive calls of domid_alloc(DOMID_INVALID)
> > + * will never return the same ID.
> > + * NB: ID#0 is reserved and shall not be allocated by
> > + * domid_alloc(DOMID_INVALID).
> > + */
> > + for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
> > + {
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 2: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
> > + }
> > + for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
> > + {
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 3: expected %u allocated %u\n", DOMID_INVALID, allocated);
> > + ASSERT(allocated == DOMID_INVALID);
> > + }
> > +
> > + /* Re-allocate first ID from [1..DOMID_FIRST_RESERVED - 1]. */
> > + expected = 1;
> > + domid_free(1);
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 4: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
> > +
> > + /* Re-allocate last ID from [1..DOMID_FIRST_RESERVED - 1]. */
> > + expected = DOMID_FIRST_RESERVED - 1;
> > + domid_free(DOMID_FIRST_RESERVED - 1);
> > + allocated = domid_alloc(DOMID_INVALID);
> > + printf("TEST 5: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
> > +
> > + /* Allocate an invalid ID. */
> > + expected = DOMID_INVALID;
> > + allocated = domid_alloc(DOMID_FIRST_RESERVED);
> > + printf("TEST 6: expected %u allocated %u\n", expected, allocated);
> > + ASSERT(allocated == expected);
>
> I would make this a bit less chatty maybe?
Ack.
>
> I think you only need to print on errors, and you probably don't want
> to ASSERT() on failure, and rather try to finish all the tests in
> order to report multiple failures in a single run.
I thought about it originally, but my "tests" depend on each other, so I'll
keep failing on the first error as is, if there's no strong objection.
>
> Thanks, Roger.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-08-07 2:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-30 17:40 [PATCH v13 0/3] xen/domain: domain ID allocation dmkhn
2025-07-30 17:40 ` [PATCH v13 1/3] xen/domain: unify " dmkhn
2025-08-05 13:38 ` Roger Pau Monné
2025-08-07 1:16 ` dmkhn
2025-07-30 17:41 ` [PATCH v13 2/3] tools/tests: introduce unit tests for domain ID allocator dmkhn
2025-08-05 14:15 ` Roger Pau Monné
2025-08-07 2:12 ` dmkhn
2025-07-30 17:41 ` [PATCH v13 3/3] xen/domain: update create_dom0() messages dmkhn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.