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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Monday, January 6th, 2025 at 1:58 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:

> 
> 
> On 04.01.2025 04:30, Denis Mukhin wrote:
> 
> > 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:
> > > 
> > > > --- 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.
> 
> 
> How does max_init_domid potentially changing affect (possible) const-ness of 
> n?

Sorry, what I meant is I kept the original name as is in v3.
Forgot to address `const`.

> 
> However it changing, ...
> 
> > > > +
> > > > + 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).
> 
> 
> ... as you confirm here, undermines what it's used for right now (if I'm
> not mistaken), and would render the variable misnamed.

Yes, the name will be a bit confusing.
Will something like `last_domid` work?

> 
> Jan