From: Denis Mukhin <dmukhin@ford.com>
Move domain ID allocation during domain creation to a dedicated
function domid_alloc().
Allocation algorithm:
- If an explicit domain ID is provided, verify its availability and
use it if ID is unused;
- Otherwise, perform an exhaustive search for the first available ID
within the [0..DOMID_FIRST_RESERVED) range, excluding hardware_domid.
This minimizes the use of max_init_domid in the code and, thus, is a
prerequisite change for enabling console input rotation across domains
with console input permission on x86 platforms (which currently is
limited to dom0, PV shim and Xen).
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v1:
- use domid_alloc() during dom0 creation on x86
---
xen/arch/arm/dom0less-build.c | 15 ++++++------
xen/arch/arm/domain_build.c | 19 +++++++++++----
xen/arch/x86/setup.c | 8 ++++++-
xen/common/domain.c | 45 +++++++++++++++++++++++++++++++++++
xen/common/domctl.c | 45 ++++-------------------------------
xen/include/xen/domain.h | 3 +++
6 files changed, 80 insertions(+), 55 deletions(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 573b0d25ae..4b9e22039e 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -975,14 +975,18 @@ void __init create_domUs(void)
.grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
};
unsigned int flags = 0U;
+ 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");
+ rc = domid_alloc(DOMID_AUTO);
+ if ( rc < 0 )
+ panic("cannot allocate domain ID for domain %s (rc = %d)\n",
+ dt_node_name(node), rc);
+ domid = rc;
if ( dt_find_property(node, "xen,static-mem", NULL) )
{
@@ -1107,12 +1111,7 @@ void __init create_domUs(void)
if ( !llc_coloring_enabled && llc_colors_str )
panic("'llc-colors' found, but LLC coloring is disabled\n");
- /*
- * The variable max_init_domid is initialized with zero, so here it's
- * very important to use the pre-increment operator to call
- * domain_create() with a domid > 0. (domid == 0 is reserved for Dom0)
- */
- d = domain_create(++max_init_domid, &d_cfg, flags);
+ 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/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2b5b433183..2d8c2931d6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2367,8 +2367,15 @@ void __init create_dom0(void)
.grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
};
unsigned int flags = CDF_privileged;
+ domid_t domid;
int rc;
+ rc = domid_alloc(get_initial_domain_id());
+ if ( rc < 0 )
+ panic("Cannot use domain ID %d (rc = %d)\n",
+ get_initial_domain_id(), rc);
+ domid = rc;
+
/* The vGIC for DOM0 is exactly emulating the hardware GIC */
dom0_cfg.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
dom0_cfg.arch.nr_spis = VGIC_DEF_NR_SPIS;
@@ -2391,19 +2398,21 @@ void __init create_dom0(void)
if ( !llc_coloring_enabled )
flags |= CDF_directmap;
- dom0 = domain_create(0, &dom0_cfg, flags);
+ 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 %d (rc = %d)\n",
+ domid, rc);
if ( alloc_dom0_vcpu0(dom0) == NULL )
- panic("Error creating domain 0 vcpu0\n");
+ panic("Error creating domain %d vcpu0\n", domid);
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 %d (rc = %d)\n",
+ domid, rc);
}
/*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d70abb7e0c..ad349528ea 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -996,6 +996,7 @@ static struct domain *__init create_dom0(struct boot_info *bi)
domid_t domid;
struct boot_module *image;
unsigned int idx;
+ int rc;
idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
if ( idx >= bi->nr_modules )
@@ -1003,6 +1004,12 @@ static struct domain *__init create_dom0(struct boot_info *bi)
image = &bi->mods[idx];
+ rc = domid_alloc(get_initial_domain_id());
+ if ( rc < 0 )
+ panic("Cannot use domain ID %d (rc = %d)\n",
+ get_initial_domain_id(), rc);
+ domid = rc;
+
if ( opt_dom0_pvh )
{
dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
@@ -1017,7 +1024,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
/* Create initial domain. Not d0 for pvshim. */
- domid = get_initial_domain_id();
d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
if ( IS_ERR(d) )
panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
diff --git a/xen/common/domain.c b/xen/common/domain.c
index b9f549c617..b07d70a7e3 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -66,6 +66,51 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
static struct domain *domain_hash[DOMAIN_HASH_SIZE];
struct domain *domain_list;
+static inline bool is_free_domid(domid_t dom)
+{
+ struct domain *d = rcu_lock_domain_by_id(dom);
+
+ if ( d )
+ rcu_unlock_domain(d);
+
+ return !d;
+}
+
+/*
+ * Allocate new domain ID based on the hint.
+ *
+ * If hint is outside of valid [0..DOMID_FIRST_RESERVED] range of IDs,
+ * perform an exhaustive search of the first free domain ID excluding
+ * hardware_domid.
+ */
+int domid_alloc(int hint)
+{
+ domid_t domid;
+
+ if ( hint >= 0 && hint < DOMID_FIRST_RESERVED )
+ {
+ if ( !is_free_domid(hint) )
+ return -EEXIST;
+
+ domid = hint;
+ }
+ else
+ {
+ for ( domid = 0; domid < DOMID_FIRST_RESERVED; domid++ )
+ {
+ if ( domid == hardware_domid )
+ continue;
+ if ( is_free_domid(domid) )
+ break;
+ }
+
+ if ( domid == DOMID_FIRST_RESERVED )
+ return -ENOMEM;
+ }
+
+ return domid;
+}
+
/*
* 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.
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index bfe2e1f9f0..3d21612660 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,34 +407,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
case XEN_DOMCTL_createdomain:
{
- domid_t dom;
- static domid_t rover = 0;
+ ret = domid_alloc(op->domain);
+ if ( ret < 0 )
+ break;
- dom = op->domain;
- if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
- {
- ret = -EEXIST;
- if ( !is_free_domid(dom) )
- break;
- }
- else
- {
- for ( dom = rover + 1; dom != rover; dom++ )
- {
- if ( dom == DOMID_FIRST_RESERVED )
- dom = 1;
- if ( is_free_domid(dom) )
- break;
- }
-
- ret = -ENOMEM;
- if ( dom == rover )
- break;
-
- rover = dom;
- }
-
- d = domain_create(dom, &op->u.createdomain, false);
+ d = domain_create(ret, &op->u.createdomain, false);
if ( IS_ERR(d) )
{
ret = PTR_ERR(d);
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 83069de501..9b7159a743 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -37,6 +37,9 @@ void arch_get_domain_info(const struct domain *d,
domid_t get_initial_domain_id(void);
+#define DOMID_AUTO (-1)
+int domid_alloc(int hint);
+
/* CDF_* constant. Internal flags for domain creation. */
/* Is this a privileged domain? */
#define CDF_privileged (1U << 0)
--
2.34.1On 01.04.2025 01:05, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
>
> Move domain ID allocation during domain creation to a dedicated
> function domid_alloc().
>
> Allocation algorithm:
> - If an explicit domain ID is provided, verify its availability and
> use it if ID is unused;
> - Otherwise, perform an exhaustive search for the first available ID
> within the [0..DOMID_FIRST_RESERVED) range, excluding hardware_domid.
>
> This minimizes the use of max_init_domid in the code and, thus, is a
> prerequisite change for enabling console input rotation across domains
> with console input permission on x86 platforms (which currently is
> limited to dom0, PV shim and Xen).
By removing the updating of max_init_domid you do - afaict - break the
remaining use site(s) of the variable.
> @@ -1003,6 +1004,12 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>
> image = &bi->mods[idx];
>
> + rc = domid_alloc(get_initial_domain_id());
> + if ( rc < 0 )
> + panic("Cannot use domain ID %d (rc = %d)\n",
> + get_initial_domain_id(), rc);
> + domid = rc;
> +
> if ( opt_dom0_pvh )
> {
> dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
Why does this need to move up, ...
> @@ -1017,7 +1024,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> /* Create initial domain. Not d0 for pvshim. */
> - domid = get_initial_domain_id();
... disconnecting the logic from the comment that is relevant there, ...
> d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
... and not so much here?
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -37,6 +37,9 @@ void arch_get_domain_info(const struct domain *d,
>
> domid_t get_initial_domain_id(void);
>
> +#define DOMID_AUTO (-1)
> +int domid_alloc(int hint);
Imo it would be better to use e.g. DOMID_INVALID as the "give me whatever
is available" indicator, allowing the function parameter to properly be
domid_t.
But first of all - can we please take a step back and re-evaluate whether
all of this re-arrangement that you're doing (not just in the patch here)
is really needed? It seems to me that it must be possible to do whatever
you ultimately want to do without re-writing quite a few pretty central
pieces that have been serving us fine for a long time. That is, rather
than make our code fit your desires, make your plans fit within the code
base we have.
Jan
On Tuesday, April 8th, 2025 at 7:26 AM, Jan Beulich <jbeulich@suse.com> wrote:
>
>
> On 01.04.2025 01:05, dmkhn@proton.me wrote:
>
> > From: Denis Mukhin dmukhin@ford.com
> >
> > Move domain ID allocation during domain creation to a dedicated
> > function domid_alloc().
> >
> > Allocation algorithm:
> > - If an explicit domain ID is provided, verify its availability and
> > use it if ID is unused;
> > - Otherwise, perform an exhaustive search for the first available ID
> > within the [0..DOMID_FIRST_RESERVED) range, excluding hardware_domid.
> >
> > This minimizes the use of max_init_domid in the code and, thus, is a
> > prerequisite change for enabling console input rotation across domains
> > with console input permission on x86 platforms (which currently is
> > limited to dom0, PV shim and Xen).
>
>
> By removing the updating of max_init_domid you do - afaict - break the
> remaining use site(s) of the variable.
Unfortunately, this patch should go together with the next patch in
this series:
xen/domain: introduce domid_top()
I mentioned dependency in the cover letter.
>
> > @@ -1003,6 +1004,12 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> >
> > image = &bi->mods[idx];
> >
> > + rc = domid_alloc(get_initial_domain_id());
> > + if ( rc < 0 )
> > + panic("Cannot use domain ID %d (rc = %d)\n",
> > + get_initial_domain_id(), rc);
> > + domid = rc;
> > +
> > if ( opt_dom0_pvh )
> > {
> > dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
>
>
> Why does this need to move up, ...
>
> > @@ -1017,7 +1024,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> > dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> >
> > /* Create initial domain. Not d0 for pvshim. */
> > - domid = get_initial_domain_id();
>
>
> ... disconnecting the logic from the comment that is relevant there, ...
>
> > d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>
>
> ... and not so much here?
So that in case domid_alloc() fails, create_dom0() will reach error state
quickly. I will re-group it as suggested.
>
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -37,6 +37,9 @@ void arch_get_domain_info(const struct domain *d,
> >
> > domid_t get_initial_domain_id(void);
> >
> > +#define DOMID_AUTO (-1)
> > +int domid_alloc(int hint);
>
>
> Imo it would be better to use e.g. DOMID_INVALID as the "give me whatever
> is available" indicator, allowing the function parameter to properly be
> domid_t.
Ack.
>
> But first of all - can we please take a step back and re-evaluate whether
> all of this re-arrangement that you're doing (not just in the patch here)
> is really needed? It seems to me that it must be possible to do whatever
> you ultimately want to do without re-writing quite a few pretty central
> pieces that have been serving us fine for a long time. That is, rather
> than make our code fit your desires, make your plans fit within the code
> base we have.
Thanks for the review and feedback!
My plan is to deliver the NS16550 emulator for x86 because that saves some
time during bring up of an embedded system.
Current xen console driver allows physical input pass-through to the domain.
This is implemented differently between arm and x86. Currently, the logic
of switching input depends on the global variable `max_init_domid`, which is
advanced on arm only (and never decreased, which makes sense for embedded
designs).
One of the planned emulator features is allowing guest OS to accept
physical input over the emulated UART. On x86, that includes dom0,
PV shim and, with emulator in place, also includes all domains with
NS16550 emulator enabled.
So I need to solve the problem of switching console input to another domain
on x86.
I see there are two ways to solve it:
(a) iterate through the list of known domains and check "UART emulator
present" property. AFAIU, that will require maintaining "current console
domain" pointer for a domain list walk.
(b) calculate the max known domain ID and then check whether domain ID
corresponds to a domain with "UART emulator present" property by iterating
through the range of domain IDs. This is current approach in the code.
So, this patch series solves problem of switching console input to another domain
on x86 using (b); as looked as less changes to me comparing to maintaining a
domain pointer.
re: code re-arrangements: while working on the emulator, getting more feedback,
this and other console-related cleanup series appeared and I think the code which
served fine for a long time, got a bit cleaner, easier to follow and, I hope,
a bit easier to maintain.
>
> Jan
Thanks,
Denis
On 09.04.2025 03:44, Denis Mukhin wrote: > On Tuesday, April 8th, 2025 at 7:26 AM, Jan Beulich <jbeulich@suse.com> wrote: >> On 01.04.2025 01:05, dmkhn@proton.me wrote: >> >>> From: Denis Mukhin dmukhin@ford.com >>> >>> Move domain ID allocation during domain creation to a dedicated >>> function domid_alloc(). >>> >>> Allocation algorithm: >>> - If an explicit domain ID is provided, verify its availability and >>> use it if ID is unused; >>> - Otherwise, perform an exhaustive search for the first available ID >>> within the [0..DOMID_FIRST_RESERVED) range, excluding hardware_domid. >>> >>> This minimizes the use of max_init_domid in the code and, thus, is a >>> prerequisite change for enabling console input rotation across domains >>> with console input permission on x86 platforms (which currently is >>> limited to dom0, PV shim and Xen). >> >> >> By removing the updating of max_init_domid you do - afaict - break the >> remaining use site(s) of the variable. > > Unfortunately, this patch should go together with the next patch in > this series: > xen/domain: introduce domid_top() > > I mentioned dependency in the cover letter. No, that would still break bisectability. Also such dependencies would better be mentioned in both the cover letter and the patch(es). Going back to the cover letter, all I find is "Patch 3 depends on patch 2." That's the natural thing in a series. Jan
© 2016 - 2025 Red Hat, Inc.