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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 31 Jul 2025, 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++)
> +
> +extern const struct vuart_ops *const __start_vuart_array[];
> +extern const struct vuart_ops *const __start_vuart_end[];
> +
> +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);
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return 0;
> +}

Maybe skip this function until we needed? Without the reference
implementation of vuart-ns16550.c it is hard to tell what it is supposed
to do.


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

This works because there is only one emulator (NS16550) but if there
were multiple possible emulators, I think we would want to only
initialize the emulator enabled in the specific domain.

One domain could only have NS16550 and another domain could only have
PL011, while both NS16550 and PL011 might be possible.

I think it is OK for now and this function can be fixed/improved when
adding the second emulator.


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

I don't think this would work with multiple emulators possible, maybe
enable or maybe not, for the same domain.

In a situation where there is both PL011 and NS16550 enable in the Xen
kconfig, but only NS16550 enabled for this specific domain,
for_each_vuart might find PL011 as the first emulator with a put_rx
implementation, but it is not actually the one the domain can use.

I think this is OK for now, but it would have to be fixed when adding a
second emulator.


> +}
> +
> +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;
> +};
> +
> +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;
> +}
> +
> +#endif /* CONFIG_HAS_VUART */
> +
> +#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..c2da180948ca 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_HAS_VUART
> +#define VUART_ARRAY     \
> +       . = ALIGN(POINTER_ALIGN); \
> +       __start_vuart_array = .;  \
> +       *(SORT(.data.vuart.*))    \
> +       __start_vuart_end = .;
> +#else
> +#define VUART_ARRAY
> +#endif
> +
>  #endif /* __XEN_LDS_H__ */
> -- 
> 2.34.1
> 
>