Hi Denis,
On Sat, Sep 6, 2025 at 2:27 AM <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 v5:
> - v5 feedback
> - Link to v5:
> https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmuk...@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.andr...@amd.com/
Best regards,
Mykola
