Re: [PATCH v2 22/35] xen/console: introduce handle_keypress_in_domain()

["Roger Pau Monné" <roger.pau@citrix.com>]

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.