Re: [PATCH v4 2/8] emul/vuart: introduce framework for UART emulators

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2025 21:21, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> 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