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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2025 20:55, dmkhn@xxxxxxxxx wrote:
> On Mon, Aug 04, 2025 at 12:11:03PM +0200, Jan Beulich wrote:
>> On 31.07.2025 21:21, dmkhn@xxxxxxxxx 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'.

Jan