Re: [PATCH v2 34/35] xen/console: enable console owners w/ emulated NS8250

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Tuesday, December 10th, 2024 at 11:38 PM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

>
>
> On 10.12.2024 23:46, Jason Andryuk wrote:
>
> > On 2024-12-05 23:42, Denis Mukhin via B4 Relay wrote:
> >
> > > From: Denis Mukhin dmukhin@xxxxxxxx
> > >
> > > Enable console focus for domains w/ virtual NS8250.
> > >
> > > Code change allows to capture the output from the guest OS now and send 
> > > it to
> > > the physical console device.
> > >
> > > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
> > > ---
> > > xen/drivers/char/console.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > > index 
> > > a26daee9c4c4b1134d0ae3d105ffdb656340b6df..798dfdf3412a2feef35e72946d6c59bee59a9251
> > >  100644
> > > --- a/xen/drivers/char/console.c
> > > +++ b/xen/drivers/char/console.c
> > > @@ -41,6 +41,9 @@
> > > #ifdef CONFIG_SBSA_VUART_CONSOLE
> > > #include <asm/vpl011.h>
> > > #endif
> > > +#if defined(CONFIG_HAS_VUART_NS8250)
> > > +#include <asm/hvm/vuart_ns8250.h>
> > > +#endif
> > >
> > > /* console: comma-separated list of console outputs. */
> > > static char __initdata opt_console[30] = OPT_CONSOLE_STR;
> > > @@ -627,6 +630,8 @@ static void handle_keypress_in_domain(struct domain 
> > > *d, char c)
> > > {
> > > #if defined(CONFIG_SBSA_VUART_CONSOLE)
> > > rc = vpl011_rx_char_xen(d, c);
> > > +#elif defined(CONFIG_HAS_VUART_NS8250)
> > > + rc = vuart_putchar(&d->arch.hvm.vuart, c);
> > > #endif
> >
> > I think it would be nicer to just use a single name and avoid ifdef-ery.
> > vuart_putchar() is generic and matches domain_has_vuart(), so that
> > seems good.
> >
> > You can then have a default stub that returns -ENODEV for when an
> > implementation is not built. (This goes along with Jan's suggestion of
> > a common, default domain_has_vuart().) Something like:
> >
> > #ifndef vuart_putchar
> > static inline int vuart_putchar(struct domain *d, char c) {
> > return -ENODEV;
> > }
> > #define vuart_putchar vuart_putchar
> > #endif
> >
> > and ARM can do:
> > #define vuart_putchar vpl011_rx_char_xen
> >
> > x86 would need to change its arguments, but that should be straight forward.
>
>
> Actually, I don't even see a need for the stub:
>
> {
> #ifdef vuart_putchar
> rc = vuart_putchar(d, c);
> #endif
> }
>
> This way behavior won't change from what there is now, when vuart_putchar()
> isn't defined.

I solved it by adding some arch-independent code for "virtual UARTs" in v3, 
which,
IMO, looks a cleaner solution.

>
> Jan