From: Denis Mukhin <dmkhn@proton.me>
From: Denis Mukhin <dmukhin@ford.com>
Currently, hypervisor code has two different domain ID allocation
implementations:
(a) Sequential IDs allocation in dom0less Arm code based on max_init_domid;
(b) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
max_init_domid (both Arm and x86).
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.
Wrap the domain ID allocation as an arch-independent function domid_alloc() in
common/domain.c based on the bitmap.
Allocation algorithm:
- If an explicit domain ID is provided, verify its availability and use it if
ID is not used;
- If DOMID_INVALID is provided, search the range [0..DOMID_FIRST_RESERVED-1],
starting from the last used ID and wrapping around as needed. It guarantees
that two consecutive calls will never return the same ID. ID#0 is excluded
to account for late hwdom case.
Also, remove is_free_domid() helper as it is not needed now.
No functional change intended.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v8:
- skip ID #0 in domid_alloc() to account for late hwdom
---
xen/arch/arm/domain_build.c | 17 +++++---
xen/arch/x86/setup.c | 11 +++--
xen/common/device-tree/dom0less-build.c | 10 +++--
xen/common/domain.c | 54 +++++++++++++++++++++++++
xen/common/domctl.c | 42 +++----------------
xen/include/xen/domain.h | 3 ++
6 files changed, 87 insertions(+), 50 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b189a7cfae..e9d563c269 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2010,6 +2010,7 @@ void __init create_dom0(void)
.grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
};
unsigned int flags = CDF_privileged | CDF_hardware;
+ domid_t domid;
int rc;
/* The vGIC for DOM0 is exactly emulating the hardware GIC */
@@ -2034,19 +2035,25 @@ void __init create_dom0(void)
if ( !llc_coloring_enabled )
flags |= CDF_directmap;
- dom0 = domain_create(0, &dom0_cfg, flags);
+ domid = domid_alloc(0);
+ if ( domid == DOMID_INVALID )
+ panic("Error allocating domain ID 0\n");
+
+ dom0 = domain_create(domid, &dom0_cfg, flags);
if ( IS_ERR(dom0) )
- panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
+ panic("Error creating domain %d (rc = %ld)\n", domid, PTR_ERR(dom0));
if ( llc_coloring_enabled && (rc = dom0_set_llc_colors(dom0)) )
- panic("Error initializing LLC coloring for domain 0 (rc = %d)\n", rc);
+ panic("Error initializing LLC coloring for domain %pd (rc = %d)\n",
+ dom0, rc);
if ( alloc_dom0_vcpu0(dom0) == NULL )
- panic("Error creating domain 0 vcpu0\n");
+ panic("Error creating domain %pdv0\n", dom0);
rc = construct_dom0(dom0);
if ( rc )
- panic("Could not set up DOM0 guest OS (rc = %d)\n", rc);
+ panic("Could not set up guest OS for domain %pd (rc = %d)\n",
+ dom0, rc);
set_xs_domain(dom0);
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1f5cb67bd0..b36ce4491b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1031,8 +1031,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) )
@@ -1064,7 +1067,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( (strlen(acpi_param) == 0) && acpi_disabled )
{
- printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
+ printk("ACPI is disabled, notifying domain %pd (acpi=off)\n", d);
safe_strcpy(acpi_param, "off");
}
@@ -1079,7 +1082,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
bd->d = d;
if ( construct_dom0(bd) != 0 )
- panic("Could not construct domain 0\n");
+ panic("Could not construct domain %pd\n", d);
bd->cmdline = NULL;
xfree(cmdline);
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 39cb2cd5c7..a509f8fecd 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -850,15 +850,13 @@ void __init create_domUs(void)
struct xen_domctl_createdomain d_cfg = {0};
unsigned int flags = 0U;
bool has_dtb = false;
+ domid_t domid;
uint32_t val;
int rc;
if ( !dt_device_is_compatible(node, "xen,domain") )
continue;
- if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
- panic("No more domain IDs available\n");
-
d_cfg.max_evtchn_port = 1023;
d_cfg.max_grant_frames = -1;
d_cfg.max_maptrack_frames = -1;
@@ -981,7 +979,11 @@ void __init create_domUs(void)
* very important to use the pre-increment operator to call
* domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
*/
- d = domain_create(++max_init_domid, &d_cfg, flags);
+ domid = domid_alloc(++max_init_domid);
+ if ( domid == DOMID_INVALID )
+ panic("Error allocating ID for domain %s\n", dt_node_name(node));
+
+ d = domain_create(domid, &d_cfg, flags);
if ( IS_ERR(d) )
panic("Error creating domain %s (rc = %ld)\n",
dt_node_name(node), PTR_ERR(d));
diff --git a/xen/common/domain.c b/xen/common/domain.c
index abf1969e60..ae0c44fcbb 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -66,6 +66,10 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
static struct domain *domain_hash[DOMAIN_HASH_SIZE];
struct domain *domain_list;
+/* Non-system domain ID allocator. */
+static DEFINE_SPINLOCK(domid_lock);
+static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
+
/*
* Insert a domain into the domlist/hash. This allows the domain to be looked
* up by domid, and therefore to be the subject of hypercalls/etc.
@@ -1449,6 +1453,8 @@ void domain_destroy(struct domain *d)
TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
+ domid_free(d->domain_id);
+
/* Remove from the domlist/hash. */
domlist_remove(d);
@@ -2405,6 +2411,54 @@ domid_t get_initial_domain_id(void)
return hardware_domid;
}
+domid_t domid_alloc(domid_t domid)
+{
+ spin_lock(&domid_lock);
+
+ if ( domid < DOMID_FIRST_RESERVED )
+ {
+ if ( __test_and_set_bit(domid, domid_bitmap) )
+ domid = DOMID_INVALID;
+ }
+ else
+ {
+ static domid_t domid_last;
+ /* NB: account for late hwdom case, skip ID#0 */
+ const domid_t reserved_domid = 0;
+ const bool reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
+
+ domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
+ domid_last);
+
+ if ( domid == DOMID_FIRST_RESERVED )
+ domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
+
+ if ( domid == DOMID_FIRST_RESERVED )
+ {
+ domid = DOMID_INVALID;
+ }
+ else
+ {
+ __set_bit(domid, domid_bitmap);
+ domid_last = domid;
+ }
+
+ if ( !reserved )
+ __clear_bit(reserved_domid, domid_bitmap);
+ }
+
+ spin_unlock(&domid_lock);
+
+ return domid;
+}
+
+void domid_free(domid_t domid)
+{
+ spin_lock(&domid_lock);
+ __clear_bit(domid, domid_bitmap);
+ spin_unlock(&domid_lock);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index bfe2e1f9f0..8ef0c147c9 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -49,20 +49,6 @@ static int xenctl_bitmap_to_nodemask(nodemask_t *nodemask,
MAX_NUMNODES);
}
-static inline int is_free_domid(domid_t dom)
-{
- struct domain *d;
-
- if ( dom >= DOMID_FIRST_RESERVED )
- return 0;
-
- if ( (d = rcu_lock_domain_by_id(dom)) == NULL )
- return 1;
-
- rcu_unlock_domain(d);
- return 0;
-}
-
void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
{
struct vcpu *v;
@@ -421,36 +407,18 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
case XEN_DOMCTL_createdomain:
{
- domid_t dom;
- static domid_t rover = 0;
+ domid_t domid = domid_alloc(op->domain);
- dom = op->domain;
- if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
+ if ( domid == DOMID_INVALID )
{
ret = -EEXIST;
- if ( !is_free_domid(dom) )
- break;
- }
- else
- {
- for ( dom = rover + 1; dom != rover; dom++ )
- {
- if ( dom == DOMID_FIRST_RESERVED )
- dom = 1;
- if ( is_free_domid(dom) )
- break;
- }
-
- ret = -ENOMEM;
- if ( dom == rover )
- break;
-
- rover = dom;
+ break;
}
- d = domain_create(dom, &op->u.createdomain, false);
+ d = domain_create(domid, &op->u.createdomain, false);
if ( IS_ERR(d) )
{
+ domid_free(domid);
ret = PTR_ERR(d);
d = NULL;
break;
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index e10baf2615..8aab05ae93 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -38,6 +38,9 @@ void arch_get_domain_info(const struct domain *d,
domid_t get_initial_domain_id(void);
+domid_t domid_alloc(domid_t domid);
+void domid_free(domid_t domid);
+
/* CDF_* constant. Internal flags for domain creation. */
/* Is this a privileged domain? */
#define CDF_privileged (1U << 0)
--
2.34.1On 29.05.2025 00:50, dmkhn@proton.me wrote:
> @@ -1079,7 +1082,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> - panic("Could not construct domain 0\n");
> + panic("Could not construct domain %pd\n", d);
Just one nit I only noticed when reading Julien's reply: The word "domain" is
now becoming redundant here, and hence would imo better be dropped.
Jan
Hi Denis,
On 28/05/2025 23:50, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmkhn@proton.me>
>
> From: Denis Mukhin <dmukhin@ford.com>
>
> Currently, hypervisor code has two different domain ID allocation
> implementations:
>
> (a) Sequential IDs allocation in dom0less Arm code based on max_init_domid;
>
> (b) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
> max_init_domid (both Arm and x86).
>
> 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.
>
> Wrap the domain ID allocation as an arch-independent function domid_alloc() in
> common/domain.c based on the bitmap.
>
> Allocation algorithm:
> - If an explicit domain ID is provided, verify its availability and use it if
> ID is not used;
> - If DOMID_INVALID is provided, search the range [0..DOMID_FIRST_RESERVED-1],
> starting from the last used ID and wrapping around as needed. It guarantees
> that two consecutive calls will never return the same ID. ID#0 is excluded
> to account for late hwdom case.
>
> Also, remove is_free_domid() helper as it is not needed now.
>
> No functional change intended.
>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v8:
> - skip ID #0 in domid_alloc() to account for late hwdom
> ---
> xen/arch/arm/domain_build.c | 17 +++++---
> xen/arch/x86/setup.c | 11 +++--
> xen/common/device-tree/dom0less-build.c | 10 +++--
> xen/common/domain.c | 54 +++++++++++++++++++++++++
> xen/common/domctl.c | 42 +++----------------
> xen/include/xen/domain.h | 3 ++
> 6 files changed, 87 insertions(+), 50 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b189a7cfae..e9d563c269 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2010,6 +2010,7 @@ void __init create_dom0(void)
> .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
> };
> unsigned int flags = CDF_privileged | CDF_hardware;
> + domid_t domid;
> int rc;
>
> /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> @@ -2034,19 +2035,25 @@ void __init create_dom0(void)
> if ( !llc_coloring_enabled )
> flags |= CDF_directmap;
>
> - dom0 = domain_create(0, &dom0_cfg, flags);
> + domid = domid_alloc(0);
> + if ( domid == DOMID_INVALID )
> + panic("Error allocating domain ID 0\n");
> +
> + dom0 = domain_create(domid, &dom0_cfg, flags);
> if ( IS_ERR(dom0) )
> - panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
> + panic("Error creating domain %d (rc = %ld)\n", domid, PTR_ERR(dom0));
The change in the panic here and below seems unrelated to the goal of
the patch. I am ok to keep them here, but I think it should be mentioned
in the commit message.
>
> if ( llc_coloring_enabled && (rc = dom0_set_llc_colors(dom0)) )
> - panic("Error initializing LLC coloring for domain 0 (rc = %d)\n", rc);
> + panic("Error initializing LLC coloring for domain %pd (rc = %d)\n",
> + dom0, rc);
>
> if ( alloc_dom0_vcpu0(dom0) == NULL )
> - panic("Error creating domain 0 vcpu0\n");
> + panic("Error creating domain %pdv0\n", dom0);
>
> rc = construct_dom0(dom0);
> if ( rc )
> - panic("Could not set up DOM0 guest OS (rc = %d)\n", rc);
> + panic("Could not set up guest OS for domain %pd (rc = %d)\n",
> + dom0, rc);
>
> set_xs_domain(dom0);
> }
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 1f5cb67bd0..b36ce4491b 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1031,8 +1031,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. */
NIT: The two spaces were valid here. This is in fact quite common to
unambiguously mark the end of a sentence.
> + 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) )
> @@ -1064,7 +1067,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>
> if ( (strlen(acpi_param) == 0) && acpi_disabled )
> {
> - printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> + printk("ACPI is disabled, notifying domain %pd (acpi=off)\n", d);
> safe_strcpy(acpi_param, "off");
> }
>
> @@ -1079,7 +1082,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> - panic("Could not construct domain 0\n");
> + panic("Could not construct domain %pd\n", d);
>
> bd->cmdline = NULL;
> xfree(cmdline);
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 39cb2cd5c7..a509f8fecd 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -850,15 +850,13 @@ void __init create_domUs(void)
> struct xen_domctl_createdomain d_cfg = {0};
> unsigned int flags = 0U;
> bool has_dtb = false;
> + domid_t domid;
> uint32_t val;
> int rc;
>
> if ( !dt_device_is_compatible(node, "xen,domain") )
> continue;
>
> - if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
I can't find a similar check in domid_alloc(). But if the value is
unlikely above DOMID_FIRST_RESERVED, then we would end up to allocate a
random domid.
> - panic("No more domain IDs available\n");
> -
> d_cfg.max_evtchn_port = 1023;>
d_cfg.max_grant_frames = -1;
> d_cfg.max_maptrack_frames = -1;
> @@ -981,7 +979,11 @@ void __init create_domUs(void)
> * very important to use the pre-increment operator to call
> * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
> */
> - d = domain_create(++max_init_domid, &d_cfg, flags);
> + domid = domid_alloc(++max_init_domid);
In the commit message you wrote:
"""
(b) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
max_init_domid (both Arm and x86).
"""
I read it as max_init_domid should have been moved to common code. I see
this is done in the next patch. So I would suggest to clarify this will
be handled separately.
> + if ( domid == DOMID_INVALID )
> + panic("Error allocating ID for domain %s\n", dt_node_name(node));
> +
> + d = domain_create(domid, &d_cfg, flags);
> if ( IS_ERR(d) )
> panic("Error creating domain %s (rc = %ld)\n",
> dt_node_name(node), PTR_ERR(d));
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index abf1969e60..ae0c44fcbb 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -66,6 +66,10 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
> static struct domain *domain_hash[DOMAIN_HASH_SIZE];
> struct domain *domain_list;
>
> +/* Non-system domain ID allocator. */
> +static DEFINE_SPINLOCK(domid_lock);
> +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> +
> /*
> * Insert a domain into the domlist/hash. This allows the domain to be looked
> * up by domid, and therefore to be the subject of hypercalls/etc.
> @@ -1449,6 +1453,8 @@ void domain_destroy(struct domain *d)
>
> TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
>
> + domid_free(d->domain_id);
> +
> /* Remove from the domlist/hash. */
> domlist_remove(d);
>
> @@ -2405,6 +2411,54 @@ domid_t get_initial_domain_id(void)
> return hardware_domid;
> }
>
> +domid_t domid_alloc(domid_t domid)
> +{
> + spin_lock(&domid_lock);
> +
> + if ( domid < DOMID_FIRST_RESERVED )
> + {
> + if ( __test_and_set_bit(domid, domid_bitmap) )
> + domid = DOMID_INVALID;
> + }
> + else
> + {
> + static domid_t domid_last;
> + /* NB: account for late hwdom case, skip ID#0 */
I am somewhat confused with this comment. For the late hwdom case, I
thought we were using a non-zero ID. Dom0 should also always be the
first dom0 to be reserved. Can you clarify?
That said, if you want to skip to dom0. Wouldn't it be better to have
domid_last set to 1 and then ...
> + const domid_t reserved_domid = 0;> + const bool
reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
> +
> + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
> + domid_last);
> +
> + if ( domid == DOMID_FIRST_RESERVED )
> + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
... use 1 here? This would avoid to temporarily mark the domid 0 as
reserved.
Cheers,
--
Julien Grall
On Thu, Jun 05, 2025 at 10:58:48PM +0100, Julien Grall wrote:
> Hi Denis,
>
> On 28/05/2025 23:50, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmkhn@proton.me>
> >
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Currently, hypervisor code has two different domain ID allocation
> > implementations:
> >
> > (a) Sequential IDs allocation in dom0less Arm code based on max_init_domid;
> >
> > (b) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
> > max_init_domid (both Arm and x86).
> >
> > 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.
> >
> > Wrap the domain ID allocation as an arch-independent function domid_alloc() in
> > common/domain.c based on the bitmap.
> >
> > Allocation algorithm:
> > - If an explicit domain ID is provided, verify its availability and use it if
> > ID is not used;
> > - If DOMID_INVALID is provided, search the range [0..DOMID_FIRST_RESERVED-1],
> > starting from the last used ID and wrapping around as needed. It guarantees
> > that two consecutive calls will never return the same ID. ID#0 is excluded
> > to account for late hwdom case.
> >
> > Also, remove is_free_domid() helper as it is not needed now.
> >
> > No functional change intended.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v8:
> > - skip ID #0 in domid_alloc() to account for late hwdom
> > ---
> > xen/arch/arm/domain_build.c | 17 +++++---
> > xen/arch/x86/setup.c | 11 +++--
> > xen/common/device-tree/dom0less-build.c | 10 +++--
> > xen/common/domain.c | 54 +++++++++++++++++++++++++
> > xen/common/domctl.c | 42 +++----------------
> > xen/include/xen/domain.h | 3 ++
> > 6 files changed, 87 insertions(+), 50 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index b189a7cfae..e9d563c269 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2010,6 +2010,7 @@ void __init create_dom0(void)
> > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
> > };
> > unsigned int flags = CDF_privileged | CDF_hardware;
> > + domid_t domid;
> > int rc;
> >
> > /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> > @@ -2034,19 +2035,25 @@ void __init create_dom0(void)
> > if ( !llc_coloring_enabled )
> > flags |= CDF_directmap;
> >
> > - dom0 = domain_create(0, &dom0_cfg, flags);
> > + domid = domid_alloc(0);
> > + if ( domid == DOMID_INVALID )
> > + panic("Error allocating domain ID 0\n");
> > +
> > + dom0 = domain_create(domid, &dom0_cfg, flags);
> > if ( IS_ERR(dom0) )
> > - panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
> > + panic("Error creating domain %d (rc = %ld)\n", domid, PTR_ERR(dom0));
>
> The change in the panic here and below seems unrelated to the goal of
> the patch. I am ok to keep them here, but I think it should be mentioned
> in the commit message.
Will do.
>
> >
> > if ( llc_coloring_enabled && (rc = dom0_set_llc_colors(dom0)) )
> > - panic("Error initializing LLC coloring for domain 0 (rc = %d)\n", rc);
> > + panic("Error initializing LLC coloring for domain %pd (rc = %d)\n",
> > + dom0, rc);
> >
> > if ( alloc_dom0_vcpu0(dom0) == NULL )
> > - panic("Error creating domain 0 vcpu0\n");
> > + panic("Error creating domain %pdv0\n", dom0);
> >
> > rc = construct_dom0(dom0);
> > if ( rc )
> > - panic("Could not set up DOM0 guest OS (rc = %d)\n", rc);
> > + panic("Could not set up guest OS for domain %pd (rc = %d)\n",
> > + dom0, rc);
> >
> > set_xs_domain(dom0);
> > }
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 1f5cb67bd0..b36ce4491b 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1031,8 +1031,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. */
>
> NIT: The two spaces were valid here. This is in fact quite common to
> unambiguously mark the end of a sentence.
Yep, I changed the text in comment and forgot to keep the double space.
>
> > + 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) )
> > @@ -1064,7 +1067,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> >
> > if ( (strlen(acpi_param) == 0) && acpi_disabled )
> > {
> > - printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> > + printk("ACPI is disabled, notifying domain %pd (acpi=off)\n", d);
> > safe_strcpy(acpi_param, "off");
> > }
> >
> > @@ -1079,7 +1082,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> >
> > bd->d = d;
> > if ( construct_dom0(bd) != 0 )
> > - panic("Could not construct domain 0\n");
> > + panic("Could not construct domain %pd\n", d);
> >
> > bd->cmdline = NULL;
> > xfree(cmdline);
> > diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> > index 39cb2cd5c7..a509f8fecd 100644
> > --- a/xen/common/device-tree/dom0less-build.c
> > +++ b/xen/common/device-tree/dom0less-build.c
> > @@ -850,15 +850,13 @@ void __init create_domUs(void)
> > struct xen_domctl_createdomain d_cfg = {0};
> > unsigned int flags = 0U;
> > bool has_dtb = false;
> > + domid_t domid;
> > uint32_t val;
> > int rc;
> >
> > if ( !dt_device_is_compatible(node, "xen,domain") )
> > continue;
> >
> > - if ( (max_init_domid + 1) >= DOMID_FIRST_RESERVED )
>
> I can't find a similar check in domid_alloc(). But if the value is
> unlikely above DOMID_FIRST_RESERVED, then we would end up to allocate a
> random domid.
Yes, thanks.
I think I need to add tools/tests with a self-test for the domain ID allocation
code.
>
> > - panic("No more domain IDs available\n");
> > -
> > d_cfg.max_evtchn_port = 1023;>
> d_cfg.max_grant_frames = -1;
> > d_cfg.max_maptrack_frames = -1;
> > @@ -981,7 +979,11 @@ void __init create_domUs(void)
> > * very important to use the pre-increment operator to call
> > * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
> > */
> > - d = domain_create(++max_init_domid, &d_cfg, flags);
> > + domid = domid_alloc(++max_init_domid);
>
> In the commit message you wrote:
>
>
> """
> (b) Sequential IDs allocation in XEN_DOMCTL_createdomain; does not use
> max_init_domid (both Arm and x86).
> """
>
> I read it as max_init_domid should have been moved to common code. I see
> this is done in the next patch. So I would suggest to clarify this will
> be handled separately.
Will do.
>
>
> > + if ( domid == DOMID_INVALID )
> > + panic("Error allocating ID for domain %s\n", dt_node_name(node));
> > +
> > + d = domain_create(domid, &d_cfg, flags);
> > if ( IS_ERR(d) )
> > panic("Error creating domain %s (rc = %ld)\n",
> > dt_node_name(node), PTR_ERR(d));
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index abf1969e60..ae0c44fcbb 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -66,6 +66,10 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
> > static struct domain *domain_hash[DOMAIN_HASH_SIZE];
> > struct domain *domain_list;
> >
> > +/* Non-system domain ID allocator. */
> > +static DEFINE_SPINLOCK(domid_lock);
> > +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> > +
> > /*
> > * Insert a domain into the domlist/hash. This allows the domain to be looked
> > * up by domid, and therefore to be the subject of hypercalls/etc.
> > @@ -1449,6 +1453,8 @@ void domain_destroy(struct domain *d)
> >
> > TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
> >
> > + domid_free(d->domain_id);
> > +
> > /* Remove from the domlist/hash. */
> > domlist_remove(d);
> >
> > @@ -2405,6 +2411,54 @@ domid_t get_initial_domain_id(void)
> > return hardware_domid;
> > }
> >
> > +domid_t domid_alloc(domid_t domid)
> > +{
> > + spin_lock(&domid_lock);
> > +
> > + if ( domid < DOMID_FIRST_RESERVED )
> > + {
> > + if ( __test_and_set_bit(domid, domid_bitmap) )
> > + domid = DOMID_INVALID;
> > + }
> > + else
> > + {
> > + static domid_t domid_last;
> > + /* NB: account for late hwdom case, skip ID#0 */
>
> I am somewhat confused with this comment. For the late hwdom case, I
> thought we were using a non-zero ID. Dom0 should also always be the
> first dom0 to be reserved. Can you clarify?
My current understanding is:
- the ID of "domain 0" (privileged domain) can be overridden at the boot-time
via hardware_domid parameter.
- there's only one reserved (and configurable) domain ID == hardware_domid in
the allocation range (which is 0 by default).
- get_initial_domain_id() returns the reserved domain ID value (which is
used in the in the follow on change to keep the change isolated).
Is my understanding correct?
I will update the comment.
>
> That said, if you want to skip to dom0. Wouldn't it be better to have
> domid_last set to 1 and then ...
>
> > + const domid_t reserved_domid = 0;> + const bool
> reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
> > +
> > + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
> > + domid_last);
> > +
> > + if ( domid == DOMID_FIRST_RESERVED )
> > + domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
>
> ... use 1 here? This would avoid to temporarily mark the domid 0 as
> reserved.
>
> Cheers,
>
> --
> Julien Grall
>
>
Hi Denis,
On 06/06/2025 07:55, dmkhn@proton.me wrote:
> On Thu, Jun 05, 2025 at 10:58:48PM +0100, Julien Grall wrote:
>>> + if ( domid == DOMID_INVALID )
>>> + panic("Error allocating ID for domain %s\n", dt_node_name(node));
>>> +
>>> + d = domain_create(domid, &d_cfg, flags);
>>> if ( IS_ERR(d) )
>>> panic("Error creating domain %s (rc = %ld)\n",
>>> dt_node_name(node), PTR_ERR(d));
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index abf1969e60..ae0c44fcbb 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -66,6 +66,10 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
>>> static struct domain *domain_hash[DOMAIN_HASH_SIZE];
>>> struct domain *domain_list;
>>>
>>> +/* Non-system domain ID allocator. */
>>> +static DEFINE_SPINLOCK(domid_lock);
>>> +static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
>>> +
>>> /*
>>> * Insert a domain into the domlist/hash. This allows the domain to be looked
>>> * up by domid, and therefore to be the subject of hypercalls/etc.
>>> @@ -1449,6 +1453,8 @@ void domain_destroy(struct domain *d)
>>>
>>> TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
>>>
>>> + domid_free(d->domain_id);
>>> +
>>> /* Remove from the domlist/hash. */
>>> domlist_remove(d);
>>>
>>> @@ -2405,6 +2411,54 @@ domid_t get_initial_domain_id(void)
>>> return hardware_domid;
>>> }
>>>
>>> +domid_t domid_alloc(domid_t domid)
>>> +{
>>> + spin_lock(&domid_lock);
>>> +
>>> + if ( domid < DOMID_FIRST_RESERVED )
>>> + {
>>> + if ( __test_and_set_bit(domid, domid_bitmap) )
>>> + domid = DOMID_INVALID;
>>> + }
>>> + else
>>> + {
>>> + static domid_t domid_last;
>>> + /* NB: account for late hwdom case, skip ID#0 */
>>
>> I am somewhat confused with this comment. For the late hwdom case, I
>> thought we were using a non-zero ID. Dom0 should also always be the
>> first dom0 to be reserved. Can you clarify?
>
> My current understanding is:
> - the ID of "domain 0" (privileged domain) can be overridden at the boot-time
> via hardware_domid parameter.
> > - there's only one reserved (and configurable) domain ID ==
hardware_domid in
> the allocation range (which is 0 by default).
> > - get_initial_domain_id() returns the reserved domain ID value
(which is
> used in the in the follow on change to keep the change isolated).
>
> Is my understanding correct?
I have replied yesterday night on a separate thread about this behavior
[1]. Rather than duplicating it, I would suggest to move the
conversation there.
In short, I believe the late domain support was recently broken.
Cheers,
[1]
https://lore.kernel.org/xen-devel/20250528225030.2652166-1-dmukhin@ford.com/T/#mdcdf3802a913859243ff6ce841
445cfab265145f
--
Julien Grall
© 2016 - 2025 Red Hat, Inc.