From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E40A8CA1007 for ; Mon, 1 Sep 2025 22:11:39 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.1105462.1456459 (Exim 4.92) (envelope-from ) id 1utCkJ-0003li-Np; Mon, 01 Sep 2025 22:11:27 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 1105462.1456459; Mon, 01 Sep 2025 22:11:27 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1utCkJ-0003lZ-JD; Mon, 01 Sep 2025 22:11:27 +0000 Received: by outflank-mailman (input) for mailman id 1105462; Mon, 01 Sep 2025 22:11:26 +0000 Received: from mail.xenproject.org ([104.130.215.37]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1utCkI-0003kD-2L for xen-devel@lists.xenproject.org; Mon, 01 Sep 2025 22:11:26 +0000 Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.96) (envelope-from ) id 1utCkH-001MWT-0f; Mon, 01 Sep 2025 22:11:25 +0000 Received: from [19.12.91.86] (helo=localhost) by xenbits.xenproject.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1utCkH-00CPCS-08; Mon, 01 Sep 2025 22:11:25 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID :Subject:Cc:To:Date:From; bh=kTpcAsQXOC1lRh6OHUM0WMrajhFSsDU5wbqwDlXpeyw=; b= oCKemQ/6pDkCLzBZLtnOA0SLVcznTVBT6+8U5tSj2XtDlqTHQCQpC/p7EH6HZ3GXC4vsJWvMleTU8 +5qufqZCXOp+zCm52uDf38hM5trTzuPuJ25QhZVSdtDe/IoErSPOGtAiEUyRiuP6z2gm0FNaGgoJY AAyto94X4RjWCIklM=; From: dmukhin@xen.org Date: Mon, 1 Sep 2025 15:11:24 -0700 To: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, andrew.cooper3@citrix.com, anthony.perard@vates.tech, jbeulich@suse.com, julien@xen.org, michal.orzel@amd.com, roger.pau@citrix.com, sstabellini@kernel.org, dmukhin@ford.com Subject: Re: [PATCH v5 01/15] emul/vuart: introduce framework for UART emulators Message-ID: References: <20250828235409.2835815-1-dmukhin@ford.com> <20250828235409.2835815-2-dmukhin@ford.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Fri, Aug 29, 2025 at 12:27:13PM -0700, Stefano Stabellini wrote: > On Thu, 28 Aug 2025, dmukhin@xen.org wrote: > > From: Denis Mukhin > > > > Introduce a driver framework to abstract UART emulators in the hypervisor. > > > > That allows for architecture-independent handling of virtual UARTs in the > > console driver and simplifies enabling new UART emulators. > > > > The framework is built under CONFIG_VUART_FRAMEWORK, which will be > > automatically enabled once the user enables any UART emulator. > > > > Current implementation supports maximum of one vUART of each kind per domain. > > > > Use new domain_has_vuart() in the console driver code to check whether to > > forward console input to the domain using vUART. > > > > Enable console forwarding over vUART for hardware domains with a vUART. That > > enables console forwarding to dom0 on x86, since console can be forwarded only > > to Xen, dom0 and pvshim on x86 as of now. > > > > Note: existing vUARTs are deliberately *not* hooked to the new framework to > > minimize the scope of the patch: vpl011 (i.e. SBSA) emulator and "vuart" (i.e. > > minimalistic MMIO-mapped dtuart for hwdoms on Arm) are kept unmodified. > > > > No functional changes for non-x86 architectures. > > > > Signed-off-by: Denis Mukhin > > --- > > Changes since v4: > > - addressed feedback > > - Link to v4: https://lore.kernel.org/xen-devel/20250731192130.3948419-3-dmukhin@ford.com/ > > --- > > xen/arch/arm/xen.lds.S | 1 + > > xen/arch/ppc/xen.lds.S | 1 + > > xen/arch/riscv/xen.lds.S | 1 + > > xen/arch/x86/xen.lds.S | 1 + > > xen/common/Kconfig | 2 + > > xen/common/Makefile | 1 + > > xen/common/emul/Kconfig | 6 ++ > > xen/common/emul/Makefile | 1 + > > xen/common/emul/vuart/Kconfig | 6 ++ > > xen/common/emul/vuart/Makefile | 1 + > > xen/common/emul/vuart/vuart.c | 156 +++++++++++++++++++++++++++++++++ > > xen/common/keyhandler.c | 3 + > > xen/drivers/char/console.c | 6 +- > > xen/include/xen/serial.h | 3 + > > xen/include/xen/vuart.h | 116 ++++++++++++++++++++++++ > > xen/include/xen/xen.lds.h | 10 +++ > > 16 files changed, 314 insertions(+), 1 deletion(-) > > create mode 100644 xen/common/emul/Kconfig > > create mode 100644 xen/common/emul/Makefile > > create mode 100644 xen/common/emul/vuart/Kconfig > > create mode 100644 xen/common/emul/vuart/Makefile > > create mode 100644 xen/common/emul/vuart/vuart.c > > create mode 100644 xen/include/xen/vuart.h > > > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > > index db17ff1efa98..cd05b18770f4 100644 > > --- a/xen/arch/arm/xen.lds.S > > +++ b/xen/arch/arm/xen.lds.S > > @@ -58,6 +58,7 @@ SECTIONS > > *(.rodata) > > *(.rodata.*) > > VPCI_ARRAY > > + VUART_ARRAY > > *(.data.rel.ro) > > *(.data.rel.ro.*) > > > > diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S > > index 1de0b77fc6b9..f9d4e5b0dcd8 100644 > > --- a/xen/arch/ppc/xen.lds.S > > +++ b/xen/arch/ppc/xen.lds.S > > @@ -52,6 +52,7 @@ SECTIONS > > *(.rodata) > > *(.rodata.*) > > VPCI_ARRAY > > + VUART_ARRAY > > *(.data.rel.ro) > > *(.data.rel.ro.*) > > > > diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S > > index edcadff90bfe..59dcaa5fef9a 100644 > > --- a/xen/arch/riscv/xen.lds.S > > +++ b/xen/arch/riscv/xen.lds.S > > @@ -47,6 +47,7 @@ SECTIONS > > *(.rodata) > > *(.rodata.*) > > VPCI_ARRAY > > + VUART_ARRAY > > *(.data.rel.ro) > > *(.data.rel.ro.*) > > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > index 966e514f2034..d877b93a6964 100644 > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -132,6 +132,7 @@ SECTIONS > > *(.rodata) > > *(.rodata.*) > > VPCI_ARRAY > > + VUART_ARRAY > > *(.data.rel.ro) > > *(.data.rel.ro.*) > > > > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > > index 76f9ce705f7a..78a32b69e2b2 100644 > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -676,4 +676,6 @@ config PM_STATS > > Enable collection of performance management statistics to aid in > > analyzing and tuning power/performance characteristics of the system > > > > +source "common/emul/Kconfig" > > + > > endmenu > > diff --git a/xen/common/Makefile b/xen/common/Makefile > > index c316957fcb36..c0734480ee4b 100644 > > --- a/xen/common/Makefile > > +++ b/xen/common/Makefile > > @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o > > obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/ > > obj-$(CONFIG_IOREQ_SERVER) += dm.o > > obj-y += domain.o > > +obj-y += emul/ > > obj-y += event_2l.o > > obj-y += event_channel.o > > obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o > > diff --git a/xen/common/emul/Kconfig b/xen/common/emul/Kconfig > > new file mode 100644 > > index 000000000000..7c6764d1756b > > --- /dev/null > > +++ b/xen/common/emul/Kconfig > > @@ -0,0 +1,6 @@ > > +menu "Domain Emulation Features" > > + visible if EXPERT > > + > > +source "common/emul/vuart/Kconfig" > > + > > +endmenu > > diff --git a/xen/common/emul/Makefile b/xen/common/emul/Makefile > > new file mode 100644 > > index 000000000000..ae0b575c3901 > > --- /dev/null > > +++ b/xen/common/emul/Makefile > > @@ -0,0 +1 @@ > > +obj-$(CONFIG_VUART_FRAMEWORK) += vuart/ > > diff --git a/xen/common/emul/vuart/Kconfig b/xen/common/emul/vuart/Kconfig > > new file mode 100644 > > index 000000000000..ce1b976b7da7 > > --- /dev/null > > +++ b/xen/common/emul/vuart/Kconfig > > @@ -0,0 +1,6 @@ > > +config VUART_FRAMEWORK > > + bool > > + > > +menu "UART Emulation" > > + > > +endmenu > > diff --git a/xen/common/emul/vuart/Makefile b/xen/common/emul/vuart/Makefile > > new file mode 100644 > > index 000000000000..97f792dc6641 > > --- /dev/null > > +++ b/xen/common/emul/vuart/Makefile > > @@ -0,0 +1 @@ > > +obj-y += vuart.o > > diff --git a/xen/common/emul/vuart/vuart.c b/xen/common/emul/vuart/vuart.c > > new file mode 100644 > > index 000000000000..7b277d00d5c7 > > --- /dev/null > > +++ b/xen/common/emul/vuart/vuart.c > > @@ -0,0 +1,156 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * UART emulator framework. > > + * > > + * Copyright 2025 Ford Motor Company > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#define for_each_emulator(e) \ > > + for ( e = vuart_array_start; e < vuart_array_end; e++ ) > > + > > +extern const struct vuart_emulator vuart_array_start[]; > > +extern const struct vuart_emulator vuart_array_end[]; > > + > > +static const struct vuart_emulator * > > +vuart_match_by_compatible(struct domain *d, const char *compat) > > +{ > > + const struct vuart_emulator *emulator; > > + > > + if ( d->console.vuart ) > > + return NULL; > > + > > + for_each_emulator(emulator) > > + if ( emulator->compatible && > > + !strncmp(emulator->compatible, compat, > > + strlen(emulator->compatible)) ) > > strncmp will continue until the given count even if compat is shorter I checked lib/strncmp.c. I'll flip the arguments, so that strncmp() will return early if compat is shorter than emulator->compatible > > > > + return emulator; > > + > > + return NULL; > > +} > > + > > +static struct vuart *vuart_find_by_console_permission(const struct domain *d) > > +{ > > + struct vuart *vuart = d->console.vuart; > > + > > + ASSERT(d->console.input_allowed); > > This ASSERT looks suspicious as we haven't even checked that vuart!=NULL > yet. I would remove it Ack > > > > + if ( !vuart || !vuart->emulator || !vuart->emulator->put_rx || > > + !(vuart->flags & VUART_CONSOLE_INPUT)) > > + return NULL; > > + > > + return vuart; > > +} > > + > > +struct vuart *vuart_find_by_io_range(struct domain *d, unsigned long addr, > > + unsigned long size) > > +{ > > + struct vuart *vuart = d->console.vuart; > > + > > + if ( !vuart || !vuart->info ) > > + return NULL; > > You might as well call vuart_find_by_console_permission that has all the > checks already vuart_find_by_io_range() is used in the I/O hook to find the vUART by its I/O port range. Using vuart_find_by_console_permission() here may be problematic if the vUART does not support/enable console forwarding. I'll keep this check as is for now. > > > + > > + if ( addr >= vuart->info->base_addr && > > + addr + size - 1 <= vuart->info->base_addr + vuart->info->size - 1 ) > > + return vuart; > > + > > + return NULL; > > +} > > + > > +int vuart_init(struct domain *d, struct vuart_info *info) > > +{ > > + const struct vuart_emulator *emulator; > > + struct vuart *vuart; > > + int rc; > > + > > + emulator = vuart_match_by_compatible(d, info->compatible); > > + if ( !emulator ) > > + return -ENODEV; > > + > > + vuart = xzalloc(typeof(*vuart)); > > + if ( !vuart ) > > + return -ENOMEM; > > + > > + vuart->info = xvzalloc(typeof(*info)); > > + if ( !vuart->info ) > > + { > > + rc = -ENOMEM; > > + goto err_out; > > + } > > + memcpy(vuart->info, info, sizeof(*info)); > > one thing to note is that the fields (strings) compatible and name are > copied by address, I am not if it is OK but FYI Thanks, will update vuart_info struct. > > > > + vuart->vdev = emulator->alloc(d, vuart->info); > > + if ( IS_ERR(vuart->vdev) ) > > + { > > + rc = PTR_ERR(vuart->vdev); > > + goto err_out; > > this path leads to vuart->info not being freed Thanks > > > > + } > > + > > + vuart->emulator = emulator; > > + vuart->owner = d; > > + vuart->flags |= VUART_CONSOLE_INPUT; > > + > > + d->console.input_allowed = true; > > + d->console.vuart = vuart; > > + > > + return 0; > > + > > + err_out: > > + XVFREE(vuart); > > + return rc; > > +} > > + > > +/* > > + * Release any resources taken by UART emulators. > > + * > > + * NB: no flags are cleared, since currently exit() is called only during > > + * domain destroy. > > + */ > > +void vuart_deinit(struct domain *d) > > +{ > > + struct vuart *vuart = d->console.vuart; > > + > > + if ( vuart ) > > + { > > + vuart->emulator->free(vuart); > > should we pass vuart or vuart->vdev here? The emulator state is supposed > to be vuart->vdev? That should be vuart->vdev; thanks! > > > + XVFREE(vuart->info); > > + } > > + > > + XVFREE(d->console.vuart); > > +} > > + > > +void vuart_dump_state(const struct domain *d) > > +{ > > + struct vuart *vuart = d->console.vuart; > > + > > + if ( vuart ) > > + vuart->emulator->dump_state(vuart); > > also here vuart->vdev? Ack > > > > +} > > + > > +/* > > + * Put character to the *first* suitable emulated UART's FIFO. > > + */ > > +int vuart_put_rx(struct domain *d, char c) > > +{ > > + struct vuart *vuart = vuart_find_by_console_permission(d); > > + > > + return vuart ? vuart->emulator->put_rx(vuart, c) : -ENODEV; > > and here? Ack > > > > +} > > + > > +bool domain_has_vuart(const struct domain *d) > > +{ > > + return vuart_find_by_console_permission(d); > > +} > > + > > +/* > > + * Local variables: > > + * mode: C > > + * c-file-style: "BSD" > > + * c-basic-offset: 4 > > + * indent-tabs-mode: nil > > + * End: > > + */ > > diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c > > index cb6df2823b00..156e64d9eb58 100644 > > --- a/xen/common/keyhandler.c > > +++ b/xen/common/keyhandler.c > > @@ -22,6 +22,7 @@ > > #include > > #include > > #include > > +#include > > #include > > > > static unsigned char keypress_key; > > @@ -352,6 +353,8 @@ static void cf_check dump_domains(unsigned char key) > > v->periodic_period / 1000000); > > } > > } > > + > > + vuart_dump_state(d); > > } > > > > for_each_domain ( d ) > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > > index 9bd5b4825da6..d5164897a776 100644 > > --- a/xen/drivers/char/console.c > > +++ b/xen/drivers/char/console.c > > @@ -33,6 +33,7 @@ > > #include > > #include > > #include > > +#include > > > > #ifdef CONFIG_X86 > > #include > > @@ -596,11 +597,12 @@ static void __serial_rx(char c) > > if ( !d ) > > return; > > > > - if ( is_hardware_domain(d) ) > > + if ( is_hardware_domain(d) && !domain_has_vuart(d) ) > > { > > /* > > * Deliver input to the hardware domain buffer, unless it is > > * already full. > > + * NB: must be the first check: hardware domain may have emulated UART. > > */ > > if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE ) > > serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c; > > @@ -611,6 +613,8 @@ static void __serial_rx(char c) > > */ > > send_global_virq(VIRQ_CONSOLE); > > } > > + else if ( domain_has_vuart(d) ) > > + rc = vuart_put_rx(d, c); > > #ifdef CONFIG_SBSA_VUART_CONSOLE > > else > > /* Deliver input to the emulated UART. */ > > diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h > > index 8e1844555208..d7e81f098359 100644 > > --- a/xen/include/xen/serial.h > > +++ b/xen/include/xen/serial.h > > @@ -36,6 +36,9 @@ struct vuart_info { > > unsigned long data_off; /* Data register offset */ > > unsigned long status_off; /* Status register offset */ > > unsigned long status; /* Ready status value */ > > + unsigned int irq; /* Interrupt */ > > + const char *compatible; /* Compatible string */ > > + const char *name; /* User-friendly name */ > > }; > > > > struct serial_port { > > diff --git a/xen/include/xen/vuart.h b/xen/include/xen/vuart.h > > new file mode 100644 > > index 000000000000..ca025b4179be > > --- /dev/null > > +++ b/xen/include/xen/vuart.h > > @@ -0,0 +1,116 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * UART emulator framework. > > + * > > + * Copyright 2025 Ford Motor Company > > + */ > > + > > +#ifndef XEN_VUART_H > > +#define XEN_VUART_H > > + > > +#include > > +#include > > + > > +struct vuart_emulator; > > + > > +enum { > > + VUART_CONSOLE_INPUT = 0U << 1, /** Physical console input forwarding. */ > > code style: single * Ack > This flag is zero, is that intended? Will fix, thanks > > > > +}; > > + > > + > > +/* > > + * FIXME: #ifdef is temporary to avoid clash with > > + * arch/arm/include/asm/domain.h > > + */ > > +#ifdef CONFIG_VUART_FRAMEWORK > > +struct vuart { > > + const struct vuart_emulator *emulator; > > + struct vuart_info *info; > > + struct domain *owner; > > + uint32_t flags; > > + void *vdev; > > +}; > > +#endif > > + > > +struct vuart_emulator { > > + /* UART compatible string. Cannot be NULL or empty. */ > > + const char *compatible; > > + > > + /* > > + * Allocate emulated UART state (RX/TX FIFOs, locks, initialize registers, > > + * hook I/O handlers, etc.) > > + * Cannot be NULL. > > + */ > > + void *(*alloc)(struct domain *d, const struct vuart_info *info); > > + > > + /* > > + * Release resources used to emulate UART state (flush RX/TX FIFOs, unhook > > + * I/O handlers, etc.). > > + * Cannot be NULL. > > + */ > > + void (*free)(void *arg); > > + > > + /* > > + * Print emulated UART state, including registers, on the console. > > + * Can be NULL. > > + */ > > + void (*dump_state)(void *arg); > > + > > + /* > > + * Place character to the emulated RX FIFO. > > + * Used to forward physical console input to the guest OS. > > + * Can be NULL. > > + */ > > + int (*put_rx)(void *arg, char c); > > +}; > > + > > +#define VUART_REGISTER(name, x) \ > > + static const struct vuart_emulator name##_entry \ > > + __used_section(".data.rel.ro.vuart") = x > > + > > +struct vuart *vuart_find_by_io_range(struct domain *d, > > + unsigned long base_addr, > > + unsigned long size); > > + > > +int vuart_put_rx(struct domain *d, char c); > > + > > +#ifdef CONFIG_VUART_FRAMEWORK > > + > > +int vuart_init(struct domain *d, struct vuart_info *info); > > +void vuart_deinit(struct domain *d); > > +void vuart_dump_state(const struct domain *d); > > +bool domain_has_vuart(const struct domain *d); > > + > > +#else > > + > > +static inline int vuart_init(struct domain *d, struct vuart_info *info) > > +{ > > + return 0; > > +} > > + > > +static inline void vuart_deinit(struct domain *d) > > +{ > > +} > > + > > +static inline void vuart_dump_state(const struct domain *d) > > +{ > > +} > > + > > +static inline bool domain_has_vuart(const struct domain *d) > > +{ > > + return false; > > +} > > + > > +#endif /* CONFIG_VUART_FRAMEWORK */ > > + > > +#endif /* XEN_VUART_H */ > > + > > +/* > > + * Local variables: > > + * mode: C > > + * c-file-style: "BSD" > > + * c-basic-offset: 4 > > + * indent-tabs-mode: nil > > + * End: > > + */ > > + > > diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h > > index b126dfe88792..2d65f32ddad3 100644 > > --- a/xen/include/xen/xen.lds.h > > +++ b/xen/include/xen/xen.lds.h > > @@ -194,4 +194,14 @@ > > #define VPCI_ARRAY > > #endif > > > > +#ifdef CONFIG_VUART_FRAMEWORK > > +#define VUART_ARRAY \ > > + . = ALIGN(POINTER_ALIGN); \ > > + vuart_array_start = .; \ > > + *(.data.rel.ro.vuart) \ > > + vuart_array_end = .; > > +#else > > +#define VUART_ARRAY > > +#endif > > + > > #endif /* __XEN_LDS_H__ */ > > -- > > 2.51.0 > >