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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.01.2025 21:03, Denis Mukhin wrote:
> 
> 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?

Well. First and foremost you need to make sure that existing uses of the
variable will continue to function as-is. Aiui that contradicts your
re-purposing of it. Which in turn would eliminate the question of how to
best rename it.

Jan