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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Tuesday, December 10th, 2024 at 2:46 PM, Jason Andryuk 
<jason.andryuk@xxxxxxx> 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.
>
> What do you think?

I think this is a good suggestion, I had same plans for this code, TBH.
I only planned to do that later, but now addressed in v3.
I solved it by adding arch-independent virtdev-uart.c shim, each vUART
implementation should register itself in this shim.

Fixed.

>
> Regards,
> Jason