Re: [PATCH v2 19/35] xen/console: introduce console_set_owner()

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 2:12 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:41:49PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > 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@xxxxxxxx
> > ---
> > 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.

Fixed.

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

Fixed.

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

`max_init_domid` can change at run-time actually (e.g. after `xl create`).
I kept `n` as is.

>
> > +
> > + 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?)

Yes, `max_init_domid` is advanced on each domain creation (v3).

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

Fixed.

>
> > /*
> > * 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).

TBH, I did not test w/ conswitch=x originally, but after addressing
your other feedback that is now fixed.
Thank you.

>
> Thanks, Roger.