Re: [PATCH v3 07/24] xen/console: introduce framework for UART emulators

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2025 02:58, Denis Mukhin via B4 Relay 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 from Xen
> console driver and simplifies enabling new architecture-dependent UART
> emulators.
> 
> The framework is built under CONFIG_HAS_VUART, which is automatically enabled
> once the user selects a specific UART emulator.
> 
> All domains w/ enabled vUART will have VIRTDEV_UART bit set in
> d->arch.emulation_flags.
> 
> Current implementation supports maximum of one vUART per domain, excluding
> emulators for hardware domains.
> 
> Use domain_has_vuart() in Xen console driver code to check whether the
> domain can own the physical console focus.

Purely from how it is spelled out here this looks wrong: A domain having a
vUART may not necessarily imply it may also own console focus. Imo the two
need to be treated separately (perhaps even involving XSM), with it merely
being a prereq to have a vUART in order to possible also own console focus.

> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -785,7 +785,7 @@ static int __init construct_domU(struct domain *d,
>       */
>      if ( kinfo.vpl011 )
>      {
> -        rc = domain_vpl011_init(d, NULL);
> +        rc = virtdev_uart_init(d, NULL);

Like I said for the public header, "virt" as in "virtdev" is ambiguous.
Is there anything wrong with calling this function vuart_init()? While
you may say that the 'v' in there is then as ambiguous, I think that's
not actually the case.

> @@ -891,7 +891,7 @@ void __init create_domUs(void)
>               * d->arch.vpl011.irq. So the logic to find the vIRQ has to
>               * be hardcoded.
>               * The logic here shall be consistent with the one in
> -             * domain_vpl011_init().
> +             * vpl011_init().
>               */

Since you relaxed the tying to vpl011 in the earlier hunk, why is the
tight connection being retained here?

> @@ -30,10 +31,7 @@ static int handle_vuart_init(struct domain *d,
>                               struct xen_domctl_vuart_op *vuart_op)
>  {
>      int rc;
> -    struct vpl011_init_info info;
> -
> -    info.console_domid = vuart_op->console_domid;
> -    info.gfn = _gfn(vuart_op->gfn);
> +    struct virtdev_uart_params info;
>  
>      if ( d->creation_finished )
>          return -EPERM;
> @@ -41,8 +39,11 @@ static int handle_vuart_init(struct domain *d,
>      if ( vuart_op->type != XEN_DOMCTL_VUART_TYPE_VPL011 )
>          return -EOPNOTSUPP;
>  
> -    rc = domain_vpl011_init(d, &info);
> +    info.console_domid = vuart_op->console_domid;
> +    info.gfn = _gfn(vuart_op->gfn);
> +    info.evtchn = (evtchn_port_t)-1;

Where's the literal -1 coming from? Port 0 being guaranteed invalid, that's
what we normally use as sentinel. (It's also unclear why the field needs
setting now, when it wasn't set before.)

Also: Can't all three fields be set in the variable's initializer?

> @@ -783,6 +788,12 @@ void domain_vpl011_deinit(struct domain *d)
>          XFREE(vpl011->backend.xen);
>  }
>  
> +static void cf_check vpl011_dump(struct domain *d)
> +{
> +}

If at all possible, can we try to avoid having empty handler functions.
Putting NULL there and having the caller check is generally preferable,
at least from cf_check perspective.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -30,6 +30,7 @@
>  #include <xen/vpci.h>
>  #include <xen/nospec.h>
>  #include <xen/vm_event.h>
> +#include <xen/virtdev-uart.h>
>  #include <asm/shadow.h>
>  #include <asm/hap.h>
>  #include <asm/current.h>

Why would this be needed at this time?

> --- a/xen/drivers/Makefile
> +++ b/xen/drivers/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_HAS_VPCI) += vpci/
>  obj-$(CONFIG_HAS_PASSTHROUGH) += passthrough/
>  obj-$(CONFIG_ACPI) += acpi/
>  obj-$(CONFIG_VIDEO) += video/
> +obj-$(CONFIG_HAS_VUART) += virtdev-uart.o

I'm unconvinced we want any C files directly under drivers/.

> --- /dev/null
> +++ b/xen/drivers/virtdev-uart.c
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/errno.h>
> +#include <xen/event.h>
> +#include <xen/virtdev-uart.h>
> +#include <public/virtdev.h>
> +
> +extern const struct virtdev_uart *__start_virtdev_uart;

Imo this wants to be an array from the very beginning, no matter that
for now you expect the array to have just (at most) one element.

> +int virtdev_uart_init(struct domain *d, struct virtdev_uart_params *params)
> +{
> +    int rc;
> +
> +    ASSERT(__start_virtdev_uart);

What is this to guard against? If the linker script doesn't define the
symbol, linking will fail (as the symbol isn't weak). If it does define
it, its address will be guaranteed non-NULL. What you instead need to
assure is for ...

> +    rc = __start_virtdev_uart->init(d, params);

... this de-ref to actually be within bounds (i.e. __start_virtdev_uart
< __end_virtdev_uart at the very least).

> +    if ( rc )
> +        return rc;
> +
> +#if !defined(__i386__) && !defined(__x86_64__)
> +    d->arch.emulation_flags |= VIRTDEV_UART;
> +#endif

This isn't how emulation_flags has been used so far: The field is set
once, and then isn't further modified. Question is what you mean to
achieve by setting this conditionally. Depending on that it may be
possible to suggest alternatives.

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -54,6 +54,9 @@ void arch_get_domain_info(const struct domain *d,
>  
>  #define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
>  #define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
> +#define domain_has_vuart(d) \
> +    ( IS_ENABLED(CONFIG_HAS_VUART) && \
> +      (d)->arch.emulation_flags & VIRTDEV_UART )

Nit: Parentheses around the & operation, to separate it from the && one.

As to the IS_ENABLED(): If you look at how CDF_staticmem and
CDF_directmap you'll find that we conditionally #define them to zero
when the respective CONFIG_* isn't / aren't enabled. That would be nice
to have here, too.

> --- /dev/null
> +++ b/xen/include/xen/virtdev-uart.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef XEN__VIRTDEV_UART_H
> +#define XEN__VIRTDEV_UART_H
> +
> +#include <public/xen.h>
> +#include <public/event_channel.h>
> +#include <xen/types.h>
> +
> +struct virtdev_uart_params {
> +    domid_t console_domid;
> +    gfn_t gfn;
> +    evtchn_port_t evtchn;
> +};
> +
> +struct virtdev_uart {
> +    int (*putchar)(struct domain *d, char c);
> +    int (*init)(struct domain *d, struct virtdev_uart_params *params);
> +    void (*exit)(struct domain *d);
> +    void (*dump)(struct domain *d);
> +};
> +
> +#define VIRTDEV_UART_REGISTER(x) \
> +    static const struct virtdev_uart *x##_entry \
> +           __used_section(".data.virtdev.uart") = \
> +    &(const struct virtdev_uart){ \
> +        .init    = x ## _init, \
> +        .exit    = x ## _exit, \
> +        .dump    = x ## _dump, \
> +        .putchar = x ## _putchar, \
> +    }

Why the extra level of indirection? Can't the section consist of instances
of struct virtdev_uart rather than pointers to such?

> +#ifdef CONFIG_HAS_VUART
> +
> +int virtdev_uart_putchar(struct domain *d, char c);
> +int virtdev_uart_init(struct domain *d, struct virtdev_uart_params *params);
> +void virtdev_uart_exit(struct domain *d);
> +void virtdev_uart_dump(struct domain *d);
> +
> +#else
> +
> +static inline int virtdev_uart_putchar(struct domain *d, char c)
> +{
> +    ASSERT_UNREACHABLE();
> +    return -ENODEV;
> +}
> +
> +static inline int virtdev_uart_init(struct domain *d,
> +                                    struct virtdev_uart_params *params)
> +{
> +    return 0;
> +}
> +
> +static inline void virtdev_uart_exit(struct domain *d)
> +{
> +}
> +
> +static inline void virtdev_uart_dump(struct domain *d)
> +{
> +}

Are all of these stubs really needed? The sole putchar call site suggests
the stub isn't needed (as the call will be DCE'd by the compiler). The
dump invocation likely also wants guarding by a domain_has_vuart() check.
Perhaps also the exit one.

At least for the dump hook it would also be quite desirable if it could
have a pointer-to-const parameter.

Jan