[PATCH v9 0/3] xen/domain: domain ID allocation

[PATCH v9 0/3] xen/domain: domain ID allocation

[dmkhn]

The patch series adds new library calls for allocating domain IDs.

Patch 1 introduces new domid_{alloc,free} calls.
Patch 2 adjusts hardware domain ID treatment on Arm.
Patch 3 is an RFC: introduces new CONFIG_MAX_DOMID parameter to limit the
number of user domains during run-time.

Link to v8: https://lore.kernel.org/xen-devel/20250521000024.2944685-1-dmukhin@ford.com/
Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1841666928

Denis Mukhin (3):
  xen/domain: unify domain ID allocation
  xen/domain: adjust domain ID allocation for Arm
  xen/domain: introduce CONFIG_MAX_DOMID

 xen/arch/arm/domain_build.c             | 17 +++++--
 xen/arch/x86/cpu/mcheck/mce.c           |  2 +-
 xen/arch/x86/cpu/vpmu.c                 |  2 +-
 xen/arch/x86/setup.c                    | 11 +++--
 xen/common/Kconfig                      |  8 ++++
 xen/common/device-tree/dom0less-build.c | 17 ++++---
 xen/common/domain.c                     | 62 +++++++++++++++++++++++--
 xen/common/domctl.c                     | 42 ++---------------
 xen/common/sched/core.c                 |  4 +-
 xen/drivers/passthrough/vtd/iommu.c     |  2 +-
 xen/include/xen/domain.h                |  3 ++
 11 files changed, 107 insertions(+), 63 deletions(-)

-- 
2.34.1

[PATCH v9 1/3] xen/domain: unify domain ID allocation

[dmkhn]

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.1

[PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[dmkhn]

From: Denis Mukhin <dmkhn@proton.me>

From: Denis Mukhin <dmukhin@ford.com>

Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
with a call to get_initial_domain_id() (returns the value of hardware_domid on
Arm).

Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
ID is skipped during domain ID allocation to cover domU case in dom0less
configuration. That also fixes a potential issue with re-using ID#0 for domUs
when get_initial_domain_id() returns non-zero.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v8:
- rebased 
---
 xen/arch/arm/domain_build.c             | 4 ++--
 xen/common/device-tree/dom0less-build.c | 9 +++------
 xen/common/domain.c                     | 4 ++--
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e9d563c269..0ad80b020a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2035,9 +2035,9 @@ void __init create_dom0(void)
     if ( !llc_coloring_enabled )
         flags |= CDF_directmap;
 
-    domid = domid_alloc(0);
+    domid = domid_alloc(get_initial_domain_id());
     if ( domid == DOMID_INVALID )
-        panic("Error allocating domain ID 0\n");
+        panic("Error allocating domain ID %d\n", get_initial_domain_id());
 
     dom0 = domain_create(domid, &dom0_cfg, flags);
     if ( IS_ERR(dom0) )
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index a509f8fecd..9a6015f4ce 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -974,14 +974,11 @@ void __init create_domUs(void)
 
         arch_create_domUs(node, &d_cfg, flags);
 
-        /*
-         * 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)
-         */
-        domid = domid_alloc(++max_init_domid);
+        domid = domid_alloc(DOMID_INVALID);
         if ( domid == DOMID_INVALID )
             panic("Error allocating ID for domain %s\n", dt_node_name(node));
+        if ( max_init_domid < domid )
+            max_init_domid = domid;
 
         d = domain_create(domid, &d_cfg, flags);
         if ( IS_ERR(d) )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ae0c44fcbb..129b4fcb37 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -2423,8 +2423,8 @@ domid_t domid_alloc(domid_t domid)
     else
     {
         static domid_t domid_last;
-        /* NB: account for late hwdom case, skip ID#0 */
-        const domid_t reserved_domid = 0;
+        /* NB: account for late hwdom case */
+        const domid_t reserved_domid = get_initial_domain_id();
         const bool reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
 
         domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
-- 
2.34.1

[PATCH v9 3/3] xen/domain: introduce CONFIG_MAX_DOMID

[dmkhn]

From: Denis Mukhin <dmkhn@proton.me>

From: Denis Mukhin <dmukhin@ford.com>

Embedded deployments of Xen do not need to have support for more than dozen of
domains.

Introduce build-time configuration option to limit the number of domains during
run-time.

Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v8:
- dropped hunk w/ compile-time check for DOMID_FIRST_RESERVED
- updated CONFIG_MAX_DOMID explanation
- dropped public header file changes
---
 xen/arch/x86/cpu/mcheck/mce.c       |  2 +-
 xen/arch/x86/cpu/vpmu.c             |  2 +-
 xen/common/Kconfig                  |  8 ++++++++
 xen/common/domain.c                 | 20 +++++++++++---------
 xen/common/sched/core.c             |  4 ++--
 xen/drivers/passthrough/vtd/iommu.c |  2 +-
 6 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 1c348e557d..ee8ddd33b0 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -1493,7 +1493,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
             d = rcu_lock_domain_by_any_id(mc_msrinject->mcinj_domid);
             if ( d == NULL )
             {
-                if ( mc_msrinject->mcinj_domid >= DOMID_FIRST_RESERVED )
+                if ( mc_msrinject->mcinj_domid >= CONFIG_MAX_DOMID )
                     return x86_mcerr("do_mca inject: incompatible flag "
                                      "MC_MSRINJ_F_GPADDR with domain %d",
                                      -EINVAL, domid);
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index c28192ea26..67d423e088 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -174,7 +174,7 @@ void vpmu_do_interrupt(void)
      * in XENPMU_MODE_ALL, for everyone.
      */
     if ( (vpmu_mode & XENPMU_MODE_ALL) ||
-         (sampled->domain->domain_id >= DOMID_FIRST_RESERVED) )
+         (sampled->domain->domain_id >= CONFIG_MAX_DOMID) )
     {
         sampling = choose_hwdom_vcpu();
         if ( !sampling )
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 3d66d09397..ef083856b8 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -579,4 +579,12 @@ config BUDDY_ALLOCATOR_SIZE
 	  Amount of memory reserved for the buddy allocator to serve Xen heap,
 	  working alongside the colored one.
 
+config MAX_DOMID
+	int "Maximum domain ID"
+	range 1 32752
+	default 32752
+	help
+	  Specifies the maximum domain ID (dom0 or late hwdom, predefined
+	  domains, post-boot domains, excluding Xen system domains).
+
 endmenu
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 129b4fcb37..87e5be35e5 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -68,7 +68,7 @@ struct domain *domain_list;
 
 /* Non-system domain ID allocator. */
 static DEFINE_SPINLOCK(domid_lock);
-static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
+static DECLARE_BITMAP(domid_bitmap, CONFIG_MAX_DOMID);
 
 /*
  * Insert a domain into the domlist/hash.  This allows the domain to be looked
@@ -154,7 +154,7 @@ int domain_init_states(void)
     ASSERT(rw_is_write_locked_by_me(&current->domain->event_lock));
 
     dom_state_changed = xvzalloc_array(unsigned long,
-                                       BITS_TO_LONGS(DOMID_FIRST_RESERVED));
+                                       BITS_TO_LONGS(CONFIG_MAX_DOMID));
     if ( !dom_state_changed )
         return -ENOMEM;
 
@@ -234,7 +234,7 @@ int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d,
     while ( dom_state_changed )
     {
         dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
-        if ( dom >= DOMID_FIRST_RESERVED )
+        if ( dom >= CONFIG_MAX_DOMID )
             break;
         if ( test_and_clear_bit(dom, dom_state_changed) )
         {
@@ -823,7 +823,7 @@ struct domain *domain_create(domid_t domid,
     /* Sort out our idea of is_hardware_domain(). */
     if ( (flags & CDF_hardware) || domid == hardware_domid )
     {
-        if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
+        if ( hardware_domid < 0 || hardware_domid >= CONFIG_MAX_DOMID )
             panic("The value of hardware_dom must be a valid domain ID\n");
 
         /* late_hwdom is only allowed for dom0. */
@@ -2413,9 +2413,11 @@ domid_t get_initial_domain_id(void)
 
 domid_t domid_alloc(domid_t domid)
 {
+    BUILD_BUG_ON(DOMID_FIRST_RESERVED < CONFIG_MAX_DOMID);
+
     spin_lock(&domid_lock);
 
-    if ( domid < DOMID_FIRST_RESERVED )
+    if ( domid < CONFIG_MAX_DOMID )
     {
         if ( __test_and_set_bit(domid, domid_bitmap) )
             domid = DOMID_INVALID;
@@ -2427,13 +2429,13 @@ domid_t domid_alloc(domid_t domid)
         const domid_t reserved_domid = get_initial_domain_id();
         const bool reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
 
-        domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
+        domid = find_next_zero_bit(domid_bitmap, CONFIG_MAX_DOMID,
                                    domid_last);
 
-        if ( domid == DOMID_FIRST_RESERVED )
-            domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
+        if ( domid == CONFIG_MAX_DOMID )
+            domid = find_next_zero_bit(domid_bitmap, CONFIG_MAX_DOMID, 0);
 
-        if ( domid == DOMID_FIRST_RESERVED )
+        if ( domid == CONFIG_MAX_DOMID )
         {
             domid = DOMID_INVALID;
         }
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 9043414290..f1bfb6f6a2 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -867,7 +867,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid)
     int ret;
 
     ASSERT(d->cpupool == NULL);
-    ASSERT(d->domain_id < DOMID_FIRST_RESERVED);
+    ASSERT(d->domain_id < CONFIG_MAX_DOMID);
 
     if ( (ret = cpupool_add_domain(d, poolid)) )
         return ret;
@@ -891,7 +891,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid)
 
 void sched_destroy_domain(struct domain *d)
 {
-    ASSERT(d->domain_id < DOMID_FIRST_RESERVED);
+    ASSERT(d->domain_id < CONFIG_MAX_DOMID);
 
     if ( d->cpupool )
     {
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index c55f02c97e..5df85ca629 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1509,7 +1509,7 @@ int domain_context_mapping_one(
 
         prev_did = context_domain_id(lctxt);
         domid = did_to_domain_id(iommu, prev_did);
-        if ( domid < DOMID_FIRST_RESERVED )
+        if ( domid < CONFIG_MAX_DOMID )
             prev_dom = rcu_lock_domain_by_id(domid);
         else if ( pdev ? domid == pdev->arch.pseudo_domid : domid > DOMID_MASK )
             prev_dom = rcu_lock_domain(dom_io);
-- 
2.34.1

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Stefano Stabellini]

On Wed, 28 May 2025, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmkhn@proton.me>
> 
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
> with a call to get_initial_domain_id() (returns the value of hardware_domid on
> Arm).
> 
> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
> ID is skipped during domain ID allocation to cover domU case in dom0less
> configuration. That also fixes a potential issue with re-using ID#0 for domUs
> when get_initial_domain_id() returns non-zero.

It looks like this sentence should be removed from the commit message as
not valid anymore.

Aside from that, the code changes below as clear.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v8:
> - rebased 
> ---
>  xen/arch/arm/domain_build.c             | 4 ++--
>  xen/common/device-tree/dom0less-build.c | 9 +++------
>  xen/common/domain.c                     | 4 ++--
>  3 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e9d563c269..0ad80b020a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
>      if ( !llc_coloring_enabled )
>          flags |= CDF_directmap;
>  
> -    domid = domid_alloc(0);
> +    domid = domid_alloc(get_initial_domain_id());
>      if ( domid == DOMID_INVALID )
> -        panic("Error allocating domain ID 0\n");
> +        panic("Error allocating domain ID %d\n", get_initial_domain_id());
>  
>      dom0 = domain_create(domid, &dom0_cfg, flags);
>      if ( IS_ERR(dom0) )
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index a509f8fecd..9a6015f4ce 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -974,14 +974,11 @@ void __init create_domUs(void)
>  
>          arch_create_domUs(node, &d_cfg, flags);
>  
> -        /*
> -         * 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)
> -         */
> -        domid = domid_alloc(++max_init_domid);
> +        domid = domid_alloc(DOMID_INVALID);
>          if ( domid == DOMID_INVALID )
>              panic("Error allocating ID for domain %s\n", dt_node_name(node));
> +        if ( max_init_domid < domid )
> +            max_init_domid = domid;
>  
>          d = domain_create(domid, &d_cfg, flags);
>          if ( IS_ERR(d) )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index ae0c44fcbb..129b4fcb37 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -2423,8 +2423,8 @@ domid_t domid_alloc(domid_t domid)
>      else
>      {
>          static domid_t domid_last;
> -        /* NB: account for late hwdom case, skip ID#0 */
> -        const domid_t reserved_domid = 0;
> +        /* NB: account for late hwdom case */
> +        const domid_t reserved_domid = get_initial_domain_id();
>          const bool reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
>  
>          domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
> -- 
> 2.34.1
> 
> 

Re: [PATCH v9 3/3] xen/domain: introduce CONFIG_MAX_DOMID

[Stefano Stabellini]

On Wed, 28 May 2025, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmkhn@proton.me>
> 
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Embedded deployments of Xen do not need to have support for more than dozen of
> domains.
> 
> Introduce build-time configuration option to limit the number of domains during
> run-time.
> 
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>

There is one DOMID_FIRST_RESERVED check in xen/arch/arm/tee/ffa.c that
should be changed too


> ---
> Changes since v8:
> - dropped hunk w/ compile-time check for DOMID_FIRST_RESERVED
> - updated CONFIG_MAX_DOMID explanation
> - dropped public header file changes
> ---
>  xen/arch/x86/cpu/mcheck/mce.c       |  2 +-
>  xen/arch/x86/cpu/vpmu.c             |  2 +-
>  xen/common/Kconfig                  |  8 ++++++++
>  xen/common/domain.c                 | 20 +++++++++++---------
>  xen/common/sched/core.c             |  4 ++--
>  xen/drivers/passthrough/vtd/iommu.c |  2 +-
>  6 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> index 1c348e557d..ee8ddd33b0 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -1493,7 +1493,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
>              d = rcu_lock_domain_by_any_id(mc_msrinject->mcinj_domid);
>              if ( d == NULL )
>              {
> -                if ( mc_msrinject->mcinj_domid >= DOMID_FIRST_RESERVED )
> +                if ( mc_msrinject->mcinj_domid >= CONFIG_MAX_DOMID )
>                      return x86_mcerr("do_mca inject: incompatible flag "
>                                       "MC_MSRINJ_F_GPADDR with domain %d",
>                                       -EINVAL, domid);
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index c28192ea26..67d423e088 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -174,7 +174,7 @@ void vpmu_do_interrupt(void)
>       * in XENPMU_MODE_ALL, for everyone.
>       */
>      if ( (vpmu_mode & XENPMU_MODE_ALL) ||
> -         (sampled->domain->domain_id >= DOMID_FIRST_RESERVED) )
> +         (sampled->domain->domain_id >= CONFIG_MAX_DOMID) )
>      {
>          sampling = choose_hwdom_vcpu();
>          if ( !sampling )
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 3d66d09397..ef083856b8 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -579,4 +579,12 @@ config BUDDY_ALLOCATOR_SIZE
>  	  Amount of memory reserved for the buddy allocator to serve Xen heap,
>  	  working alongside the colored one.
>  
> +config MAX_DOMID
> +	int "Maximum domain ID"
> +	range 1 32752
> +	default 32752
> +	help
> +	  Specifies the maximum domain ID (dom0 or late hwdom, predefined
> +	  domains, post-boot domains, excluding Xen system domains).

Written like this it would seem that the maximum domain ID is usable,
e.g. that 32752 is a valid domid number. Actually 32752 is 0x7ff0 which
is DOMID_FIRST_RESERVED == DOMID_SELF and cannot be used.

I think we should change the description:


Specifies the maximum domain ID (dom0 or late hwdom, predefined domains,
post-boot domains, excluding Xen system domains). This value indicates
the first domain ID that is out of bounds and cannot be used for domain
allocation.



>  endmenu
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 129b4fcb37..87e5be35e5 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -68,7 +68,7 @@ struct domain *domain_list;
>  
>  /* Non-system domain ID allocator. */
>  static DEFINE_SPINLOCK(domid_lock);
> -static DECLARE_BITMAP(domid_bitmap, DOMID_FIRST_RESERVED);
> +static DECLARE_BITMAP(domid_bitmap, CONFIG_MAX_DOMID);
>  
>  /*
>   * Insert a domain into the domlist/hash.  This allows the domain to be looked
> @@ -154,7 +154,7 @@ int domain_init_states(void)
>      ASSERT(rw_is_write_locked_by_me(&current->domain->event_lock));
>  
>      dom_state_changed = xvzalloc_array(unsigned long,
> -                                       BITS_TO_LONGS(DOMID_FIRST_RESERVED));
> +                                       BITS_TO_LONGS(CONFIG_MAX_DOMID));
>      if ( !dom_state_changed )
>          return -ENOMEM;
>  
> @@ -234,7 +234,7 @@ int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d,
>      while ( dom_state_changed )
>      {
>          dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
> -        if ( dom >= DOMID_FIRST_RESERVED )
> +        if ( dom >= CONFIG_MAX_DOMID )
>              break;
>          if ( test_and_clear_bit(dom, dom_state_changed) )
>          {
> @@ -823,7 +823,7 @@ struct domain *domain_create(domid_t domid,
>      /* Sort out our idea of is_hardware_domain(). */
>      if ( (flags & CDF_hardware) || domid == hardware_domid )
>      {
> -        if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
> +        if ( hardware_domid < 0 || hardware_domid >= CONFIG_MAX_DOMID )
>              panic("The value of hardware_dom must be a valid domain ID\n");
>  
>          /* late_hwdom is only allowed for dom0. */
> @@ -2413,9 +2413,11 @@ domid_t get_initial_domain_id(void)
>  
>  domid_t domid_alloc(domid_t domid)
>  {
> +    BUILD_BUG_ON(DOMID_FIRST_RESERVED < CONFIG_MAX_DOMID);
> +
>      spin_lock(&domid_lock);
>  
> -    if ( domid < DOMID_FIRST_RESERVED )
> +    if ( domid < CONFIG_MAX_DOMID )
>      {
>          if ( __test_and_set_bit(domid, domid_bitmap) )
>              domid = DOMID_INVALID;
> @@ -2427,13 +2429,13 @@ domid_t domid_alloc(domid_t domid)
>          const domid_t reserved_domid = get_initial_domain_id();
>          const bool reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
>  
> -        domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,
> +        domid = find_next_zero_bit(domid_bitmap, CONFIG_MAX_DOMID,
>                                     domid_last);
>  
> -        if ( domid == DOMID_FIRST_RESERVED )
> -            domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED, 0);
> +        if ( domid == CONFIG_MAX_DOMID )
> +            domid = find_next_zero_bit(domid_bitmap, CONFIG_MAX_DOMID, 0);
>  
> -        if ( domid == DOMID_FIRST_RESERVED )
> +        if ( domid == CONFIG_MAX_DOMID )
>          {
>              domid = DOMID_INVALID;
>          }
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 9043414290..f1bfb6f6a2 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -867,7 +867,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid)
>      int ret;
>  
>      ASSERT(d->cpupool == NULL);
> -    ASSERT(d->domain_id < DOMID_FIRST_RESERVED);
> +    ASSERT(d->domain_id < CONFIG_MAX_DOMID);
>  
>      if ( (ret = cpupool_add_domain(d, poolid)) )
>          return ret;
> @@ -891,7 +891,7 @@ int sched_init_domain(struct domain *d, unsigned int poolid)
>  
>  void sched_destroy_domain(struct domain *d)
>  {
> -    ASSERT(d->domain_id < DOMID_FIRST_RESERVED);
> +    ASSERT(d->domain_id < CONFIG_MAX_DOMID);
>  
>      if ( d->cpupool )
>      {
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index c55f02c97e..5df85ca629 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1509,7 +1509,7 @@ int domain_context_mapping_one(
>  
>          prev_did = context_domain_id(lctxt);
>          domid = did_to_domain_id(iommu, prev_did);
> -        if ( domid < DOMID_FIRST_RESERVED )
> +        if ( domid < CONFIG_MAX_DOMID )
>              prev_dom = rcu_lock_domain_by_id(domid);
>          else if ( pdev ? domid == pdev->arch.pseudo_domid : domid > DOMID_MASK )
>              prev_dom = rcu_lock_domain(dom_io);
> -- 
> 2.34.1
> 
> 

Re: [PATCH v9 3/3] xen/domain: introduce CONFIG_MAX_DOMID

[Jan Beulich]

On 29.05.2025 00:50, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmkhn@proton.me>
> 
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Embedded deployments of Xen do not need to have support for more than dozen of
> domains.
> 
> Introduce build-time configuration option to limit the number of domains during
> run-time.
> 
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v8:
> - dropped hunk w/ compile-time check for DOMID_FIRST_RESERVED
> - updated CONFIG_MAX_DOMID explanation
> - dropped public header file changes
> ---
>  xen/arch/x86/cpu/mcheck/mce.c       |  2 +-
>  xen/arch/x86/cpu/vpmu.c             |  2 +-
>  xen/common/Kconfig                  |  8 ++++++++
>  xen/common/domain.c                 | 20 +++++++++++---------
>  xen/common/sched/core.c             |  4 ++--
>  xen/drivers/passthrough/vtd/iommu.c |  2 +-
>  6 files changed, 24 insertions(+), 14 deletions(-)

What about checks against DOMID_FIRST_RESERVED in common/domctl.c? Indeed they
must not be changed, but when not changing them, how are several of the checks
you actually change going to be correct? (I'm sorry for noticing this only now;
it should have occurred to me earlier on.)

Jan

Re: [PATCH v9 1/3] xen/domain: unify domain ID allocation

[Julien Grall]

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

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Julien Grall]

Hi Denis,

On 28/05/2025 23:50, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmkhn@proton.me>
> 
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
> with a call to get_initial_domain_id() (returns the value of hardware_domid on
> Arm).

I am not entirely why this is done. Are you intending to pass a 
different domain ID? If so...

> 
> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
> ID is skipped during domain ID allocation to cover domU case in dom0less
> configuration. That also fixes a potential issue with re-using ID#0 for domUs
> when get_initial_domain_id() returns non-zero.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v8:
> - rebased
> ---
>   xen/arch/arm/domain_build.c             | 4 ++--
>   xen/common/device-tree/dom0less-build.c | 9 +++------
>   xen/common/domain.c                     | 4 ++--
>   3 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e9d563c269..0ad80b020a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)

... naming like create_dom0() probably wants to be renamed.

That said, I am not convinced a domain other than 0 should have full 
privilege by default. So I would argue it should stay as ...

>       if ( !llc_coloring_enabled )
>           flags |= CDF_directmap;
>   
> -    domid = domid_alloc(0);
> +    domid = domid_alloc(get_initial_domain_id());

... 0.

>       if ( domid == DOMID_INVALID )
> -        panic("Error allocating domain ID 0\n");
> +        panic("Error allocating domain ID %d\n", get_initial_domain_id());
>   
>       dom0 = domain_create(domid, &dom0_cfg, flags);
>       if ( IS_ERR(dom0) )
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index a509f8fecd..9a6015f4ce 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -974,14 +974,11 @@ void __init create_domUs(void)
>   
>           arch_create_domUs(node, &d_cfg, flags);
>   
> -        /*
> -         * 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)
> -         */
> -        domid = domid_alloc(++max_init_domid);
> +        domid = domid_alloc(DOMID_INVALID);
>           if ( domid == DOMID_INVALID )
>               panic("Error allocating ID for domain %s\n", dt_node_name(node));
> +        if ( max_init_domid < domid )
> +            max_init_domid = domid;
>   
>           d = domain_create(domid, &d_cfg, flags);
>           if ( IS_ERR(d) )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index ae0c44fcbb..129b4fcb37 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -2423,8 +2423,8 @@ domid_t domid_alloc(domid_t domid)
>       else
>       {
>           static domid_t domid_last;
> -        /* NB: account for late hwdom case, skip ID#0 */
> -        const domid_t reserved_domid = 0;
> +        /* NB: account for late hwdom case */
> +        const domid_t reserved_domid = get_initial_domain_id();

This is somewhat confusing to modify domid_alloc() in a patch that is 
meant to modify only the Arm allocation. Can you clarify why this can't 
be done earlier?

>           const bool reserved = __test_and_set_bit(reserved_domid, domid_bitmap);
>   
>           domid = find_next_zero_bit(domid_bitmap, DOMID_FIRST_RESERVED,

Cheers,

-- 
Julien Grall

Re: [PATCH v9 1/3] xen/domain: unify domain ID allocation

[dmkhn]

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
> 
> 

Re: [PATCH v9 1/3] xen/domain: unify domain ID allocation

[Jan Beulich]

On 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

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Julien Grall]

Hi Denis,

On 05/06/2025 23:05, 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>
>>
>> Remove the hardcoded domain ID 0 allocation for hardware domain and 
>> replace it
>> with a call to get_initial_domain_id() (returns the value of 
>> hardware_domid on
>> Arm).
> 
> I am not entirely why this is done. Are you intending to pass a 
> different domain ID? If so...
> 
>>
>> Update domid_alloc(DOMID_INVALID) case to ensure that 
>> get_initial_domain_id()
>> ID is skipped during domain ID allocation to cover domU case in dom0less
>> configuration. That also fixes a potential issue with re-using ID#0 
>> for domUs
>> when get_initial_domain_id() returns non-zero.
>>
>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>> ---
>> Changes since v8:
>> - rebased
>> ---
>>   xen/arch/arm/domain_build.c             | 4 ++--
>>   xen/common/device-tree/dom0less-build.c | 9 +++------
>>   xen/common/domain.c                     | 4 ++--
>>   3 files changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index e9d563c269..0ad80b020a 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
> 
> ... naming like create_dom0() probably wants to be renamed.
> 
> That said, I am not convinced a domain other than 0 should have full 
> privilege by default. So I would argue it should stay as ...
> 
>>       if ( !llc_coloring_enabled )
>>           flags |= CDF_directmap;
>> -    domid = domid_alloc(0);
>> +    domid = domid_alloc(get_initial_domain_id());
> 
> ... 0.

Looking at the implementation of get_initial_domain_id(), I noticed the 
behavior was changed for x86 by [1].

Before, get_initial_domain_id() was returning 0 except for the PV shim.
But now, it would could return the domain ID specified on the command 
line (via hardware_dom).

 From my understanding, the goal of the command line was to create the 
hardware domain *after* boot. So initially we create dom0 and then 
initialize the hardware domain. With the patch below, this has changed.

However, from the commit message, I don't understand why. It seems like 
we broke late hwdom?

For instance, late_hwdom_init() has the following assert:

     dom0 = rcu_lock_domain_by_id(0);
     ASSERT(dom0 != NULL);

Jan, I saw you were involved in the review of the series. Any idea why 
this was changed?

Cheers,

[1] https://lore.kernel.org/all/20250306075819.154361-1-dmkhn@proton.me/

-- 
Julien Grall

Re: [PATCH v9 1/3] xen/domain: unify domain ID allocation

[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

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Jan Beulich]

On 06.06.2025 23:29, Julien Grall wrote:
> Hi Denis,
> 
> On 05/06/2025 23:05, 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>
>>>
>>> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
>>> with a call to get_initial_domain_id() (returns the value of hardware_domid on
>>> Arm).
>>
>> I am not entirely why this is done. Are you intending to pass a different domain ID? If so...
>>
>>>
>>> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
>>> ID is skipped during domain ID allocation to cover domU case in dom0less
>>> configuration. That also fixes a potential issue with re-using ID#0 for domUs
>>> when get_initial_domain_id() returns non-zero.
>>>
>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>>> ---
>>> Changes since v8:
>>> - rebased
>>> ---
>>>   xen/arch/arm/domain_build.c             | 4 ++--
>>>   xen/common/device-tree/dom0less-build.c | 9 +++------
>>>   xen/common/domain.c                     | 4 ++--
>>>   3 files changed, 7 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index e9d563c269..0ad80b020a 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
>>
>> ... naming like create_dom0() probably wants to be renamed.
>>
>> That said, I am not convinced a domain other than 0 should have full privilege by default. So I would argue it should stay as ...
>>
>>>       if ( !llc_coloring_enabled )
>>>           flags |= CDF_directmap;
>>> -    domid = domid_alloc(0);
>>> +    domid = domid_alloc(get_initial_domain_id());
>>
>> ... 0.
> 
> Looking at the implementation of get_initial_domain_id(), I noticed the behavior was changed for x86 by [1].
> 
> Before, get_initial_domain_id() was returning 0 except for the PV shim.
> But now, it would could return the domain ID specified on the command line (via hardware_dom).
> 
> From my understanding, the goal of the command line was to create the hardware domain *after* boot. So initially we create dom0 and then initialize the hardware domain. With the patch below, this has changed.
> 
> However, from the commit message, I don't understand why. It seems like we broke late hwdom?
> 
> For instance, late_hwdom_init() has the following assert:
> 
>     dom0 = rcu_lock_domain_by_id(0);
>     ASSERT(dom0 != NULL);
> 
> Jan, I saw you were involved in the review of the series. Any idea why this was changed?

I simply overlooked this aspect when looking at the change. You're right, things
were broken there. Unless a simple and clean fix can be made relatively soon, I
think this simply needs reverting (which may mean to revert any later commits
that depend on that). I can't help noting that in this console rework there were
way too many issues, and I fear more than just this one may have slipped
through. I therefore wonder whether taken as a whole this was/is worth both the
submitter's and all the reviewers' time.

Jan

> [1] https://lore.kernel.org/all/20250306075819.154361-1-dmkhn@proton.me/
> 

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[dmkhn]

On Tue, Jun 10, 2025 at 08:53:12AM +0200, Jan Beulich wrote:
> On 06.06.2025 23:29, Julien Grall wrote:
> > Hi Denis,
> >
> > On 05/06/2025 23:05, 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>
> >>>
> >>> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
> >>> with a call to get_initial_domain_id() (returns the value of hardware_domid on
> >>> Arm).
> >>
> >> I am not entirely why this is done. Are you intending to pass a different domain ID? If so...
> >>
> >>>
> >>> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
> >>> ID is skipped during domain ID allocation to cover domU case in dom0less
> >>> configuration. That also fixes a potential issue with re-using ID#0 for domUs
> >>> when get_initial_domain_id() returns non-zero.
> >>>
> >>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> >>> ---
> >>> Changes since v8:
> >>> - rebased
> >>> ---
> >>>   xen/arch/arm/domain_build.c             | 4 ++--
> >>>   xen/common/device-tree/dom0less-build.c | 9 +++------
> >>>   xen/common/domain.c                     | 4 ++--
> >>>   3 files changed, 7 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>> index e9d563c269..0ad80b020a 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
> >>
> >> ... naming like create_dom0() probably wants to be renamed.
> >>
> >> That said, I am not convinced a domain other than 0 should have full privilege by default. So I would argue it should stay as ...
> >>
> >>>       if ( !llc_coloring_enabled )
> >>>           flags |= CDF_directmap;
> >>> -    domid = domid_alloc(0);
> >>> +    domid = domid_alloc(get_initial_domain_id());
> >>
> >> ... 0.
> >
> > Looking at the implementation of get_initial_domain_id(), I noticed the behavior was changed for x86 by [1].
> >
> > Before, get_initial_domain_id() was returning 0 except for the PV shim.
> > But now, it would could return the domain ID specified on the command line (via hardware_dom).
> >
> > From my understanding, the goal of the command line was to create the hardware domain *after* boot. So initially we create dom0 and then initialize the hardware domain. With the patch below, this has changed.
> >
> > However, from the commit message, I don't understand why. It seems like we broke late hwdom?
> >
> > For instance, late_hwdom_init() has the following assert:
> >
> >     dom0 = rcu_lock_domain_by_id(0);
> >     ASSERT(dom0 != NULL);
> >
> > Jan, I saw you were involved in the review of the series. Any idea why this was changed?
> 
> I simply overlooked this aspect when looking at the change. You're right, things
> were broken there. Unless a simple and clean fix can be made relatively soon, I
> think this simply needs reverting (which may mean to revert any later commits
> that depend on that). I can't help noting that in this console rework there were
> way too many issues, and I fear more than just this one may have slipped
> through. I therefore wonder whether taken as a whole this was/is worth both the
> submitter's and all the reviewers' time.

Yes, sorry, I overlooked late_hwdom_init() modification.

IMO, the clean fix would be adding another command line parameter
`control_domid` (with default value 0), make get_initial_domain_id() return it
instead of current `hardware_domid` and update late_hwdom_init() to use
`control_domid` insted of open-coded 0.

That should align with the effort of splitting priveleged dom0 into control and
hardware domains: control domain will be the first domain ID allocated,
followed by the hardware domain.

> 
> Jan
> 
> > [1] https://lore.kernel.org/all/20250306075819.154361-1-dmkhn@proton.me/
> >
> 

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Jan Beulich]

On 10.06.2025 10:02, dmkhn@proton.me wrote:
> On Tue, Jun 10, 2025 at 08:53:12AM +0200, Jan Beulich wrote:
>> On 06.06.2025 23:29, Julien Grall wrote:
>>> Hi Denis,
>>>
>>> On 05/06/2025 23:05, 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>
>>>>>
>>>>> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
>>>>> with a call to get_initial_domain_id() (returns the value of hardware_domid on
>>>>> Arm).
>>>>
>>>> I am not entirely why this is done. Are you intending to pass a different domain ID? If so...
>>>>
>>>>>
>>>>> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
>>>>> ID is skipped during domain ID allocation to cover domU case in dom0less
>>>>> configuration. That also fixes a potential issue with re-using ID#0 for domUs
>>>>> when get_initial_domain_id() returns non-zero.
>>>>>
>>>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>>>>> ---
>>>>> Changes since v8:
>>>>> - rebased
>>>>> ---
>>>>>   xen/arch/arm/domain_build.c             | 4 ++--
>>>>>   xen/common/device-tree/dom0less-build.c | 9 +++------
>>>>>   xen/common/domain.c                     | 4 ++--
>>>>>   3 files changed, 7 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>> index e9d563c269..0ad80b020a 100644
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
>>>>
>>>> ... naming like create_dom0() probably wants to be renamed.
>>>>
>>>> That said, I am not convinced a domain other than 0 should have full privilege by default. So I would argue it should stay as ...
>>>>
>>>>>       if ( !llc_coloring_enabled )
>>>>>           flags |= CDF_directmap;
>>>>> -    domid = domid_alloc(0);
>>>>> +    domid = domid_alloc(get_initial_domain_id());
>>>>
>>>> ... 0.
>>>
>>> Looking at the implementation of get_initial_domain_id(), I noticed the behavior was changed for x86 by [1].
>>>
>>> Before, get_initial_domain_id() was returning 0 except for the PV shim.
>>> But now, it would could return the domain ID specified on the command line (via hardware_dom).
>>>
>>> From my understanding, the goal of the command line was to create the hardware domain *after* boot. So initially we create dom0 and then initialize the hardware domain. With the patch below, this has changed.
>>>
>>> However, from the commit message, I don't understand why. It seems like we broke late hwdom?
>>>
>>> For instance, late_hwdom_init() has the following assert:
>>>
>>>     dom0 = rcu_lock_domain_by_id(0);
>>>     ASSERT(dom0 != NULL);
>>>
>>> Jan, I saw you were involved in the review of the series. Any idea why this was changed?
>>
>> I simply overlooked this aspect when looking at the change. You're right, things
>> were broken there. Unless a simple and clean fix can be made relatively soon, I
>> think this simply needs reverting (which may mean to revert any later commits
>> that depend on that). I can't help noting that in this console rework there were
>> way too many issues, and I fear more than just this one may have slipped
>> through. I therefore wonder whether taken as a whole this was/is worth both the
>> submitter's and all the reviewers' time.
> 
> Yes, sorry, I overlooked late_hwdom_init() modification.
> 
> IMO, the clean fix would be adding another command line parameter
> `control_domid` (with default value 0), make get_initial_domain_id() return it
> instead of current `hardware_domid` and update late_hwdom_init() to use
> `control_domid` insted of open-coded 0.

No, no new command line option will address this. Original behavior needs to be
restored (either by correcting the earlier change or, as said, be reverting).

Jan

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[dmkhn]

On Tue, Jun 10, 2025 at 10:26:22AM +0200, Jan Beulich wrote:
> On 10.06.2025 10:02, dmkhn@proton.me wrote:
> > On Tue, Jun 10, 2025 at 08:53:12AM +0200, Jan Beulich wrote:
> >> On 06.06.2025 23:29, Julien Grall wrote:
> >>> Hi Denis,
> >>>
> >>> On 05/06/2025 23:05, 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>
> >>>>>
> >>>>> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
> >>>>> with a call to get_initial_domain_id() (returns the value of hardware_domid on
> >>>>> Arm).
> >>>>
> >>>> I am not entirely why this is done. Are you intending to pass a different domain ID? If so...
> >>>>
> >>>>>
> >>>>> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
> >>>>> ID is skipped during domain ID allocation to cover domU case in dom0less
> >>>>> configuration. That also fixes a potential issue with re-using ID#0 for domUs
> >>>>> when get_initial_domain_id() returns non-zero.
> >>>>>
> >>>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> >>>>> ---
> >>>>> Changes since v8:
> >>>>> - rebased
> >>>>> ---
> >>>>>   xen/arch/arm/domain_build.c             | 4 ++--
> >>>>>   xen/common/device-tree/dom0less-build.c | 9 +++------
> >>>>>   xen/common/domain.c                     | 4 ++--
> >>>>>   3 files changed, 7 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>>>> index e9d563c269..0ad80b020a 100644
> >>>>> --- a/xen/arch/arm/domain_build.c
> >>>>> +++ b/xen/arch/arm/domain_build.c
> >>>>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
> >>>>
> >>>> ... naming like create_dom0() probably wants to be renamed.
> >>>>
> >>>> That said, I am not convinced a domain other than 0 should have full privilege by default. So I would argue it should stay as ...
> >>>>
> >>>>>       if ( !llc_coloring_enabled )
> >>>>>           flags |= CDF_directmap;
> >>>>> -    domid = domid_alloc(0);
> >>>>> +    domid = domid_alloc(get_initial_domain_id());
> >>>>
> >>>> ... 0.
> >>>
> >>> Looking at the implementation of get_initial_domain_id(), I noticed the behavior was changed for x86 by [1].
> >>>
> >>> Before, get_initial_domain_id() was returning 0 except for the PV shim.
> >>> But now, it would could return the domain ID specified on the command line (via hardware_dom).
> >>>
> >>> From my understanding, the goal of the command line was to create the hardware domain *after* boot. So initially we create dom0 and then initialize the hardware domain. With the patch below, this has changed.
> >>>
> >>> However, from the commit message, I don't understand why. It seems like we broke late hwdom?
> >>>
> >>> For instance, late_hwdom_init() has the following assert:
> >>>
> >>>     dom0 = rcu_lock_domain_by_id(0);
> >>>     ASSERT(dom0 != NULL);
> >>>
> >>> Jan, I saw you were involved in the review of the series. Any idea why this was changed?
> >>
> >> I simply overlooked this aspect when looking at the change. You're right, things
> >> were broken there. Unless a simple and clean fix can be made relatively soon, I
> >> think this simply needs reverting (which may mean to revert any later commits
> >> that depend on that). I can't help noting that in this console rework there were
> >> way too many issues, and I fear more than just this one may have slipped
> >> through. I therefore wonder whether taken as a whole this was/is worth both the
> >> submitter's and all the reviewers' time.
> >
> > Yes, sorry, I overlooked late_hwdom_init() modification.
> >
> > IMO, the clean fix would be adding another command line parameter
> > `control_domid` (with default value 0), make get_initial_domain_id() return it
> > instead of current `hardware_domid` and update late_hwdom_init() to use
> > `control_domid` insted of open-coded 0.
> 
> No, no new command line option will address this. Original behavior needs to be
> restored (either by correcting the earlier change or, as said, be reverting).

Correction of the earlier change:

  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/b7f194650307a08a9e6da5aa9fdd1f8a9afd67eb

re: command line option: I meant something like this:

  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/51a519de6ea73ff3b650fd9bd4f3c5c63f64c069

> 
> Jan

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Julien Grall]

Hi Denis,

On 10/06/2025 10:04, dmkhn@proton.me wrote:
> On Tue, Jun 10, 2025 at 10:26:22AM +0200, Jan Beulich wrote:
>> On 10.06.2025 10:02, dmkhn@proton.me wrote:
>>> On Tue, Jun 10, 2025 at 08:53:12AM +0200, Jan Beulich wrote:
>>>> On 06.06.2025 23:29, Julien Grall wrote:
>>>>> Hi Denis,
>>>>>
>>>>> On 05/06/2025 23:05, 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>
>>>>>>>
>>>>>>> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
>>>>>>> with a call to get_initial_domain_id() (returns the value of hardware_domid on
>>>>>>> Arm).
>>>>>>
>>>>>> I am not entirely why this is done. Are you intending to pass a different domain ID? If so...
>>>>>>
>>>>>>>
>>>>>>> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
>>>>>>> ID is skipped during domain ID allocation to cover domU case in dom0less
>>>>>>> configuration. That also fixes a potential issue with re-using ID#0 for domUs
>>>>>>> when get_initial_domain_id() returns non-zero.
>>>>>>>
>>>>>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>>>>>>> ---
>>>>>>> Changes since v8:
>>>>>>> - rebased
>>>>>>> ---
>>>>>>>    xen/arch/arm/domain_build.c             | 4 ++--
>>>>>>>    xen/common/device-tree/dom0less-build.c | 9 +++------
>>>>>>>    xen/common/domain.c                     | 4 ++--
>>>>>>>    3 files changed, 7 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>>>> index e9d563c269..0ad80b020a 100644
>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
>>>>>>
>>>>>> ... naming like create_dom0() probably wants to be renamed.
>>>>>>
>>>>>> That said, I am not convinced a domain other than 0 should have full privilege by default. So I would argue it should stay as ...
>>>>>>
>>>>>>>        if ( !llc_coloring_enabled )
>>>>>>>            flags |= CDF_directmap;
>>>>>>> -    domid = domid_alloc(0);
>>>>>>> +    domid = domid_alloc(get_initial_domain_id());
>>>>>>
>>>>>> ... 0.
>>>>>
>>>>> Looking at the implementation of get_initial_domain_id(), I noticed the behavior was changed for x86 by [1].
>>>>>
>>>>> Before, get_initial_domain_id() was returning 0 except for the PV shim.
>>>>> But now, it would could return the domain ID specified on the command line (via hardware_dom).
>>>>>
>>>>>  From my understanding, the goal of the command line was to create the hardware domain *after* boot. So initially we create dom0 and then initialize the hardware domain. With the patch below, this has changed.
>>>>>
>>>>> However, from the commit message, I don't understand why. It seems like we broke late hwdom?
>>>>>
>>>>> For instance, late_hwdom_init() has the following assert:
>>>>>
>>>>>      dom0 = rcu_lock_domain_by_id(0);
>>>>>      ASSERT(dom0 != NULL);
>>>>>
>>>>> Jan, I saw you were involved in the review of the series. Any idea why this was changed?
>>>>
>>>> I simply overlooked this aspect when looking at the change. You're right, things
>>>> were broken there. Unless a simple and clean fix can be made relatively soon, I
>>>> think this simply needs reverting (which may mean to revert any later commits
>>>> that depend on that). I can't help noting that in this console rework there were
>>>> way too many issues, and I fear more than just this one may have slipped
>>>> through. I therefore wonder whether taken as a whole this was/is worth both the
>>>> submitter's and all the reviewers' time.
>>>
>>> Yes, sorry, I overlooked late_hwdom_init() modification.
>>>
>>> IMO, the clean fix would be adding another command line parameter
>>> `control_domid` (with default value 0), make get_initial_domain_id() return it
>>> instead of current `hardware_domid` and update late_hwdom_init() to use
>>> `control_domid` insted of open-coded 0.
>>
>> No, no new command line option will address this. Original behavior needs to be
>> restored (either by correcting the earlier change or, as said, be reverting).
> 
> Correction of the earlier change:
> 
>    https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/b7f194650307a08a9e6da5aa9fdd1f8a9afd67eb
> 
> re: command line option: I meant something like this:
> 
>    https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/51a519de6ea73ff3b650fd9bd4f3c5c63f64c069

I am with Jan here. This used to work before without "control_domid", so 
we ought to keep the same.

But even if we are ok to break compatibility, I don't see the value of 
"control_domid". The implication of setting "hardware_domid" is you will
have a separate control domain. At which point, why would it matter to 
specify the domain ID?

Cheers,

-- 
Julien Grall

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Stefano Stabellini]

+Jason

On Tue, 10 Jun 2025, Julien Grall wrote:
> But even if we are ok to break compatibility, I don't see the value of
> "control_domid". The implication of setting "hardware_domid" is you will
> have a separate control domain. At which point, why would it matter to specify
> the domain ID?
 
I just wanted to say that while we (AMD) are looking for a hardware
domain / control domain separation for safety reasons, I don't think we
have a need to specify the domid for either one.

Jason, is that correct?

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[Jason Andryuk]

On 2025-06-10 14:33, Stefano Stabellini wrote:
> +Jason
> 
> On Tue, 10 Jun 2025, Julien Grall wrote:
>> But even if we are ok to break compatibility, I don't see the value of
>> "control_domid". The implication of setting "hardware_domid" is you will
>> have a separate control domain. At which point, why would it matter to specify
>> the domain ID?
>   
> I just wanted to say that while we (AMD) are looking for a hardware
> domain / control domain separation for safety reasons, I don't think we
> have a need to specify the domid for either one.

Specifying domids isn't really necessary, but it can be convenience or 
usability improvement with dom0less/Hyperlaunch.  But I don't think 
control_domid is necessary.

hardware_domid is not used for dom0less/Hyperlaunch with split control 
and hardware domains.  The "capabilities" device tree (DT) property 
specifies the role of the domain.

Hyperlaunch lets you specify a domid in the DT - there is some 
auto-allocation logic, but I haven't use it.  dom0less doesn't allow 
specifying a domid today, but it could.  dom0less domains are assigned 
domids sequentially, so you can determine it from the order in the DT.

Knowing the domids can be helpful for configuring userspace, and that 
only really matters for dom0less/Hyperlaunch.  e.g. xenstored wants to 
know which domid is control.

I think it's nice to have a domid property so that you know when 
configuring the system which domain is which.  You can explicitly read 
the domid out of the DT and know what it is.  Since dom0 userspace needs 
to refer to domids, this make it clear which domain is which for, as an 
example, connecting disks.

hardware_domid= is the way of enabling later hardware domain 
functionality.  dom0 boots as dom0.  When it creates domid == 
hardware_domid, that new domain becomes the hardware domain, and dom0 
loses hwdom and becomes just the control domain.  It's a legacy feature 
and was a workaround for when Xen could only create a single domain at boot.

Regards,
Jason

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[dmkhn]

On Tue, Jun 10, 2025 at 10:44:39AM +0100, Julien Grall wrote:
> Hi Denis,
> 
> On 10/06/2025 10:04, dmkhn@proton.me wrote:
> > On Tue, Jun 10, 2025 at 10:26:22AM +0200, Jan Beulich wrote:
> >> On 10.06.2025 10:02, dmkhn@proton.me wrote:
> >>> On Tue, Jun 10, 2025 at 08:53:12AM +0200, Jan Beulich wrote:
> >>>> On 06.06.2025 23:29, Julien Grall wrote:
> >>>>> Hi Denis,
> >>>>>
> >>>>> On 05/06/2025 23:05, 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>
> >>>>>>>
> >>>>>>> Remove the hardcoded domain ID 0 allocation for hardware domain and replace it
> >>>>>>> with a call to get_initial_domain_id() (returns the value of hardware_domid on
> >>>>>>> Arm).
> >>>>>>
> >>>>>> I am not entirely why this is done. Are you intending to pass a different domain ID? If so...
> >>>>>>
> >>>>>>>
> >>>>>>> Update domid_alloc(DOMID_INVALID) case to ensure that get_initial_domain_id()
> >>>>>>> ID is skipped during domain ID allocation to cover domU case in dom0less
> >>>>>>> configuration. That also fixes a potential issue with re-using ID#0 for domUs
> >>>>>>> when get_initial_domain_id() returns non-zero.
> >>>>>>>
> >>>>>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> >>>>>>> ---
> >>>>>>> Changes since v8:
> >>>>>>> - rebased
> >>>>>>> ---
> >>>>>>>    xen/arch/arm/domain_build.c             | 4 ++--
> >>>>>>>    xen/common/device-tree/dom0less-build.c | 9 +++------
> >>>>>>>    xen/common/domain.c                     | 4 ++--
> >>>>>>>    3 files changed, 7 insertions(+), 10 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>>>>>> index e9d563c269..0ad80b020a 100644
> >>>>>>> --- a/xen/arch/arm/domain_build.c
> >>>>>>> +++ b/xen/arch/arm/domain_build.c
> >>>>>>> @@ -2035,9 +2035,9 @@ void __init create_dom0(void)
> >>>>>>
> >>>>>> ... naming like create_dom0() probably wants to be renamed.
> >>>>>>
> >>>>>> That said, I am not convinced a domain other than 0 should have full privilege by default. So I would argue it should stay as ...
> >>>>>>
> >>>>>>>        if ( !llc_coloring_enabled )
> >>>>>>>            flags |= CDF_directmap;
> >>>>>>> -    domid = domid_alloc(0);
> >>>>>>> +    domid = domid_alloc(get_initial_domain_id());
> >>>>>>
> >>>>>> ... 0.
> >>>>>
> >>>>> Looking at the implementation of get_initial_domain_id(), I noticed the behavior was changed for x86 by [1].
> >>>>>
> >>>>> Before, get_initial_domain_id() was returning 0 except for the PV shim.
> >>>>> But now, it would could return the domain ID specified on the command line (via hardware_dom).
> >>>>>
> >>>>>  From my understanding, the goal of the command line was to create the hardware domain *after* boot. So initially we create dom0 and then initialize the hardware domain. With the patch below, this has changed.
> >>>>>
> >>>>> However, from the commit message, I don't understand why. It seems like we broke late hwdom?
> >>>>>
> >>>>> For instance, late_hwdom_init() has the following assert:
> >>>>>
> >>>>>      dom0 = rcu_lock_domain_by_id(0);
> >>>>>      ASSERT(dom0 != NULL);
> >>>>>
> >>>>> Jan, I saw you were involved in the review of the series. Any idea why this was changed?
> >>>>
> >>>> I simply overlooked this aspect when looking at the change. You're right, things
> >>>> were broken there. Unless a simple and clean fix can be made relatively soon, I
> >>>> think this simply needs reverting (which may mean to revert any later commits
> >>>> that depend on that). I can't help noting that in this console rework there were
> >>>> way too many issues, and I fear more than just this one may have slipped
> >>>> through. I therefore wonder whether taken as a whole this was/is worth both the
> >>>> submitter's and all the reviewers' time.
> >>>
> >>> Yes, sorry, I overlooked late_hwdom_init() modification.
> >>>
> >>> IMO, the clean fix would be adding another command line parameter
> >>> `control_domid` (with default value 0), make get_initial_domain_id() return it
> >>> instead of current `hardware_domid` and update late_hwdom_init() to use
> >>> `control_domid` insted of open-coded 0.
> >>
> >> No, no new command line option will address this. Original behavior needs to be
> >> restored (either by correcting the earlier change or, as said, be reverting).
> >
> > Correction of the earlier change:
> >
> >    https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/b7f194650307a08a9e6da5aa9fdd1f8a9afd67eb
> >
> > re: command line option: I meant something like this:
> >
> >    https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/51a519de6ea73ff3b650fd9bd4f3c5c63f64c069
> 
> I am with Jan here. This used to work before without "control_domid", so
> we ought to keep the same.

OK, thanks for the feedback!
I will post the correction of the earlier change.

> 
> But even if we are ok to break compatibility, I don't see the value of
> "control_domid". The implication of setting "hardware_domid" is you will
> have a separate control domain. At which point, why would it matter to
> specify the domain ID?

I thought there's a plan to use symbols in the ongoing hyperlauch reworks, but
as later Jason explains, this is not the case.

> 
> Cheers,
> 
> --
> Julien Grall
> 

Re: [PATCH v9 2/3] xen/domain: adjust domain ID allocation for Arm

[dmkhn]

On Tue, Jun 10, 2025 at 05:37:33PM -0400, Jason Andryuk wrote:
> On 2025-06-10 14:33, Stefano Stabellini wrote:
> > +Jason
> >
> > On Tue, 10 Jun 2025, Julien Grall wrote:
> >> But even if we are ok to break compatibility, I don't see the value of
> >> "control_domid". The implication of setting "hardware_domid" is you will
> >> have a separate control domain. At which point, why would it matter to specify
> >> the domain ID?
> >
> > I just wanted to say that while we (AMD) are looking for a hardware
> > domain / control domain separation for safety reasons, I don't think we
> > have a need to specify the domid for either one.
> 
> Specifying domids isn't really necessary, but it can be convenience or
> usability improvement with dom0less/Hyperlaunch.  But I don't think
> control_domid is necessary.
> 
> hardware_domid is not used for dom0less/Hyperlaunch with split control
> and hardware domains.  The "capabilities" device tree (DT) property
> specifies the role of the domain.
> 
> Hyperlaunch lets you specify a domid in the DT - there is some
> auto-allocation logic, but I haven't use it.  dom0less doesn't allow
> specifying a domid today, but it could.  dom0less domains are assigned
> domids sequentially, so you can determine it from the order in the DT.
> 
> Knowing the domids can be helpful for configuring userspace, and that
> only really matters for dom0less/Hyperlaunch.  e.g. xenstored wants to
> know which domid is control.
> 
> I think it's nice to have a domid property so that you know when
> configuring the system which domain is which.  You can explicitly read
> the domid out of the DT and know what it is.  Since dom0 userspace needs
> to refer to domids, this make it clear which domain is which for, as an
> example, connecting disks.
> 
> hardware_domid= is the way of enabling later hardware domain
> functionality.  dom0 boots as dom0.  When it creates domid ==
> hardware_domid, that new domain becomes the hardware domain, and dom0
> loses hwdom and becomes just the control domain.  It's a legacy feature
> and was a workaround for when Xen could only create a single domain at boot.

Thanks for explanation!

> 
> Regards,
> Jason