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 v5:
- v5 feedback
- Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmukhin@ford.com/
---
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
+ 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.0Hi Denis,
On Sat, Sep 6, 2025 at 2:27 AM <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 v5:
> - v5 feedback
> - Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmukhin@ford.com/
> ---
> 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
On Mon, Sep 08, 2025 at 11:37:03AM +0300, Mykola Kvach wrote: > > --- 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)? The plan is to add MMIO-based implementation for x86 first. > > If not, wouldn’t it be more appropriate to move the ns16x50-specific > implementation into x86-specific directories instead of common? There was discussion on that and the agreement is to move all vUARTs into common/emul/vuart and start using common/emul as a placeholder for common emulation code, including vPCI.
Hi Denis,
On Sat, Sep 6, 2025 at 2:27 AM <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 v5:
> - v5 feedback
> - Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmukhin@ford.com/
> ---
> 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>
I noticed that this include ...
> +#include <xen/vuart.h>
... is out of alphabetical order. All other includes in this block
are correctly sorted.
> #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) )
Currently, the Xen glossary defines a "hardware domain" as:
A domain, commonly dom0, which shares responsibility with Xen
about the system as a whole.
It has been historically correct to treat is_hardware_domain(d) as
equivalent to dom0. However, according to patch series [1], this is
no longer guaranteed:
"Setting hardware domain as domid 0 is removed."
After these changes, the assumption that hardware domain always equals
dom0 may not hold. As a result, the above comment in the code could
become misleading. Consider updating it to something like:
/* Limit NS16550 emulator to the hardware domain only for now */
to reflect the new semantics.
> + {
> + struct vuart_info info = {
> + .name = "COM2",
> + .compatible = "ns16550",
> + .base_addr = 0x2f8,
> + .size = 8,
> + .irq = 3,
> + };
Consider defining COM2 base address and IRQ in a shared header
(e.g., VUART_COM2_BASE and VUART_COM2_IRQ) rather than using
the magic numbers 0x2f8 and 3 here. This would allow reuse in
`__start_xen` and other places, and makes the code clearer and
easier to maintain.
> +
> + 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
> + 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;
Instead of hardcoding 0xff here, consider using UINT8_MAX. This makes
it clear that the value is the maximum for uint8_t and avoids magic
numbers.
Similarly, in other places where constants for 16-bit or 32-bit
unsigned integers are used, it would be good to replace them with
UINT16_MAX and UINT32_MAX respectively for consistency and clarity.
> + 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')
Instead of defining the op(dir) macro, it might be cleaner to compute
the direction character once at the beginning, e.g.:
const char dir_char = (dir == IOREQ_WRITE) ? 'W' : 'R';
and then use dir_char in printk()/debug. This avoids the local macro
and makes the code easier to follow.
> + 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);
AFAICT ns16x50_io_read() and ns16x50_io_write() have identical
signatures. Would it be cleaner to store them in an array of
function pointers and call through that, e.g.:
rc = ns16x50_io_op[dir == IOREQ_WRITE](vdev, reg, size, data);
This would avoid the if/else block here.
> + }
> + 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);
The ns16x50_debug() call is duplicated in both branches of the
IOREQ_WRITE check, differing only in whether it's placed before or
after the access. Would it make sense to move this debug print
outside the condition, so the same code is used for both read and
write paths (assuming the "data" is not modified during a write)?
> + }
> + 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);
The ns16x50_err() call doesn’t require holding vdev->lock, so it would
be cleaner to move it after spin_unlock(). That way the critical section
is shorter.
> +
> + 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) )
Currently, ns16x50_alloc() first calls vuart_find_by_io_range() and
only afterwards checks if the domain is an HVM domain. Wouldn’t it
be more logical to perform the HVM check first? If the console is
only available for HVM domains, the extra check for an uninitialized
vuart field seems unnecessary.
> + {
> + 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 )
If ns16x50_init(vdev) fails, vdev should be freed with xvfree() to
avoid a memory leak.
Currently, ns16x50_init() always returns 0. If it is not planned to
return other values in the future, it may be simpler to make the
function return void, avoiding the need for the rc variable and
conditional checks.
> + 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, \
dump_state is set to NULL, but vuart_dump_state() in the previous
commit does not check this pointer. If all commits up to this one
are applied and domain state is dumped, this could result in a
NULL pointer dereference and crash the hypervisor.
Consider adding a NULL check in vuart_dump_state() or initializing
dump_state properly.
> + .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
>
>
[1] https://patchew.org/Xen/20250416212911.410946-1-jason.andryuk@amd.com/
Best regards,
Mykola
On Sat, Sep 06, 2025 at 11:37:28PM +0300, Mykola Kvach wrote:
> Hi Denis,
>
> On Sat, Sep 6, 2025 at 2:27 AM <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 v5:
> > - v5 feedback
> > - Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmukhin@ford.com/
> > ---
> > 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>
>
> I noticed that this include ...
>
> > +#include <xen/vuart.h>
>
> ... is out of alphabetical order. All other includes in this block
> are correctly sorted.
Thanks, will update.
>
> > #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) )
>
> Currently, the Xen glossary defines a "hardware domain" as:
>
> A domain, commonly dom0, which shares responsibility with Xen
> about the system as a whole.
>
> It has been historically correct to treat is_hardware_domain(d) as
> equivalent to dom0. However, according to patch series [1], this is
> no longer guaranteed:
>
> "Setting hardware domain as domid 0 is removed."
>
> After these changes, the assumption that hardware domain always equals
> dom0 may not hold. As a result, the above comment in the code could
> become misleading. Consider updating it to something like:
>
> /* Limit NS16550 emulator to the hardware domain only for now */
>
> to reflect the new semantics.
I adjusted the code a bit so that it is possible to test vUART for
either hwdom or domU; will remove the comment in v7.
>
> > + {
> > + struct vuart_info info = {
> > + .name = "COM2",
> > + .compatible = "ns16550",
> > + .base_addr = 0x2f8,
> > + .size = 8,
> > + .irq = 3,
> > + };
>
> Consider defining COM2 base address and IRQ in a shared header
> (e.g., VUART_COM2_BASE and VUART_COM2_IRQ) rather than using
> the magic numbers 0x2f8 and 3 here. This would allow reuse in
> `__start_xen` and other places, and makes the code clearer and
> easier to maintain.
PC platform resources are standard and not going to change, so
I think it is satisfactory to use hardcoded numbers here:
COM1 - 0x3f8 / IRQ#4
COM2 - 0x2f8 / IRQ#3
COM3 - 0x3fe / IRQ#4
COM4 - 0x2fe / IRQ#3
Other PC I/O ports still use hardcoded values, e.g. dom0_setup_permissions().
Anyway, this code will change again, once xl support is plumbed.
So, I would keep that as is for now...
[..]
> > +/*
> > + * 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;
>
> Instead of hardcoding 0xff here, consider using UINT8_MAX. This makes
> it clear that the value is the maximum for uint8_t and avoids magic
> numbers.
>
> Similarly, in other places where constants for 16-bit or 32-bit
> unsigned integers are used, it would be good to replace them with
> UINT16_MAX and UINT32_MAX respectively for consistency and clarity.
Agreed, will do.
Thanks.
[..]
> > +/*
> > + * 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')
>
> Instead of defining the op(dir) macro, it might be cleaner to compute
> the direction character once at the beginning, e.g.:
>
> const char dir_char = (dir == IOREQ_WRITE) ? 'W' : 'R';
>
> and then use dir_char in printk()/debug. This avoids the local macro
> and makes the code easier to follow.
Thanks for suggestion.
Will update.
>
> > + 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);
>
> AFAICT ns16x50_io_read() and ns16x50_io_write() have identical
> signatures. Would it be cleaner to store them in an array of
> function pointers and call through that, e.g.:
>
> rc = ns16x50_io_op[dir == IOREQ_WRITE](vdev, reg, size, data);
>
> This would avoid the if/else block here.
I remember I tried this way, but eventually decided to keep just direct calls
for simplicity.
>
> > + }
> > + 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);
>
> The ns16x50_debug() call is duplicated in both branches of the
> IOREQ_WRITE check, differing only in whether it's placed before or
> after the access. Would it make sense to move this debug print
> outside the condition, so the same code is used for both read and
> write paths (assuming the "data" is not modified during a write)?
Yep, that will work, thanks!
>
> > + }
> > + 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);
>
> The ns16x50_err() call doesn’t require holding vdev->lock, so it would
> be cleaner to move it after spin_unlock(). That way the critical section
> is shorter.
Ack.
>
> > +
> > + 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) )
>
> Currently, ns16x50_alloc() first calls vuart_find_by_io_range() and
> only afterwards checks if the domain is an HVM domain. Wouldn’t it
> be more logical to perform the HVM check first? If the console is
> only available for HVM domains, the extra check for an uninitialized
> vuart field seems unnecessary.
Ack.
>
> > + {
> > + 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 )
>
> If ns16x50_init(vdev) fails, vdev should be freed with xvfree() to
> avoid a memory leak.
Whoops, thanks.
>
> Currently, ns16x50_init() always returns 0. If it is not planned to
> return other values in the future, it may be simpler to make the
> function return void, avoiding the need for the rc variable and
> conditional checks.
There will be non-zero return values in the future.
>
> > + 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, \
>
> dump_state is set to NULL, but vuart_dump_state() in the previous
> commit does not check this pointer. If all commits up to this one
> are applied and domain state is dumped, this could result in a
> NULL pointer dereference and crash the hypervisor.
Thanks; will fix vuart.c
>
> Consider adding a NULL check in vuart_dump_state() or initializing
> dump_state properly.
>
> > + .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
> >
> >
>
> [1] https://patchew.org/Xen/20250416212911.410946-1-jason.andryuk@amd.com/
>
> Best regards,
> Mykola
On Fri, 5 Sep 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 v5:
> - v5 feedback
> - Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmukhin@ford.com/
> ---
> 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
> + 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.
There is a question about adding the kconfig option early in the series.
I think it would be best to add it as last patch
> endmenu
On Fri, Sep 05, 2025 at 07:03:19PM -0700, Stefano Stabellini wrote: > On Fri, 5 Sep 2025, dmukhin@xen.org wrote: > > From: Denis Mukhin <dmukhin@ford.com> > > [..] > > --- 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 > > + 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. > > There is a question about adding the kconfig option early in the series. > I think it would be best to add it as last patch Will do.
On Fri, Sep 05, 2025 at 04:27:02PM -0700, dmukhin@xen.org wrote: > From: Denis Mukhin <dmukhin@ford.com> > [..] > --- 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 > + select VUART_FRAMEWORK > + default n Forgot to drop '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. > +
© 2016 - 2025 Red Hat, Inc.