On Fri, Aug 29, 2025 at 12:57:43PM -0700, Stefano Stabellini wrote:
> On Thu, 28 Aug 2025, dmuk...@xen.org wrote:
> > From: Denis Mukhin <dmuk...@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 <dmuk...@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
> >
