From: Denis Mukhin <dmukhin@ford.com>
The physical console input rotation depends on max_init_domid symbol, which is
managed differently across architectures.
Instead of trying to manage max_init_domid in the arch-common code the console
input rotation code can be reworked by removing dependency on max_init_domid
entirely.
To do that, introduce domid_find_with_input_allowed() in arch-independent
location to find the ID of the next possible console owner domain. The IDs
are rotated across non-system domain IDs and DOMID_XEN.
Also, introduce helper console_set_domid() for updating identifier of the
current console input owner (points to Xen or domain).
Use domid_find_with_input_allowed() and console_set_domid() in
console_switch_input().
Remove uses of max_init_domid in the code.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v4:
- fixed domid_find_with_input_allowed()
- kept switching to get_initial_domain_id() in console_endboot()
---
xen/arch/arm/include/asm/setup.h | 2 -
xen/arch/arm/setup.c | 2 -
xen/arch/ppc/include/asm/setup.h | 2 -
xen/arch/riscv/include/asm/setup.h | 2 -
xen/arch/x86/include/asm/setup.h | 2 -
xen/common/device-tree/dom0less-build.c | 2 -
xen/common/domain.c | 33 +++++++++
xen/drivers/char/console.c | 90 +++++++++----------------
xen/include/xen/domain.h | 1 +
9 files changed, 65 insertions(+), 71 deletions(-)
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 6cf272c160..f107e8eebb 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -25,8 +25,6 @@ struct map_range_data
struct rangeset *irq_ranges;
};
-extern domid_t max_init_domid;
-
void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
size_t estimate_efi_size(unsigned int mem_nr_banks);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 734e23da44..0a18d479f9 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -61,8 +61,6 @@ struct cpuinfo_arm __read_mostly system_cpuinfo;
bool __read_mostly acpi_disabled;
#endif
-domid_t __read_mostly max_init_domid;
-
static __used void init_done(void)
{
int rc;
diff --git a/xen/arch/ppc/include/asm/setup.h b/xen/arch/ppc/include/asm/setup.h
index e4f64879b6..956fa6985a 100644
--- a/xen/arch/ppc/include/asm/setup.h
+++ b/xen/arch/ppc/include/asm/setup.h
@@ -1,6 +1,4 @@
#ifndef __ASM_PPC_SETUP_H__
#define __ASM_PPC_SETUP_H__
-#define max_init_domid (0)
-
#endif /* __ASM_PPC_SETUP_H__ */
diff --git a/xen/arch/riscv/include/asm/setup.h b/xen/arch/riscv/include/asm/setup.h
index c9d69cdf51..d1fc64b673 100644
--- a/xen/arch/riscv/include/asm/setup.h
+++ b/xen/arch/riscv/include/asm/setup.h
@@ -5,8 +5,6 @@
#include <xen/types.h>
-#define max_init_domid (0)
-
void setup_mm(void);
void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index ac34c69855..b67de8577f 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -69,6 +69,4 @@ extern bool opt_dom0_verbose;
extern bool opt_dom0_cpuid_faulting;
extern bool opt_dom0_msr_relaxed;
-#define max_init_domid (0)
-
#endif
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 9a6015f4ce..703f20faed 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -977,8 +977,6 @@ void __init create_domUs(void)
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 d75ece1b61..4a54bc27a3 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
spin_unlock(&domid_lock);
}
+/*
+ * Find the ID of the next possible console owner domain.
+ *
+ * @return Domain ID: DOMID_XEN or non-system domain IDs within
+ * the range of [0..DOMID_FIRST_RESERVED-1].
+ */
+domid_t domid_find_with_input_allowed(domid_t hint)
+{
+ domid_t domid = DOMID_XEN;
+
+ if ( hint < DOMID_FIRST_RESERVED )
+ {
+ struct domain *d;
+
+ rcu_read_lock(&domlist_read_lock);
+
+ for ( d = domid_to_domain(hint);
+ d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
+ d = rcu_dereference(d->next_in_list) )
+ {
+ if ( d->console.input_allowed )
+ {
+ domid = d->domain_id;
+ break;
+ }
+ }
+
+ rcu_read_unlock(&domlist_read_lock);
+ }
+
+ return domid;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 9a9836ba91..37289d5558 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -498,26 +498,17 @@ static void cf_check conring_dump_keyhandler(unsigned char key)
/*
* CTRL-<switch_char> changes input direction, rotating among Xen, Dom0,
- * and the DomUs started from Xen at boot.
+ * and the DomUs.
*/
#define switch_code (opt_conswitch[0]-'a'+1)
-/*
- * console_rx=0 => input to xen
- * console_rx=1 => input to dom0 (or the sole shim domain)
- * console_rx=N => input to dom(N-1)
- */
-static unsigned int __read_mostly console_rx = 0;
-#define max_console_rx (max_init_domid + 1)
+/* Console owner domain identifier. */
+static domid_t __read_mostly console_rx = DOMID_XEN;
struct domain *console_get_domain(void)
{
- struct domain *d;
+ struct domain *d = rcu_lock_domain_by_id(console_rx);
- if ( console_rx == 0 )
- return NULL;
-
- d = rcu_lock_domain_by_id(console_rx - 1);
if ( !d )
return NULL;
@@ -535,43 +526,14 @@ void console_put_domain(struct domain *d)
rcu_unlock_domain(d);
}
-static void console_switch_input(void)
+static void console_set_domid(domid_t domid)
{
- unsigned int next_rx = console_rx;
+ if ( domid == DOMID_XEN )
+ printk("*** Serial input to Xen");
+ else
+ printk("*** Serial input to DOM%u", domid);
- /*
- * Rotate among Xen, dom0 and boot-time created domUs while skipping
- * switching serial input to non existing domains.
- */
- for ( ; ; )
- {
- domid_t domid;
- struct domain *d;
-
- if ( next_rx++ >= max_console_rx )
- {
- console_rx = 0;
- printk("*** Serial input to Xen");
- break;
- }
-
- if ( consoled_is_enabled() && next_rx == 1 )
- domid = get_initial_domain_id();
- else
- domid = next_rx - 1;
- d = rcu_lock_domain_by_id(domid);
- if ( d )
- {
- rcu_unlock_domain(d);
-
- if ( !d->console.input_allowed )
- continue;
-
- console_rx = next_rx;
- printk("*** Serial input to DOM%u", domid);
- break;
- }
- }
+ console_rx = domid;
if ( switch_code )
printk(" (type 'CTRL-%c' three times to switch input)",
@@ -579,12 +541,30 @@ static void console_switch_input(void)
printk("\n");
}
+/*
+ * Switch console focus.
+ * Rotates input focus among Xen and domains with console input permission.
+ */
+static void console_switch_input(void)
+{
+ domid_t hint;
+
+ if ( console_rx == DOMID_XEN )
+ hint = get_initial_domain_id();
+ else
+ hint = console_rx + 1;
+
+ hint = domid_find_with_input_allowed(hint);
+
+ console_set_domid(hint);
+}
+
static void __serial_rx(char c)
{
struct domain *d;
int rc = 0;
- if ( console_rx == 0 )
+ if ( console_rx == DOMID_XEN )
return handle_keypress(c, false);
d = console_get_domain();
@@ -1169,14 +1149,6 @@ void __init console_endboot(void)
video_endboot();
- /*
- * If user specifies so, we fool the switch routine to redirect input
- * straight back to Xen. I use this convoluted method so we still print
- * a useful 'how to switch' message.
- */
- if ( opt_conswitch[1] == 'x' )
- console_rx = max_console_rx;
-
register_keyhandler('w', conring_dump_keyhandler,
"synchronously dump console ring buffer (dmesg)", 0);
register_irq_keyhandler('+', &do_inc_thresh,
@@ -1186,8 +1158,8 @@ void __init console_endboot(void)
register_irq_keyhandler('G', &do_toggle_guest,
"toggle host/guest log level adjustment", 0);
- /* Serial input is directed to DOM0 by default. */
- console_switch_input();
+ if ( opt_conswitch[1] != 'x' )
+ (void)console_set_domid(get_initial_domain_id());
}
int __init console_has(const char *device)
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 8aab05ae93..a88eb34f3f 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -36,6 +36,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info);
void arch_get_domain_info(const struct domain *d,
struct xen_domctl_getdomaininfo *info);
+domid_t domid_find_with_input_allowed(domid_t hint);
domid_t get_initial_domain_id(void);
domid_t domid_alloc(domid_t domid);
--
2.34.1On 31.05.2025 01:19, dmkhn@proton.me wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
> spin_unlock(&domid_lock);
> }
>
> +/*
> + * Find the ID of the next possible console owner domain.
> + *
> + * @return Domain ID: DOMID_XEN or non-system domain IDs within
> + * the range of [0..DOMID_FIRST_RESERVED-1].
> + */
> +domid_t domid_find_with_input_allowed(domid_t hint)
> +{
> + domid_t domid = DOMID_XEN;
> +
> + if ( hint < DOMID_FIRST_RESERVED )
> + {
> + struct domain *d;
> +
> + rcu_read_lock(&domlist_read_lock);
> +
> + for ( d = domid_to_domain(hint);
If the domain with ID "hint" went away, what is being switched to changes
compared to behavior prior to this patch, if I'm not mistaken. While this
_may_ be acceptable, not saying so in the description is imo a no-go.
> + d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
What's the DOMID_FIRST_RESERVED check for? And where's the put_domain()
for the get_domain() here?
> + d = rcu_dereference(d->next_in_list) )
> + {
> + if ( d->console.input_allowed )
> + {
> + domid = d->domain_id;
> + break;
> + }
> + }
> +
> + rcu_read_unlock(&domlist_read_lock);
> + }
> +
> + return domid;
> +}
My concern remains: With many domains, the loop here may take quite a few
iterations. That's even more concerning because it regresses right away in
environments where along with boot-time created domains (eligible for
console focus) later further domains are created (non of which are eligible
for console focus). That is, the step from last boot-time created back to
DOMID_XEN may now take excessively long.
Jan
On Wed, Jun 04, 2025 at 02:55:40PM +0200, Jan Beulich wrote:
> On 31.05.2025 01:19, dmkhn@proton.me wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
> > spin_unlock(&domid_lock);
> > }
> >
> > +/*
> > + * Find the ID of the next possible console owner domain.
> > + *
> > + * @return Domain ID: DOMID_XEN or non-system domain IDs within
> > + * the range of [0..DOMID_FIRST_RESERVED-1].
> > + */
> > +domid_t domid_find_with_input_allowed(domid_t hint)
> > +{
> > + domid_t domid = DOMID_XEN;
> > +
> > + if ( hint < DOMID_FIRST_RESERVED )
> > + {
> > + struct domain *d;
> > +
> > + rcu_read_lock(&domlist_read_lock);
> > +
> > + for ( d = domid_to_domain(hint);
>
> If the domain with ID "hint" went away, what is being switched to changes
> compared to behavior prior to this patch, if I'm not mistaken. While this
> _may_ be acceptable, not saying so in the description is imo a no-go.
Will correct, thanks.
>
> > + d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
>
> What's the DOMID_FIRST_RESERVED check for? And where's the put_domain()
> for the get_domain() here?
>
> > + d = rcu_dereference(d->next_in_list) )
> > + {
> > + if ( d->console.input_allowed )
> > + {
> > + domid = d->domain_id;
> > + break;
> > + }
> > + }
> > +
> > + rcu_read_unlock(&domlist_read_lock);
> > + }
> > +
> > + return domid;
> > +}
>
> My concern remains: With many domains, the loop here may take quite a few
> iterations. That's even more concerning because it regresses right away in
> environments where along with boot-time created domains (eligible for
> console focus) later further domains are created (non of which are eligible
> for console focus). That is, the step from last boot-time created back to
> DOMID_XEN may now take excessively long.
Julien in the other reply suggested to have a list of domains with console
input permission only. Will rework.
>
> Jan
Hi Jan,
On 04/06/2025 13:55, Jan Beulich wrote:
> On 31.05.2025 01:19, dmkhn@proton.me wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
>> spin_unlock(&domid_lock);
>> }
>>
>> +/*
>> + * Find the ID of the next possible console owner domain.
>> + *
>> + * @return Domain ID: DOMID_XEN or non-system domain IDs within
>> + * the range of [0..DOMID_FIRST_RESERVED-1].
>> + */
>> +domid_t domid_find_with_input_allowed(domid_t hint)
>> +{
>> + domid_t domid = DOMID_XEN;
>> +
>> + if ( hint < DOMID_FIRST_RESERVED )
>> + {
>> + struct domain *d;
>> +
>> + rcu_read_lock(&domlist_read_lock);
>> +
>> + for ( d = domid_to_domain(hint);
>
> If the domain with ID "hint" went away, what is being switched to changes
> compared to behavior prior to this patch, if I'm not mistaken. While this
> _may_ be acceptable, not saying so in the description is imo a no-go.
>
>> + d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
>
> What's the DOMID_FIRST_RESERVED check for? And where's the put_domain()
> for the get_domain() here?
>
>> + d = rcu_dereference(d->next_in_list) )
>> + {
>> + if ( d->console.input_allowed )
>> + {
>> + domid = d->domain_id;
>> + break;
>> + }
>> + }
>> +
>> + rcu_read_unlock(&domlist_read_lock);
>> + }
>> +
>> + return domid;
>> +}
>
> My concern remains: With many domains, the loop here may take quite a few
> iterations. That's even more concerning because it regresses right away in
> environments where along with boot-time created domains (eligible for
> console focus) later further domains are created (non of which are eligible
> for console focus). That is, the step from last boot-time created back to
> DOMID_XEN may now take excessively long.
+1. I vaguely recall making the same concern earlier in the review. One
possibility would be to have a list of domains where the console is
supported. I don't think it would be necessary to have the list sorted
as I view the console switching a debug facility.
Cheers,
--
Julien Grall
On Thu, Jun 05, 2025 at 11:20:32PM +0100, Julien Grall wrote:
> Hi Jan,
>
> On 04/06/2025 13:55, Jan Beulich wrote:
> > On 31.05.2025 01:19, dmkhn@proton.me wrote:
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
> >> spin_unlock(&domid_lock);
> >> }
> >>
> >> +/*
> >> + * Find the ID of the next possible console owner domain.
> >> + *
> >> + * @return Domain ID: DOMID_XEN or non-system domain IDs within
> >> + * the range of [0..DOMID_FIRST_RESERVED-1].
> >> + */
> >> +domid_t domid_find_with_input_allowed(domid_t hint)
> >> +{
> >> + domid_t domid = DOMID_XEN;
> >> +
> >> + if ( hint < DOMID_FIRST_RESERVED )
> >> + {
> >> + struct domain *d;
> >> +
> >> + rcu_read_lock(&domlist_read_lock);
> >> +
> >> + for ( d = domid_to_domain(hint);
> >
> > If the domain with ID "hint" went away, what is being switched to changes
> > compared to behavior prior to this patch, if I'm not mistaken. While this
> > _may_ be acceptable, not saying so in the description is imo a no-go.
> >
> >> + d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
> >
> > What's the DOMID_FIRST_RESERVED check for? And where's the put_domain()
> > for the get_domain() here?
> >
> >> + d = rcu_dereference(d->next_in_list) )
> >> + {
> >> + if ( d->console.input_allowed )
> >> + {
> >> + domid = d->domain_id;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + rcu_read_unlock(&domlist_read_lock);
> >> + }
> >> +
> >> + return domid;
> >> +}
> >
> > My concern remains: With many domains, the loop here may take quite a few
> > iterations. That's even more concerning because it regresses right away in
> > environments where along with boot-time created domains (eligible for
> > console focus) later further domains are created (non of which are eligible
> > for console focus). That is, the step from last boot-time created back to
> > DOMID_XEN may now take excessively long.
>
> +1. I vaguely recall making the same concern earlier in the review. One
> possibility would be to have a list of domains where the console is
> supported. I don't think it would be necessary to have the list sorted
> as I view the console switching a debug facility.
Jan, Julien,
Thanks for the feedback!
Will rework.
>
> Cheers,
>
> --
> Julien Grall
>
>
On Fri, 30 May 2025, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
>
> The physical console input rotation depends on max_init_domid symbol, which is
> managed differently across architectures.
>
> Instead of trying to manage max_init_domid in the arch-common code the console
> input rotation code can be reworked by removing dependency on max_init_domid
> entirely.
>
> To do that, introduce domid_find_with_input_allowed() in arch-independent
> location to find the ID of the next possible console owner domain. The IDs
> are rotated across non-system domain IDs and DOMID_XEN.
>
> Also, introduce helper console_set_domid() for updating identifier of the
> current console input owner (points to Xen or domain).
>
> Use domid_find_with_input_allowed() and console_set_domid() in
> console_switch_input().
>
> Remove uses of max_init_domid in the code.
>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v4:
> - fixed domid_find_with_input_allowed()
> - kept switching to get_initial_domain_id() in console_endboot()
> ---
> xen/arch/arm/include/asm/setup.h | 2 -
> xen/arch/arm/setup.c | 2 -
> xen/arch/ppc/include/asm/setup.h | 2 -
> xen/arch/riscv/include/asm/setup.h | 2 -
> xen/arch/x86/include/asm/setup.h | 2 -
> xen/common/device-tree/dom0less-build.c | 2 -
> xen/common/domain.c | 33 +++++++++
> xen/drivers/char/console.c | 90 +++++++++----------------
> xen/include/xen/domain.h | 1 +
> 9 files changed, 65 insertions(+), 71 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 6cf272c160..f107e8eebb 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -25,8 +25,6 @@ struct map_range_data
> struct rangeset *irq_ranges;
> };
>
> -extern domid_t max_init_domid;
> -
> void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
>
> size_t estimate_efi_size(unsigned int mem_nr_banks);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 734e23da44..0a18d479f9 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -61,8 +61,6 @@ struct cpuinfo_arm __read_mostly system_cpuinfo;
> bool __read_mostly acpi_disabled;
> #endif
>
> -domid_t __read_mostly max_init_domid;
> -
> static __used void init_done(void)
> {
> int rc;
> diff --git a/xen/arch/ppc/include/asm/setup.h b/xen/arch/ppc/include/asm/setup.h
> index e4f64879b6..956fa6985a 100644
> --- a/xen/arch/ppc/include/asm/setup.h
> +++ b/xen/arch/ppc/include/asm/setup.h
> @@ -1,6 +1,4 @@
> #ifndef __ASM_PPC_SETUP_H__
> #define __ASM_PPC_SETUP_H__
>
> -#define max_init_domid (0)
> -
> #endif /* __ASM_PPC_SETUP_H__ */
> diff --git a/xen/arch/riscv/include/asm/setup.h b/xen/arch/riscv/include/asm/setup.h
> index c9d69cdf51..d1fc64b673 100644
> --- a/xen/arch/riscv/include/asm/setup.h
> +++ b/xen/arch/riscv/include/asm/setup.h
> @@ -5,8 +5,6 @@
>
> #include <xen/types.h>
>
> -#define max_init_domid (0)
> -
> void setup_mm(void);
>
> void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
> index ac34c69855..b67de8577f 100644
> --- a/xen/arch/x86/include/asm/setup.h
> +++ b/xen/arch/x86/include/asm/setup.h
> @@ -69,6 +69,4 @@ extern bool opt_dom0_verbose;
> extern bool opt_dom0_cpuid_faulting;
> extern bool opt_dom0_msr_relaxed;
>
> -#define max_init_domid (0)
> -
> #endif
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 9a6015f4ce..703f20faed 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -977,8 +977,6 @@ void __init create_domUs(void)
> 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 d75ece1b61..4a54bc27a3 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
> spin_unlock(&domid_lock);
> }
>
> +/*
> + * Find the ID of the next possible console owner domain.
> + *
> + * @return Domain ID: DOMID_XEN or non-system domain IDs within
> + * the range of [0..DOMID_FIRST_RESERVED-1].
> + */
> +domid_t domid_find_with_input_allowed(domid_t hint)
> +{
> + domid_t domid = DOMID_XEN;
> +
> + if ( hint < DOMID_FIRST_RESERVED )
> + {
> + struct domain *d;
> +
> + rcu_read_lock(&domlist_read_lock);
> +
> + for ( d = domid_to_domain(hint);
> + d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
The get_domain(d) worries me because it is increasing the domain's
refcnt but I don't see a corresponding call to put_domain to decrease
it.
If I keep rotating between consoles, I could keep increasing refcnt
indefinitely?
I think we either need a corresponding put_domain(d) call when the domain
loses input focus, or we remove the get_domain(d) based on the fact that
we don't need it. I think before this patch we didn't increment refcnt
when a domain has focus but I am not sure it was correct.
> + d = rcu_dereference(d->next_in_list) )
> + {
> + if ( d->console.input_allowed )
> + {
> + domid = d->domain_id;
> + break;
> + }
> + }
> +
> + rcu_read_unlock(&domlist_read_lock);
> + }
> +
> + return domid;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 9a9836ba91..37289d5558 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -498,26 +498,17 @@ static void cf_check conring_dump_keyhandler(unsigned char key)
>
> /*
> * CTRL-<switch_char> changes input direction, rotating among Xen, Dom0,
> - * and the DomUs started from Xen at boot.
> + * and the DomUs.
> */
> #define switch_code (opt_conswitch[0]-'a'+1)
> -/*
> - * console_rx=0 => input to xen
> - * console_rx=1 => input to dom0 (or the sole shim domain)
> - * console_rx=N => input to dom(N-1)
> - */
> -static unsigned int __read_mostly console_rx = 0;
>
> -#define max_console_rx (max_init_domid + 1)
> +/* Console owner domain identifier. */
> +static domid_t __read_mostly console_rx = DOMID_XEN;
>
> struct domain *console_get_domain(void)
> {
> - struct domain *d;
> + struct domain *d = rcu_lock_domain_by_id(console_rx);
>
> - if ( console_rx == 0 )
> - return NULL;
> -
> - d = rcu_lock_domain_by_id(console_rx - 1);
> if ( !d )
> return NULL;
>
> @@ -535,43 +526,14 @@ void console_put_domain(struct domain *d)
> rcu_unlock_domain(d);
> }
>
> -static void console_switch_input(void)
> +static void console_set_domid(domid_t domid)
> {
> - unsigned int next_rx = console_rx;
> + if ( domid == DOMID_XEN )
> + printk("*** Serial input to Xen");
> + else
> + printk("*** Serial input to DOM%u", domid);
>
> - /*
> - * Rotate among Xen, dom0 and boot-time created domUs while skipping
> - * switching serial input to non existing domains.
> - */
> - for ( ; ; )
> - {
> - domid_t domid;
> - struct domain *d;
> -
> - if ( next_rx++ >= max_console_rx )
> - {
> - console_rx = 0;
> - printk("*** Serial input to Xen");
> - break;
> - }
> -
> - if ( consoled_is_enabled() && next_rx == 1 )
> - domid = get_initial_domain_id();
> - else
> - domid = next_rx - 1;
> - d = rcu_lock_domain_by_id(domid);
> - if ( d )
> - {
> - rcu_unlock_domain(d);
> -
> - if ( !d->console.input_allowed )
> - continue;
> -
> - console_rx = next_rx;
> - printk("*** Serial input to DOM%u", domid);
> - break;
> - }
> - }
> + console_rx = domid;
>
> if ( switch_code )
> printk(" (type 'CTRL-%c' three times to switch input)",
> @@ -579,12 +541,30 @@ static void console_switch_input(void)
> printk("\n");
> }
>
> +/*
> + * Switch console focus.
> + * Rotates input focus among Xen and domains with console input permission.
> + */
> +static void console_switch_input(void)
> +{
> + domid_t hint;
> +
> + if ( console_rx == DOMID_XEN )
> + hint = get_initial_domain_id();
> + else
> + hint = console_rx + 1;
> +
> + hint = domid_find_with_input_allowed(hint);
> +
> + console_set_domid(hint);
> +}
> +
> static void __serial_rx(char c)
> {
> struct domain *d;
> int rc = 0;
>
> - if ( console_rx == 0 )
> + if ( console_rx == DOMID_XEN )
> return handle_keypress(c, false);
>
> d = console_get_domain();
> @@ -1169,14 +1149,6 @@ void __init console_endboot(void)
>
> video_endboot();
>
> - /*
> - * If user specifies so, we fool the switch routine to redirect input
> - * straight back to Xen. I use this convoluted method so we still print
> - * a useful 'how to switch' message.
> - */
> - if ( opt_conswitch[1] == 'x' )
> - console_rx = max_console_rx;
> -
> register_keyhandler('w', conring_dump_keyhandler,
> "synchronously dump console ring buffer (dmesg)", 0);
> register_irq_keyhandler('+', &do_inc_thresh,
> @@ -1186,8 +1158,8 @@ void __init console_endboot(void)
> register_irq_keyhandler('G', &do_toggle_guest,
> "toggle host/guest log level adjustment", 0);
>
> - /* Serial input is directed to DOM0 by default. */
> - console_switch_input();
> + if ( opt_conswitch[1] != 'x' )
> + (void)console_set_domid(get_initial_domain_id());
> }
>
> int __init console_has(const char *device)
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 8aab05ae93..a88eb34f3f 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -36,6 +36,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info);
> void arch_get_domain_info(const struct domain *d,
> struct xen_domctl_getdomaininfo *info);
>
> +domid_t domid_find_with_input_allowed(domid_t hint);
> domid_t get_initial_domain_id(void);
>
> domid_t domid_alloc(domid_t domid);
> --
> 2.34.1
>
>
On 03.06.2025 02:36, Stefano Stabellini wrote:
> On Fri, 30 May 2025, dmkhn@proton.me wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
>> spin_unlock(&domid_lock);
>> }
>>
>> +/*
>> + * Find the ID of the next possible console owner domain.
>> + *
>> + * @return Domain ID: DOMID_XEN or non-system domain IDs within
>> + * the range of [0..DOMID_FIRST_RESERVED-1].
>> + */
>> +domid_t domid_find_with_input_allowed(domid_t hint)
>> +{
>> + domid_t domid = DOMID_XEN;
>> +
>> + if ( hint < DOMID_FIRST_RESERVED )
>> + {
>> + struct domain *d;
>> +
>> + rcu_read_lock(&domlist_read_lock);
>> +
>> + for ( d = domid_to_domain(hint);
>> + d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
>
> The get_domain(d) worries me because it is increasing the domain's
> refcnt but I don't see a corresponding call to put_domain to decrease
> it.
>
> If I keep rotating between consoles, I could keep increasing refcnt
> indefinitely?
>
> I think we either need a corresponding put_domain(d) call when the domain
> loses input focus, or we remove the get_domain(d) based on the fact that
> we don't need it. I think before this patch we didn't increment refcnt
> when a domain has focus but I am not sure it was correct.
I think it was. The code was - aiui - specifically prepared to deal with
domains going away behind its back. A domain having input focus should
not prevent it from being (fully) destroyed.
Jan
On Mon, Jun 02, 2025 at 05:36:25PM -0700, Stefano Stabellini wrote:
> On Fri, 30 May 2025, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > The physical console input rotation depends on max_init_domid symbol, which is
> > managed differently across architectures.
> >
> > Instead of trying to manage max_init_domid in the arch-common code the console
> > input rotation code can be reworked by removing dependency on max_init_domid
> > entirely.
> >
> > To do that, introduce domid_find_with_input_allowed() in arch-independent
> > location to find the ID of the next possible console owner domain. The IDs
> > are rotated across non-system domain IDs and DOMID_XEN.
> >
> > Also, introduce helper console_set_domid() for updating identifier of the
> > current console input owner (points to Xen or domain).
> >
> > Use domid_find_with_input_allowed() and console_set_domid() in
> > console_switch_input().
> >
> > Remove uses of max_init_domid in the code.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v4:
> > - fixed domid_find_with_input_allowed()
> > - kept switching to get_initial_domain_id() in console_endboot()
> > ---
> > xen/arch/arm/include/asm/setup.h | 2 -
> > xen/arch/arm/setup.c | 2 -
> > xen/arch/ppc/include/asm/setup.h | 2 -
> > xen/arch/riscv/include/asm/setup.h | 2 -
> > xen/arch/x86/include/asm/setup.h | 2 -
> > xen/common/device-tree/dom0less-build.c | 2 -
> > xen/common/domain.c | 33 +++++++++
> > xen/drivers/char/console.c | 90 +++++++++----------------
> > xen/include/xen/domain.h | 1 +
> > 9 files changed, 65 insertions(+), 71 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> > index 6cf272c160..f107e8eebb 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -25,8 +25,6 @@ struct map_range_data
> > struct rangeset *irq_ranges;
> > };
> >
> > -extern domid_t max_init_domid;
> > -
> > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> >
> > size_t estimate_efi_size(unsigned int mem_nr_banks);
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 734e23da44..0a18d479f9 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -61,8 +61,6 @@ struct cpuinfo_arm __read_mostly system_cpuinfo;
> > bool __read_mostly acpi_disabled;
> > #endif
> >
> > -domid_t __read_mostly max_init_domid;
> > -
> > static __used void init_done(void)
> > {
> > int rc;
> > diff --git a/xen/arch/ppc/include/asm/setup.h b/xen/arch/ppc/include/asm/setup.h
> > index e4f64879b6..956fa6985a 100644
> > --- a/xen/arch/ppc/include/asm/setup.h
> > +++ b/xen/arch/ppc/include/asm/setup.h
> > @@ -1,6 +1,4 @@
> > #ifndef __ASM_PPC_SETUP_H__
> > #define __ASM_PPC_SETUP_H__
> >
> > -#define max_init_domid (0)
> > -
> > #endif /* __ASM_PPC_SETUP_H__ */
> > diff --git a/xen/arch/riscv/include/asm/setup.h b/xen/arch/riscv/include/asm/setup.h
> > index c9d69cdf51..d1fc64b673 100644
> > --- a/xen/arch/riscv/include/asm/setup.h
> > +++ b/xen/arch/riscv/include/asm/setup.h
> > @@ -5,8 +5,6 @@
> >
> > #include <xen/types.h>
> >
> > -#define max_init_domid (0)
> > -
> > void setup_mm(void);
> >
> > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> > diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
> > index ac34c69855..b67de8577f 100644
> > --- a/xen/arch/x86/include/asm/setup.h
> > +++ b/xen/arch/x86/include/asm/setup.h
> > @@ -69,6 +69,4 @@ extern bool opt_dom0_verbose;
> > extern bool opt_dom0_cpuid_faulting;
> > extern bool opt_dom0_msr_relaxed;
> >
> > -#define max_init_domid (0)
> > -
> > #endif
> > diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> > index 9a6015f4ce..703f20faed 100644
> > --- a/xen/common/device-tree/dom0less-build.c
> > +++ b/xen/common/device-tree/dom0less-build.c
> > @@ -977,8 +977,6 @@ void __init create_domUs(void)
> > 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 d75ece1b61..4a54bc27a3 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -2461,6 +2461,39 @@ void domid_free(domid_t domid)
> > spin_unlock(&domid_lock);
> > }
> >
> > +/*
> > + * Find the ID of the next possible console owner domain.
> > + *
> > + * @return Domain ID: DOMID_XEN or non-system domain IDs within
> > + * the range of [0..DOMID_FIRST_RESERVED-1].
> > + */
> > +domid_t domid_find_with_input_allowed(domid_t hint)
> > +{
> > + domid_t domid = DOMID_XEN;
> > +
> > + if ( hint < DOMID_FIRST_RESERVED )
> > + {
> > + struct domain *d;
> > +
> > + rcu_read_lock(&domlist_read_lock);
> > +
> > + for ( d = domid_to_domain(hint);
> > + d && get_domain(d) && d->domain_id < DOMID_FIRST_RESERVED;
>
> The get_domain(d) worries me because it is increasing the domain's
> refcnt but I don't see a corresponding call to put_domain to decrease
> it.
>
> If I keep rotating between consoles, I could keep increasing refcnt
> indefinitely?
>
> I think we either need a corresponding put_domain(d) call when the domain
> loses input focus, or we remove the get_domain(d) based on the fact that
> we don't need it. I think before this patch we didn't increment refcnt
> when a domain has focus but I am not sure it was correct.
Thank you for catching this!
My understanding the code should be doing refcnt-ing properly to
access the domain fields, so there has to be a put_domain() call.
Will update.
>
>
> > + d = rcu_dereference(d->next_in_list) )
> > + {
> > + if ( d->console.input_allowed )
> > + {
> > + domid = d->domain_id;
> > + break;
> > + }
> > + }
> > +
> > + rcu_read_unlock(&domlist_read_lock);
> > + }
> > +
> > + return domid;
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 9a9836ba91..37289d5558 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -498,26 +498,17 @@ static void cf_check conring_dump_keyhandler(unsigned char key)
> >
> > /*
> > * CTRL-<switch_char> changes input direction, rotating among Xen, Dom0,
> > - * and the DomUs started from Xen at boot.
> > + * and the DomUs.
> > */
> > #define switch_code (opt_conswitch[0]-'a'+1)
> > -/*
> > - * console_rx=0 => input to xen
> > - * console_rx=1 => input to dom0 (or the sole shim domain)
> > - * console_rx=N => input to dom(N-1)
> > - */
> > -static unsigned int __read_mostly console_rx = 0;
> >
> > -#define max_console_rx (max_init_domid + 1)
> > +/* Console owner domain identifier. */
> > +static domid_t __read_mostly console_rx = DOMID_XEN;
> >
> > struct domain *console_get_domain(void)
> > {
> > - struct domain *d;
> > + struct domain *d = rcu_lock_domain_by_id(console_rx);
> >
> > - if ( console_rx == 0 )
> > - return NULL;
> > -
> > - d = rcu_lock_domain_by_id(console_rx - 1);
> > if ( !d )
> > return NULL;
> >
> > @@ -535,43 +526,14 @@ void console_put_domain(struct domain *d)
> > rcu_unlock_domain(d);
> > }
> >
> > -static void console_switch_input(void)
> > +static void console_set_domid(domid_t domid)
> > {
> > - unsigned int next_rx = console_rx;
> > + if ( domid == DOMID_XEN )
> > + printk("*** Serial input to Xen");
> > + else
> > + printk("*** Serial input to DOM%u", domid);
> >
> > - /*
> > - * Rotate among Xen, dom0 and boot-time created domUs while skipping
> > - * switching serial input to non existing domains.
> > - */
> > - for ( ; ; )
> > - {
> > - domid_t domid;
> > - struct domain *d;
> > -
> > - if ( next_rx++ >= max_console_rx )
> > - {
> > - console_rx = 0;
> > - printk("*** Serial input to Xen");
> > - break;
> > - }
> > -
> > - if ( consoled_is_enabled() && next_rx == 1 )
> > - domid = get_initial_domain_id();
> > - else
> > - domid = next_rx - 1;
> > - d = rcu_lock_domain_by_id(domid);
> > - if ( d )
> > - {
> > - rcu_unlock_domain(d);
> > -
> > - if ( !d->console.input_allowed )
> > - continue;
> > -
> > - console_rx = next_rx;
> > - printk("*** Serial input to DOM%u", domid);
> > - break;
> > - }
> > - }
> > + console_rx = domid;
> >
> > if ( switch_code )
> > printk(" (type 'CTRL-%c' three times to switch input)",
> > @@ -579,12 +541,30 @@ static void console_switch_input(void)
> > printk("\n");
> > }
> >
> > +/*
> > + * Switch console focus.
> > + * Rotates input focus among Xen and domains with console input permission.
> > + */
> > +static void console_switch_input(void)
> > +{
> > + domid_t hint;
> > +
> > + if ( console_rx == DOMID_XEN )
> > + hint = get_initial_domain_id();
> > + else
> > + hint = console_rx + 1;
> > +
> > + hint = domid_find_with_input_allowed(hint);
> > +
> > + console_set_domid(hint);
> > +}
> > +
> > static void __serial_rx(char c)
> > {
> > struct domain *d;
> > int rc = 0;
> >
> > - if ( console_rx == 0 )
> > + if ( console_rx == DOMID_XEN )
> > return handle_keypress(c, false);
> >
> > d = console_get_domain();
> > @@ -1169,14 +1149,6 @@ void __init console_endboot(void)
> >
> > video_endboot();
> >
> > - /*
> > - * If user specifies so, we fool the switch routine to redirect input
> > - * straight back to Xen. I use this convoluted method so we still print
> > - * a useful 'how to switch' message.
> > - */
> > - if ( opt_conswitch[1] == 'x' )
> > - console_rx = max_console_rx;
> > -
> > register_keyhandler('w', conring_dump_keyhandler,
> > "synchronously dump console ring buffer (dmesg)", 0);
> > register_irq_keyhandler('+', &do_inc_thresh,
> > @@ -1186,8 +1158,8 @@ void __init console_endboot(void)
> > register_irq_keyhandler('G', &do_toggle_guest,
> > "toggle host/guest log level adjustment", 0);
> >
> > - /* Serial input is directed to DOM0 by default. */
> > - console_switch_input();
> > + if ( opt_conswitch[1] != 'x' )
> > + (void)console_set_domid(get_initial_domain_id());
> > }
> >
> > int __init console_has(const char *device)
> > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> > index 8aab05ae93..a88eb34f3f 100644
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -36,6 +36,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info);
> > void arch_get_domain_info(const struct domain *d,
> > struct xen_domctl_getdomaininfo *info);
> >
> > +domid_t domid_find_with_input_allowed(domid_t hint);
> > domid_t get_initial_domain_id(void);
> >
> > domid_t domid_alloc(domid_t domid);
> > --
> > 2.34.1
> >
> >
© 2016 - 2025 Red Hat, Inc.