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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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.

Jan