Re: [PATCH v2 31/35] x86/hvm: introduce NS8250 UART emulator

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.12.2024 05:42, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Add initial in-hypervisor emulator for NS8250/NS16x50-compatible UARTs under
> CONFIG_HAS_VUART_NS8250.
> 
> In parallel domain creation scenario (hyperlaunch), NS8520 emulator helps
> early guest OS bringup debugging, because it eliminates dependency on the
> external emulator being operational by the time domains are created. Also,
> there's no early console facility similar to vpl011 to support x86 guest OS
> bring up.
> 
> By default, CONFIG_HAS_VUART_NS8250 enables emulatio of NS8250 at I/O port
> 0x3f8, IRQ#4 in guest OS.
> 
> Limitations:
> - Only x86;
> - Only Linux guest tested so far;
> - Only legacy COM{1,2,3,4} resources, no customization;
> - Only Xen console as a backend, no inter-domain communication (similar to
>   vpl011 on Arm);
> - Only 8-bit characters;
> - Baud rate is not emulated;
> - FIFO-less mode is not emulated properly;

With in particular this, why would it be called 8250 emulation in the first
place? The driver Xen uses for itself is in ns16550.c; I'd suggest to call
the child here ns16550 emulation right away, using vns16550.c as the file
name.

> --- a/xen/arch/x86/hvm/Kconfig
> +++ b/xen/arch/x86/hvm/Kconfig
> @@ -61,3 +61,17 @@ config HVM_FEP
>         for use in production.
>  
>         If unsure, say N.
> +
> +config HAS_VUART_NS8250
> +     bool "NS8250-compatible UART Emulation"

HAS_* options may not have prompts; they merely declare something to the
rest of the build system.

> +     depends on HVM && HAS_IOPORTS

Why HAS_IOPORTS?

> +     default n

No need for this.

> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -29,3 +29,4 @@ obj-y += vm_event.o
>  obj-y += vmsi.o
>  obj-y += vpic.o
>  obj-y += vpt.o
> +obj-$(CONFIG_HAS_VUART_NS8250) += vuart_ns8250.o

If the vuart name pretix is to remain, then please avoid underscores in
the names of new files, in favor of dashes.

> --- /dev/null
> +++ b/xen/arch/x86/hvm/vuart_ns8250.c
> @@ -0,0 +1,886 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * NS8250-compatible UART Emulator.
> + *
> + * Limitations:
> + * - Only x86;
> + * - Only Linux guest tested so far;
> + * - Only legacy COM{1,2,3,4} resources, no customization;
> + * - Only Xen console as a backend, no inter-domain communication (similar to
> + *   vpl011 on Arm);
> + * - Only 8-bit characters;
> + * - Baud rate is not emulated;
> + * - FIFO-less mode is not emulated properly;
> + * - RX FIFO interrupt moderation (FCR) is not emulated properly, TL16C750
> + *   has special FCR handling;
> + * - No integration w/ VM snapshotting (HVM_REGISTER_SAVE_RESTORE() and
> + *   friends);
> + * - Assumes no ISA-device IRQ sharing;
> + * - MMIO-based UART is not supported;
> + * - PCI UART is not supported.
> + */
> +
> +#define pr_fmt(fmt)         "ns8250: " fmt
> +#define pr_log_level        hvm_ns8250_log_level
> +
> +/* Development debugging */
> +#define NS8250_LOG_LEVEL    0
> +
> +#include <xen/types.h>
> +#include <xen/event.h>
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/trace.h>
> +#include <xen/resource.h>
> +#include <xen/ctype.h>
> +#include <xen/param.h>
> +#include <xen/console.h> /* console_input_domid() */
> +#include <asm/setup.h>   /* max_init_domid */
> +#include <asm/iocap.h>
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/io.h>
> +#include <asm/hvm/vuart_ns8250.h>
> +
> +#if !defined(pr_fmt)
> +#define pr_fmt(fmt) fmt
> +#endif
> +
> +#if !defined(pr_log_level)
> +#define pr_log_level 0
> +#endif
> +
> +#define pr_err(fmt, args...) \
> +    gprintk(KERN_ERR, pr_fmt(fmt), ## args)
> +#define pr_warn(fmt, args...) \
> +    if (pr_log_level) gprintk(KERN_WARNING, pr_fmt(fmt), ## args)
> +#define pr_info(fmt, args...) \
> +    if (pr_log_level) gprintk(KERN_INFO, pr_fmt(fmt), ## args)
> +#define pr_debug(fmt, args...) \
> +    if (pr_log_level) gprintk(KERN_DEBUG, pr_fmt(fmt), ## args)

On top of Roger's remark here: _If_ you use such wrapper macros, then
please arrange for them to be usable in arbitrary context. Think of

    if ( condition )
        pr_info(...);
    else
        ...

> +/*
> + * NS8250 emulator state machine logging (development only)
> + * FIXME: use similar to parse_guest_loglvl()
> + */
> +static unsigned int __read_mostly hvm_ns8250_log_level = NS8250_LOG_LEVEL;
> +integer_param("hvm.ns8250.log_level", hvm_ns8250_log_level);

How can this be a command line option, when there may be many domains
making use of the in-Xen driver? This and ...

> +/*
> + * Default emulated NS8250 resources.
> + * If not specified, COM1 (I/O port 0x3f8, IRQ#4) is emulated.
> + * FIXME: follow Linux'es console= syntax or re-use
> + * ns16550_parse_port_config().
> + */
> +static char __read_mostly hvm_ns8250_console[64];
> +string_param("hvm.ns8250.console", hvm_ns8250_console);

... this need to be per-domain settings; a command line option may be
applicable to Dom0 alone.

> +/* I/O access mask */
> +static const uint32_t io_access_mask[] = {
> +    [0] = 0X00000000U,
> +    [1] = 0X000000FFU,
> +    [2] = 0X0000FFFFU,
> +    [4] = 0XFFFFFFFFU,
> +};

I don't think this is needed; we're doing similar port I/O emulation in
various other places, without resorting to such arrays.

> +/*
> + * Legacy IBM PC NS8250 resources.
> + * There are only 4 I/O port ranges, hardcoding all of them here.
> + */
> +static const struct {
> +    const char *name;
> +    const struct resource *res;
> +} ns8250_x86_legacy_uarts[4] = {
> +    [0] = {
> +        .name = "com1",
> +        .res = (const struct resource[]){
> +            { .type = IORESOURCE_IO,  .addr = 0x3F8, .size = UART_MAX },

Considering this, ...

> +static int ns8250_io_write8(
> +    struct vuart_ns8250 *vdev, uint32_t reg, uint8_t *data)
> +{
> +    uint8_t val;
> +    int rc = 0;
> +
> +    val = *data;
> +
> +    switch ( reg )
> +    {
> +    /* DLAB=0 */
> +    case UART_THR:
> +        if ( vdev->regs[UART_MCR] & UART_MCR_LOOP )
> +        {
> +            ns8250_fifo_rx_putchar(vdev, val);
> +            vdev->regs[UART_LSR] |= UART_LSR_OE;
> +        }
> +        else
> +            ns8250_fifo_tx_putchar(vdev, val);
> +
> +        vdev->flags |= NS8250_IRQ_THRE_PENDING;
> +
> +        break;
> +
> +    case UART_IER:
> +        if ( val & vdev->regs[UART_IER] & UART_IER_ETHREI )
> +            vdev->flags |= NS8250_IRQ_THRE_PENDING;
> +
> +        vdev->regs[UART_IER] = val & UART_IER_MASK;
> +
> +        break;
> +
> +    case UART_FCR: /* WO */
> +        if ( val & UART_FCR_CLRX )
> +            ns8250_fifo_rx_reset(vdev);
> +
> +        if ( val & UART_FCR_CLTX )
> +            ns8250_fifo_tx_reset(vdev);
> +
> +        if ( !(val & UART_FCR_ENABLE) )
> +            val = 0;
> +
> +        vdev->regs[UART_FCR] = val & (UART_FCR_ENABLE |
> +                                      UART_FCR_DMA |
> +                                      UART_FCR_TRG_MASK);
> +
> +        break;
> +
> +    case UART_LCR:
> +        vdev->regs[UART_LCR] = val;
> +        break;
> +
> +    case UART_MCR: {
> +        uint8_t msr_curr, msr_next, msr_delta;
> +
> +        msr_curr = vdev->regs[UART_MSR];
> +        msr_next = 0;
> +        msr_delta = 0;
> +
> +        /* Set modem status */
> +        if ( val & UART_MCR_LOOP )
> +        {
> +            if ( val & UART_MCR_DTR )
> +                msr_next |= UART_MSR_DSR;
> +            if ( val & UART_MCR_RTS )
> +                msr_next |= UART_MSR_CTS;
> +            if ( val & UART_MCR_OUT1 )
> +                msr_next |= UART_MSR_RI;
> +            if ( val & UART_MCR_OUT2 )
> +                msr_next |= UART_MSR_DCD;
> +        }
> +        else
> +            msr_next |= UART_MSR_DCD | UART_MSR_DSR | UART_MSR_CTS;
> +
> +        /* Calculate changes in modem status */
> +        if ( (msr_curr & UART_MSR_CTS) ^ (msr_next & UART_MSR_CTS) )
> +            msr_delta |= UART_MSR_DCTS;
> +        if ( (msr_curr & UART_MCR_RTS) ^ (msr_next & UART_MCR_RTS) )
> +            msr_delta |= UART_MSR_DDSR;
> +        if ( (msr_curr & UART_MSR_RI)  & (msr_next & UART_MSR_RI) )
> +            msr_delta |= UART_MSR_TERI;
> +        if ( (msr_curr & UART_MSR_DCD) ^ (msr_next & UART_MSR_DCD) )
> +            msr_delta |= UART_MSR_DDCD;
> +
> +        vdev->regs[UART_MCR] = val & UART_MCR_MASK;
> +        vdev->regs[UART_MSR] = msr_next | msr_delta;
> +
> +        break;
> +    }
> +
> +    case UART_LSR: /* RO */
> +    case UART_MSR: /* RO */
> +        rc = -EINVAL;
> +        break;
> +
> +    /*
> +     * NB: Firmware like OVMF rely on SCR presence to initialize the ns8250
> +     * driver.
> +     */
> +    case UART_SCR:
> +        vdev->regs[UART_SCR] = val;
> +        break;
> +
> +    /* DLAB=1 */
> +    case UART_MAX + UART_DLL:

... how can you go at or past UART_MAX here and ...

> +        vdev->dl = (vdev->dl & 0xFF00U) | val;
> +        break;
> +
> +    case UART_MAX + UART_DLM:

... here? I notice a caller up the tree sets things up like this, but this
feels pretty fragile. How would one easily spot all producers and consumers
without also hitting all other uses of UART_MAX?

> +static int cf_check ns8250_io_handle(
> +    int dir, unsigned int addr, unsigned int size, uint32_t *data)
> +{
> +#define op(dir)     (((dir) == IOREQ_WRITE) ? 'W' : 'R')
> +    struct domain *d = rcu_lock_current_domain();
> +    struct vuart_ns8250 *vdev = &d->arch.hvm.vuart;
> +    uint32_t offset, reg;
> +    int rc;
> +
> +    spin_lock(&vdev->lock);
> +
> +    BUG_ON( vdev->owner != d );
> +
> +    if ( !(vdev->flags & NS8250_READY) )
> +    {
> +        pr_err("%c io 0x%04x %d 0x%08"PRIx32": propagate to external I/O 
> emulator\n",
> +                op(dir), addr, size, *data);
> +        rc = X86EMUL_UNHANDLEABLE;
> +        goto out;
> +    }
> +
> +    reg = addr - vdev->io_addr;
> +    BUG_ON( reg >= UART_MAX );
> +    if ( reg % size != 0 )
> +    {
> +        pr_err("%c 0x%04x %d 0x%08"PRIx32": unaligned access\n",
> +                op(dir), addr, size, *data & io_access_mask[size]);
> +        rc = X86EMUL_OKAY;
> +        goto out;
> +    }
> +
> +    /* Redirect access to divisor latch registers */
> +    if ( !!(vdev->regs[UART_LCR] & UART_LCR_DLAB)
> +            && (reg == UART_DLL || reg == UART_DLM) )
> +        offset = UART_MAX + reg;
> +    else
> +        offset = reg;
> +
> +    if ( dir == IOREQ_WRITE )
> +    {
> +        pr_debug("%c 0x%04x %d 0x%08"PRIx32" %s[0x%02"PRIx32"]\n",
> +                op(dir), addr, size,
> +                *data & io_access_mask[size],
> +                ns8250_regname(vdev, offset, dir), reg);
> +        rc = ns8250_io_write(vdev, offset, size, data);
> +    }
> +    else
> +    {
> +        rc = ns8250_io_read(vdev, offset, size, data);
> +        pr_debug("%c 0x%04x %d 0x%08"PRIx32" %s[0x%02"PRIx32"]\n",
> +                op(dir), addr, size,
> +                *data & io_access_mask[size],
> +                ns8250_regname(vdev, offset, dir), reg);
> +    }
> +    if ( rc )
> +        pr_err("%c 0x%04x %d 0x%08"PRIx32": unsupported access\n",
> +                op(dir), addr, size, *data & io_access_mask[size]);
> +    rc = X86EMUL_OKAY;
> +
> +out:

Nit: Missing indentation (see ./CODING_STYLE). Also elsewhere.

> --- /dev/null
> +++ b/xen/arch/x86/include/asm/hvm/vuart_ns8250.h
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * NS8250-compatible UART Emulator.
> + */
> +#if !defined(HVM__VUART_NS8250_H)
> +#define HVM__VUART_NS8250_H
> +
> +#include <xen/spinlock.h>
> +#include <xen/8250-uart.h>
> +#include <public/io/console.h> /* xencons_interface */

I assume you mean ...

> +/*
> + * NS8250 emulator operational flags.
> + */
> +enum {
> +    /* Emulator is ready */
> +    NS8250_READY                = BIT(0, U),
> +    /* Trigger re-delivery of THRE interrupt */
> +    NS8250_IRQ_THRE_PENDING     = BIT(1, U),
> +};
> +
> +/*
> + * Virtual NS8250 device state.
> + */
> +struct vuart_ns8250 {
> +    uint16_t dl;                    /* Divisor Latch */
> +    uint8_t regs[UART_MAX];         /* Registers */
> +    uint32_t flags;                 /* Virtual device flags */
> +    uint64_t io_addr;               /* Guest I/O region base address */
> +    uint64_t io_size;               /* Guest I/O region size */
> +    int irq;                        /* Guest IRQ# */
> +    struct xencons_interface *cons; /* Emulated RX/TX FIFOs */

... this use, but that doesn't even require a forward decl of the struct
in C (in C++ such a forward decl would be all that's needed).

Jan