| Message ID | 20241205-vuart-ns8250-v1-19-e9aa923127eb@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce NS8250 UART emulator | expand |
On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote: > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -463,82 +463,100 @@ static void cf_check dump_console_ring_key(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_owner=0 => input to xen > - * console_owner=1 => input to dom0 (or the sole shim domain) > - * console_owner=N => input to dom(N-1) > + * Current console owner domain ID: either Xen or domain w/ d->is_console == > + * true. The switching of number space may better have been a separate patch. Albeit maybe I'm just not seeing why it wants combining with the introduction of console_set_owner(). Actually, is this switching actually complete? What about late hwdom, which has a non-zero domain ID? > + * Initialized in console_endboot(). > */ > -static unsigned int __read_mostly console_owner = 0; > +static domid_t __read_mostly console_owner; > > -#define max_console_rx (max_init_domid + 1) > +static struct domain *rcu_lock_domain_console_by_id(domid_t domid) I think "domain" and "console" want switching in the name, as it's a domain you're locking, not a console. > +int console_set_owner(domid_t domid) static? Iirc Misra doesn't like non-static functions which aren't called from any other CU. Jan
On Thu, Dec 05, 2024 at 08:41:49PM -0800, Denis Mukhin via B4 Relay wrote: > From: Denis Mukhin <dmukhin@ford.com> > > console_set_owner() is introduced for setting the new console owner. > > Switches console owner to domain ID vs range of integer numbers mapped to > domain IDs. > > This a public API to console driver, will be used in the follow on code change. > > Signed-off-by: Denis Mukhin <dmukhin@ford.com> > --- > xen/drivers/char/console.c | 122 ++++++++++++++++++++++++++------------------- > xen/include/xen/console.h | 1 + > 2 files changed, 71 insertions(+), 52 deletions(-) > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 8cbac54c66044ae8581e486a782102b75c8bfaa9..52cf64dbf6fd18d599cb88835d03501a23b3e3c4 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -463,82 +463,100 @@ static void cf_check dump_console_ring_key(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_owner=0 => input to xen > - * console_owner=1 => input to dom0 (or the sole shim domain) > - * console_owner=N => input to dom(N-1) > + * Current console owner domain ID: either Xen or domain w/ d->is_console == > + * true. > + * > + * Initialized in console_endboot(). > */ > -static unsigned int __read_mostly console_owner = 0; > +static domid_t __read_mostly console_owner; Should this be initialized to DOMID_XEN? I assume it doesn't make much difference because the variable is not checked before console_endboot() anyway, but it might be safer to initialize to a value that assigns the console to Xen. > > -#define max_console_rx (max_init_domid + 1) > +static struct domain *rcu_lock_domain_console_by_id(domid_t domid) > +{ > + struct domain *d; > + > + d = rcu_lock_domain_by_id(domid); > + Nit: I would remove this newline. > + if ( d == NULL ) > + return NULL; > + > + if ( d->is_console ) > + return d; > + > + rcu_unlock_domain(d); > + > + return NULL; > +} > > -#ifdef CONFIG_SBSA_VUART_CONSOLE > /* Make sure to rcu_unlock_domain after use */ > struct domain *rcu_lock_domain_console_owner(void) > { > - if ( console_owner == 0 ) > - return NULL; > - return rcu_lock_domain_by_id(console_owner - 1); > + return rcu_lock_domain_console_by_id(console_owner); > } > -#endif > > -static void console_find_owner(void) > +static bool console_owner_possible(domid_t domid) > { > - unsigned int next_rx = console_owner; > - > - /* > - * 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_owner = 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 == NULL ) > - continue; > - > - if ( d->is_console ) > - { > - rcu_unlock_domain(d); > - console_owner = next_rx; > - printk("*** Serial input to DOM%u", domid); > - break; > - } > + struct domain *d; > > + d = rcu_lock_domain_console_by_id(domid); > + if ( d != NULL ) > rcu_unlock_domain(d); > - } > + > + return d != NULL; > +} > + > +int console_set_owner(domid_t domid) > +{ > + if ( domid == DOMID_XEN ) > + printk("*** Serial input to Xen"); > + else if ( console_owner_possible(domid) ) > + printk("*** Serial input to DOM%u", domid); > + else > + return -ENOENT; > + > + console_owner = domid; > > if ( switch_code ) > printk(" (type 'CTRL-%c' three times to switch input)", > opt_conswitch[0]); > printk("\n"); > + > + return 0; > +} > + > +/* > + * Switch console input focus. > + * Rotates input focus among Xen, dom0 and boot-time created domUs while > + * skipping switching serial input to non existing domains. > + */ > +static void console_find_owner(void) > +{ > + domid_t i, n = max_init_domid + 1; n can be made const, I would even rename to nr for clarity, but that's personal taste. > + > + if ( console_owner == DOMID_XEN ) > + i = get_initial_domain_id(); > + else > + i = console_owner + 1; > + > + for ( ; i < n; i++ ) > + if ( !console_set_owner(i) ) > + break; Hm, that could be a non-trivial amount of iteration if max_init_domid is bumped for every domain created as you have it in patch 11/35 (albeit I'm not sure that was intended?) > + if ( i == n ) > + console_set_owner(DOMID_XEN); > } > > static void __serial_rx(char c) > { > switch ( console_owner ) > { > - case 0: > + case DOMID_XEN: > return handle_keypress(c, false); > > - case 1: > + case 0: If console_owner now strictly contains a domid you cannot assume that domid 0 is the hardware domain, you will need to handle this differently and check whether the domain pointed by console_owner passes the is_hardware_domain() check. > /* > * Deliver input to the hardware domain buffer, unless it is > * already full. > @@ -556,7 +574,7 @@ static void __serial_rx(char c) > #ifdef CONFIG_SBSA_VUART_CONSOLE > default: > { > - struct domain *d = rcu_lock_domain_by_id(console_owner - 1); > + struct domain *d = rcu_lock_domain_by_id(console_owner); > > /* > * If we have a properly initialized vpl011 console for the > @@ -567,7 +585,7 @@ static void __serial_rx(char c) > vpl011_rx_char_xen(d, c); > else > printk("Cannot send chars to Dom%d: no UART available\n", > - console_owner - 1); > + console_owner); > > if ( d != NULL ) > rcu_unlock_domain(d); > @@ -1126,7 +1144,7 @@ void __init console_endboot(void) > * a useful 'how to switch' message. > */ > if ( opt_conswitch[1] == 'x' ) > - console_owner = max_console_rx; > + console_owner = DOMID_XEN; Hm, are you sure this still works as expected? Setting console_owner == DOMID_XEN will cause the call to switch_serial_input() below to switch the console back to the first domain in the system. Also initializing console_owner to 0 by default would also cause the call to switch_serial_input() to possibly switch it to the first domain after domid 0 (or back to Xen). Thanks, Roger.
On 12.12.2024 11:12, Roger Pau Monné wrote: > On Thu, Dec 05, 2024 at 08:41:49PM -0800, Denis Mukhin via B4 Relay wrote: >> +static struct domain *rcu_lock_domain_console_by_id(domid_t domid) >> +{ >> + struct domain *d; >> + >> + d = rcu_lock_domain_by_id(domid); >> + > > Nit: I would remove this newline. Or even better make the function call the variable's initializer. Jan
On Thu, Dec 12, 2024 at 12:59:30PM +0100, Jan Beulich wrote: > On 12.12.2024 11:12, Roger Pau Monné wrote: > > On Thu, Dec 05, 2024 at 08:41:49PM -0800, Denis Mukhin via B4 Relay wrote: > >> +static struct domain *rcu_lock_domain_console_by_id(domid_t domid) > >> +{ > >> + struct domain *d; > >> + > >> + d = rcu_lock_domain_by_id(domid); > >> + > > > > Nit: I would remove this newline. > > Or even better make the function call the variable's initializer. Indeed, didn't catch that one. Thanks, Roger.
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 8cbac54c66044ae8581e486a782102b75c8bfaa9..52cf64dbf6fd18d599cb88835d03501a23b3e3c4 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -463,82 +463,100 @@ static void cf_check dump_console_ring_key(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_owner=0 => input to xen - * console_owner=1 => input to dom0 (or the sole shim domain) - * console_owner=N => input to dom(N-1) + * Current console owner domain ID: either Xen or domain w/ d->is_console == + * true. + * + * Initialized in console_endboot(). */ -static unsigned int __read_mostly console_owner = 0; +static domid_t __read_mostly console_owner; -#define max_console_rx (max_init_domid + 1) +static struct domain *rcu_lock_domain_console_by_id(domid_t domid) +{ + struct domain *d; + + d = rcu_lock_domain_by_id(domid); + + if ( d == NULL ) + return NULL; + + if ( d->is_console ) + return d; + + rcu_unlock_domain(d); + + return NULL; +} -#ifdef CONFIG_SBSA_VUART_CONSOLE /* Make sure to rcu_unlock_domain after use */ struct domain *rcu_lock_domain_console_owner(void) { - if ( console_owner == 0 ) - return NULL; - return rcu_lock_domain_by_id(console_owner - 1); + return rcu_lock_domain_console_by_id(console_owner); } -#endif -static void console_find_owner(void) +static bool console_owner_possible(domid_t domid) { - unsigned int next_rx = console_owner; - - /* - * 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_owner = 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 == NULL ) - continue; - - if ( d->is_console ) - { - rcu_unlock_domain(d); - console_owner = next_rx; - printk("*** Serial input to DOM%u", domid); - break; - } + struct domain *d; + d = rcu_lock_domain_console_by_id(domid); + if ( d != NULL ) rcu_unlock_domain(d); - } + + return d != NULL; +} + +int console_set_owner(domid_t domid) +{ + if ( domid == DOMID_XEN ) + printk("*** Serial input to Xen"); + else if ( console_owner_possible(domid) ) + printk("*** Serial input to DOM%u", domid); + else + return -ENOENT; + + console_owner = domid; if ( switch_code ) printk(" (type 'CTRL-%c' three times to switch input)", opt_conswitch[0]); printk("\n"); + + return 0; +} + +/* + * Switch console input focus. + * Rotates input focus among Xen, dom0 and boot-time created domUs while + * skipping switching serial input to non existing domains. + */ +static void console_find_owner(void) +{ + domid_t i, n = max_init_domid + 1; + + if ( console_owner == DOMID_XEN ) + i = get_initial_domain_id(); + else + i = console_owner + 1; + + for ( ; i < n; i++ ) + if ( !console_set_owner(i) ) + break; + if ( i == n ) + console_set_owner(DOMID_XEN); } static void __serial_rx(char c) { switch ( console_owner ) { - case 0: + case DOMID_XEN: return handle_keypress(c, false); - case 1: + case 0: /* * Deliver input to the hardware domain buffer, unless it is * already full. @@ -556,7 +574,7 @@ static void __serial_rx(char c) #ifdef CONFIG_SBSA_VUART_CONSOLE default: { - struct domain *d = rcu_lock_domain_by_id(console_owner - 1); + struct domain *d = rcu_lock_domain_by_id(console_owner); /* * If we have a properly initialized vpl011 console for the @@ -567,7 +585,7 @@ static void __serial_rx(char c) vpl011_rx_char_xen(d, c); else printk("Cannot send chars to Dom%d: no UART available\n", - console_owner - 1); + console_owner); if ( d != NULL ) rcu_unlock_domain(d); @@ -1126,7 +1144,7 @@ void __init console_endboot(void) * a useful 'how to switch' message. */ if ( opt_conswitch[1] == 'x' ) - console_owner = max_console_rx; + console_owner = DOMID_XEN; register_keyhandler('w', dump_console_ring_key, "synchronously dump console ring buffer (dmesg)", 0); diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h index 0e211e44d9703c804e18f52c9743916f8d2a9d4e..57c482cfbf2da15b011e64841ea086e779f4588d 100644 --- a/xen/include/xen/console.h +++ b/xen/include/xen/console.h @@ -32,6 +32,7 @@ void console_start_log_everything(void); void console_end_log_everything(void); struct domain *rcu_lock_domain_console_owner(void); +int console_set_owner(domid_t); /* * Steal output from the console. Returns +ve identifier, else -ve error.