Re: [PATCH 00/36] Introduce NS8250 UART emulator

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/11/2024 11:21 pm, Denis Mukhin via B4 Relay wrote:
> The patch series introduces 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.
>
> The NS8250 emulator is disabled by default.
>
> Series
> ======
> - patches 1-2: some miscellaneous random fixes, added into the series
>   because I stepped into those while debugging NS8250 emulator.
>
> - patches 3-14: preparation fixes for xen console and NS8250 emulator.
>
> - patches 15-29: xen console driver cleanup and preparation for enabling
>   physical serial console focus assignment to the guest VM w/ virtual NS8250.
>
> - patches 30-36: initial NS8250 emulator. That adds the I/O port emulator
>   for legacy PC COM UARTs, Kconfig option, enabling emulator and libxl
>   plumbing.
>
> 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.
>
> Testing
> =======
>
> I tested boot of HVM linux guest w/ OVMF as the virtual firmware.
>
> The emulator, if enabled via CONFIG_HAS_VUART_NS8250=y, will use COM1 (0x3f8)
> resources by default.
>
> To test w/ virtual COM1, the guest kernel parameters should contain
>   earlycon=uart,io,0x3f8,115200n8 console=uart,io,0x3f8,115200n8
>
> Xen is able to forward physical console input to the domain w/ virtual NS8250.
> To switch the console focus press Ctrl+aaa. If console= is given to the HVM
> kernel, then the user shall be able to see the login prompt on xen console 
> once
> console focus is switched to the HVM guest.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

Hello,

Thankyou for this.  It's an excellent start for first posting to the list.

Two things stand out at a glance.

First, xmalloc/free are the expected functions to use, and kmalloc/free
are not equivalent.  We have some drivers ported from Linux, hence the
compatibility, and if it needs to extend beyond smmu.c then there's
linux-compat.h where definitions can live, but we really don't want them
visible generally.

Second and more importantly, I'm afraid this won't pass CI right now. 
Your function pointees (e.g. ns8250_iir_check_lsi()/etc) need a cf_check
attribute on them, in order to function when running on CET-IBT capable
hardware.

>From the root of the Xen tree, if you run:

CONTAINER=bookworm-x86_64-gcc-ibt ./automation/scripts/containerize

then you'll get a build environment with suitable diagnostics. 
Unfortunately we're still relying on an out-of-tree GCC patch to have a
compiler that can point out the problems at build time, as opposed to
encountering the #CP exceptions at runtime.


I'll try to have a closer look at the rest of the series tomorrow.

~Andrew