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

[dmkhn@xxxxxxxxx]

On Mon, Aug 04, 2025 at 12:53:36PM +0200, Jan Beulich wrote:
> 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?

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
>