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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Monday, December 16th, 2024 at 7:04 AM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

>
>
> 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.

NS8250 is the predecessor of NS16550, registers are defined in 8250-uart.h,
hence I used vuart_ns8250.c.

I do not have a strong opinion on this naming. 16550 makes sense because
all UARTs have a FIFO now.

Renamed to vuart-ns16550.c.

>
> > --- 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.

There are examples in the code using HAS_xxx configuration exactly the way
I used it. Specifically, all physical UART drivers are declared under HAS_xxx
(drivers/char/Kconfig).

I reworked that part.

>
> > + depends on HVM && HAS_IOPORTS
>
>
> Why HAS_IOPORTS?

It is meant to highlight the fact that MMIO-based UART is not supported.
It is not currently possible to enable the emulator for, say, Arm platforms.

>
> > + default n
>
>
> No need for this.

Fixed.

>
> > --- 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.

I've seen such request few times on the mailing list and it seems the
recommendation is not followed all the time in the code base.

It's just all files in xen/arch/x86/hvm have underscores instead of
dashes.

IMO, this rule needs an explicit documentation in the coding style
guide. I'm happy to follow either dash- or underscore-way.

Addressed.

>
> > --- /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
> ...

Done.

>
> > +/*
> > + * 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 ...

TBH, I did not plan for configuring vUARTs per-domain. In its current shape,
the emulator is strictly for debugging purposes and not ready to be enabled
in production setup. Which means global configuration is fine, IMO. But then
I decided that it's trivial to allow selecton of any of x86 legacy UARTs.

Such global configuration was hooked into the command line parsing because
it allows to test xen + dom0 w/o rebuild.

Reworked to Kconfig setting.

>
> > +/*
> > + * 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.

Dropped.

>
> > +/*
> > + * 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?

The idea was to flatten the register address space regardless DLAB.
I also agree with your point: having all register handling stated explicitly
makes the code more understandable.

Reworked.

>
> > +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.

Fixed.

>
> > --- /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).

I declared vUART handler as `void *` in hvm_domain and moved all the data
types to .c, so there's no need in header file at all.

>
> Jan