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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Sat, Sep 6, 2025 at 2:27 AM <dmukhin@xxxxxxx> 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_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@xxxxxxxx>
> ---
> Changes since v5:
> - addressed v5 feedback
> - Link to v5: 
> https://lore.kernel.org/xen-devel/20250828235409.2835815-2-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  | 157 +++++++++++++++++++++++++++++++++
>  xen/common/keyhandler.c        |   3 +
>  xen/drivers/char/console.c     |   6 +-
>  xen/include/xen/sched.h        |   4 +
>  xen/include/xen/serial.h       |   3 +
>  xen/include/xen/vuart.h        | 116 ++++++++++++++++++++++++
>  xen/include/xen/xen.lds.h      |  10 +++
>  17 files changed, 319 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 0c7d0f5d46e1..8c8462565050 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
>  obj-$(CONFIG_IOREQ_SERVER) += dm.o
>  obj-y += domain.o
>  obj-y += domid.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

If VUART_FRAMEWORK has no dependencies, it can be enabled on any
architecture. For example, I tried enabling it on arm64 and the build
fails:

  ./include/xen/vuart.h:26:8: error: redefinition of ‘struct vuart’

Should this config be restricted (e.g. "depends on X86") or the code
adjusted to handle non-x86 architectures properly?

> +
> +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..3dfcba217248
> --- /dev/null
> +++ b/xen/common/emul/vuart/vuart.c
> @@ -0,0 +1,157 @@
> +/* 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(const struct domain *d, const char *compat)
> +{
> +    const struct vuart_emulator *emulator;
> +
> +    for_each_emulator(emulator)
> +        if ( emulator->compatible &&
> +             !strncmp(compat, emulator->compatible,
> +                      strlen(emulator->compatible)) )
> +            return emulator;
> +
> +    return NULL;
> +}
> +
> +const static struct vuart *
> +vuart_find_by_console_permission(const struct domain *d)

During the first review of this patch I thought you planned to add a
searching procedure into this and the next function in one of the later
commits. However, looking at the series now, it seems these functions
remain unchanged and don’t actually perform any searching.

Do you think the current names are accurate, or would it make sense to
rename them to better reflect their purpose?

> +{
> +    const struct vuart *vuart = d->console.vuart;
> +
> +    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;
> +
> +    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;
> +
> +    if ( d->console.vuart )
> +        return -EBUSY;
> +
> +    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));
> +
> +    vuart->vdev = emulator->alloc(d, vuart->info);
> +    if ( IS_ERR(vuart->vdev) )
> +    {
> +        rc = PTR_ERR(vuart->vdev);
> +        goto err_out;
> +    }
> +
> +    vuart->emulator = emulator;
> +    vuart->owner = d;
> +    vuart->flags |= VUART_CONSOLE_INPUT;
> +
> +    d->console.input_allowed = true;
> +    d->console.vuart = vuart;
> +
> +    return 0;
> +
> + err_out:
> +    if ( vuart )
> +        xvfree(vuart->info);
> +    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->vdev);
> +        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->vdev);
> +}
> +
> +/*
> + * Put character to the *first* suitable emulated UART's FIFO.
> + */
> +int vuart_put_rx(struct domain *d, char c)
> +{
> +    const struct vuart *vuart = vuart_find_by_console_permission(d);
> +
> +    return vuart ? vuart->emulator->put_rx(vuart->vdev, c) : -ENODEV;
> +}
> +
> +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/sched.h b/xen/include/xen/sched.h
> index 02bdc256ce37..613f4596e33d 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -23,6 +23,7 @@
>  #include <asm/atomic.h>
>  #include <asm/current.h>
>  #include <xen/vpci.h>
> +#include <xen/vuart.h>
>  #include <xen/wait.h>
>  #include <public/xen.h>
>  #include <public/domctl.h>
> @@ -660,6 +661,9 @@ struct domain
>      struct {
>          /* Permission to take ownership of the physical console input. */
>          bool input_allowed;
> +#ifdef CONFIG_VUART_FRAMEWORK
> +        struct vuart *vuart;
> +#endif
>      } console;
>  } __aligned(PAGE_SIZE);
>
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 8e1844555208..123eee67df35 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 */
> +    char compatible[16];        /* Compatible string */
> +    char name[16];              /* 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..54f2f29f3f4a
> --- /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 = BIT(0, U), /* Physical console input forwarding. */
> +};
> +
> +
> +/*
> + * 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
>
>

Best regards,
Mykola