On 31.07.2025 21:22, dm...@proton.me wrote:
> From: Denis Mukhin <dmuk...@ford.com>
>
> Add initial in-hypervisor emulator for NS8250/NS16x50-compatible UARTs under
> CONFIG_VUART_NS16550 for x86 port of Xen.
>
> x86 port of Xen lacks vUART facility similar to Arm's SBSA emulator to support
> x86 guest OS bring up in the embedded setups.
>
> In parallel domain creation scenario (hyperlaunch), NS16550 emulator helps
> early guest firmware and/or OS bringup debugging, because it eliminates
> dependency on the external emulator (qemu) being operational by the time
> domains are created.
>
> The emulator also allows to forward the physical console input to the x86
> domain which is useful when a system has only one physical UART for early
> debugging and this UART is owned by Xen. Such functionality is limited to dom0
> use currently.
>
> By default, CONFIG_VUART_NS16550 enables emulation of NS16550 at I/O port
> 0x3f8, IRQ#4 in guest OS (legacy COM1).
>
> Legacy COM resources can be selected at built-time and cannot be configured
> per-domain via .cfg or DT yet.
>
> Introduce new emulation flag for virtual UART on x86 and plumb it through
> domain creation code so NS16550 emulator can be instantiated properly.
>
> Please refer to the NS16550 emulator code for full list of limitations.
>
> Signed-off-by: Denis Mukhin <dmuk...@ford.com>
> ---
> Changes since v3:
> - feedback addressed
> - adjusted to new vUART framework APIs
> - Link to v3:
> https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-21-c5d36b31d...@ford.com/
> ---
> xen/arch/x86/hvm/hvm.c | 9 +
> xen/arch/x86/include/asm/domain.h | 4 +-
> xen/arch/x86/include/asm/hvm/domain.h | 4 +
> xen/common/emul/vuart/Kconfig | 48 ++
> xen/common/emul/vuart/Makefile | 1 +
> xen/common/emul/vuart/vuart-ns16550.c | 1009 +++++++++++++++++++++++++
> xen/common/emul/vuart/vuart.c | 4 +
> xen/include/public/arch-x86/xen.h | 4 +-
> xen/include/xen/resource.h | 3 +
> 9 files changed, 1084 insertions(+), 2 deletions(-)
> create mode 100644 xen/common/emul/vuart/vuart-ns16550.c
Overall I think this patch is too large to sensibly review. Surely base
structure
and then (incrementally) fleshing out of the hooks can be separated from one
another?
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -31,6 +31,7 @@
> #include <xen/nospec.h>
> #include <xen/vm_event.h>
> #include <xen/console.h>
> +#include <xen/vuart.h>
> #include <asm/shadow.h>
> #include <asm/hap.h>
> #include <asm/current.h>
> @@ -702,6 +703,10 @@ int hvm_domain_initialise(struct domain *d,
> if ( rc != 0 )
> goto fail1;
>
> + rc = vuart_init(d, NULL);
> + if ( rc != 0 )
> + goto out_vioapic_deinit;
> +
> stdvga_init(d);
>
> rtc_init(d);
> @@ -725,6 +730,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) )
Would be better if vuart_deinit() was idempotent, and hence could be called
unconditionally here.
> @@ -787,6 +794,8 @@ void hvm_domain_destroy(struct domain *d)
> if ( hvm_funcs.domain_destroy )
> alternative_vcall(hvm_funcs.domain_destroy, d);
>
> + vuart_deinit(d);
You require a fair level of idempotency already anyway, as a domain may not
have any vUART, so this call already needs to be "capabale" of doing nothing.
> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -149,6 +149,10 @@ struct hvm_domain {
> #ifdef CONFIG_MEM_SHARING
> struct mem_sharing_domain mem_sharing;
> #endif
> +
> +#ifdef CONFIG_VUART_NS16550
> + void *vuart; /* Virtual UART handle. */
> +#endif
> };
With your framework you allow for multiple vUART drivers. Either the field
looks misnamed or the CONFIG_* option checked is the wrong one.
Also, why's this x86-specific? NS16550s can exist anywhere, can't they?
(The present, but presumably temporary tying to x86 looks to be the use of
I/O ports.)
> --- a/xen/common/emul/vuart/Kconfig
> +++ b/xen/common/emul/vuart/Kconfig
> @@ -3,4 +3,52 @@ config HAS_VUART
>
> menu "UART Emulation"
>
> +config VUART_NS16550
> + bool "NS16550-compatible UART Emulation" if EXPERT
> + depends on X86 && HVM
> + select HAS_VUART
> + help
> + In-hypervisor NS16550/NS16x50 UART emulation.
> +
> + Only legacy PC I/O ports are emulated.
> +
> + This is strictly for testing purposes (such as early HVM guest
> console),
> + and not appropriate for use in production.
> +
> +choice VUART_NS16550_PC
> + prompt "IBM PC COM resources"
> + depends on VUART_NS16550
> + default VUART_NS16550_PC_COM1
> + help
> + Default emulated NS16550 resources.
> +
> +config VUART_NS16550_PC_COM1
> + bool "COM1 (I/O port 0x3f8, IRQ#4)"
> +
> +config VUART_NS16550_PC_COM2
> + bool "COM2 (I/O port 0x2f8, IRQ#3)"
> +
> +config VUART_NS16550_PC_COM3
> + bool "COM3 (I/O port 0x3e8, IRQ#4)"
> +
> +config VUART_NS16550_PC_COM4
> + bool "COM4 (I/O port 0x2e8, IRQ#3)"
> +
> +endchoice
> +
> +config VUART_NS16550_LOG_LEVEL
> + int "UART emulator verbosity level"
> + range 0 3
> + default "1"
> + depends on VUART_NS16550
> + help
> + Set the default log level of UART emulator.
> + See include/xen/config.h for more details.
For someone merely running kconfig but not otherwise knowing the sources,
this isn't an overly helful pointer. But I question the need for such a
control anyway, and I think I did say so already before.
> +config VUART_NS16550_DEBUG
> + bool "UART emulator development debugging"
> + depends on VUART_NS16550
&& DEBUG ?
> --- a/xen/common/emul/vuart/Makefile
> +++ b/xen/common/emul/vuart/Makefile
> @@ -1 +1,2 @@
> obj-$(CONFIG_HAS_VUART) += vuart.o
> +obj-$(CONFIG_VUART_NS16550) += vuart-ns16550.o
I don't think files in this directory need a vuart- name prefix.
> --- /dev/null
> +++ b/xen/common/emul/vuart/vuart-ns16550.c
> @@ -0,0 +1,1009 @@
> +/* 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 HVM domains support (build-time), PVH domains are not supported
> yet;
> + * - Only legacy COM{1,2,3,4} resources via Kconfig, custom I/O ports/IRQs
> + * are not supported;
> + * - 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 toolstack integration;
> + * - 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 "ns16550"
> +#define pr_fmt(fmt) pr_prefix ": " fmt
> +#define pr_log_level CONFIG_VUART_NS16550_LOG_LEVEL
> +
> +#include <xen/8250-uart.h>
> +#include <xen/console.h>
> +#include <xen/iocap.h>
> +#include <xen/ioreq.h>
> +#include <xen/resource.h>
> +#include <xen/vuart.h>
> +#include <xen/xvmalloc.h>
> +
> +#include <public/io/console.h>
Except for cases where Xen itself runs as a guest, I don't think any of these
headers should be used in Xen sources. If I'm not mistaken, ...
> +/*
> + * Virtual NS16550 device state.
> + */
> +struct vuart_ns16550 {
> + struct xencons_interface cons; /* Emulated RX/TX FIFOs */
... this also isn't to communicate with some remote, but merely to use some
of the fields conveniently.
> + uint8_t regs[NS16550_EMU_REGS_NUM]; /* Emulated registers */
> + unsigned int irq; /* Emulated IRQ# */
> + uint64_t io_addr; /* Emulated I/O region base address
> */
> + uint64_t io_size; /* Emulated I/O region size */
These are huge; for the size that's true even if considering future MMIO-
based emulation.
> + const char *name; /* Device name */
> + struct domain *owner; /* Owner domain */
> + spinlock_t lock; /* Protection */
> +};
> +
> +/*
> + * Virtual device description.
> + */
> +struct virtdev_desc {
> + const char *name;
> + const struct resource *res;
> +};
> +
> +/*
> + * Legacy IBM PC NS16550 resources.
> + * There are only 4 I/O port ranges, hardcoding all of them here.
> + */
> +static const struct virtdev_desc x86_pc_uarts[4] = {
> + [0] = {
> + .name = "COM1",
> + .res = (const struct resource[]){
> + { .type = IORESOURCE_IO, .addr = 0x3f8, .size =
> NS16550_REGS_NUM },
> + { .type = IORESOURCE_IRQ, .addr = 4, .size = 1 },
> + { .type = IORESOURCE_UNKNOWN },
> + },
> + },
> + [1] = {
> + .name = "COM2",
> + .res = (const struct resource[]){
> + { .type = IORESOURCE_IO, .addr = 0x2f8, .size =
> NS16550_REGS_NUM },
> + { .type = IORESOURCE_IRQ, .addr = 3, .size = 1 },
> + { .type = IORESOURCE_UNKNOWN },
> + },
> + },
> + [2] = {
> + .name = "COM3",
> + .res = (const struct resource[]){
> + { .type = IORESOURCE_IO, .addr = 0x3e8, .size =
> NS16550_REGS_NUM },
> + { .type = IORESOURCE_IRQ, .addr = 4, .size = 1 },
> + { .type = IORESOURCE_UNKNOWN },
> + },
> + },
> + [3] = {
> + .name = "COM4",
> + .res = (const struct resource[]){
> + { .type = IORESOURCE_IO, .addr = 0x2e8, .size =
> NS16550_REGS_NUM },
> + { .type = IORESOURCE_IRQ, .addr = 3, .size = 1 },
> + { .type = IORESOURCE_UNKNOWN },
> + },
> + },
> +};
The choice of COMn is at build time. Why do we need all four configurations
resident not only in the binary, but even at (post-init) runtime? Also, the
way you do initialization of .res, I think adding __initconst to the main
array wouldn't have the effect of pulling all those inti .init.* as well.
For the time being I simply don't see the need for the extra level of
indirection: All instances have two entries (plus the then likely not
necessary sentinel).
> +static bool cf_check ns16550_iir_check_lsi(const struct vuart_ns16550 *vdev)
> +{
> + return !!(vdev->regs[UART_LSR] & UART_LSR_MASK);
No need for !! (also elsewhere).
> --- a/xen/include/xen/resource.h
> +++ b/xen/include/xen/resource.h
> @@ -31,4 +31,7 @@ struct resource {
>
> #define resource_size(res) ((res)->size)
>
> +#define for_each_resource(res) \
> + for ( ; (res) && (res)->type != IORESOURCE_UNKNOWN; (res)++ )
I'm not sure this is a good generic #define; imo it wants keeping local to
the one file that uses it.
Jan
