Re: [PATCH v8 01/26] emul/vuart: introduce framework for UART emulators

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/11/26 8:47 PM, dmukhin@xxxxxxxx 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_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.
d->console->vuart is a single pointer and vuart_init() returns -EBUSY
if it is already set, so it is one vUART per domain in total, not one
of each kind.

> 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 <dmukhin@xxxxxxxx>
> ---
>   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/ns165x50.c |   0
This adds an empty file whose name looks like a typo of ns16x50.c.
Please drop it.

>   xen/common/emul/vuart/vuart.c    | 173 +++++++++++++++++++++++++++++++
>   xen/common/keyhandler.c          |   3 +
>   xen/drivers/char/console.c       |   6 +-
>   xen/include/xen/sched.h          |   5 +
>   xen/include/xen/serial.h         |   5 +
>   xen/include/xen/vuart.h          | 115 ++++++++++++++++++++
>   xen/include/xen/xen.lds.h        |  10 ++
>   18 files changed, 337 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/ns165x50.c
>   create mode 100644 xen/common/emul/vuart/vuart.c
>   create mode 100644 xen/include/xen/vuart.h
[...]

> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index f69d47d18934..60f2d9087589 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_DEVICE_TREE_PARSE) += device-tree/
>   obj-$(CONFIG_IOREQ_SERVER) += dm.o
>   obj-y += domain.o
>   obj-y += domid.o
> +obj-y += emul/
I think this doesn't make much difference at the moment, but it might be 
useful to make this conditional so that architectures that don't need 
any emulators can exclude them and save some space.
[...]

> diff --git a/xen/common/emul/vuart/ns165x50.c b/xen/common/emul/vuart/ns165x50.c
> new file mode 100644
> index 000000000000..e69de29bb2d1
This adds an empty file whose name looks like a typo of ns16x50.c.
Please drop it.

> diff --git a/xen/common/emul/vuart/vuart.c b/xen/common/emul/vuart/vuart.c
> new file mode 100644
> index 000000000000..bd3b756fb4d6
> --- /dev/null
> +++ b/xen/common/emul/vuart/vuart.c
> @@ -0,0 +1,173 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * UART emulator framework.
> + *
Is this line with only one start really needed?

> + */
> +
> +#include <asm/types.h>
Nit: xen/ headers usually come before asm/ ones.

Also, I think it will be better to use xen/types.h to avoid depenendcy 
in common code from arch-specific headers.
> +
> +#include <xen/err.h>
> +#include <xen/resource.h>
> +#include <xen/sched.h>
> +#include <xen/vuart.h>
> +#include <xen/xvmalloc.h>
> +
> +#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(const struct domain *d, const char *compat)
> +{
> +    const struct vuart_emulator *emulator;
> +
> +    for_each_emulator(emulator)
> +        if ( emulator->compatible &&
> +             !strncmp(compat, emulator->compatible,
> +                      strlen(emulator->compatible)) )
> +            return emulator;
This is a prefix match: "ns16550foo" matches "ns16550", and if one
emulator's compatible is a prefix of another's, whichever is linked
first wins. Is that intended? If not, a plain strcmp() would do.

Also, d is unused, and struct vuart_emulator documents compatible as
"Cannot be NULL or empty", so the NULL check either isn't needed or the
comment should say otherwise, or just ASSERT() will be enough here.

> +
> +    return NULL;
> +}
> +
> +const static struct vuart *
Nit: static const.

> +vuart_find_by_flags(const struct domain *d, unsigned int flags)
> +{
> +    const struct vuart *vuart = d->console->vuart;
> +
> +    if ( vuart && (vuart->info->flags & flags) )
> +        return vuart;
> +
> +    return NULL;
> +}
> +
> +struct vuart *vuart_find_by_resource(const struct domain *d,
> +                                     unsigned int type,
> +                                     unsigned long addr,
> +                                     unsigned long size)
> +{
> +    struct vuart *vuart = d->console->vuart;
> +
> +    if ( vuart &&
> +         (type & vuart->info->base_type) &&
> +         addr >= vuart->info->base_addr &&
> +         addr + size <= vuart->info->base_addr + vuart->info->size )
> +        return vuart;
This has no user in this patch; Wouldn't it better to introduce together 
with its first user? Also, base_addr is paddr_t while addr/size are 
unsigned long, and addr + size can wrap; checking "size <= base_addr + 
vuart->info->size - addr" after the lower bound avoids that.
> +
> +    return NULL;
> +}
> +
> +int vuart_init(struct domain *d, const struct vuart_info *info)
> +{
> +    const struct vuart_emulator *emulator;
> +    struct vuart *vuart;
> +    int rc;
> +
> +    if ( !info )
> +        return 0; /* Nothing to do */
> +
> +    if ( d->console->vuart )
> +        return -EBUSY;
> +
> +    emulator = vuart_match_by_compatible(d, info->compatible);
> +    if ( !emulator )
> +        return -ENODEV;
> +
> +    vuart = xzalloc(typeof(*vuart));
> +    if ( !vuart )
> +        return -ENOMEM;
> +
> +    vuart->info = xvzalloc(typeof(*vuart->info));
> +    if ( !vuart->info )
> +    {
> +        rc = -ENOMEM;
> +        goto err_out1;
> +    }
> +    memcpy(vuart->info, info, sizeof(*info));
vuart comes from xzalloc() but is released with xvfree() here and with
XVFREE() in vuart_deinit(). That works, since xvfree() falls back to
xfree(), but please pair the allocators. Also, why a separate
allocation for info? Embedding a struct vuart_info in struct vuart
would drop one allocation, one error path and the memcpy().

> +
> +    vuart->vdev = emulator->alloc(d, vuart->info);
> +    if ( IS_ERR(vuart->vdev) )
> +    {
> +        rc = PTR_ERR(vuart->vdev);
> +        goto err_out2;
> +    }
> +
> +    vuart->emulator = emulator;
> +    vuart->owner = d;
> +
> +    d->console->input_allowed = true;
> +    d->console->vuart = vuart;
input_allowed is set regardless of VUART_CONSOLE_INPUT, whereas
vuart_put_rx() and domain_has_vuart() act only on a vUART with that
flag. For a vUART without the flag, the domain becomes selectable with
CTRL-a, but all input to it is dropped silently. Should this be
conditional on info->flags & VUART_CONSOLE_INPUT?

> +
> +    return 0;
> +
> + err_out2:
> +    xvfree(vuart->info);
> + err_out1:
> +    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.
s/exit()/vuart_deinit()/ ?

> + */
> +void vuart_deinit(struct domain *d)
> +{
> +    struct vuart *vuart = d->console->vuart;
> +
> +    if ( vuart )
> +    {
> +        vuart->emulator->free(vuart->vdev);
> +        xvfree(vuart->info);
> +    }
> +
> +    XVFREE(d->console->vuart);
> +}
> +
> +/*
> + * Print emulated UART state on the console.
> + *
> + * Must be called under rcu_lock_domain().
The only caller, dump_domains(), holds rcu_read_lock(&domlist_read_lock)
rather than rcu_lock_domain(). That is fine, but the comment should
match.

[...]


> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -33,6 +33,7 @@
>   #include <xen/dom0less-build.h>
>   #include <xen/sections.h>
>   #include <xen/consoled.h>
> +#include <xen/vuart.h>
>   
>   #ifdef CONFIG_X86
>   #include <asm/guest.h>
> @@ -639,11 +640,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.
The NB is now inside the block it refers to, which makes it hard to
follow. Since the condition itself excludes a hwdom with a vUART, does
the order still matter? If it does, I'd put the note above the if ().

>            */
>           if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
>               serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> @@ -654,6 +656,8 @@ static void __serial_rx(char c)
>            */
>           send_global_virq(VIRQ_CONSOLE);
>       }
> +    else if ( domain_has_vuart(d) )
> +        rc = vuart_put_rx(d, c);
ns16x50_put_rx() (10/26) returns -EBUSY while LCR.DLAB is set, DLL is 0
or MCR.LOOP is set, i.e. while the guest is programming the UART, so
every character typed in that window is dropped and the warning below
is printed for each of them. Are we okay with that spam even during a 
short peroid?
>   #ifdef CONFIG_SBSA_VUART_CONSOLE
>       else
>           /* Deliver input to the emulated UART. */
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ce1fa636db36..4fa7f62490ca 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -24,6 +24,7 @@
>   #include <asm/atomic.h>
>   #include <asm/current.h>
>   #include <xen/vpci.h>
> +#include <xen/vuart.h>
struct domain_console only needs a pointer, so a forward declaration
"struct vuart;" is enough.

>   #include <xen/wait.h>
>   #include <public/xen.h>
>   #include <public/domctl.h>
> @@ -382,6 +383,10 @@ struct domain_console {
>       unsigned int idx;
>       spinlock_t lock;
>       char buf[256];
> +
> +#ifdef CONFIG_VUART_FRAMEWORK
> +    struct vuart *vuart;
> +#endif
>   };
>   
>   enum __packed domain_shutdown_state {
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 3a71da767dd7..ec481893fede 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -32,10 +32,15 @@ enum serial_port_state {
>   
>   struct vuart_info {
>       paddr_t base_addr;          /* Base address of the UART */
> +    unsigned int base_type;     /* Base I/O resource type */
>       unsigned long size;         /* Size of the memory region */
>       unsigned long data_off;     /* Data register offset */
>       unsigned long status_off;   /* Status register offset */
>       unsigned long status;       /* Ready status value */
> +    unsigned long flags;        /* Emulation flags */
> +    unsigned int irq;           /* Emulated IRQ # */
> +    char compatible[16];        /* Compatible string */
> +    char name[16];              /* User-friendly name */
>   };
Today struct vuart_info describes a *physical* UART: ns16550.c,
cadence-uart.c and mvebu-uart.c fill it in for Arm's hwdom vuart. With
this change it also becomes the configuration of an *emulated* one, so
every physical driver's instance grows fields that are meaningless for
it, and data_off/status_off/status are meaningless for an emulator. A
separate structure for the emulator configuration would keep the two
apart.

>   
>   struct serial_port {
> diff --git a/xen/include/xen/vuart.h b/xen/include/xen/vuart.h
> new file mode 100644
> index 000000000000..b5785b3c6b8f
> --- /dev/null
> +++ b/xen/include/xen/vuart.h
> @@ -0,0 +1,115 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * UART emulator framework.
> + *
It seems like this line with start should be dropped, shouldn't it?

> + */
> +
> +#ifndef XEN_VUART_H
> +#define XEN_VUART_H
> +
> +#include <xen/serial.h>
> +#include <public/xen.h>
> +
> +struct vuart_emulator;
> +
> +enum {
> +    VUART_CONSOLE_INPUT = BIT(0, U), /* Physical console input forwarding. */
> +};
> +
> +/*
> + * FIXME: #ifdef is temporary to avoid clash with
> + *   arch/arm/include/asm/domain.h
> + */
> +#ifdef CONFIG_VUART_FRAMEWORK
> +struct vuart {
As long as the name clashes with Arm's struct vuart, the framework
can't be enabled on Arm. Picking a non-clashing name now looks simpler
than carrying the FIXME.

> +    const struct vuart_emulator *emulator;
> +    struct vuart_info *info;
> +    struct domain *owner;
> +    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 (e.g. emulated registers) on the diagnostic
> +     * 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_resource(const struct domain *d,
> +                                     unsigned int type,
> +                                     unsigned long addr,
> +                                     unsigned long size);
> +
> +int vuart_put_rx(const struct domain *d, char c);
> +
> +#ifdef CONFIG_VUART_FRAMEWORK
These two are declared outside the #ifdef with no stubs. The console.c
call builds with the framework off only because domain_has_vuart() is
an inline returning false and the call gets eliminated. I'd move both
under the #ifdef, next to the other declarations, with a stub for
vuart_put_rx().

> +
> +int vuart_init(struct domain *d, const 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, const 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:
> + */
> +
git reports "new blank line at EOF" here.

> diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
> index 958f8256b0fa..5e630d371471 100644
> --- a/xen/include/xen/xen.lds.h
> +++ b/xen/include/xen/xen.lds.h
> @@ -231,6 +231,16 @@
>   #define VPCI_ARRAY
>   #endif
>   
> +#ifdef CONFIG_VUART_FRAMEWORK
> +#define VUART_ARRAY              \
> +       . = ALIGN(POINTER_ALIGN); \
> +       vuart_array_start = .;    \
> +       *(.data.rel.ro.vuart)     \
> +       vuart_array_end = .;
Nit: VPCI_ARRAY right above uses __start_vpci_array/__end_vpci_array;
__start_vuart_array/__end_vuart_array would be consistent.

~ Oleksii