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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 2:51 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:41:52PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > 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@xxxxxxxx
> > ---
> > 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)

Fixed.

>
> > + {
> > /*
> > * 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. */

Fixed.

>
> I would however place the comment inside the `if` body.

Sure, no problem. Fixed.

>
> > + 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.

I removed the ifdef in v3 and reworked domain_has_vuart()
so that it checks build flag and d->arch.emulated_flags under the hood.

>
> > }
> > -
> > + /*
> > + * 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:

Addressed.

>
> 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.

Thanks; fixed.

>
> > +}
> > +
> > +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.

Yep, that is the only caller; fixed.

>
> Thanks, Roger.