Re: [PATCH v4 5/8] emul/vuart-ns16550: introduce NS16550-compatible UART emulator (x86)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2025 21:22, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> 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 <dmukhin@xxxxxxxx>
> ---
> 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-c5d36b31d66c@xxxxxxxx/
> ---
>  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