| Message ID | 20241205-vuart-ns8250-v1-22-e9aa923127eb@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce NS8250 UART emulator | expand |
On Thu, Dec 05, 2024 at 08:41:52PM -0800, Denis Mukhin via B4 Relay wrote: > From: Denis Mukhin <dmukhin@ford.com> > > With introduction of NS8250 emulator for x86, the logic of switching console > focus gets more convoluted: HVM domain w/ NS8205 must be able to receive the > physical console input for guest VM debugging. > > Also, existing code does not honor `hardware_dom=` xen command line parameter > (hardware domain ID does _not_ necessarily starts from 0). > > Introduce handle_keypress_in_domain() to account for all scenarios of console > input forwarding. > > Signed-off-by: Denis Mukhin <dmukhin@ford.com> > --- > xen/drivers/char/console.c | 72 +++++++++++++++++++++++++++------------------- > 1 file changed, 42 insertions(+), 30 deletions(-) > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 6261bdb5a2ac1075bc89fa408c0fd6cfef380ae6..ce3639a4cdcda00ea63e3bf119bc3b242cbfdf6a 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -570,14 +570,16 @@ static void console_init_owner(void) > console_set_owner(domid); > } > > -static void __serial_rx(char c) > +static void handle_keypress_in_domain(struct domain *d, char c) > { > - switch ( console_owner ) > - { > - case DOMID_XEN: > - return handle_keypress(c, false); > + int rc = 0; > > - case 0: > + /* > + * Deliver input to the hardware domain buffer. > + * NB: must be the first check: hardware domain may have emulated UART. > + */ > + if ( d == hardware_domain ) is_hardware_domain(d) > + { > /* > * Deliver input to the hardware domain buffer, unless it is > * already full. > @@ -590,34 +592,44 @@ static void __serial_rx(char c) > * getting stuck. > */ > send_global_virq(VIRQ_CONSOLE); > - break; > - > -#ifdef CONFIG_SBSA_VUART_CONSOLE > - default: > - { > - struct domain *d = rcu_lock_domain_by_id(console_owner); > - > - /* > - * If we have a properly initialized vpl011 console for the > - * domain, without a full PV ring to Dom0 (in that case input > - * comes from the PV ring), then send the character to it. > - */ > - if ( d != NULL ) > - vpl011_rx_char_xen(d, c); > - else > - printk("Cannot send chars to Dom%d: no UART available\n", > - console_owner); > - > - if ( d != NULL ) > - rcu_unlock_domain(d); > - > - break; > } > + /* > + * Deliver input to the emulated UART. > + */ For one-line comments you can use: /* Deliver input to the emulated UART. */ I would however place the comment inside the `if` body. > + else if ( domain_has_vuart(d) ) > + { > +#if defined(CONFIG_SBSA_VUART_CONSOLE) > + rc = vpl011_rx_char_xen(d, c); > #endif You can possibly make the preprocessor conditional also contain the if condition itself? As otherwise the if condition is dead code. > } > - > + /* > + * Deliver input to the PV shim console. > + */ > if ( consoled_is_enabled() ) > - consoled_guest_tx(c); > + rc = consoled_guest_tx(c); > + > + if ( rc && rc != -ENODEV ) > + printk(KERN_WARNING "console input domain %d: not ready: %d\n", > + d->domain_id, rc); XENLOG_ERR instead of KERN_WARNING, and use %pd to print domains, ie: printk(XENLOG_ERR "%pd: delivery of console input failed: %d\n", d, rc); And I wonder whether this should be printed just once per domain, or whether the domain should be marked as not having a console (is_console = false) after delivery of input keys failed. Otherwise you could spam the console with such error messages on every serial key press. > +} > + > +static void __serial_rx(char c) > +{ > + struct domain *d; > + > + if ( console_owner == DOMID_XEN ) > + { > + handle_keypress(c, false); > + return; > + } > + > + d = rcu_lock_domain_console_owner(); > + if ( d == NULL ) > + return; > + > + handle_keypress_in_domain(d, c); Is __serial_rx() the only caller of handle_keypress_in_domain() after the series? If so, I'm not sure it's worth placing this logic in a separate function. Thanks, Roger.
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 6261bdb5a2ac1075bc89fa408c0fd6cfef380ae6..ce3639a4cdcda00ea63e3bf119bc3b242cbfdf6a 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -570,14 +570,16 @@ static void console_init_owner(void) console_set_owner(domid); } -static void __serial_rx(char c) +static void handle_keypress_in_domain(struct domain *d, char c) { - switch ( console_owner ) - { - case DOMID_XEN: - return handle_keypress(c, false); + int rc = 0; - case 0: + /* + * Deliver input to the hardware domain buffer. + * NB: must be the first check: hardware domain may have emulated UART. + */ + if ( d == hardware_domain ) + { /* * Deliver input to the hardware domain buffer, unless it is * already full. @@ -590,34 +592,44 @@ static void __serial_rx(char c) * getting stuck. */ send_global_virq(VIRQ_CONSOLE); - break; - -#ifdef CONFIG_SBSA_VUART_CONSOLE - default: - { - struct domain *d = rcu_lock_domain_by_id(console_owner); - - /* - * If we have a properly initialized vpl011 console for the - * domain, without a full PV ring to Dom0 (in that case input - * comes from the PV ring), then send the character to it. - */ - if ( d != NULL ) - vpl011_rx_char_xen(d, c); - else - printk("Cannot send chars to Dom%d: no UART available\n", - console_owner); - - if ( d != NULL ) - rcu_unlock_domain(d); - - break; } + /* + * Deliver input to the emulated UART. + */ + else if ( domain_has_vuart(d) ) + { +#if defined(CONFIG_SBSA_VUART_CONSOLE) + rc = vpl011_rx_char_xen(d, c); #endif } - + /* + * Deliver input to the PV shim console. + */ if ( consoled_is_enabled() ) - consoled_guest_tx(c); + rc = consoled_guest_tx(c); + + if ( rc && rc != -ENODEV ) + printk(KERN_WARNING "console input domain %d: not ready: %d\n", + d->domain_id, rc); +} + +static void __serial_rx(char c) +{ + struct domain *d; + + if ( console_owner == DOMID_XEN ) + { + handle_keypress(c, false); + return; + } + + d = rcu_lock_domain_console_owner(); + if ( d == NULL ) + return; + + handle_keypress_in_domain(d, c); + + rcu_unlock_domain(d); } static void cf_check serial_rx(char c)