Re: [PATCH v5 01/15] emul/vuart: introduce framework for UART emulators

[dmukhin@xen.org]

On Fri, Aug 29, 2025 at 12:27:13PM -0700, Stefano Stabellini wrote:
> On Thu, 28 Aug 2025, dmukhin@xen.org wrote:
> > From: Denis Mukhin <dmukhin@ford.com> 
> > 
> > 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_VUART_FRAMEWORK, 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.
> > 
> > Enable console forwarding over vUART for hardware domains with a vUART. That
> > enables console forwarding to dom0 on x86, since console can be forwarded only
> > to Xen, dom0 and pvshim on x86 as of now.
> > 
> > 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.
> > 
> > No functional changes for non-x86 architectures.
> > 
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v4:
> > - addressed feedback
> > - Link to v4: https://lore.kernel.org/xen-devel/20250731192130.3948419-3-dmukhin@ford.com/
> > ---
> >  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  | 156 +++++++++++++++++++++++++++++++++
> >  xen/common/keyhandler.c        |   3 +
> >  xen/drivers/char/console.c     |   6 +-
> >  xen/include/xen/serial.h       |   3 +
> >  xen/include/xen/vuart.h        | 116 ++++++++++++++++++++++++
> >  xen/include/xen/xen.lds.h      |  10 +++
> >  16 files changed, 314 insertions(+), 1 deletion(-)
> >  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 db17ff1efa98..cd05b18770f4 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 966e514f2034..d877b93a6964 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -132,6 +132,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 76f9ce705f7a..78a32b69e2b2 100644
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -676,4 +676,6 @@ config PM_STATS
> >  	  Enable collection of performance management statistics to aid in
> >  	  analyzing and tuning power/performance characteristics of the system
> >  
> > +source "common/emul/Kconfig"
> > +
> >  endmenu
> > 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..ae0b575c3901
> > --- /dev/null
> > +++ b/xen/common/emul/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_VUART_FRAMEWORK) += vuart/
> > diff --git a/xen/common/emul/vuart/Kconfig b/xen/common/emul/vuart/Kconfig
> > new file mode 100644
> > index 000000000000..ce1b976b7da7
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/Kconfig
> > @@ -0,0 +1,6 @@
> > +config VUART_FRAMEWORK
> > +	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..97f792dc6641
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/Makefile
> > @@ -0,0 +1 @@
> > +obj-y += vuart.o
> > diff --git a/xen/common/emul/vuart/vuart.c b/xen/common/emul/vuart/vuart.c
> > new file mode 100644
> > index 000000000000..7b277d00d5c7
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/vuart.c
> > @@ -0,0 +1,156 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * UART emulator framework.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +#include <xen/err.h>
> > +#include <xen/sched.h>
> > +#include <xen/vuart.h>
> > +#include <xen/xvmalloc.h>
> > +
> > +#define for_each_emulator(e) \
> > +    for ( e = vuart_array_start; e < vuart_array_end; e++ )
> > +
> > +extern const struct vuart_emulator vuart_array_start[];
> > +extern const struct vuart_emulator vuart_array_end[];
> > +
> > +static const struct vuart_emulator *
> > +vuart_match_by_compatible(struct domain *d, const char *compat)
> > +{
> > +    const struct vuart_emulator *emulator;
> > +
> > +    if ( d->console.vuart )
> > +        return NULL;
> > +
> > +    for_each_emulator(emulator)
> > +        if ( emulator->compatible &&
> > +             !strncmp(emulator->compatible, compat,
> > +                      strlen(emulator->compatible)) )
> 
> strncmp will continue until the given count even if compat is shorter

I checked lib/strncmp.c. I'll flip the arguments, so that strncmp() will
return early if compat is shorter than emulator->compatible 

> 
> 
> > +            return emulator;
> > +
> > +    return NULL;
> > +}
> > +
> > +static struct vuart *vuart_find_by_console_permission(const struct domain *d)
> > +{
> > +    struct vuart *vuart = d->console.vuart;
> > +
> > +    ASSERT(d->console.input_allowed);
> 
> This ASSERT looks suspicious as we haven't even checked that vuart!=NULL
> yet. I would remove it

Ack

> 
> 
> > +    if ( !vuart || !vuart->emulator || !vuart->emulator->put_rx ||
> > +         !(vuart->flags & VUART_CONSOLE_INPUT))
> > +        return NULL;
> > +
> > +    return vuart;
> > +}
> > +
> > +struct vuart *vuart_find_by_io_range(struct domain *d, unsigned long addr,
> > +                                     unsigned long size)
> > +{
> > +    struct vuart *vuart = d->console.vuart;
> > +
> > +    if ( !vuart || !vuart->info )
> > +        return NULL;
> 
> You might as well call vuart_find_by_console_permission that has all the
> checks already

vuart_find_by_io_range() is used in the I/O hook to find the vUART by its
I/O port range.

Using vuart_find_by_console_permission() here may be problematic if the
vUART does not support/enable console forwarding.

I'll keep this check as is for now.

> 
> > +
> > +    if ( addr >= vuart->info->base_addr &&
> > +         addr + size - 1 <= vuart->info->base_addr + vuart->info->size - 1 )
> > +        return vuart;
> > +
> > +    return NULL;
> > +}
> > +
> > +int vuart_init(struct domain *d, struct vuart_info *info)
> > +{
> > +    const struct vuart_emulator *emulator;
> > +    struct vuart *vuart;
> > +    int rc;
> > +
> > +    emulator = vuart_match_by_compatible(d, info->compatible);
> > +    if ( !emulator )
> > +        return -ENODEV;
> > +
> > +    vuart = xzalloc(typeof(*vuart));
> > +    if ( !vuart )
> > +        return -ENOMEM;
> > +
> > +    vuart->info = xvzalloc(typeof(*info));
> > +    if ( !vuart->info )
> > +    {
> > +        rc = -ENOMEM;
> > +        goto err_out;
> > +    }
> > +    memcpy(vuart->info, info, sizeof(*info));
> 
> one thing to note is that the fields (strings) compatible and name are
> copied by address, I am not if it is OK but FYI

Thanks, will update vuart_info struct.
> 
> 
> > +    vuart->vdev = emulator->alloc(d, vuart->info);
> > +    if ( IS_ERR(vuart->vdev) )
> > +    {
> > +        rc = PTR_ERR(vuart->vdev);
> > +        goto err_out;
> 
> this path leads to vuart->info not being freed

Thanks

> 
> 
> > +    }
> > +
> > +    vuart->emulator = emulator;
> > +    vuart->owner = d;
> > +    vuart->flags |= VUART_CONSOLE_INPUT;
> > +
> > +    d->console.input_allowed = true;
> > +    d->console.vuart = vuart;
> > +
> > +    return 0;
> > +
> > + err_out:
> > +    XVFREE(vuart);
> > +    return rc;
> > +}
> > +
> > +/*
> > + * 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)
> > +{
> > +    struct vuart *vuart = d->console.vuart;
> > +
> > +    if ( vuart )
> > +    {
> > +        vuart->emulator->free(vuart);
> 
> should we pass vuart or vuart->vdev here? The emulator state is supposed
> to be vuart->vdev?

That should be vuart->vdev; thanks!

> 
> > +        XVFREE(vuart->info);
> > +    }
> > +
> > +    XVFREE(d->console.vuart);
> > +}
> > +
> > +void vuart_dump_state(const struct domain *d)
> > +{
> > +    struct vuart *vuart = d->console.vuart;
> > +
> > +    if ( vuart )
> > +        vuart->emulator->dump_state(vuart);
> 
> also here vuart->vdev?

Ack

> 
> 
> > +}
> > +
> > +/*
> > + * Put character to the *first* suitable emulated UART's FIFO.
> > + */
> > +int vuart_put_rx(struct domain *d, char c)
> > +{
> > +    struct vuart *vuart = vuart_find_by_console_permission(d);
> > +
> > +    return vuart ? vuart->emulator->put_rx(vuart, c) : -ENODEV;
> 
> and here?

Ack

> 
> 
> > +}
> > +
> > +bool domain_has_vuart(const struct domain *d)
> > +{
> > +    return vuart_find_by_console_permission(d);
> > +}
> > +
> > +/*
> > + * 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 cb6df2823b00..156e64d9eb58 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;
> > @@ -352,6 +353,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 9bd5b4825da6..d5164897a776 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>
> > @@ -596,11 +597,12 @@ static void __serial_rx(char c)
> >      if ( !d )
> >          return;
> >  
> > -    if ( is_hardware_domain(d) )
> > +    if ( is_hardware_domain(d) && !domain_has_vuart(d) )
> >      {
> >          /*
> >           * 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/serial.h b/xen/include/xen/serial.h
> > index 8e1844555208..d7e81f098359 100644
> > --- a/xen/include/xen/serial.h
> > +++ b/xen/include/xen/serial.h
> > @@ -36,6 +36,9 @@ struct vuart_info {
> >      unsigned long data_off;     /* Data register offset */
> >      unsigned long status_off;   /* Status register offset */
> >      unsigned long status;       /* Ready status value */
> > +    unsigned int irq;           /* Interrupt */
> > +    const char *compatible;     /* Compatible string */
> > +    const char *name;           /* User-friendly name */
> >  };
> >  
> >  struct serial_port {
> > diff --git a/xen/include/xen/vuart.h b/xen/include/xen/vuart.h
> > new file mode 100644
> > index 000000000000..ca025b4179be
> > --- /dev/null
> > +++ b/xen/include/xen/vuart.h
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * UART emulator framework.
> > + *
> > + * Copyright 2025 Ford Motor Company
> > + */
> > +
> > +#ifndef XEN_VUART_H
> > +#define XEN_VUART_H
> > +
> > +#include <xen/serial.h>
> > +#include <public/xen.h>
> > +
> > +struct vuart_emulator;
> > +
> > +enum {
> > +    VUART_CONSOLE_INPUT = 0U << 1, /** Physical console input forwarding. */
> 
> code style: single *

Ack

> This flag is zero, is that intended?

Will fix, thanks

> 
> 
> > +};
> > +
> > +
> > +/*
> > + * FIXME: #ifdef is temporary to avoid clash with
> > + *   arch/arm/include/asm/domain.h
> > + */
> > +#ifdef CONFIG_VUART_FRAMEWORK
> > +struct vuart {
> > +    const struct vuart_emulator *emulator;
> > +    struct vuart_info *info;
> > +    struct domain *owner;
> > +    uint32_t flags;
> > +    void *vdev;
> > +};
> > +#endif
> > +
> > +struct vuart_emulator {
> > +    /* UART compatible string. Cannot be NULL or empty. */
> > +    const char *compatible;
> > +
> > +    /*
> > +     * Allocate emulated UART state (RX/TX FIFOs, locks, initialize registers,
> > +     * hook I/O handlers, etc.)
> > +     * Cannot be NULL.
> > +     */
> > +    void *(*alloc)(struct domain *d, const struct vuart_info *info);
> > +
> > +    /*
> > +     * Release resources used to emulate UART state (flush RX/TX FIFOs, unhook
> > +     * I/O handlers, etc.).
> > +     * Cannot be NULL.
> > +     */
> > +    void (*free)(void *arg);
> > +
> > +    /*
> > +     * Print emulated UART state, including registers, on the console.
> > +     * Can be NULL.
> > +     */
> > +    void (*dump_state)(void *arg);
> > +
> > +    /*
> > +     * Place character to the emulated RX FIFO.
> > +     * Used to forward physical console input to the guest OS.
> > +     * Can be NULL.
> > +     */
> > +    int (*put_rx)(void *arg, char c);
> > +};
> > +
> > +#define VUART_REGISTER(name, x) \
> > +    static const struct vuart_emulator name##_entry \
> > +        __used_section(".data.rel.ro.vuart") = x
> > +
> > +struct vuart *vuart_find_by_io_range(struct domain *d,
> > +                                     unsigned long base_addr,
> > +                                     unsigned long size);
> > +
> > +int vuart_put_rx(struct domain *d, char c);
> > +
> > +#ifdef CONFIG_VUART_FRAMEWORK
> > +
> > +int vuart_init(struct domain *d, struct vuart_info *info);
> > +void vuart_deinit(struct domain *d);
> > +void vuart_dump_state(const struct domain *d);
> > +bool domain_has_vuart(const struct domain *d);
> > +
> > +#else
> > +
> > +static inline int vuart_init(struct domain *d, struct vuart_info *info)
> > +{
> > +    return 0;
> > +}
> > +
> > +static inline void vuart_deinit(struct domain *d)
> > +{
> > +}
> > +
> > +static inline void vuart_dump_state(const struct domain *d)
> > +{
> > +}
> > +
> > +static inline bool domain_has_vuart(const struct domain *d)
> > +{
> > +    return false;
> > +}
> > +
> > +#endif /* CONFIG_VUART_FRAMEWORK */
> > +
> > +#endif /* XEN_VUART_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > +
> > diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
> > index b126dfe88792..2d65f32ddad3 100644
> > --- a/xen/include/xen/xen.lds.h
> > +++ b/xen/include/xen/xen.lds.h
> > @@ -194,4 +194,14 @@
> >  #define VPCI_ARRAY
> >  #endif
> >  
> > +#ifdef CONFIG_VUART_FRAMEWORK
> > +#define VUART_ARRAY              \
> > +       . = ALIGN(POINTER_ALIGN); \
> > +       vuart_array_start = .;    \
> > +       *(.data.rel.ro.vuart)     \
> > +       vuart_array_end = .;
> > +#else
> > +#define VUART_ARRAY
> > +#endif
> > +
> >  #endif /* __XEN_LDS_H__ */
> > -- 
> > 2.51.0
> >