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

[dmkhn@xxxxxxxxx]

On Mon, Aug 04, 2025 at 12:11:03PM +0200, Jan Beulich wrote:
> On 31.07.2025 21:21, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Introduce a driver framework to abstract UART emulators in the hypervisor.
> >
> > That allows for architecture-independent handling of virtual UARTs in the
> > console driver and simplifies enabling new UART emulators.
> >
> > The framework is built under CONFIG_HAS_VUART, which will be automatically
> > enabled once the user enables any UART emulator.
> 
> Yet then still - why "HAS"? Call it just VUART or VUART_FRAMEWORK or some 
> such.
> 
> > --- 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"`?

> 
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> >  obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
> >  obj-$(CONFIG_IOREQ_SERVER) += dm.o
> >  obj-y += domain.o
> > +obj-y += emul/
> >  obj-y += event_2l.o
> >  obj-y += event_channel.o
> >  obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
> > diff --git a/xen/common/emul/Kconfig b/xen/common/emul/Kconfig
> > new file mode 100644
> > index 000000000000..7c6764d1756b
> > --- /dev/null
> > +++ b/xen/common/emul/Kconfig
> > @@ -0,0 +1,6 @@
> > +menu "Domain Emulation Features"
> > +   visible if EXPERT
> > +
> > +source "common/emul/vuart/Kconfig"
> > +
> > +endmenu
> > diff --git a/xen/common/emul/Makefile b/xen/common/emul/Makefile
> > new file mode 100644
> > index 000000000000..670682102c13
> > --- /dev/null
> > +++ b/xen/common/emul/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_HAS_VUART) += vuart/
> 
> With this you can ...
> 
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_HAS_VUART) += vuart.o
> 
> ... use the simpler obj-y here.

Thanks.

> 
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/vuart.c
> > @@ -0,0 +1,112 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * UART emulator framework.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +#include <xen/errno.h>
> > +#include <xen/sched.h>
> > +#include <xen/vuart.h>
> > +
> > +#define VUART_ARRAY_SIZE    (__start_vuart_end - __start_vuart_array)
> > +
> > +#define for_each_vuart(vdev) \
> > +    for (unsigned __i = 0; \
> > +         __i < VUART_ARRAY_SIZE && (vdev = __start_vuart_array[__i], 1); \
> > +         __i++)
> 
> Nit: Xen style please. Any preferably no leading underscores; in no case
> two of them.

Ack.

> 
> > +extern const struct vuart_ops *const __start_vuart_array[];
> > +extern const struct vuart_ops *const __start_vuart_end[];
> 
> Is there an actual need for this extra level of indirection? It is in the
> process of being done away with for vPCI.

Ack.

> 
> > +int vuart_add_node(struct domain *d, const void *node)
> > +{
> > +    const struct vuart_ops *vdev;
> > +    int rc;
> > +
> > +    for_each_vuart(vdev)
> > +    {
> > +        if ( !vdev->add_node )
> > +            continue;
> > +
> > +        rc = vdev->add_node(d, node);
> 
> Here and below - shouldn't you call hooks only when the kind of driver is
> actually enabled for the domkain in question?

Thanks; this looks a bit raw...

I will rework that in the follow on change. I will drop vuart_add_node()
from NS16550 series since it is not used yet.

> 
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +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.

> 
> > +void vuart_deinit(struct domain *d)
> > +{
> > +    const struct vuart_ops *vdev;
> > +
> > +    for_each_vuart(vdev)
> > +        vdev->deinit(d);
> > +}
> 
> I can perhaps see why this hook wants to uniformly be set, but ...
> 
> > +void vuart_dump_state(const struct domain *d)
> > +{
> > +    const struct vuart_ops *vdev;
> > +
> > +    for_each_vuart(vdev)
> > +        vdev->dump_state(d);
> > +}
> 
> ... state dumping pretty surely wants to be optional?

Ack.

> 
> > +/*
> > + * 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.

> 
> > +int vuart_put_rx(struct domain *d, char c)
> > +{
> > +    const struct vuart_ops *vdev = NULL;
> > +
> > +    ASSERT(domain_has_vuart(d));
> > +
> > +    for_each_vuart(vdev)
> > +        if ( vdev->put_rx )
> 
> ... but only kind B offers this hook?
> 
> > +            break;
> > +
> > +    return vdev ? vdev->put_rx(d, c) : -ENODEV;
> 
> The check for NULL helps for the "no vUART drivers" case, but it won't
> help if you exhausted the array without finding a driver with the wanted
> hook.

Ack.

> 
> > +}
> > +
> > +bool domain_has_vuart(const struct domain *d)
> > +{
> > +    uint32_t mask = 0;
> 
> unsigned int?

Ack.

> 
> > --- 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

```

> 
> > --- /dev/null
> > +++ b/xen/include/xen/vuart.h
> > @@ -0,0 +1,84 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * UART emulator framework.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +#ifndef XEN_VUART_H
> > +#define XEN_VUART_H
> > +
> > +#include <public/xen.h>
> > +#include <public/event_channel.h>
> > +#include <xen/types.h>
> 
> The order is wrong - types must be available before public headers are 
> included.

Ack.

> 
> > +struct vuart_params {
> > +    domid_t console_domid;
> > +    gfn_t gfn;
> > +    evtchn_port_t evtchn;
> > +};
> > +
> > +struct vuart_ops {
> > +    int (*add_node)(struct domain *d, const void *node);
> > +    int (*init)(struct domain *d, struct vuart_params *params);
> > +    void (*deinit)(struct domain *d);
> > +    void (*dump_state)(const struct domain *d);
> > +    int (*put_rx)(struct domain *d, char c);
> > +};
> > +
> > +#define VUART_REGISTER(name, x) \
> > +    static const struct vuart_ops *const __name##_entry \
> > +        __used_section(".data.vuart." #name) = (x);
> > +
> > +#ifdef CONFIG_HAS_VUART
> > +
> > +int vuart_add_node(struct domain *d, const void *node);
> > +int vuart_init(struct domain *d, struct vuart_params *params);
> > +void vuart_deinit(struct domain *d);
> > +void vuart_dump_state(const struct domain *d);
> > +int vuart_put_rx(struct domain *d, char c);
> > +bool domain_has_vuart(const struct domain *d);
> > +
> > +#else
> > +
> > +static inline int vuart_add_node(struct domain *d, const void *node)
> > +{
> > +    return 0;
> > +}
> > +
> > +static inline int vuart_init(struct domain *d, struct vuart_params *params)
> > +{
> > +    return 0;
> > +}
> > +
> > +static inline void vuart_deinit(struct domain *d)
> > +{
> > +}
> > +
> > +static inline void vuart_dump_state(const struct domain *d)
> > +{
> > +}
> > +
> > +static inline int vuart_put_rx(struct domain *d, char c)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +    return -ENODEV;
> > +}
> > +
> > +static inline bool domain_has_vuart(const struct domain *d)
> > +{
> > +    return false;
> > +}
> 
> With this, some of the other stubs should not be necessary. Declarations
> will suffice, e.g. for vuart_put_rx().

Thanks, will update.

> 
> > --- a/xen/include/xen/xen.lds.h
> > +++ b/xen/include/xen/xen.lds.h
> > @@ -194,4 +194,14 @@
> >  #define VPCI_ARRAY
> >  #endif
> >
> > +#ifdef CONFIG_HAS_VUART
> > +#define VUART_ARRAY     \
> > +       . = ALIGN(POINTER_ALIGN); \
> > +       __start_vuart_array = .;  \
> > +       *(SORT(.data.vuart.*))    \
> 
> This is r/o data afaict, so would want naming .rodata.vuart.*. Which in
> turn means the uses of the macros need to move up in the linker scripts.

Ack.

> 
> Jan
>