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

[dmukhin@xen.org]

On Fri, Aug 29, 2025 at 12:57:43PM -0700, Stefano Stabellini wrote:
> On Thu, 28 Aug 2025, dmukhin@xen.org wrote:
> > From: Denis Mukhin <dmukhin@ford.com> 
> > 
> > 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@ford.com>
> > ---
> > Changes since v4:
> > - new patch 
> > ---
> >  xen/arch/x86/hvm/hvm.c          |  20 ++
> >  xen/common/emul/vuart/Kconfig   |  18 ++
> >  xen/common/emul/vuart/Makefile  |   1 +
> >  xen/common/emul/vuart/ns16x50.c | 362 ++++++++++++++++++++++++++++++++
> >  xen/include/xen/sched.h         |   4 +
> >  5 files changed, 405 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..26760cf995df 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,21 @@ int hvm_domain_initialise(struct domain *d,
> >      if ( rc != 0 )
> >          goto fail1;
> >  
> > +    if ( IS_ENABLED(CONFIG_VUART_NS16X50) )
> 
> maybe only for the hardware domain?
> or only input_allowed = true only for the hardware domain?

Agreed; I can enable it for dom0 for now.

The plan is have vuart_info populated by xl similarly how it is
done for vpl011.

> 
> > +    {
> > +        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 +728,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 +792,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..539e6d5e4fc7 100644
> > --- a/xen/common/emul/vuart/Kconfig
> > +++ b/xen/common/emul/vuart/Kconfig
> > @@ -3,4 +3,22 @@ config VUART_FRAMEWORK
> >  
> >  menu "UART Emulation"
> >  
> > +config VUART_NS16X50
> > +	bool "NS16550-compatible UART Emulator" if EXPERT
> > +	depends on X86 && HVM
> > +	select VUART_FRAMEWORK
> 
> default n

Ack

> 
> > +	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..f0479e1022fb
> > --- /dev/null
> > +++ b/xen/common/emul/vuart/ns16x50.c
> > @@ -0,0 +1,362 @@
> > +/* 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, KERN_ERR, vdev, fmt, ## args)
> > +#define ns16x50_warn(vdev, fmt, args...) \
> > +    ns16x50_log(1, KERN_WARNING, vdev, fmt, ## args)
> > +#define ns16x50_info(vdev, fmt, args...) \
> > +    ns16x50_log(2, KERN_INFO, vdev, fmt, ## args)
> > +#define ns16x50_debug(vdev, fmt, args...) \
> > +    ns16x50_log(3, KERN_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 {
> > +    struct xencons_interface cons;      /* Emulated RX/TX FIFOs */
> > +    uint8_t regs[NS16X50_EMU_REGS_NUM]; /* Emulated registers */
> > +    const char *name;                   /* Device name */
> > +    struct domain *owner;               /* Owner domain */
> > +    const struct vuart_info *info;      /* UART description */
> > +    spinlock_t lock;                    /* Protection */
> > +};
> > +
> > +/*
> > + * 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(KERN_ERR "%c io 0x%04x %d: not initialized\n",
> 
> XENLOG_ERR

Will update KERN to XENLOG everywhere in the new code.

> 
> 
> > +               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);
> 
> 
> For this one we could consider returning X86EMUL_UNHANDLEABLE but I am not sure
> as it would crash the guest.

re: crashing the guest: that is the main reason why I kept X86EMUL_OKAY.
I think the emulator should not crash the guest OS since this is facility
for OS bringup debugging.

> 
> > +        goto out;
> > +    }
> > +
> > +    dlab = 0;
> > +    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);
> 
> it should unregister the handlers

Unfortunately, there's no unregister_portio_handler() and AFAIU the reason for
that is there's no need in it since I/O handlers will be destroyed during
domain destruction when ns16x50_deinit() is called.

> 
> 
> > +}
> > +
> > +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);
> > +    }
> > +
> > +    /* Save convenience pointer. */
> > +    vdev->name = info->name;
> > +    vdev->owner = d;
> > +    vdev->info = info;
> 
> the spinlock should be initialized 

Ack

> 
> 
> > +    rc = ns16x50_init(vdev);
> > +    if ( rc )
> > +        return ERR_PTR(rc);
> > +
> > +    return vdev;
> > +}
> > +
> > +static void cf_check ns16x50_free(void *arg)
> > +{
> > +    struct vuart_ns16x50 *vdev = arg;
> > +
> > +    if ( vdev )
> > +        ns16x50_deinit(vdev);
> > +
> > +    XVFREE(vdev);
> 
> XVFREE should only be called if ( vdev )

Ack

> 
> 
> > +}
> > +
> > +#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:
> > + */
> > 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
> 
> this should be in patch #1, as patch #1 does:
> 
> d->console.vuart = vuart;

Yes, indeed, thanks.
Will move to patch #1.

> 
> 
> >      } console;
> >  } __aligned(PAGE_SIZE);
> >  
> > -- 
> > 2.51.0
> >