| Message ID | 20230320081935.18367-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] xen/console: Skip switching serial input to non existing domains | expand |
On 20.03.2023 09:19, Michal Orzel wrote: > @@ -483,15 +485,34 @@ struct domain *console_input_domain(void) > > static void switch_serial_input(void) > { > - if ( console_rx == max_init_domid + 1 ) > - { > - console_rx = 0; > - printk("*** Serial input to Xen"); > - } > - else > + unsigned int next_rx = console_rx + 1; > + > + /* > + * Rotate among Xen, dom0 and boot-time created domUs while skipping > + * switching serial input to non existing domains. > + */ > + while ( next_rx <= max_console_rx + 1 ) > { > - console_rx++; > - printk("*** Serial input to DOM%d", console_rx - 1); > + if ( next_rx == max_console_rx + 1 ) Part of the earlier problems stemmed from the comparison being == here. Could I talk you into using >= instead? > + { > + console_rx = 0; > + printk("*** Serial input to Xen"); > + break; > + } > + else No need for "else" after "break" (or alike). Omitting it will not only decrease indentation, but also make more visible that the earlier if() won't "fall through". > + { > + struct domain *d = rcu_lock_domain_by_id(next_rx - 1); > + > + if ( d ) > + { > + rcu_unlock_domain(d); > + console_rx = next_rx; > + printk("*** Serial input to DOM%d", console_rx - 1); While I expect the compiler will transform this to using "next_rx" here anyway, I think it would be nice if it was written like this right away. Since you touch the printk() anyway, please also switch to using the more applicable %u. With the adjustments Reviewed-by: Jan Beulich <jbeulich@suse.com> One other transformation for you to consider is to switch to a base layout like unsigned int next_rx = console_rx; while ( next_rx++ <= max_console_rx ) { ... } i.e. without a separate increment at the bottom of the loop. Which, now that I've spelled it out, raises the question of why the outer loop needs a condition in the first place (because as written above it clearly is always true). So perhaps better (and more directly showing what's going on) unsigned int next_rx = console_rx; for ( ; ; ) { if ( next_rx++ >= max_console_rx ) ... } Jan
On 20/03/2023 12:17, Jan Beulich wrote: > > > On 20.03.2023 09:19, Michal Orzel wrote: >> @@ -483,15 +485,34 @@ struct domain *console_input_domain(void) >> >> static void switch_serial_input(void) >> { >> - if ( console_rx == max_init_domid + 1 ) >> - { >> - console_rx = 0; >> - printk("*** Serial input to Xen"); >> - } >> - else >> + unsigned int next_rx = console_rx + 1; >> + >> + /* >> + * Rotate among Xen, dom0 and boot-time created domUs while skipping >> + * switching serial input to non existing domains. >> + */ >> + while ( next_rx <= max_console_rx + 1 ) >> { >> - console_rx++; >> - printk("*** Serial input to DOM%d", console_rx - 1); >> + if ( next_rx == max_console_rx + 1 ) > > Part of the earlier problems stemmed from the comparison being == here. > Could I talk you into using >= instead? With the loop condition unmodified it would not make sense as it would be impossible. However, because of what you wrote below, I will do this together with other modifications. > >> + { >> + console_rx = 0; >> + printk("*** Serial input to Xen"); >> + break; >> + } >> + else > > No need for "else" after "break" (or alike). Omitting it will not only > decrease indentation, but also make more visible that the earlier if() > won't "fall through". > ok. >> + { >> + struct domain *d = rcu_lock_domain_by_id(next_rx - 1); >> + >> + if ( d ) >> + { >> + rcu_unlock_domain(d); >> + console_rx = next_rx; >> + printk("*** Serial input to DOM%d", console_rx - 1); > > While I expect the compiler will transform this to using "next_rx" > here anyway, I think it would be nice if it was written like this > right away. ok. > > Since you touch the printk() anyway, please also switch to using the > more applicable %u. ok. > > With the adjustments > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > One other transformation for you to consider is to switch to a base > layout like > > unsigned int next_rx = console_rx; > while ( next_rx++ <= max_console_rx ) > { > ... > } > > i.e. without a separate increment at the bottom of the loop. Which, > now that I've spelled it out, raises the question of why the outer > loop needs a condition in the first place (because as written above > it clearly is always true). So perhaps better (and more directly > showing what's going on) > > unsigned int next_rx = console_rx; > for ( ; ; ) > { > if ( next_rx++ >= max_console_rx ) > ... > } Makes sense to me so I will do this assuming that you agree on adding your Rb tag also for this approach. ~Michal
On 20.03.2023 13:07, Michal Orzel wrote: > On 20/03/2023 12:17, Jan Beulich wrote: >> One other transformation for you to consider is to switch to a base >> layout like >> >> unsigned int next_rx = console_rx; >> while ( next_rx++ <= max_console_rx ) >> { >> ... >> } >> >> i.e. without a separate increment at the bottom of the loop. Which, >> now that I've spelled it out, raises the question of why the outer >> loop needs a condition in the first place (because as written above >> it clearly is always true). So perhaps better (and more directly >> showing what's going on) >> >> unsigned int next_rx = console_rx; >> for ( ; ; ) >> { >> if ( next_rx++ >= max_console_rx ) >> ... >> } > Makes sense to me so I will do this assuming that you agree on adding your Rb tag also > for this approach. I do, yes. Jan
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 51e5408f2114..86aa2b9c7165 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -473,6 +473,8 @@ static void cf_check dump_console_ring_key(unsigned char key) */ static unsigned int __read_mostly console_rx = 0; +#define max_console_rx (max_init_domid + 1) + /* Make sure to rcu_unlock_domain after use */ struct domain *console_input_domain(void) { @@ -483,15 +485,34 @@ struct domain *console_input_domain(void) static void switch_serial_input(void) { - if ( console_rx == max_init_domid + 1 ) - { - console_rx = 0; - printk("*** Serial input to Xen"); - } - else + unsigned int next_rx = console_rx + 1; + + /* + * Rotate among Xen, dom0 and boot-time created domUs while skipping + * switching serial input to non existing domains. + */ + while ( next_rx <= max_console_rx + 1 ) { - console_rx++; - printk("*** Serial input to DOM%d", console_rx - 1); + if ( next_rx == max_console_rx + 1 ) + { + console_rx = 0; + printk("*** Serial input to Xen"); + break; + } + else + { + struct domain *d = rcu_lock_domain_by_id(next_rx - 1); + + if ( d ) + { + rcu_unlock_domain(d); + console_rx = next_rx; + printk("*** Serial input to DOM%d", console_rx - 1); + break; + } + + next_rx++; + } } if ( switch_code ) @@ -1089,7 +1110,7 @@ void __init console_endboot(void) * a useful 'how to switch' message. */ if ( opt_conswitch[1] == 'x' ) - console_rx = max_init_domid + 1; + console_rx = max_console_rx; register_keyhandler('w', dump_console_ring_key, "synchronously dump console ring buffer (dmesg)", 0);
At the moment, we direct serial input to hardware domain by default. This does not make any sense when running in true dom0less mode, since such domain does not exist. As a result, users wishing to write to an emulated UART of a domU are always forced to execute CTRL-AAA first. The same issue is when rotating among serial inputs, where we always have to go through hardware domain case. This problem can be elaborated further to all the domains that no longer exist. Modify switch_serial_input() so that we skip switching serial input to non existing domains. Take the opportunity to define and make use of macro max_console_rx to make it clear what 'max_init_domid + 1' means in the console code context. For now, to minimize the required changes and to match the current behavior with hwdom, the default input goes to the first real domain. The choice is more or less arbitrary since dom0less domUs are supposedly equal. This will be handled in the future by adding support in boot time configuration for marking a specific domain preferred in terms of directing serial input to. Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- Changes in v3: - properly handle case where domain with highest ID no longer exists - define max_console_rx Changes in v2: - was: xen/console: Handle true dom0less case when switching serial input - use a more generic approach to handle all non-existing domains --- xen/drivers/char/console.c | 39 +++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-)