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

[dmkhn@xxxxxxxxx]

On Wed, Aug 06, 2025 at 05:06:24PM +0200, Roger Pau Monné wrote:
[..]
> > @@ -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.
> 
> Listing the limitations here might not be the beast approach, this is
> likely to get out of sync as it's too far away from the code
> implementation.
> 
> If anything those comments want to be closer together to where the
> feature would otherwise be implemented.

I wanted to have some rough overview of the emulation capabilities in one
place so it is easy to get a grasp of how emulator can be used.

But I agree that can get out of hand if not maintained; I plan to maintain
that code.

> 
> > + */
> > +
> > +#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>
> > +
> > +#define pr_err(fmt, args...) do { \
> > +    gprintk(KERN_ERR, pr_fmt(fmt), ## args); \
> > +} while (0)
> > +
> > +#define pr_warn(fmt, args...) do { \
> > +    if ( pr_log_level >= 1) \
> > +        gprintk(KERN_WARNING, pr_fmt(fmt), ## args); \
> > +} while (0)
> > +
> > +#define pr_info(fmt, args...) do { \
> > +    if ( pr_log_level >= 2 ) \
> > +        gprintk(KERN_INFO, pr_fmt(fmt), ## args); \
> > +} while (0)
> > +
> > +#define pr_debug(fmt, args...) do { \
> > +    if ( pr_log_level >= 3 ) \
> > +        gprintk(KERN_DEBUG, pr_fmt(fmt), ## args); \
> > +} while (0)
> 
> We would use the pr_* set of logging functions for code imported from
> Linux, but for Xen code we would directly use the gprintk() functions
> rather than wrap them as you do.

Oh, I see, pr_ is a "reserved namespace".

I will rename these to ns16550_ since those are extremely helpful for
debugging.

> 
> > +
> > +#if defined(CONFIG_VUART_NS16550_PC_COM1)
> > +#define X86_PC_UART_IDX         0
> > +#elif defined(CONFIG_VUART_NS16550_PC_COM2)
> > +#define X86_PC_UART_IDX         1
> > +#elif defined(CONFIG_VUART_NS16550_PC_COM3)
> > +#define X86_PC_UART_IDX         2
> > +#elif defined(CONFIG_VUART_NS16550_PC_COM4)
> > +#define X86_PC_UART_IDX         3
> > +#else
> > +#error "Unsupported I/O port"
> > +#endif
> > +
> > +#ifdef CONFIG_VUART_NS16550_DEBUG
> > +#define guest_prefix            "FROM GUEST "
> > +#else
> > +#define guest_prefix            ""
> > +#endif
> > +
> > +/*
> > + * Number of supported registers in the UART.
> > + */
> > +#define NS16550_REGS_NUM        ( UART_SCR + 1 )
> 
> Extra spaces around parentheses? (here and below)

Ack.

> 
> > +
> > +/*
> > + * Number of emulated registers.
> > + *
> > + * - Emulated registers [0..NS16550_REGS_NUM] are R/W registers for DLAB=0.
> > + * - DLAB=1, R/W, DLL            = NS16550_REGS_NUM + 0
> > + * - DLAB=1, R/W, DLM            = NS16550_REGS_NUM + 1
> > + * -         R/O, IIR (IIR_THR)  = NS16550_REGS_NUM + 2
> > + */
> > +#define NS16550_EMU_REGS_NUM    ( NS16550_REGS_NUM + 3 )
> > +
> > +/*
> > + * Virtual NS16550 device state.
> > + */
> > +struct vuart_ns16550 {
> > +    struct xencons_interface cons;      /* Emulated RX/TX FIFOs */
> > +    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 */
> > +    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] = {
> 
> You don't need the explicit array indexes?

Muscle memory; I will nuke that huge static array in v5.

> 
> > +        .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 },
> > +        },
> > +    },
> > +};
[..]
> > diff --git a/xen/include/public/arch-x86/xen.h 
> > b/xen/include/public/arch-x86/xen.h
> > index fc2487986642..f905e1252c70 100644
> > --- a/xen/include/public/arch-x86/xen.h
> > +++ b/xen/include/public/arch-x86/xen.h
> > @@ -283,13 +283,15 @@ struct xen_arch_domainconfig {
> >  #define XEN_X86_EMU_USE_PIRQ        (1U<<_XEN_X86_EMU_USE_PIRQ)
> >  #define _XEN_X86_EMU_VPCI           10
> >  #define XEN_X86_EMU_VPCI            (1U<<_XEN_X86_EMU_VPCI)
> > +#define _XEN_X86_EMU_NS16550        11
> > +#define XEN_X86_EMU_NS16550         (1U<<_XEN_X86_EMU_NS16550)
> >
> >  #define XEN_X86_EMU_ALL             (XEN_X86_EMU_LAPIC | XEN_X86_EMU_HPET 
> > |  \
> >                                       XEN_X86_EMU_PM | XEN_X86_EMU_RTC |    
> >   \
> >                                       XEN_X86_EMU_IOAPIC | XEN_X86_EMU_PIC 
> > |  \
> >                                       XEN_X86_EMU_VGA | XEN_X86_EMU_IOMMU | 
> >   \
> >                                       XEN_X86_EMU_PIT | 
> > XEN_X86_EMU_USE_PIRQ |\
> > -                                     XEN_X86_EMU_VPCI)
> > +                                     XEN_X86_EMU_VPCI | 
> > XEN_X86_EMU_NS16550)
> 
> libxl also consumes XEN_X86_EMU_ALL, and with the proposed change here
> it will create all HVM domains with XEN_X86_EMU_NS16550, which I don't
> think it's indented?
> 
> Overall I agree for Jan it would be better if this patch could be
> split into somehow smaller units.  Is this something feasible?  We
> don't want a patch for each register handle, but maybe you cna somehow
> grup those into functional sections, so that patches can be < 250
> lines?
> 
> Maybe you have already considered this approach and it wasn't
> feasible?

There __were__ pretty comprehensive reviews of this very emulator code
in the past without raising such concern (and this is v4).

Just in case, the most comprehensive review was this one (thanks Roger!):
  https://lore.kernel.org/xen-devel/Z1wd4iAmVzv1ISPZ@macbook.local/