On Mon, Aug 04, 2025 at 12:53:36PM +0200, Jan Beulich wrote:
> On 31.07.2025 21:22, [email protected] wrote:
> > From: Denis Mukhin <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > Changes since v3:
> > - feedback addressed
> > - adjusted to new vUART framework APIs
> > - Link to v3:
> > https://lore.kernel.org/xen-devel/[email protected]/
> > ---
> > 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?
I'll do a split.
>
> > --- 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.
Agree, vuart_deinit() is idempotent even in this submisson.
Will update.
>
> > @@ -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.
Agree; will update.
>
> 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.)
struct hvm_domain is arch-specific.
I do not think I need to add NS16550 to, say RISC-V's, hvm_domain without
implementing MMIO part and guest DT-binding generation.
>
> > --- 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.
I'll drop that Kconfig setting.
>
> > +config VUART_NS16550_DEBUG
> > + bool "UART emulator development debugging"
> > + depends on VUART_NS16550
>
> && DEBUG ?
I will drop that Kconfig.
>
> > --- 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.
Ack.
Hmm, there's already ns16550.c which is UART driver, so it may be confusing to
have two ns16550s (although in different directories).
I do not have a strong preference on the naming here.
>
> > --- /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, ...
I'll double check, thanks.
>
> > +/*
> > + * 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.
The plan is to add peer-to-peer connection over vUART similarly to existing
vpl011.
>
> > + 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.
Ack.
>
> > + 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).
Will rework that.
>
> > +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).
Ack.
>
> > --- 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.
Ack.
>
> Jan
>
