Re: [PATCH v6 03/15] emul/ns16x50: implement emulator stub

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

On Sat, Sep 6, 2025 at 2:27 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> The change is the first on the way on introducing minimally functional
> NS16550-compatible UART emulator.
>
> Define UART state and a set of emulated registers.
>
> Implement alloc/free vUART hooks.
>
> Stub out I/O port handler.
>
> Add initialization of the NS16x50-compatible UART emulator state machine.
>
> Plumb debug logging.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v5:
> - v5 feedback
> - Link to v5: 
> https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmukhin@xxxxxxxx/
> ---
>  xen/arch/x86/hvm/hvm.c          |  21 ++
>  xen/common/emul/vuart/Kconfig   |  19 ++
>  xen/common/emul/vuart/Makefile  |   1 +
>  xen/common/emul/vuart/ns16x50.c | 366 ++++++++++++++++++++++++++++++++
>  4 files changed, 407 insertions(+)
>  create mode 100644 xen/common/emul/vuart/ns16x50.c
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 23bd7f078a1d..91c971f11e14 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -28,6 +28,7 @@
>  #include <xen/softirq.h>
>  #include <xen/trace.h>
>  #include <xen/vm_event.h>
> +#include <xen/vuart.h>
>  #include <xen/vpci.h>
>  #include <xen/wait.h>
>  #include <xen/warning.h>
> @@ -689,6 +690,22 @@ int hvm_domain_initialise(struct domain *d,
>      if ( rc != 0 )
>          goto fail1;
>
> +    /* Limit NS16550 emulator for dom0 only for now. */
> +    if ( IS_ENABLED(CONFIG_VUART_NS16X50) && is_hardware_domain(d) )
> +    {
> +        struct vuart_info info = {
> +            .name       = "COM2",
> +            .compatible = "ns16550",
> +            .base_addr  = 0x2f8,
> +            .size       = 8,
> +            .irq        = 3,
> +        };
> +
> +        rc = vuart_init(d, &info);
> +        if ( rc )
> +            goto out_vioapic_deinit;
> +    }
> +
>      stdvga_init(d);
>
>      rtc_init(d);
> @@ -712,6 +729,8 @@ int hvm_domain_initialise(struct domain *d,
>      return 0;
>
>   fail2:
> +    vuart_deinit(d);
> + out_vioapic_deinit:
>      vioapic_deinit(d);
>   fail1:
>      if ( is_hardware_domain(d) )
> @@ -774,6 +793,8 @@ void hvm_domain_destroy(struct domain *d)
>      if ( hvm_funcs.domain_destroy )
>          alternative_vcall(hvm_funcs.domain_destroy, d);
>
> +    vuart_deinit(d);
> +
>      vioapic_deinit(d);
>
>      XFREE(d->arch.hvm.pl_time);
> diff --git a/xen/common/emul/vuart/Kconfig b/xen/common/emul/vuart/Kconfig
> index ce1b976b7da7..a27d7ca135af 100644
> --- a/xen/common/emul/vuart/Kconfig
> +++ b/xen/common/emul/vuart/Kconfig
> @@ -3,4 +3,23 @@ config VUART_FRAMEWORK
>
>  menu "UART Emulation"
>
> +config VUART_NS16X50
> +       bool "NS16550-compatible UART Emulator" if EXPERT
> +       depends on X86 && HVM

Currently VUART_NS16X50 depends on X86, so the code is only
usable on x86. Are there any plans to support this vUART on non-x86
architectures (e.g. ARM, where some UARTs are also ns16550-compatible)?

If not, wouldn’t it be more appropriate to move the ns16x50-specific
implementation into x86-specific directories instead of common?

> +       select VUART_FRAMEWORK
> +       default n
> +       help
> +         In-hypervisor NS16x50 UART emulation.
> +
> +         Only legacy PC COM2 port is emulated.
> +
> +         This is strictly for testing purposes (such as early HVM guest 
> console),
> +         and not appropriate for use in production.
> +
> +config VUART_NS16X50_DEBUG
> +       bool "NS16550-compatible UART Emulator Debugging"
> +       depends on VUART_NS16X50 && DEBUG
> +       help
> +         Enable development debugging.
> +
>  endmenu
> diff --git a/xen/common/emul/vuart/Makefile b/xen/common/emul/vuart/Makefile
> index 97f792dc6641..fe904f6cb65d 100644
> --- a/xen/common/emul/vuart/Makefile
> +++ b/xen/common/emul/vuart/Makefile
> @@ -1 +1,2 @@
>  obj-y += vuart.o
> +obj-$(CONFIG_VUART_NS16X50) += ns16x50.o
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> new file mode 100644
> index 000000000000..0462a961e785
> --- /dev/null
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -0,0 +1,366 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * NS16550-compatible UART Emulator.
> + *
> + * See:
> + * - Serial and UART Tutorial:
> + *     
> https://download.freebsd.org/doc/en/articles/serial-uart/serial-uart_en.pdf
> + * - UART w/ 16 byte FIFO:
> + *     https://www.ti.com/lit/ds/symlink/tl16c550c.pdf
> + * - UART w/ 64 byte FIFO:
> + *     https://www.ti.com/lit/ds/symlink/tl16c750.pdf
> + *
> + * Limitations:
> + * - Only x86;
> + * - Only Xen console as a backend, no inter-domain communication (similar to
> + *   vpl011 on Arm);
> + * - Only 8n1 emulation (8-bit data, no parity, 1 stop bit);
> + * - No baud rate emulation (reports 115200 baud to the guest OS);
> + * - No FIFO-less mode emulation;
> + * - No RX FIFO interrupt moderation (FCR) emulation;
> + * - No integration w/ VM snapshotting (HVM_REGISTER_SAVE_RESTORE() and
> + *   friends);
> + * - No ISA IRQ sharing allowed;
> + * - No MMIO-based UART emulation.
> + */
> +
> +#define pr_prefix               "ns16x50"
> +#define pr_fmt(fmt)             pr_prefix ": " fmt
> +
> +#ifdef CONFIG_VUART_NS16X50_DEBUG
> +#define guest_prefix            "FROM GUEST "
> +#define ns16x50_log_level       2
> +#else
> +#define guest_prefix            ""
> +#define ns16x50_log_level       0
> +#endif
> +
> +#include <xen/8250-uart.h>
> +#include <xen/console.h>
> +#include <xen/err.h>
> +#include <xen/iocap.h>
> +#include <xen/vuart.h>
> +#include <xen/xvmalloc.h>
> +
> +#include <public/io/console.h>
> +
> +#define ns16x50_log(n, lvl, vdev, fmt, args...) \
> +do { \
> +    if ( ns16x50_log_level >= n ) \
> +        gprintk(lvl, pr_fmt("%s: " fmt), (vdev)->name, ## args); \
> +} while (0)
> +
> +#define ns16x50_err(vdev, fmt, args...) \
> +    ns16x50_log(0, XENLOG_ERR, vdev, fmt, ## args)
> +#define ns16x50_warn(vdev, fmt, args...) \
> +    ns16x50_log(1, XENLOG_WARNING, vdev, fmt, ## args)
> +#define ns16x50_info(vdev, fmt, args...) \
> +    ns16x50_log(2, XENLOG_INFO, vdev, fmt, ## args)
> +#define ns16x50_debug(vdev, fmt, args...) \
> +    ns16x50_log(3, XENLOG_DEBUG, vdev, fmt, ## args)
> +
> +/*
> + * Number of supported registers in the UART.
> + */
> +#define NS16X50_REGS_NUM        (UART_SCR + 1)
> +
> +/*
> + * Number of emulated registers.
> + *
> + * - Emulated registers [0..NS16X50_REGS_NUM] are R/W registers for DLAB=0.
> + * - DLAB=1, R/W, DLL            = NS16X50_REGS_NUM + 0
> + * - DLAB=1, R/W, DLM            = NS16X50_REGS_NUM + 1
> + * -         R/O, IIR (IIR_THR)  = NS16X50_REGS_NUM + 2
> + */
> +#define NS16X50_EMU_REGS_NUM    (NS16X50_REGS_NUM + 3)
> +
> +/*
> + * Virtual ns16x50 device state.
> + */
> +struct vuart_ns16x50 {
> +    uint8_t regs[NS16X50_EMU_REGS_NUM]; /* Emulated registers */
> +    const struct vuart_info *info;      /* UART description */
> +    struct domain *owner;               /* Owner domain */
> +    const char *name;                   /* Device name */
> +    spinlock_t lock;                    /* Protection */
> +    struct xencons_interface cons;      /* Emulated RX/TX FIFOs */
> +};
> +
> +static uint8_t ns16x50_dlab_get(const struct vuart_ns16x50 *vdev)
> +{
> +    return 0;
> +}
> +
> +/*
> + * Emulate 8-bit write access to ns16x50 register.
> + */
> +static int ns16x50_io_write8(
> +    struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
> +{
> +    int rc = 0;
> +
> +    return rc;
> +}
> +
> +/*
> + * Emulate 16-bit write access to ns16x50 register.
> + * NB: some guest OSes use outw() to access UART_DLL.
> + */
> +static int ns16x50_io_write16(
> +    struct vuart_ns16x50 *vdev, uint32_t reg, uint16_t *data)
> +{
> +    int rc = -EINVAL;
> +
> +    return rc;
> +}
> +
> +/*
> + * Emulate write access to ns16x50 register.
> + */
> +static int ns16x50_io_write(
> +    struct vuart_ns16x50 *vdev, uint8_t reg, uint32_t size, uint32_t *data)
> +{
> +    int rc;
> +
> +    switch ( size )
> +    {
> +    case 1:
> +        rc = ns16x50_io_write8(vdev, reg, (uint8_t *)data);
> +        break;
> +
> +    case 2:
> +        rc = ns16x50_io_write16(vdev, reg, (uint16_t *)data);
> +        break;
> +
> +    default:
> +        rc = -EINVAL;
> +        break;
> +    }
> +
> +    return rc;
> +}
> +
> +/*
> + * Emulate 8-bit read access to ns16x50 register.
> + */
> +static int ns16x50_io_read8(
> +    struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
> +{
> +    uint8_t val = 0xff;
> +    int rc = 0;
> +
> +    *data = val;
> +
> +    return rc;
> +}
> +
> +/*
> + * Emulate 16-bit read access to ns16x50 register.
> + */
> +static int ns16x50_io_read16(
> +    struct vuart_ns16x50 *vdev, uint32_t reg, uint16_t *data)
> +{
> +    uint16_t val = 0xffff;
> +    int rc = -EINVAL;
> +
> +    *data = val;
> +
> +    return rc;
> +}
> +
> +/*
> + * Emulate read access to ns16x50 register.
> + */
> +static int ns16x50_io_read(
> +    struct vuart_ns16x50 *vdev, uint8_t reg, uint32_t size, uint32_t *data)
> +{
> +    int rc;
> +
> +    switch ( size )
> +    {
> +    case 1:
> +        rc = ns16x50_io_read8(vdev, reg, (uint8_t *)data);
> +        break;
> +
> +    case 2:
> +        rc = ns16x50_io_read16(vdev, reg, (uint16_t *)data);
> +        break;
> +
> +    default:
> +        *data = 0xffffffff;
> +        rc = -EINVAL;
> +        break;
> +    }
> +
> +    return rc;
> +}
> +
> +/*
> + * Emulate I/O access to ns16x50 register.
> + * Note, emulation always returns X86EMUL_OKAY, once I/O port trap is 
> enabled.
> + */
> +static int cf_check ns16x50_io_handle(
> +    int dir, unsigned int addr, unsigned int size, uint32_t *data)
> +{
> +#define op(dir)     (((dir) == IOREQ_WRITE) ? 'W' : 'R')
> +    struct domain *d = rcu_lock_current_domain();
> +    struct vuart *vuart = vuart_find_by_io_range(d, addr, size);
> +    struct vuart_ns16x50 *vdev;
> +    const struct domain *owner;
> +    const struct vuart_info *info;
> +    uint32_t reg;
> +    unsigned dlab;
> +    int rc;
> +
> +    if ( !vuart || !vuart->vdev )
> +    {
> +        printk(XENLOG_ERR "%c io 0x%04x %d: not initialized\n",
> +               op(dir), addr, size);
> +
> +        ASSERT_UNREACHABLE();
> +        goto out;
> +    }
> +    vdev = vuart->vdev;
> +
> +    owner = vuart->owner;
> +    ASSERT(owner);
> +    if ( d != owner )
> +    {
> +        ns16x50_err(vdev, "%c io 0x%04x %d: does not match current domain 
> %pv\n",
> +                    op(dir), addr, size, d);
> +
> +        ASSERT_UNREACHABLE();
> +        goto out;
> +    }
> +
> +    info = vuart->info;
> +    ASSERT(info);
> +    reg = addr - info->base_addr;
> +    if ( !IS_ALIGNED(reg, size) )
> +    {
> +        ns16x50_err(vdev, "%c 0x%04x %d: unaligned access\n",
> +                    op(dir), addr, size);
> +        goto out;
> +    }
> +
> +    dlab = ns16x50_dlab_get(vdev);
> +    if ( reg >= NS16X50_REGS_NUM )
> +    {
> +        ns16x50_err(vdev, "%c io 0x%04x %d: DLAB=%d %02"PRIx32" 
> 0x%08"PRIx32": not implemented\n",
> +                    op(dir), addr, size, dlab, reg, *data);
> +        goto out;
> +    }
> +
> +    spin_lock(&vdev->lock);
> +
> +    if ( dir == IOREQ_WRITE )
> +    {
> +        ns16x50_debug(vdev, "%c 0x%04x %d: DLAB=%d %02"PRIx32" 
> 0x%08"PRIx32"\n",
> +                      op(dir), addr, size, dlab, reg, *data);
> +        rc = ns16x50_io_write(vdev, reg, size, data);
> +    }
> +    else
> +    {
> +        rc = ns16x50_io_read(vdev, reg, size, data);
> +        ns16x50_debug(vdev, "%c 0x%04x %d: DLAB=%d %02"PRIx32" 
> 0x%08"PRIx32"\n",
> +                      op(dir), addr, size, dlab, reg, *data);
> +    }
> +    if ( rc < 0 )
> +        ns16x50_err(vdev, "%c 0x%04x %d: DLAB=%d %02"PRIx32" 0x%08"PRIx32": 
> unsupported access\n",
> +                    op(dir), addr, size, dlab, reg, *data);
> +
> +    spin_unlock(&vdev->lock);
> +
> +out:
> +    rcu_unlock_domain(d);
> +
> +    return X86EMUL_OKAY;
> +#undef op
> +}
> +
> +static int ns16x50_init(void *arg)
> +{
> +    struct vuart_ns16x50 *vdev = arg;
> +    const struct vuart_info *info = vdev->info;
> +    struct domain *d = vdev->owner;
> +
> +    ASSERT(vdev);
> +
> +    register_portio_handler(d, info->base_addr, info->size, 
> ns16x50_io_handle);
> +
> +    return 0;
> +}
> +
> +static void cf_check ns16x50_deinit(void *arg)
> +{
> +    struct vuart_ns16x50 *vdev = arg;
> +
> +    ASSERT(vdev);
> +}
> +
> +static void * cf_check ns16x50_alloc(struct domain *d, const struct 
> vuart_info *info)
> +{
> +    struct vuart_ns16x50 *vdev;
> +    int rc;
> +
> +    if ( !info )
> +        return ERR_PTR(-EINVAL);
> +
> +    if ( vuart_find_by_io_range(d, info->base_addr, info->size) )
> +    {
> +        ns16x50_err(info, "already registered\n");
> +        return ERR_PTR(-EBUSY);
> +    }
> +
> +    if ( !is_hvm_domain(d) )
> +    {
> +        ns16x50_err(info, "not an HVM domain\n");
> +        return ERR_PTR(-ENOSYS);
> +    }
> +
> +    vdev = xvzalloc(typeof(*vdev));
> +    if ( !vdev )
> +    {
> +        ns16x50_err(info, "failed to allocate memory\n");
> +        return ERR_PTR(-ENOMEM);
> +    }
> +
> +    spin_lock_init(&vdev->lock);
> +    vdev->name = info->name;
> +    vdev->owner = d;
> +    vdev->info = info;
> +
> +    rc = ns16x50_init(vdev);
> +    if ( rc )
> +        return ERR_PTR(rc);
> +
> +    return vdev;
> +}
> +
> +static void cf_check ns16x50_free(void *arg)
> +{
> +    if ( arg )
> +        ns16x50_deinit(arg);
> +
> +    xvfree(arg);
> +}
> +
> +#define ns16x50_emulator                \
> +{                                       \
> +    .compatible = "ns16550",            \
> +    .alloc      = ns16x50_alloc,        \
> +    .free       = ns16x50_free,         \
> +    .dump_state = NULL,                 \
> +    .put_rx     = NULL,                 \
> +}
> +
> +VUART_REGISTER(ns16x50, ns16x50_emulator);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.51.0
>
>

Best regards,
Mykola