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) )
+ {
+ 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
+ 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",
+ 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 = 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);
+}
+
+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;
+
+ 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);
+}
+
+#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
} console;
} __aligned(PAGE_SIZE);
--
2.51.0On 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?
> + {
> + 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
> + 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
> + 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.
> + 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
> +}
> +
> +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
> + 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 )
> +}
> +
> +#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;
> } console;
> } __aligned(PAGE_SIZE);
>
> --
> 2.51.0
>
On 29.08.2025 21:57, Stefano Stabellini wrote:
> On Thu, 28 Aug 2025, dmukhin@xen.org wrote:
>> +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 )
Why would this be? Like free(), both xfree() and xvfree() are fine to be
called with a NULL pointer. What's odd here is that the uppercase form (the
wrapper macro) is used - clearing the local variable is pointless when it
is about to go out of scope anyway.
Jan
On Tue, Sep 02, 2025 at 11:36:19AM +0200, Jan Beulich wrote:
> On 29.08.2025 21:57, Stefano Stabellini wrote:
> > On Thu, 28 Aug 2025, dmukhin@xen.org wrote:
> >> +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 )
>
> Why would this be? Like free(), both xfree() and xvfree() are fine to be
> called with a NULL pointer. What's odd here is that the uppercase form (the
> wrapper macro) is used - clearing the local variable is pointless when it
> is about to go out of scope anyway.
Thank you for remark!
I switched the code to xvfree() in v6
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
> >
On 02.09.2025 01:11, dmukhin@xen.org wrote: > On Fri, Aug 29, 2025 at 12:57:43PM -0700, Stefano Stabellini wrote: >> On Thu, 28 Aug 2025, dmukhin@xen.org wrote: >>> --- 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 No "default n" should ever be put anywhere; it's simply redundant. Jan
On Tue, Sep 02, 2025 at 11:32:31AM +0200, Jan Beulich wrote: > On 02.09.2025 01:11, dmukhin@xen.org wrote: > > On Fri, Aug 29, 2025 at 12:57:43PM -0700, Stefano Stabellini wrote: > >> On Thu, 28 Aug 2025, dmukhin@xen.org wrote: > >>> --- 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 > > No "default n" should ever be put anywhere; it's simply redundant. Kept code as is in v6.
On Fri, Sep 05, 2025 at 04:34:34PM -0700, dmukhin@xen.org wrote: > On Tue, Sep 02, 2025 at 11:32:31AM +0200, Jan Beulich wrote: > > On 02.09.2025 01:11, dmukhin@xen.org wrote: > > > On Fri, Aug 29, 2025 at 12:57:43PM -0700, Stefano Stabellini wrote: > > >> On Thu, 28 Aug 2025, dmukhin@xen.org wrote: > > >>> --- 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 > > > > No "default n" should ever be put anywhere; it's simply redundant. > > Kept code as is in v6. Sorry, v6 has "default n" update which I need to remove.
© 2016 - 2025 Red Hat, Inc.