On 31.07.2025 21:21, dm...@proton.me wrote:
> From: Denis Mukhin <dmuk...@ford.com>
>
> 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_HAS_VUART, which will be automatically
> enabled once the user enables any UART emulator.
Yet then still - why "HAS"? Call it just VUART or VUART_FRAMEWORK or some such.
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -1,6 +1,8 @@
>
> menu "Common Features"
>
> +source "common/emul/Kconfig"
> +
> config COMPAT
Why at the very top?
> --- 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..670682102c13
> --- /dev/null
> +++ b/xen/common/emul/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_HAS_VUART) += vuart/
With this you can ...
> --- /dev/null
> +++ b/xen/common/emul/vuart/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_HAS_VUART) += vuart.o
... use the simpler obj-y here.
> --- /dev/null
> +++ b/xen/common/emul/vuart/vuart.c
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * UART emulator framework.
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/vuart.h>
> +
> +#define VUART_ARRAY_SIZE (__start_vuart_end - __start_vuart_array)
> +
> +#define for_each_vuart(vdev) \
> + for (unsigned __i = 0; \
> + __i < VUART_ARRAY_SIZE && (vdev = __start_vuart_array[__i], 1); \
> + __i++)
Nit: Xen style please. Any preferably no leading underscores; in no case
two of them.
> +extern const struct vuart_ops *const __start_vuart_array[];
> +extern const struct vuart_ops *const __start_vuart_end[];
Is there an actual need for this extra level of indirection? It is in the
process of being done away with for vPCI.
> +int vuart_add_node(struct domain *d, const void *node)
> +{
> + const struct vuart_ops *vdev;
> + int rc;
> +
> + for_each_vuart(vdev)
> + {
> + if ( !vdev->add_node )
> + continue;
> +
> + rc = vdev->add_node(d, node);
Here and below - shouldn't you call hooks only when the kind of driver is
actually enabled for the domkain in question?
> + if ( rc )
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +int vuart_init(struct domain *d, struct vuart_params *params)
> +{
> + const struct vuart_ops *vdev;
> + int rc;
> +
> + if ( !domain_has_vuart(d) )
> + return 0;
> +
> + for_each_vuart(vdev)
> + {
> + rc = vdev->init(d, params);
> + if ( rc )
> + return rc;
> + }
> +
> + d->console.input_allowed = true;
Unconditionally?
> +void vuart_deinit(struct domain *d)
> +{
> + const struct vuart_ops *vdev;
> +
> + for_each_vuart(vdev)
> + vdev->deinit(d);
> +}
I can perhaps see why this hook wants to uniformly be set, but ...
> +void vuart_dump_state(const struct domain *d)
> +{
> + const struct vuart_ops *vdev;
> +
> + for_each_vuart(vdev)
> + vdev->dump_state(d);
> +}
... state dumping pretty surely wants to be optional?
> +/*
> + * Put character to the first suitable emulated UART's FIFO.
> + */
What's "suitable"? Along the lines of the earlier remark, what if the domain
has vUART kind A configured, ...
> +int vuart_put_rx(struct domain *d, char c)
> +{
> + const struct vuart_ops *vdev = NULL;
> +
> + ASSERT(domain_has_vuart(d));
> +
> + for_each_vuart(vdev)
> + if ( vdev->put_rx )
... but only kind B offers this hook?
> + break;
> +
> + return vdev ? vdev->put_rx(d, c) : -ENODEV;
The check for NULL helps for the "no vUART drivers" case, but it won't
help if you exhausted the array without finding a driver with the wanted
hook.
> +}
> +
> +bool domain_has_vuart(const struct domain *d)
> +{
> + uint32_t mask = 0;
unsigned int?
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -22,6 +22,7 @@
> #include <xen/mm.h>
> #include <xen/watchdog.h>
> #include <xen/init.h>
> +#include <xen/vuart.h>
> #include <asm/div64.h>
>
> static unsigned char keypress_key;
> @@ -354,6 +355,8 @@ static void cf_check dump_domains(unsigned char key)
> v->periodic_period / 1000000);
> }
> }
> +
> + vuart_dump_state(d);
How verbose is this going to get?
> --- /dev/null
> +++ b/xen/include/xen/vuart.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * UART emulator framework.
> + *
> + * Copyright 2025 Ford Motor Company
> + */
> +
> +#ifndef XEN_VUART_H
> +#define XEN_VUART_H
> +
> +#include <public/xen.h>
> +#include <public/event_channel.h>
> +#include <xen/types.h>
The order is wrong - types must be available before public headers are included.
> +struct vuart_params {
> + domid_t console_domid;
> + gfn_t gfn;
> + evtchn_port_t evtchn;
> +};
> +
> +struct vuart_ops {
> + int (*add_node)(struct domain *d, const void *node);
> + int (*init)(struct domain *d, struct vuart_params *params);
> + void (*deinit)(struct domain *d);
> + void (*dump_state)(const struct domain *d);
> + int (*put_rx)(struct domain *d, char c);
> +};
> +
> +#define VUART_REGISTER(name, x) \
> + static const struct vuart_ops *const __name##_entry \
> + __used_section(".data.vuart." #name) = (x);
> +
> +#ifdef CONFIG_HAS_VUART
> +
> +int vuart_add_node(struct domain *d, const void *node);
> +int vuart_init(struct domain *d, struct vuart_params *params);
> +void vuart_deinit(struct domain *d);
> +void vuart_dump_state(const struct domain *d);
> +int vuart_put_rx(struct domain *d, char c);
> +bool domain_has_vuart(const struct domain *d);
> +
> +#else
> +
> +static inline int vuart_add_node(struct domain *d, const void *node)
> +{
> + return 0;
> +}
> +
> +static inline int vuart_init(struct domain *d, struct vuart_params *params)
> +{
> + return 0;
> +}
> +
> +static inline void vuart_deinit(struct domain *d)
> +{
> +}
> +
> +static inline void vuart_dump_state(const struct domain *d)
> +{
> +}
> +
> +static inline int vuart_put_rx(struct domain *d, char c)
> +{
> + ASSERT_UNREACHABLE();
> + return -ENODEV;
> +}
> +
> +static inline bool domain_has_vuart(const struct domain *d)
> +{
> + return false;
> +}
With this, some of the other stubs should not be necessary. Declarations
will suffice, e.g. for vuart_put_rx().
> --- a/xen/include/xen/xen.lds.h
> +++ b/xen/include/xen/xen.lds.h
> @@ -194,4 +194,14 @@
> #define VPCI_ARRAY
> #endif
>
> +#ifdef CONFIG_HAS_VUART
> +#define VUART_ARRAY \
> + . = ALIGN(POINTER_ALIGN); \
> + __start_vuart_array = .; \
> + *(SORT(.data.vuart.*)) \
This is r/o data afaict, so would want naming .rodata.vuart.*. Which in
turn means the uses of the macros need to move up in the linker scripts.
Jan
