On Mon, Aug 11, 2025 at 09:34:58AM +0200, Jan Beulich wrote:
> On 09.08.2025 20:55, dm...@proton.me wrote:
> > On Mon, Aug 04, 2025 at 12:11:03PM +0200, Jan Beulich wrote:
> >> On 31.07.2025 21:21, dm...@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
