Re: [PATCH v4 2/8] emul/vuart: introduce framework for UART emulators

[dmkhn@proton.me]

On Mon, Aug 11, 2025 at 09:34:58AM +0200, Jan Beulich wrote:
> On 09.08.2025 20:55, dmkhn@proton.me wrote:
> > On Mon, Aug 04, 2025 at 12:11:03PM +0200, Jan Beulich wrote:
> >> On 31.07.2025 21:21, dmkhn@proton.me wrote:
> >>> --- a/xen/common/Kconfig
> >>> +++ b/xen/common/Kconfig
> >>> @@ -1,6 +1,8 @@
> >>>
> >>>  menu "Common Features"
> >>>
> >>> +source "common/emul/Kconfig"
> >>> +
> >>>  config COMPAT
> >>
> >> Why at the very top?
> >
> > I did not find a better place, since the settings are not sorted and to me it
> > makes sense to list emulation capabilities first...
> >
> > Where would be the best location for that submenu?
> > Close to another submenu `source "common/sched/Kconfig"`?
> 
> At least below there. Possibly yet further down.
> 
> >>> +int vuart_init(struct domain *d, struct vuart_params *params)
> >>> +{
> >>> +    const struct vuart_ops *vdev;
> >>> +    int rc;
> >>> +
> >>> +    if ( !domain_has_vuart(d) )
> >>> +        return 0;
> >>> +
> >>> +    for_each_vuart(vdev)
> >>> +    {
> >>> +        rc = vdev->init(d, params);
> >>> +        if ( rc )
> >>> +            return rc;
> >>> +    }
> >>> +
> >>> +    d->console.input_allowed = true;
> >>
> >> Unconditionally?
> >
> > Thanks.
> > That should be a least under rc == 0.
> 
> You only ever make it there with rc == 0, though. (In fact that variable's
> scope would better be just the loop body.)
> 
> >>> +/*
> >>> + * Put character to the first suitable emulated UART's FIFO.
> >>> + */
> >>
> >> What's "suitable"? Along the lines of the earlier remark, what if the domain
> >> has vUART kind A configured, ...
> >
> > "suitable" is meant to be the first emulator with put_rx != NULL.
> > I will update that.
> 
> Except that, as iirc Roger also pointed out, "first emulator with put_rx != NULL"
> is a questionable condition.
> 
> >>> --- a/xen/common/keyhandler.c
> >>> +++ b/xen/common/keyhandler.c
> >>> @@ -22,6 +22,7 @@
> >>>  #include <xen/mm.h>
> >>>  #include <xen/watchdog.h>
> >>>  #include <xen/init.h>
> >>> +#include <xen/vuart.h>
> >>>  #include <asm/div64.h>
> >>>
> >>>  static unsigned char keypress_key;
> >>> @@ -354,6 +355,8 @@ static void cf_check dump_domains(unsigned char key)
> >>>                             v->periodic_period / 1000000);
> >>>              }
> >>>          }
> >>> +
> >>> +        vuart_dump_state(d);
> >>
> >> How verbose is this going to get?
> >
> > Looks something like this:
> > ```
> > (XEN) [   88.334893] 'q' pressed -> dumping domain info (now = 88334828303)
> > [..]
> > (XEN) [   88.335673] Virtual ns16550 (COM2) I/O port 0x02f8 IRQ#3 owner d0
> > (XEN) [   88.335681]   RX FIFO size 1024 in_prod 258 in_cons 258 used 0
> > (XEN) [   88.335689]   TX FIFO size 2048 out_prod 15 out_cons 0 used 15
> > (XEN) [   88.335696]   00 RBR 02 THR 6f DLL 01 DLM 00
> > (XEN) [   88.335703]   01 IER 05
> > (XEN) [   88.335709]   02 FCR 81 IIR c1
> > (XEN) [   88.335715]   03 LCR 13
> > (XEN) [   88.335720]   04 MCR 0b
> > (XEN) [   88.335726]   05 LSR 60
> > (XEN) [   88.335731]   06 MSR b0
> > (XEN) [   88.335736]   07 SCR 00
> >
> > ```
> 
> Definitely too much (for my taste) to put under 'q'.

I'll try to limit the number of printed lines; register dump can be made
compact for sure.

> 
> Jan