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

[dmkhn@xxxxxxxxx]

On Wed, Aug 06, 2025 at 04:24:52PM +0200, Roger Pau Monné wrote:
> On Thu, Jul 31, 2025 at 07:21:49PM +0000, 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.
> >
> > Current implementation supports maximum of one vUART of each kind per 
> > domain.
> >
> > Use new domain_has_vuart() in the console driver code to check whether to
> > forward console input to the domain using vUART.
> >
> > Note: existing vUARTs are deliberately *not* hooked to the new framework to
> > minimize the scope of the patch: vpl011 (i.e. SBSA) emulator and "vuart" 
> > (i.e.
> > minimalistic MMIO-mapped dtuart for hwdoms on Arm) are kept unmodified.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > ---
> > Changes since v3:
> > - new patch
> > - original patch from ML: 
> > https://lore.kernel.org/xen-devel/20250624035443.344099-16-dmukhin@xxxxxxxx/
> > ---
> >  xen/arch/arm/xen.lds.S         |   1 +
> >  xen/arch/ppc/xen.lds.S         |   1 +
> >  xen/arch/riscv/xen.lds.S       |   1 +
> >  xen/arch/x86/xen.lds.S         |   1 +
> >  xen/common/Kconfig             |   2 +
> >  xen/common/Makefile            |   1 +
> >  xen/common/emul/Kconfig        |   6 ++
> >  xen/common/emul/Makefile       |   1 +
> >  xen/common/emul/vuart/Kconfig  |   6 ++
> >  xen/common/emul/vuart/Makefile |   1 +
> >  xen/common/emul/vuart/vuart.c  | 112 +++++++++++++++++++++++++++++++++
> >  xen/common/keyhandler.c        |   3 +
> >  xen/drivers/char/console.c     |   4 ++
> >  xen/include/xen/vuart.h        |  84 +++++++++++++++++++++++++
> >  xen/include/xen/xen.lds.h      |  10 +++
> >  15 files changed, 234 insertions(+)
> >  create mode 100644 xen/common/emul/Kconfig
> >  create mode 100644 xen/common/emul/Makefile
> >  create mode 100644 xen/common/emul/vuart/Kconfig
> >  create mode 100644 xen/common/emul/vuart/Makefile
> >  create mode 100644 xen/common/emul/vuart/vuart.c
> >  create mode 100644 xen/include/xen/vuart.h
> >
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index 9f30c3a13ed1..bdba7eaa4f65 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -58,6 +58,7 @@ SECTIONS
> >         *(.rodata)
> >         *(.rodata.*)
> >         VPCI_ARRAY
> > +       VUART_ARRAY
> >         *(.data.rel.ro)
> >         *(.data.rel.ro.*)
> >
> > diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
> > index 1de0b77fc6b9..f9d4e5b0dcd8 100644
> > --- a/xen/arch/ppc/xen.lds.S
> > +++ b/xen/arch/ppc/xen.lds.S
> > @@ -52,6 +52,7 @@ SECTIONS
> >          *(.rodata)
> >          *(.rodata.*)
> >          VPCI_ARRAY
> > +        VUART_ARRAY
> >          *(.data.rel.ro)
> >          *(.data.rel.ro.*)
> >
> > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> > index edcadff90bfe..59dcaa5fef9a 100644
> > --- a/xen/arch/riscv/xen.lds.S
> > +++ b/xen/arch/riscv/xen.lds.S
> > @@ -47,6 +47,7 @@ SECTIONS
> >          *(.rodata)
> >          *(.rodata.*)
> >          VPCI_ARRAY
> > +        VUART_ARRAY
> >          *(.data.rel.ro)
> >          *(.data.rel.ro.*)
> >
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 8e9cac75b09e..43426df33092 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -136,6 +136,7 @@ SECTIONS
> >         *(.rodata)
> >         *(.rodata.*)
> >         VPCI_ARRAY
> > +       VUART_ARRAY
> >         *(.data.rel.ro)
> >         *(.data.rel.ro.*)
> >
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> > index 16936418a6e6..4e0bd524dc43 100644
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -1,6 +1,8 @@
> >
> >  menu "Common Features"
> >
> > +source "common/emul/Kconfig"
> > +
> >  config COMPAT
> >     bool
> >     help
> > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > index c316957fcb36..c0734480ee4b 100644
> > --- 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/
> > diff --git a/xen/common/emul/vuart/Kconfig b/xen/common/emul/vuart/Kconfig
> > new file mode 100644
> > index 000000000000..02f7dd6dc1a1
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/Kconfig
> > @@ -0,0 +1,6 @@
> > +config HAS_VUART
> > +   bool
> > +
> > +menu "UART Emulation"
> > +
> > +endmenu
> > diff --git a/xen/common/emul/vuart/Makefile b/xen/common/emul/vuart/Makefile
> > new file mode 100644
> > index 000000000000..c6400b001e85
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_HAS_VUART) += vuart.o
> > diff --git a/xen/common/emul/vuart/vuart.c b/xen/common/emul/vuart/vuart.c
> > new file mode 100644
> > index 000000000000..14a7f8bd8b79
> > --- /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++)
> 
> Could you possibly do:
> 
> #define for_each_vuart(vdev) \
>     for ( vdev = __start_vuart_array; vdev < __start_vuart_end: vdev++ )
> 
> To avoid the extra __i variable in the inner scope?
> 
> > +
> > +extern const struct vuart_ops *const __start_vuart_array[];
> > +extern const struct vuart_ops *const __start_vuart_end[];
> 
> Naming here looks weird, why not __vuart_{start,end}?  Or
> __{start,end}_vuart_array.

I overlooked that, thanks.

> 
> > +
> > +int vuart_add_node(struct domain *d, const void *node)
> 
> What's the purpose of this function?  There's no comment here or in
> the declaration to figure out what's the purpose.  It's also not being
> called, which makes it unreachable code.  MISRA will likely complain
> about it?

That supposed to ge a hook to generate virtual firmware description for the
guest OS. On Arm/RISC-V that would be DT, on x86 that would be DSDT record in
guest's ACPI.

Stefano already asked to drop that until it is actually used, like for Arm's
vUARTs.

Will remove in v5.

> 
> > +{
> > +    const struct vuart_ops *vdev;
> > +    int rc;
> > +
> > +    for_each_vuart(vdev)
> > +    {
> > +        if ( !vdev->add_node )
> > +            continue;
> > +
> > +        rc = vdev->add_node(d, node);
> > +        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;
> 
> Don't you need the domain_has_vuart() checks in all the handlers?
> Otherwise you are pointlessly iterating and calling handlers that
> won't do anything?

All handlers should check for vuart handler (per-vUART data structure)
not being NULL. Will update that code in v5.

> 
> > +
> > +    for_each_vuart(vdev)
> > +    {
> > +        rc = vdev->init(d, params);
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +
> > +    d->console.input_allowed = true;
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * Release any resources taken by UART emulators.
> > + *
> > + * NB: no flags are cleared, since currently exit() is called only during
> > + * domain destroy.
> > + */
> > +void vuart_deinit(struct domain *d)
> > +{
> > +    const struct vuart_ops *vdev;
> > +
> > +    for_each_vuart(vdev)
> > +        vdev->deinit(d);
> > +}
> > +
> > +void vuart_dump_state(const struct domain *d)
> > +{
> > +    const struct vuart_ops *vdev;
> > +
> > +    for_each_vuart(vdev)
> > +        vdev->dump_state(d);
> > +}
> > +
> > +/*
> > + * Put character to the first suitable emulated UART's FIFO.
> > + */
> > +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 )
> > +            break;
> > +
> > +    return vdev ? vdev->put_rx(d, c) : -ENODEV;
> 
> The above functions seems to be designed to deal with multiple vUARTs
> in-use by the same domain, while the put_rx code gives up as soon as
> it finds an implementation that has the ->put_rx() hook set.

Correct, that is to account for hwdom vUART (or just "vuart" on Arm) which by
design has no RX buffer/register emulation.

> 
> > +}
> > +
> > +bool domain_has_vuart(const struct domain *d)
> > +{
> > +    uint32_t mask = 0;
> > +
> > +    return !!(d->emulation_flags & mask);
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> > index eccd97c565c6..af427d25dc0d 100644
> > --- 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);
> >      }
> >
> >      for_each_domain ( d )
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 963c7b043cd8..93254979817b 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -33,6 +33,7 @@
> >  #include <asm/setup.h>
> >  #include <xen/sections.h>
> >  #include <xen/consoled.h>
> > +#include <xen/vuart.h>
> >
> >  #ifdef CONFIG_X86
> >  #include <asm/guest.h>
> > @@ -601,6 +602,7 @@ static void __serial_rx(char c)
> >          /*
> >           * Deliver input to the hardware domain buffer, unless it is
> >           * already full.
> > +         * NB: must be the first check: hardware domain may have emulated 
> > UART.
> >           */
> >          if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> >              serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > @@ -611,6 +613,8 @@ static void __serial_rx(char c)
> >           */
> >          send_global_virq(VIRQ_CONSOLE);
> >      }
> > +    else if ( domain_has_vuart(d) )
> > +        rc = vuart_put_rx(d, c);
> >  #ifdef CONFIG_SBSA_VUART_CONSOLE
> >      else
> >          /* Deliver input to the emulated UART. */
> > diff --git a/xen/include/xen/vuart.h b/xen/include/xen/vuart.h
> > new file mode 100644
> > index 000000000000..e843026df4b1
> > --- /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>
> > +
> > +struct vuart_params {
> > +    domid_t console_domid;
> > +    gfn_t gfn;
> > +    evtchn_port_t evtchn;
> 
> I think this should be empty initially, as there's no implementation
> that uses the hooks it's completely opaque to me what should be placed
> in vuart_params.

Good point, will do.

> 
> > +};
> > +
> > +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);
> 
> We haven't been very good at this, but ideally hooks should be
> documented as to which task they are expected to perform, so that
> future implementations have some initial help in understanding how
> this is supposed to work.

Agreed.

> 
> > +};
> > +
> > +#define VUART_REGISTER(name, x) \
> > +    static const struct vuart_ops *const __name##_entry \
> > +        __used_section(".data.vuart." #name) = (x);
> 
> For vPCI we are moving this to a different section, I think you want
> to use ".data.rel.ro.vuart" here.

Ack.

> 
> Thanks, Roger.