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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-01-03 20: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.
>
> Note, arm/vuart.c emulator is not hooked to virtdev-uart framework because the
> emulator is limited to the hardware domains only and was not designed to own
> the physical console input. Updated arm/vuart.c APIs to have 'hwdom_' prefix
> instead of generic 'domain_' to avoid possible confusion.
It might be good to renmae xen/arch/arm/vuart.c to 
xen/arch/arm/hwdom-vuart.c and then use just the shorter vuart prefix 
instead virtdev_uart.
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
> index 
> 20050e9bb8b32bd16c2da76c2c3e0f68dab89394..355719c3af67683c153a4f7a35dad4944992846e
>  100644
> --- a/xen/drivers/Kconfig
> +++ b/xen/drivers/Kconfig
> @@ -19,4 +19,9 @@ config HAS_VPCI_GUEST_SUPPORT
>         bool
>         select HAS_VPCI
>   
> +config HAS_VUART
> +       bool "UART emulation framework"
> +       help
> +         This selects UART emulation framework.
This can be hidden, so just:

config HAS_VUART
        bool

> +
>   endmenu
> +int virtdev_uart_init(struct domain *d, struct virtdev_uart_params *params)
> +{
> +    int rc;
> +
> +    ASSERT(__start_virtdev_uart);
> +
> +    rc = __start_virtdev_uart->init(d, params);
> +    if ( rc )
> +        return rc;
> +
> +#if !defined(__i386__) && !defined(__x86_64__)
> +    d->arch.emulation_flags |= VIRTDEV_UART;
> +#endif
> +
> +    return 0;
> +}
> +
> +void virtdev_uart_exit(struct domain *d)
> +{
> +    ASSERT(__start_virtdev_uart);
> +
> +    __start_virtdev_uart->exit(d);
> +
> +#if !defined(__i386__) && !defined(__x86_64__)
> +    d->arch.emulation_flags &= ~VIRTDEV_UART;
> +#endif
I think this is only called at domain teardown, so you don't need to 
clear flags.
> +}
> +
> +int virtdev_uart_putchar(struct domain *d, char c)
> +{
> +    ASSERT(__start_virtdev_uart);
> +    ASSERT(d->arch.emulation_flags & VIRTDEV_UART);
> +
> +    return __start_virtdev_uart->putchar(d, c);
> +}
> +
> +void virtdev_uart_dump(struct domain *d)
> +{
> +    ASSERT(__start_virtdev_uart);
> +
> +    __start_virtdev_uart->dump(d);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/virtdev-uart.h b/xen/include/xen/virtdev-uart.h
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..fbe48e513996404d793d011747b3f40c236a6a57
> --- /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, \
> +    }
Are multiple types of uarts for a given architecture expected?  If only 
a single implemention is needed per-architecture, using defines or 
wrappers seems simpler to me.
Regards,
Jason