Re: [PATCH] xen/console: introduce console_{get,put}_domain()

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Friday, February 21st, 2025 at 4:04 PM, Stefano Stabellini 
<sstabellini@xxxxxxxxxx> wrote:

> 
> 
> On Wed, 19 Feb 2025, Jan Beulich wrote:
> 
> > On 18.02.2025 09:31, dmkhn@xxxxxxxxx wrote:
> > 
> > > @@ -546,31 +555,23 @@ 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_rx - 1);
> > > -
> > > - if ( d )
> > > - {
> > > - int rc = vpl011_rx_char_xen(d, c);
> > > - if ( rc )
> > > - guest_printk(d, XENLOG_G_WARNING
> > > - "failed to process console input: %d\n", rc);
> > > - rcu_unlock_domain(d);
> > > - }
> > > -
> > > - break;
> > > - }
> > > + else
> > > + / Deliver input to the emulated UART. */
> > > + rc = vpl011_rx_char_xen(d, c);
> > > #endif
> > > - }
> > > 
> > > #ifdef CONFIG_X86
> > > if ( pv_shim && pv_console )
> > > consoled_guest_tx(c);
> > > #endif
> > > +
> > > + if ( rc )
> > > + guest_printk(d, XENLOG_G_WARNING
> > > + "failed to process console input: %d\n", rc);
> > 
> > Wouldn't this better live ahead of the four shim related lines?
> > 
> > Also please correct the log level specifier here. I realize you only move
> > what was there before, but I consider i bad practice to move buggy code.
> > gprintk() already prepends XENLOG_GUEST, so instead of XENLOG_G_WARNING
> > is should just be XENLOG_WARNING. (Line wrapping is also odd here, at
> > least according to my taste. But since that's not written down anywhere,
> > I wouldn't insist on adjusting that as well.)
> > 
> > With both adjustments (provided you agree, of course)
> > Reviewed-by: Jan Beulich jbeulich@xxxxxxxx
> > Given they're reasonably simple to make, I could once again offer to
> > adjust while committing (provided an Arm ack also arrives).
> 
> 
> Acked-by: Stefano Stabellini sstabellini@xxxxxxxxxx

Thank you!