* [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-12 22:30 [PATCH v16 0/4] xen/domain: domain ID allocation dmkhn
@ 2025-08-12 22:30 ` dmkhn
2025-08-14 7:11 ` Jan Beulich
2025-08-20 21:33 ` Julien Grall
2025-08-12 22:30 ` [PATCH v16 2/4] tools/include: move xc_bitops.h to xen-tools/bitops.h dmkhn
` (2 subsequent siblings)
3 siblings, 2 replies; 19+ messages in thread
From: dmkhn @ 2025-08-12 22:30 UTC (permalink / raw)
To: xen-devel
Cc: andrew.cooper3, anthony.perard, jbeulich, julien, michal.orzel,
roger.pau, sstabellini, dmukhin, Julien Grall, Alejandro Vallejo
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: Julien Grall <jgrall@amazon.com>
Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
Changes since v15:
- fixup for check after the first pass in the bitarray in domid_alloc()
- trivial renaming for the local variable in domid_alloc()
- kept Julien's R-b, 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 | 43 ++---------
xen/common/domid.c | 95 +++++++++++++++++++++++++
xen/include/xen/domain.h | 3 +
8 files changed, 126 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 a9e4153e3cf9..aca35b8961d6 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..398da734c0c5 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 %u\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..a7e303253d1a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -692,6 +692,8 @@ static void _domain_destroy(struct domain *d)
lock_profile_deregister_struct(LOCKPROF_TYPE_PERDOM, d);
+ domid_free(d->domain_id);
+
free_domain_struct(d);
}
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f2a7caaf853c..71e712c1f316 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,19 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
case XEN_DOMCTL_createdomain:
{
- domid_t dom;
- static domid_t rover = 0;
+ /* NB: ID#0 is reserved, find the first suitable ID instead. */
+ domid_t domid = domid_alloc(op->domain ?: DOMID_INVALID);
- 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..2387ddb08300
--- /dev/null
+++ b/xen/common/domid.c
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Domain ID allocator.
+ *
+ * Covers dom0 or late hwdom, predefined domains, post-boot domains.
+ * Excludes 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_t bound = DOMID_FIRST_RESERVED;
+
+ domid = find_next_zero_bit(domid_bitmap, bound, domid_last + 1);
+ if ( domid >= bound && domid_last != 0 )
+ {
+ bound = domid_last + 1;
+ domid = find_next_zero_bit(domid_bitmap, bound, 1);
+ }
+
+ ASSERT(domid <= DOMID_FIRST_RESERVED);
+ if ( domid < bound )
+ {
+ __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] 19+ messages in thread* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-12 22:30 ` [PATCH v16 1/4] xen/domain: unify " dmkhn
@ 2025-08-14 7:11 ` Jan Beulich
2025-08-19 23:58 ` dmkhn
2025-08-20 21:33 ` Julien Grall
1 sibling, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2025-08-14 7:11 UTC (permalink / raw)
To: dmkhn
Cc: andrew.cooper3, anthony.perard, julien, michal.orzel, roger.pau,
sstabellini, dmukhin, Julien Grall, Alejandro Vallejo, xen-devel
On 13.08.2025 00:30, 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: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> ---
> Changes since v15:
> - fixup for check after the first pass in the bitarray in domid_alloc()
> - trivial renaming for the local variable in domid_alloc()
> - kept Julien's R-b, added Alejandro's R-b
Just to mention: My take is that this kind of a fix ought to invalidate all
earlier R-b. It's not just a cosmetic change, after all.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-14 7:11 ` Jan Beulich
@ 2025-08-19 23:58 ` dmkhn
2025-08-21 7:16 ` Jan Beulich
0 siblings, 1 reply; 19+ messages in thread
From: dmkhn @ 2025-08-19 23:58 UTC (permalink / raw)
To: Jan Beulich
Cc: andrew.cooper3, anthony.perard, julien, michal.orzel, roger.pau,
sstabellini, dmukhin, Julien Grall, Alejandro Vallejo, xen-devel
On Thu, Aug 14, 2025 at 09:11:11AM +0200, Jan Beulich wrote:
> On 13.08.2025 00:30, 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: Julien Grall <jgrall@amazon.com>
> > Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> > ---
> > Changes since v15:
> > - fixup for check after the first pass in the bitarray in domid_alloc()
> > - trivial renaming for the local variable in domid_alloc()
> > - kept Julien's R-b, added Alejandro's R-b
>
> Just to mention: My take is that this kind of a fix ought to invalidate all
> earlier R-b. It's not just a cosmetic change, after all.
Sorry for the hiccup here, did not mean to overrule the review process.
My bold assumption was that in case of small fixups like this it is
satisfactory to carry over previous acks.
I asked (matrix) both Julien and Alejandro to re-review and confirm.
>
> Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-19 23:58 ` dmkhn
@ 2025-08-21 7:16 ` Jan Beulich
2025-08-21 10:29 ` Alejandro Vallejo
2025-08-26 9:53 ` dmkhn
0 siblings, 2 replies; 19+ messages in thread
From: Jan Beulich @ 2025-08-21 7:16 UTC (permalink / raw)
To: dmkhn
Cc: andrew.cooper3, anthony.perard, julien, michal.orzel, roger.pau,
sstabellini, dmukhin, Julien Grall, Alejandro Vallejo, xen-devel
On 20.08.2025 01:58, dmkhn@proton.me wrote:
> On Thu, Aug 14, 2025 at 09:11:11AM +0200, Jan Beulich wrote:
>> On 13.08.2025 00:30, 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: Julien Grall <jgrall@amazon.com>
>>> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>>> ---
>>> Changes since v15:
>>> - fixup for check after the first pass in the bitarray in domid_alloc()
>>> - trivial renaming for the local variable in domid_alloc()
>>> - kept Julien's R-b, added Alejandro's R-b
>>
>> Just to mention: My take is that this kind of a fix ought to invalidate all
>> earlier R-b. It's not just a cosmetic change, after all.
>
> Sorry for the hiccup here, did not mean to overrule the review process.
>
> My bold assumption was that in case of small fixups like this it is
> satisfactory to carry over previous acks.
Acks may be okay to keep, but imo R-b need dropping when an actual bug was
fixed. Irrespective of how severe the bug was.
> I asked (matrix) both Julien and Alejandro to re-review and confirm.
While good to ask, that's of limited use. It'll be impossible later on to
figure whether such a confirmation was given. Decisions (and acks and alike
effectively fall into that category) need to be on the list, to be able to
locate them later on.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-21 7:16 ` Jan Beulich
@ 2025-08-21 10:29 ` Alejandro Vallejo
2025-08-26 9:53 ` dmkhn
2025-08-26 9:53 ` dmkhn
1 sibling, 1 reply; 19+ messages in thread
From: Alejandro Vallejo @ 2025-08-21 10:29 UTC (permalink / raw)
To: Jan Beulich, dmkhn
Cc: andrew.cooper3, anthony.perard, julien, michal.orzel, roger.pau,
sstabellini, dmukhin, Julien Grall, xen-devel
On Thu Aug 21, 2025 at 9:16 AM CEST, Jan Beulich wrote:
> On 20.08.2025 01:58, dmkhn@proton.me wrote:
>> On Thu, Aug 14, 2025 at 09:11:11AM +0200, Jan Beulich wrote:
>>> On 13.08.2025 00:30, 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: Julien Grall <jgrall@amazon.com>
>>>> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>>>> ---
>>>> Changes since v15:
>>>> - fixup for check after the first pass in the bitarray in domid_alloc()
>>>> - trivial renaming for the local variable in domid_alloc()
>>>> - kept Julien's R-b, added Alejandro's R-b
>>>
>>> Just to mention: My take is that this kind of a fix ought to invalidate all
>>> earlier R-b. It's not just a cosmetic change, after all.
>>
>> Sorry for the hiccup here, did not mean to overrule the review process.
>>
>> My bold assumption was that in case of small fixups like this it is
>> satisfactory to carry over previous acks.
>
> Acks may be okay to keep, but imo R-b need dropping when an actual bug was
> fixed.
I don't know. Unless the bugfix involves a change in the code with wide reaching
consequences I'd say it's reasonable to keep them. But that's something for you
(the committers) to decide, and this just my .02 cents.
> Irrespective of how severe the bug was.
It's not so much about the severity (imo), as the behavioural differences
involved in the fixup. In this case (afaics?) it's a straight s/==/>=/, which is
self-contained and has no wide-reaching side effects at all.
>
>> I asked (matrix) both Julien and Alejandro to re-review and confirm.
>
> While good to ask, that's of limited use. It'll be impossible later on to
> figure whether such a confirmation was given. Decisions (and acks and alike
> effectively fall into that category) need to be on the list, to be able to
> locate them later on.
>
> Jan
He meant he reached out to ask for an in-list confirmation. As far as I'm
concerned, my R-by still holds.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
@ 2025-08-26 9:53 ` dmkhn
0 siblings, 0 replies; 19+ messages in thread
From: dmkhn @ 2025-08-26 9:52 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Jan Beulich, andrew.cooper3, anthony.perard, julien, michal.orzel,
roger.pau, sstabellini, dmukhin, Julien Grall, xen-devel
On Thu, Aug 21, 2025 at 12:29:23PM +0200, Alejandro Vallejo wrote:
> On Thu Aug 21, 2025 at 9:16 AM CEST, Jan Beulich wrote:
> > On 20.08.2025 01:58, dmkhn@proton.me wrote:
> >> On Thu, Aug 14, 2025 at 09:11:11AM +0200, Jan Beulich wrote:
> >>> On 13.08.2025 00:30, 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: Julien Grall <jgrall@amazon.com>
> >>>> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> >>>> ---
> >>>> Changes since v15:
> >>>> - fixup for check after the first pass in the bitarray in domid_alloc()
> >>>> - trivial renaming for the local variable in domid_alloc()
> >>>> - kept Julien's R-b, added Alejandro's R-b
> >>>
> >>> Just to mention: My take is that this kind of a fix ought to invalidate all
> >>> earlier R-b. It's not just a cosmetic change, after all.
> >>
> >> Sorry for the hiccup here, did not mean to overrule the review process.
> >>
> >> My bold assumption was that in case of small fixups like this it is
> >> satisfactory to carry over previous acks.
> >
> > Acks may be okay to keep, but imo R-b need dropping when an actual bug was
> > fixed.
>
> I don't know. Unless the bugfix involves a change in the code with wide reaching
> consequences I'd say it's reasonable to keep them. But that's something for you
> (the committers) to decide, and this just my .02 cents.
>
> > Irrespective of how severe the bug was.
>
> It's not so much about the severity (imo), as the behavioural differences
> involved in the fixup. In this case (afaics?) it's a straight s/==/>=/, which is
> self-contained and has no wide-reaching side effects at all.
>
> >
> >> I asked (matrix) both Julien and Alejandro to re-review and confirm.
> >
> > While good to ask, that's of limited use. It'll be impossible later on to
> > figure whether such a confirmation was given. Decisions (and acks and alike
> > effectively fall into that category) need to be on the list, to be able to
> > locate them later on.
> >
> > Jan
>
> He meant he reached out to ask for an in-list confirmation. As far as I'm
> concerned, my R-by still holds.
Thanks
>
> Cheers,
> Alejandro
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
@ 2025-08-26 9:53 ` dmkhn
0 siblings, 0 replies; 19+ messages in thread
From: dmkhn @ 2025-08-26 9:53 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Jan Beulich, andrew.cooper3, anthony.perard, julien, michal.orzel,
roger.pau, sstabellini, dmukhin, Julien Grall, xen-devel
On Thu, Aug 21, 2025 at 12:29:23PM +0200, Alejandro Vallejo wrote:
> On Thu Aug 21, 2025 at 9:16 AM CEST, Jan Beulich wrote:
> > On 20.08.2025 01:58, dmkhn@proton.me wrote:
> >> On Thu, Aug 14, 2025 at 09:11:11AM +0200, Jan Beulich wrote:
> >>> On 13.08.2025 00:30, 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: Julien Grall <jgrall@amazon.com>
> >>>> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> >>>> ---
> >>>> Changes since v15:
> >>>> - fixup for check after the first pass in the bitarray in domid_alloc()
> >>>> - trivial renaming for the local variable in domid_alloc()
> >>>> - kept Julien's R-b, added Alejandro's R-b
> >>>
> >>> Just to mention: My take is that this kind of a fix ought to invalidate all
> >>> earlier R-b. It's not just a cosmetic change, after all.
> >>
> >> Sorry for the hiccup here, did not mean to overrule the review process.
> >>
> >> My bold assumption was that in case of small fixups like this it is
> >> satisfactory to carry over previous acks.
> >
> > Acks may be okay to keep, but imo R-b need dropping when an actual bug was
> > fixed.
>
> I don't know. Unless the bugfix involves a change in the code with wide reaching
> consequences I'd say it's reasonable to keep them. But that's something for you
> (the committers) to decide, and this just my .02 cents.
>
> > Irrespective of how severe the bug was.
>
> It's not so much about the severity (imo), as the behavioural differences
> involved in the fixup. In this case (afaics?) it's a straight s/==/>=/, which is
> self-contained and has no wide-reaching side effects at all.
>
> >
> >> I asked (matrix) both Julien and Alejandro to re-review and confirm.
> >
> > While good to ask, that's of limited use. It'll be impossible later on to
> > figure whether such a confirmation was given. Decisions (and acks and alike
> > effectively fall into that category) need to be on the list, to be able to
> > locate them later on.
> >
> > Jan
>
> He meant he reached out to ask for an in-list confirmation. As far as I'm
> concerned, my R-by still holds.
Thanks
>
> Cheers,
> Alejandro
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-21 7:16 ` Jan Beulich
2025-08-21 10:29 ` Alejandro Vallejo
@ 2025-08-26 9:53 ` dmkhn
1 sibling, 0 replies; 19+ messages in thread
From: dmkhn @ 2025-08-26 9:53 UTC (permalink / raw)
To: Jan Beulich
Cc: andrew.cooper3, anthony.perard, julien, michal.orzel, roger.pau,
sstabellini, dmukhin, Julien Grall, Alejandro Vallejo, xen-devel
On Thu, Aug 21, 2025 at 09:16:16AM +0200, Jan Beulich wrote:
> On 20.08.2025 01:58, dmkhn@proton.me wrote:
> > On Thu, Aug 14, 2025 at 09:11:11AM +0200, Jan Beulich wrote:
> >> On 13.08.2025 00:30, 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: Julien Grall <jgrall@amazon.com>
> >>> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> >>> ---
> >>> Changes since v15:
> >>> - fixup for check after the first pass in the bitarray in domid_alloc()
> >>> - trivial renaming for the local variable in domid_alloc()
> >>> - kept Julien's R-b, added Alejandro's R-b
> >>
> >> Just to mention: My take is that this kind of a fix ought to invalidate all
> >> earlier R-b. It's not just a cosmetic change, after all.
> >
> > Sorry for the hiccup here, did not mean to overrule the review process.
> >
> > My bold assumption was that in case of small fixups like this it is
> > satisfactory to carry over previous acks.
>
> Acks may be okay to keep, but imo R-b need dropping when an actual bug was
> fixed. Irrespective of how severe the bug was.
>
> > I asked (matrix) both Julien and Alejandro to re-review and confirm.
>
> While good to ask, that's of limited use. It'll be impossible later on to
> figure whether such a confirmation was given. Decisions (and acks and alike
> effectively fall into that category) need to be on the list, to be able to
> locate them later on.
Got it, thanks.
>
> Jan
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-12 22:30 ` [PATCH v16 1/4] xen/domain: unify " dmkhn
2025-08-14 7:11 ` Jan Beulich
@ 2025-08-20 21:33 ` Julien Grall
2025-08-26 9:51 ` dmkhn
2025-08-29 23:32 ` dmukhin
1 sibling, 2 replies; 19+ messages in thread
From: Julien Grall @ 2025-08-20 21:33 UTC (permalink / raw)
To: dmkhn, xen-devel
Cc: andrew.cooper3, anthony.perard, jbeulich, michal.orzel, roger.pau,
sstabellini, dmukhin, Julien Grall, Alejandro Vallejo
Hi Denis,
On 12/08/2025 23:30, 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: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> ---> Changes since v15:
> - fixup for check after the first pass in the bitarray in domid_alloc()
This was a good catch from Jan. Has a unit-test been added for this issue?
Anyway, my reviewed-by holds.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-20 21:33 ` Julien Grall
@ 2025-08-26 9:51 ` dmkhn
2025-08-29 23:32 ` dmukhin
1 sibling, 0 replies; 19+ messages in thread
From: dmkhn @ 2025-08-26 9:51 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, andrew.cooper3, anthony.perard, jbeulich, michal.orzel,
roger.pau, sstabellini, dmukhin, Julien Grall, Alejandro Vallejo
On Wed, Aug 20, 2025 at 10:33:16PM +0100, Julien Grall wrote:
> Hi Denis,
>
> On 12/08/2025 23:30, 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: Julien Grall <jgrall@amazon.com>
> > Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> > ---> Changes since v15:
> > - fixup for check after the first pass in the bitarray in domid_alloc()
>
> This was a good catch from Jan. Has a unit-test been added for this issue?
No, I did not update the test suite, will do.
>
> Anyway, my reviewed-by holds.
Thanks.
>
> Cheers,
>
> --
> Julien Grall
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v16 1/4] xen/domain: unify domain ID allocation
2025-08-20 21:33 ` Julien Grall
2025-08-26 9:51 ` dmkhn
@ 2025-08-29 23:32 ` dmukhin
1 sibling, 0 replies; 19+ messages in thread
From: dmukhin @ 2025-08-29 23:32 UTC (permalink / raw)
To: Julien Grall
Cc: dmkhn, xen-devel, andrew.cooper3, anthony.perard, jbeulich,
michal.orzel, roger.pau, sstabellini, dmukhin, Julien Grall,
Alejandro Vallejo
On Wed, Aug 20, 2025 at 10:33:16PM +0100, Julien Grall wrote:
> Hi Denis,
>
> On 12/08/2025 23:30, 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: Julien Grall <jgrall@amazon.com>
> > Reviewed-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> > ---> Changes since v15:
> > - fixup for check after the first pass in the bitarray in domid_alloc()
>
> This was a good catch from Jan. Has a unit-test been added for this issue?
So I looked into how to plumb the test.
The boundary conditions tests are there for IDs: 0, DOMID_FIRST_RESERVED-1,
DOMID_FIRST_RESERVED.
But to actually check that
domid_t bound = DOMID_FIRST_RESERVED;
domid = find_next_zero_bit(domid_bitmap, bound, domid_last + 1);
if ( domid >= bound ...
I need to write a test harness for bit manipulation and it is a project on its
own to make hypervisor's bitops compiled for the host...
>
> Anyway, my reviewed-by holds.
>
> Cheers,
>
> --
> Julien Grall
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v16 2/4] tools/include: move xc_bitops.h to xen-tools/bitops.h
2025-08-12 22:30 [PATCH v16 0/4] xen/domain: domain ID allocation dmkhn
2025-08-12 22:30 ` [PATCH v16 1/4] xen/domain: unify " dmkhn
@ 2025-08-12 22:30 ` dmkhn
2025-08-25 9:30 ` Anthony PERARD
2025-08-12 22:30 ` [PATCH v16 3/4] tools/tests: introduce unit tests for domain ID allocator dmkhn
2025-08-12 22:30 ` [PATCH v16 4/4] xen/domain: update create_dom0() messages dmkhn
3 siblings, 1 reply; 19+ messages in thread
From: dmkhn @ 2025-08-12 22:30 UTC (permalink / raw)
To: xen-devel
Cc: andrew.cooper3, anthony.perard, jbeulich, julien, michal.orzel,
roger.pau, sstabellini, dmukhin
From: Denis Mukhin <dmukhin@ford.com>
Move xc_bitops.h to common tools location to be shared between
the toolstack and unit test code.
Adjust the guard in xen-tools/bitops.h
Correct the #include directives and comments referring to the old
xc_bitops.h in the toolstack code.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v15:
- n/a
---
.../ctrl/xc_bitops.h => include/xen-tools/bitops.h} | 6 +++---
tools/libs/ctrl/xc_misc.c | 13 +++++++------
tools/libs/guest/xg_dom_elfloader.c | 3 ++-
tools/libs/guest/xg_dom_hvmloader.c | 3 ++-
tools/libs/guest/xg_private.h | 2 +-
tools/libs/guest/xg_sr_common.h | 3 +--
6 files changed, 16 insertions(+), 14 deletions(-)
rename tools/{libs/ctrl/xc_bitops.h => include/xen-tools/bitops.h} (95%)
diff --git a/tools/libs/ctrl/xc_bitops.h b/tools/include/xen-tools/bitops.h
similarity index 95%
rename from tools/libs/ctrl/xc_bitops.h
rename to tools/include/xen-tools/bitops.h
index 4a776dc3a57f..681482f6759f 100644
--- a/tools/libs/ctrl/xc_bitops.h
+++ b/tools/include/xen-tools/bitops.h
@@ -1,5 +1,5 @@
-#ifndef XC_BITOPS_H
-#define XC_BITOPS_H 1
+#ifndef __XEN_TOOLS_BITOPS_H__
+#define __XEN_TOOLS_BITOPS_H__
/* bitmap operations for single threaded access */
@@ -81,4 +81,4 @@ static inline void bitmap_or(void *_dst, const void *_other,
dst[i] |= other[i];
}
-#endif /* XC_BITOPS_H */
+#endif /* __XEN_TOOLS_BITOPS_H__ */
diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
index 33e87bac2868..10ddf85667a9 100644
--- a/tools/libs/ctrl/xc_misc.c
+++ b/tools/libs/ctrl/xc_misc.c
@@ -17,8 +17,8 @@
* License along with this library; If not, see <http://www.gnu.org/licenses/>.
*/
-#include "xc_bitops.h"
#include "xc_private.h"
+#include <xen-tools/bitops.h>
#include <xen/hvm/hvm_op.h>
int xc_get_max_cpus(xc_interface *xch)
@@ -94,11 +94,12 @@ xc_cpumap_t xc_cpumap_alloc(xc_interface *xch)
}
/*
- * xc_bitops.h has macros that do this as well - however they assume that
- * the bitmask is word aligned but xc_cpumap_t is only guaranteed to be
- * byte aligned and so we need byte versions for architectures which do
- * not support misaligned accesses (which is basically everyone
- * but x86, although even on x86 it can be inefficient).
+ * <xen-tools/bitops.h> has macros that do this as well - however they
+ * assume that the bitmask is word aligned but xc_cpumap_t is only
+ * guaranteed to be byte aligned and so we need byte versions for
+ * architectures which do not support misaligned accesses (which is
+ * basically everyone but x86, although even on x86 it can be
+ * inefficient).
*
* NOTE: The xc_bitops macros now use byte alignment.
* TODO: Clean up the users of this interface.
diff --git a/tools/libs/guest/xg_dom_elfloader.c b/tools/libs/guest/xg_dom_elfloader.c
index f17930d98bf7..8531e90f8e21 100644
--- a/tools/libs/guest/xg_dom_elfloader.c
+++ b/tools/libs/guest/xg_dom_elfloader.c
@@ -25,8 +25,9 @@
#include <stdarg.h>
#include <inttypes.h>
+#include <xen-tools/bitops.h>
+
#include "xg_private.h"
-#include "xc_bitops.h"
#define XEN_VER "xen-3.0"
diff --git a/tools/libs/guest/xg_dom_hvmloader.c b/tools/libs/guest/xg_dom_hvmloader.c
index 39e1e5f579a7..0f569c20c522 100644
--- a/tools/libs/guest/xg_dom_hvmloader.c
+++ b/tools/libs/guest/xg_dom_hvmloader.c
@@ -24,8 +24,9 @@
#include <inttypes.h>
#include <assert.h>
+#include <xen-tools/bitops.h>
+
#include "xg_private.h"
-#include "xc_bitops.h"
/* ------------------------------------------------------------------------ */
/* parse elf binary */
diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
index d73947094f2e..285229cf82a3 100644
--- a/tools/libs/guest/xg_private.h
+++ b/tools/libs/guest/xg_private.h
@@ -28,9 +28,9 @@
#include <sys/stat.h>
#include "xc_private.h"
-#include "xc_bitops.h"
#include "xenguest.h"
+#include <xen-tools/bitops.h>
#include <xen/memory.h>
#include <xen/elfnote.h>
#include <xen/libelf/libelf.h>
diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index 2f058ee3a6ff..2e583f2eac72 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -2,11 +2,10 @@
#define __COMMON__H
#include <stdbool.h>
+#include <xen-tools/bitops.h>
#include "xg_private.h"
#include "xg_save_restore.h"
-#include "xc_bitops.h"
-
#include "xg_sr_stream_format.h"
/* String representation of Domain Header types. */
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v16 2/4] tools/include: move xc_bitops.h to xen-tools/bitops.h
2025-08-12 22:30 ` [PATCH v16 2/4] tools/include: move xc_bitops.h to xen-tools/bitops.h dmkhn
@ 2025-08-25 9:30 ` Anthony PERARD
2025-08-26 9:28 ` dmkhn
0 siblings, 1 reply; 19+ messages in thread
From: Anthony PERARD @ 2025-08-25 9:30 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, andrew.cooper3, anthony.perard, jbeulich, julien,
michal.orzel, roger.pau, sstabellini, dmukhin
On Tue, Aug 12, 2025 at 10:30:45PM +0000, dmkhn@proton.me wrote:
> diff --git a/tools/libs/guest/xg_dom_elfloader.c b/tools/libs/guest/xg_dom_elfloader.c
> index f17930d98bf7..8531e90f8e21 100644
> --- a/tools/libs/guest/xg_dom_elfloader.c
> +++ b/tools/libs/guest/xg_dom_elfloader.c
> @@ -25,8 +25,9 @@
> #include <stdarg.h>
> #include <inttypes.h>
>
> +#include <xen-tools/bitops.h>
It doesn't looks like xg_dom_elfloader.c is using anything from
bitops.h. The last use of it was probably removed in ed04ca95981f
("libelf: rewrite symtab/strtab loading")
> +
> #include "xg_private.h"
> -#include "xc_bitops.h"
>
> #define XEN_VER "xen-3.0"
>
> diff --git a/tools/libs/guest/xg_dom_hvmloader.c b/tools/libs/guest/xg_dom_hvmloader.c
> index 39e1e5f579a7..0f569c20c522 100644
> --- a/tools/libs/guest/xg_dom_hvmloader.c
> +++ b/tools/libs/guest/xg_dom_hvmloader.c
> @@ -24,8 +24,9 @@
> #include <inttypes.h>
> #include <assert.h>
>
> +#include <xen-tools/bitops.h>
> +
I think there's two reason to remove this include:
- it doesn't looks like xg_dom_hvmloader.c is using any macro from it.
- bitops.h is already included by xg_private.h.
> #include "xg_private.h"
> -#include "xc_bitops.h"
>
> /* ------------------------------------------------------------------------ */
> /* parse elf binary */
> diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> index 2f058ee3a6ff..2e583f2eac72 100644
> --- a/tools/libs/guest/xg_sr_common.h
> +++ b/tools/libs/guest/xg_sr_common.h
> @@ -2,11 +2,10 @@
> #define __COMMON__H
>
> #include <stdbool.h>
> +#include <xen-tools/bitops.h>
It's already included in xg_private.h, so this feels unnecessary.
>
> #include "xg_private.h"
> #include "xg_save_restore.h"
> -#include "xc_bitops.h"
> -
If the extraneous includes could be removed, that would be nice. In
anycase:
Acked-by: Anthony PERARD <anthony.perard@vates.tech>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v16 2/4] tools/include: move xc_bitops.h to xen-tools/bitops.h
2025-08-25 9:30 ` Anthony PERARD
@ 2025-08-26 9:28 ` dmkhn
0 siblings, 0 replies; 19+ messages in thread
From: dmkhn @ 2025-08-26 9:28 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, andrew.cooper3, anthony.perard, jbeulich, julien,
michal.orzel, roger.pau, sstabellini, dmukhin
On Mon, Aug 25, 2025 at 11:30:20AM +0200, Anthony PERARD wrote:
> On Tue, Aug 12, 2025 at 10:30:45PM +0000, dmkhn@proton.me wrote:
> > diff --git a/tools/libs/guest/xg_dom_elfloader.c b/tools/libs/guest/xg_dom_elfloader.c
> > index f17930d98bf7..8531e90f8e21 100644
> > --- a/tools/libs/guest/xg_dom_elfloader.c
> > +++ b/tools/libs/guest/xg_dom_elfloader.c
> > @@ -25,8 +25,9 @@
> > #include <stdarg.h>
> > #include <inttypes.h>
> >
> > +#include <xen-tools/bitops.h>
>
> It doesn't looks like xg_dom_elfloader.c is using anything from
> bitops.h. The last use of it was probably removed in ed04ca95981f
> ("libelf: rewrite symtab/strtab loading")
Ack.
>
> > +
> > #include "xg_private.h"
> > -#include "xc_bitops.h"
> >
> > #define XEN_VER "xen-3.0"
> >
> > diff --git a/tools/libs/guest/xg_dom_hvmloader.c b/tools/libs/guest/xg_dom_hvmloader.c
> > index 39e1e5f579a7..0f569c20c522 100644
> > --- a/tools/libs/guest/xg_dom_hvmloader.c
> > +++ b/tools/libs/guest/xg_dom_hvmloader.c
> > @@ -24,8 +24,9 @@
> > #include <inttypes.h>
> > #include <assert.h>
> >
> > +#include <xen-tools/bitops.h>
> > +
>
> I think there's two reason to remove this include:
> - it doesn't looks like xg_dom_hvmloader.c is using any macro from it.
> - bitops.h is already included by xg_private.h.
Will remove.
>
>
> > #include "xg_private.h"
> > -#include "xc_bitops.h"
> >
> > /* ------------------------------------------------------------------------ */
> > /* parse elf binary */
> > diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> > index 2f058ee3a6ff..2e583f2eac72 100644
> > --- a/tools/libs/guest/xg_sr_common.h
> > +++ b/tools/libs/guest/xg_sr_common.h
> > @@ -2,11 +2,10 @@
> > #define __COMMON__H
> >
> > #include <stdbool.h>
> > +#include <xen-tools/bitops.h>
>
> It's already included in xg_private.h, so this feels unnecessary.
Ack.
> >
> > #include "xg_private.h"
> > #include "xg_save_restore.h"
> > -#include "xc_bitops.h"
> > -
>
>
> If the extraneous includes could be removed, that would be nice. In
> anycase:
> Acked-by: Anthony PERARD <anthony.perard@vates.tech>
Thanks
>
> Thanks,
>
> --
> Anthony PERARD
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v16 3/4] tools/tests: introduce unit tests for domain ID allocator
2025-08-12 22:30 [PATCH v16 0/4] xen/domain: domain ID allocation dmkhn
2025-08-12 22:30 ` [PATCH v16 1/4] xen/domain: unify " dmkhn
2025-08-12 22:30 ` [PATCH v16 2/4] tools/include: move xc_bitops.h to xen-tools/bitops.h dmkhn
@ 2025-08-12 22:30 ` dmkhn
2025-08-25 13:47 ` Anthony PERARD
2025-08-12 22:30 ` [PATCH v16 4/4] xen/domain: update create_dom0() messages dmkhn
3 siblings, 1 reply; 19+ messages in thread
From: dmkhn @ 2025-08-12 22:30 UTC (permalink / raw)
To: xen-devel
Cc: andrew.cooper3, anthony.perard, jbeulich, julien, michal.orzel,
roger.pau, sstabellini, dmukhin, Julien Grall
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.
Use <xen-tools/bitops.h> and xen/lib/find-next-bit.c in test hardness code.
Adjust find-next-bit.c to be compiled with __XEN_TOOLS__.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
Changes since v15:
- fixed generating harness dependencies on the fly during the test build
- kept Julien's R-b
---
tools/include/xen-tools/bitops.h | 10 ++++
tools/tests/Makefile | 1 +
tools/tests/domid/.gitignore | 3 ++
tools/tests/domid/Makefile | 84 +++++++++++++++++++++++++++++
tools/tests/domid/harness.h | 54 +++++++++++++++++++
tools/tests/domid/test-domid.c | 93 ++++++++++++++++++++++++++++++++
xen/lib/find-next-bit.c | 5 ++
7 files changed, 250 insertions(+)
create mode 100644 tools/tests/domid/.gitignore
create mode 100644 tools/tests/domid/Makefile
create mode 100644 tools/tests/domid/harness.h
create mode 100644 tools/tests/domid/test-domid.c
diff --git a/tools/include/xen-tools/bitops.h b/tools/include/xen-tools/bitops.h
index 681482f6759f..3b98fba6d74c 100644
--- a/tools/include/xen-tools/bitops.h
+++ b/tools/include/xen-tools/bitops.h
@@ -12,6 +12,16 @@
#define BITS_PER_LONG 32
#endif
+#define ffsl(x) __builtin_ffsl(x)
+
+#define BIT_WORD(nr) ((nr) / 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)]
+
#define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr) / 8]
#define BITMAP_SHIFT(_nr) ((_nr) % 8)
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index 97ba2a13894d..ac5737364623 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -2,6 +2,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..0e02715159c2
--- /dev/null
+++ b/tools/tests/domid/.gitignore
@@ -0,0 +1,3 @@
+*.o
+generated
+test-domid
diff --git a/tools/tests/domid/Makefile b/tools/tests/domid/Makefile
new file mode 100644
index 000000000000..0a124a8bfc76
--- /dev/null
+++ b/tools/tests/domid/Makefile
@@ -0,0 +1,84 @@
+# 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
+
+strip-list = $(sort $(strip $(foreach x,$(1),$(strip $(x)))))
+
+define list-c-headers
+$(shell sed -n -r \
+ 's/^[ \t]*# *include[ \t]*[<"]([^">]+)[">].*/\1/p' $(1) 2>/dev/null)
+endef
+
+define emit-harness-nested-rule
+$(1): $(CURDIR)/harness.h
+ mkdir -p $$(dir $$@)
+ ln -sf $$^ $$@
+endef
+
+define emit-harness-rules
+ifneq ($(strip $(3)),)
+$(foreach h,$(3),$(call emit-harness-nested-rule,$(CURDIR)/generated/$(h)))
+vpath $(1) $(2)
+$(1:.c=.o): $(addprefix $(CURDIR)/generated/,$(3))
+endif
+endef
+
+define vpath-with-harness-deps
+$(call emit-harness-rules,$(1),$(2),\
+ $(call strip-list,$(call list-c-headers,$(2)$(1))))
+endef
+
+.PHONY: all
+all: $(TESTS)
+
+.PHONY: run
+run: $(TESTS)
+ $(foreach t,$(TESTS),./$(t);)
+
+.PHONY: clean
+clean:
+ $(RM) -rf $(CURDIR)/generated
+ $(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__
+# find-next-bit.c
+CFLAGS += '-DEXPORT_SYMBOL(x)=' \
+ -Dfind_first_bit \
+ -Dfind_first_zero_bit \
+ -Dfind_next_bit \
+ -Dfind_next_bit_le \
+ -Dfind_next_zero_bit_le
+CFLAGS += $(APPEND_CFLAGS)
+CFLAGS += $(CFLAGS_xeninclude)
+CFLAGS += -I./generated/
+
+LDFLAGS += $(APPEND_LDFLAGS)
+
+vpath find-next-bit.c $(XEN_ROOT)/xen/lib/
+# Ubuntu {16,18}.04 need single eval at the call site.
+$(eval $(call vpath-with-harness-deps,domid.c,$(XEN_ROOT)/xen/common/))
+
+test-domid: domid.o find-next-bit.o test-domid.o
+ $(CC) $^ -o $@ $(LDFLAGS)
+
+-include $(DEPS_INCLUDE)
diff --git a/tools/tests/domid/harness.h b/tools/tests/domid/harness.h
new file mode 100644
index 000000000000..17eb22a9a854
--- /dev/null
+++ b/tools/tests/domid/harness.h
@@ -0,0 +1,54 @@
+/* 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>
+#include <xen-tools/bitops.h>
+
+typedef bool spinlock_t;
+typedef uint16_t domid_t;
+
+extern domid_t domid_alloc(domid_t domid);
+extern void domid_free(domid_t domid);
+
+extern unsigned long find_next_zero_bit(const unsigned long *addr,
+ unsigned long size,
+ unsigned long offset);
+
+#define __test_and_set_bit(nr, addr) test_and_set_bit(nr, addr)
+#define __test_and_clear_bit(nr, addr) test_and_clear_bit(nr, addr)
+#define __set_bit(nr, addr) set_bit(nr, addr)
+
+#define BUG_ON(x) assert(!(x))
+#define ASSERT(x) assert(x)
+
+#define DEFINE_SPINLOCK(l) spinlock_t l
+#define spin_lock(l) (assert(!*(l)), *(l) = true)
+#define spin_unlock(l) (assert(*(l)), *(l) = false)
+
+#define printk printf
+
+#define DOMID_FIRST_RESERVED (100)
+#define DOMID_INVALID (101)
+
+#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..51a88a6a9550
--- /dev/null
+++ b/tools/tests/domid/test-domid.c
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Unit tests for domain ID allocator.
+ *
+ * Copyright 2025 Ford Motor Company
+ */
+
+#include "harness.h"
+
+#define verify(exp, fmt, args...) do { \
+ if ( !(exp) ) \
+ printf(fmt, ## args); \
+ assert(exp); \
+} while (0);
+
+/*
+ * Fail on the first error, since tests are dependent on each other.
+ */
+int main(int argc, char **argv)
+{
+ domid_t expected, allocated;
+
+ /* Test ID cannot be allocated twice. */
+ for ( expected = 0; expected < DOMID_FIRST_RESERVED; expected++ )
+ {
+ allocated = domid_alloc(expected);
+ verify(allocated == expected,
+ "TEST 1: expected %u allocated %u\n", expected, allocated);
+ }
+ for ( expected = 0; expected < DOMID_FIRST_RESERVED; expected++ )
+ {
+ allocated = domid_alloc(expected);
+ verify(allocated == DOMID_INVALID,
+ "TEST 2: expected %u allocated %u\n", DOMID_INVALID, allocated);
+ }
+
+ /* Ensure all IDs, including ID#0 are not allocated. */
+ for ( expected = 0; expected < DOMID_FIRST_RESERVED; expected++ )
+ domid_free(expected);
+
+ /*
+ * 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);
+ verify(allocated == expected,
+ "TEST 3: expected %u allocated %u\n", expected, allocated);
+ }
+ for ( expected = 1; expected < DOMID_FIRST_RESERVED; expected++ )
+ {
+ allocated = domid_alloc(DOMID_INVALID);
+ verify(allocated == DOMID_INVALID,
+ "TEST 4: expected %u allocated %u\n", DOMID_INVALID, allocated);
+ }
+
+ /* Re-allocate first ID from [1..DOMID_FIRST_RESERVED/2]. */
+ for ( expected = 1; expected < DOMID_FIRST_RESERVED / 2; expected++ )
+ domid_free(expected);
+ for ( expected = 1; expected < DOMID_FIRST_RESERVED / 2; expected++ )
+ {
+ allocated = domid_alloc(DOMID_INVALID);
+ verify(allocated == expected,
+ "TEST 5: expected %u allocated %u\n", expected, allocated);
+ }
+
+ /* 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);
+ verify(allocated == expected,
+ "TEST 6: expected %u allocated %u\n", expected, allocated);
+
+ /* Allocate an invalid ID. */
+ expected = DOMID_INVALID;
+ allocated = domid_alloc(DOMID_FIRST_RESERVED);
+ verify(allocated == expected,
+ "TEST 7: expected %u allocated %u\n", expected, allocated);
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/lib/find-next-bit.c b/xen/lib/find-next-bit.c
index 9b8d7814f20c..539c7f2022b0 100644
--- a/xen/lib/find-next-bit.c
+++ b/xen/lib/find-next-bit.c
@@ -8,8 +8,13 @@
* as published by the Free Software Foundation; either version
* 2 of the License, or (at your option) any later version.
*/
+
+#ifdef __XEN_TOOLS__
+#include <xen-tools/bitops.h>
+#else
#include <xen/bitops.h>
#include <xen/byteorder.h>
+#endif
#define __ffs(x) (ffsl(x) - 1)
#define ffz(x) __ffs(~(x))
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v16 3/4] tools/tests: introduce unit tests for domain ID allocator
2025-08-12 22:30 ` [PATCH v16 3/4] tools/tests: introduce unit tests for domain ID allocator dmkhn
@ 2025-08-25 13:47 ` Anthony PERARD
2025-08-26 9:48 ` dmkhn
0 siblings, 1 reply; 19+ messages in thread
From: Anthony PERARD @ 2025-08-25 13:47 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, andrew.cooper3, anthony.perard, jbeulich, julien,
michal.orzel, roger.pau, sstabellini, dmukhin, Julien Grall
On Tue, Aug 12, 2025 at 10:30:50PM +0000, dmkhn@proton.me wrote:
> diff --git a/tools/tests/domid/.gitignore b/tools/tests/domid/.gitignore
> new file mode 100644
> index 000000000000..0e02715159c2
> --- /dev/null
> +++ b/tools/tests/domid/.gitignore
> @@ -0,0 +1,3 @@
> +*.o
"*.o" is already in the .gitignore at the root of the project. I don't
think it's useful here.
> +generated
> +test-domid
> diff --git a/tools/tests/domid/Makefile b/tools/tests/domid/Makefile
> new file mode 100644
> index 000000000000..0a124a8bfc76
> --- /dev/null
> +++ b/tools/tests/domid/Makefile
> @@ -0,0 +1,84 @@
> +# 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
> +
> +strip-list = $(sort $(strip $(foreach x,$(1),$(strip $(x)))))
What's that macro for? Also, what's a "list"?
> +define list-c-headers
> +$(shell sed -n -r \
Could you use "-E" instead of "-r"? (-r might not work on FreeBSD)
> + 's/^[ \t]*# *include[ \t]*[<"]([^">]+)[">].*/\1/p' $(1) 2>/dev/null)
> +endef
> +
> +define emit-harness-nested-rule
> +$(1): $(CURDIR)/harness.h
> + mkdir -p $$(dir $$@)
You can use $(@D) instead of $(dir $@). The only difference is a /
not present at the end.
> + ln -sf $$^ $$@
This should use $<, I don't think the command is going to work if
there's multiple prerequisite.
> +endef
> +
> +define emit-harness-rules
> +ifneq ($(strip $(3)),)
How many time do you need to call $(strip) ?
Also, I think I would prefer to have $(if $(strip $(3)), [the rest])
rather than actually evaluating code and generating code that we already
know is isn't going to be executed.
> +$(foreach h,$(3),$(call emit-harness-nested-rule,$(CURDIR)/generated/$(h)))
> +vpath $(1) $(2)
> +$(1:.c=.o): $(addprefix $(CURDIR)/generated/,$(3))
> +endif
> +endef
This macro fails if there's more than one "#include" in "domid.c".
And if there's no "#include" in "domid.c", then Make doesn't know how to
make "domid.o" for "test-domid".
> +
> +define vpath-with-harness-deps
> +$(call emit-harness-rules,$(1),$(2),\
> + $(call strip-list,$(call list-c-headers,$(2)$(1))))
> +endef
> +
> +.PHONY: all
> +all: $(TESTS)
> +
> +.PHONY: run
> +run: $(TESTS)
> + $(foreach t,$(TESTS),./$(t);)
This recipe doesn't work as expected. You need `set -e` or only the last
tests count.
> +
> +.PHONY: clean
> +clean:
> + $(RM) -rf $(CURDIR)/generated
$(RM) already contain the '-f' option, no need to add it a second time.
Also, we expected Make to run all commands in recipe from $(CURDIR), so
adding $(CURDIR) is unnecessary, could potentially be an issue.
> + $(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__
> +# find-next-bit.c
> +CFLAGS += '-DEXPORT_SYMBOL(x)=' \
> + -Dfind_first_bit \
> + -Dfind_first_zero_bit \
> + -Dfind_next_bit \
> + -Dfind_next_bit_le \
> + -Dfind_next_zero_bit_le
> +CFLAGS += $(APPEND_CFLAGS)
> +CFLAGS += $(CFLAGS_xeninclude)
> +CFLAGS += -I./generated/
> +
> +LDFLAGS += $(APPEND_LDFLAGS)
> +
> +vpath find-next-bit.c $(XEN_ROOT)/xen/lib/
> +# Ubuntu {16,18}.04 need single eval at the call site.
I'd rather see a comment about what's the macro is about rather that a
comment some Linux distribution. Our target is GNU Make 3.80, without
regards to a particular distribution. (Also I don't think it's useful to
point out that `eval` is needed for older version of Make, at least in
our project.)
> +$(eval $(call vpath-with-harness-deps,domid.c,$(XEN_ROOT)/xen/common/))
> +
> +test-domid: domid.o find-next-bit.o test-domid.o
> + $(CC) $^ -o $@ $(LDFLAGS)
> +
> +-include $(DEPS_INCLUDE)
> diff --git a/tools/tests/domid/test-domid.c b/tools/tests/domid/test-domid.c
> new file mode 100644
> index 000000000000..51a88a6a9550
> --- /dev/null
> +++ b/tools/tests/domid/test-domid.c
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for domain ID allocator.
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +#include "harness.h"
> +
> +#define verify(exp, fmt, args...) do { \
> + if ( !(exp) ) \
> + printf(fmt, ## args); \
> + assert(exp); \
Relying on assert() for the test isn't wise. It's useful for developing
and debugging because it calls abort(), but they can easily be get rid of,
by simply building with -DNDEBUG. Could you maybe replace it with exit()
since you already check the condition?
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v16 3/4] tools/tests: introduce unit tests for domain ID allocator
2025-08-25 13:47 ` Anthony PERARD
@ 2025-08-26 9:48 ` dmkhn
0 siblings, 0 replies; 19+ messages in thread
From: dmkhn @ 2025-08-26 9:48 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, andrew.cooper3, anthony.perard, jbeulich, julien,
michal.orzel, roger.pau, sstabellini, dmukhin, Julien Grall
On Mon, Aug 25, 2025 at 03:47:39PM +0200, Anthony PERARD wrote:
Thanks for review!
Will address in the next revision.
Please see some responses below.
> On Tue, Aug 12, 2025 at 10:30:50PM +0000, dmkhn@proton.me wrote:
> > diff --git a/tools/tests/domid/.gitignore b/tools/tests/domid/.gitignore
> > new file mode 100644
> > index 000000000000..0e02715159c2
> > --- /dev/null
> > +++ b/tools/tests/domid/.gitignore
> > @@ -0,0 +1,3 @@
> > +*.o
>
> "*.o" is already in the .gitignore at the root of the project. I don't
> think it's useful here.
Ack.
>
> > +generated
> > +test-domid
> > diff --git a/tools/tests/domid/Makefile b/tools/tests/domid/Makefile
> > new file mode 100644
> > index 000000000000..0a124a8bfc76
> > --- /dev/null
> > +++ b/tools/tests/domid/Makefile
> > @@ -0,0 +1,84 @@
> > +# 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
> > +
> > +strip-list = $(sort $(strip $(foreach x,$(1),$(strip $(x)))))
>
> What's that macro for? Also, what's a "list"?
>
> > +define list-c-headers
> > +$(shell sed -n -r \
>
> Could you use "-E" instead of "-r"? (-r might not work on FreeBSD)
re: FreeBSD: I've found there's a dedicated pipeline for Xen on FreeBSD
(cirrus CI), but I did not figure how to trigger it, will appreciate help
with that.
>
> > + 's/^[ \t]*# *include[ \t]*[<"]([^">]+)[">].*/\1/p' $(1) 2>/dev/null)
> > +endef
> > +
> > +define emit-harness-nested-rule
> > +$(1): $(CURDIR)/harness.h
> > + mkdir -p $$(dir $$@)
>
> You can use $(@D) instead of $(dir $@). The only difference is a /
> not present at the end.
>
> > + ln -sf $$^ $$@
>
> This should use $<, I don't think the command is going to work if
> there's multiple prerequisite.
>
> > +endef
> > +
> > +define emit-harness-rules
> > +ifneq ($(strip $(3)),)
>
> How many time do you need to call $(strip) ?
> Also, I think I would prefer to have $(if $(strip $(3)), [the rest])
> rather than actually evaluating code and generating code that we already
> know is isn't going to be executed.
>
> > +$(foreach h,$(3),$(call emit-harness-nested-rule,$(CURDIR)/generated/$(h)))
> > +vpath $(1) $(2)
> > +$(1:.c=.o): $(addprefix $(CURDIR)/generated/,$(3))
> > +endif
> > +endef
>
> This macro fails if there's more than one "#include" in "domid.c".
>
> And if there's no "#include" in "domid.c", then Make doesn't know how to
> make "domid.o" for "test-domid".
>
> > +
> > +define vpath-with-harness-deps
> > +$(call emit-harness-rules,$(1),$(2),\
> > + $(call strip-list,$(call list-c-headers,$(2)$(1))))
> > +endef
> > +
> > +.PHONY: all
> > +all: $(TESTS)
> > +
> > +.PHONY: run
> > +run: $(TESTS)
> > + $(foreach t,$(TESTS),./$(t);)
>
> This recipe doesn't work as expected. You need `set -e` or only the last
> tests count.
>
> > +
> > +.PHONY: clean
> > +clean:
> > + $(RM) -rf $(CURDIR)/generated
>
> $(RM) already contain the '-f' option, no need to add it a second time.
>
> Also, we expected Make to run all commands in recipe from $(CURDIR), so
> adding $(CURDIR) is unnecessary, could potentially be an issue.
>
> > + $(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__
> > +# find-next-bit.c
> > +CFLAGS += '-DEXPORT_SYMBOL(x)=' \
> > + -Dfind_first_bit \
> > + -Dfind_first_zero_bit \
> > + -Dfind_next_bit \
> > + -Dfind_next_bit_le \
> > + -Dfind_next_zero_bit_le
> > +CFLAGS += $(APPEND_CFLAGS)
> > +CFLAGS += $(CFLAGS_xeninclude)
> > +CFLAGS += -I./generated/
> > +
> > +LDFLAGS += $(APPEND_LDFLAGS)
> > +
> > +vpath find-next-bit.c $(XEN_ROOT)/xen/lib/
> > +# Ubuntu {16,18}.04 need single eval at the call site.
>
> I'd rather see a comment about what's the macro is about rather that a
> comment some Linux distribution. Our target is GNU Make 3.80, without
> regards to a particular distribution. (Also I don't think it's useful to
> point out that `eval` is needed for older version of Make, at least in
> our project.)
Ack.
>
> > +$(eval $(call vpath-with-harness-deps,domid.c,$(XEN_ROOT)/xen/common/))
> > +
> > +test-domid: domid.o find-next-bit.o test-domid.o
> > + $(CC) $^ -o $@ $(LDFLAGS)
> > +
> > +-include $(DEPS_INCLUDE)
> > diff --git a/tools/tests/domid/test-domid.c b/tools/tests/domid/test-domid.c
> > new file mode 100644
> > index 000000000000..51a88a6a9550
> > --- /dev/null
> > +++ b/tools/tests/domid/test-domid.c
> > @@ -0,0 +1,93 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Unit tests for domain ID allocator.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +#include "harness.h"
> > +
> > +#define verify(exp, fmt, args...) do { \
> > + if ( !(exp) ) \
> > + printf(fmt, ## args); \
> > + assert(exp); \
>
> Relying on assert() for the test isn't wise. It's useful for developing
> and debugging because it calls abort(), but they can easily be get rid of,
> by simply building with -DNDEBUG. Could you maybe replace it with exit()
> since you already check the condition?
Yep, will do.
>
>
> Thanks,
>
> --
> Anthony PERARD
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v16 4/4] xen/domain: update create_dom0() messages
2025-08-12 22:30 [PATCH v16 0/4] xen/domain: domain ID allocation dmkhn
` (2 preceding siblings ...)
2025-08-12 22:30 ` [PATCH v16 3/4] tools/tests: introduce unit tests for domain ID allocator dmkhn
@ 2025-08-12 22:30 ` dmkhn
3 siblings, 0 replies; 19+ messages in thread
From: dmkhn @ 2025-08-12 22:30 UTC (permalink / raw)
To: xen-devel
Cc: andrew.cooper3, anthony.perard, jbeulich, julien, michal.orzel,
roger.pau, sstabellini, dmukhin, Alejandro Vallejo, Julien Grall
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>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
Changes since v15:
- 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 aca35b8961d6..670f634b8b0d 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 398da734c0c5..bf21c55f7193 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] 19+ messages in thread