Re: [PATCH v1] xen/riscv: add initialization support for virtual SBI UART (vSBI UART)

[dmkhn@proton.me]

Hi Oleksii,

On Mon, May 12, 2025 at 05:55:21PM +0200, Oleksii Kurochko wrote:
> This is the first step toward supporting a vSBI UART.
> 
> The implementation checks for the presence of the "vsbi_uart" property
> in the device tree. If present, the vSBI UART is initialized by:
> - Allocating a structure that holds Xen console rings and character
>   buffers.
> - Initializing the vSBI UART spinlock.
> 
> This commit introduces the following:
> - domain_vsbi_uart_init() and domain_vsbi_uart_deinit() functions.
> - A new arch_kernel_info structure with a vsbi_uart member.
> - A vsbi_uart structure to hold information related to the vSBI
>   driver, including:
>   - Whether the vSBI UART backend is in the domain or in Xen.
>   - If the backend is in Xen: details such as ring buffer, ring page,
>     Xen console ring indexes, and character buffers.
>   - A spinlock for synchronization.
> 
> Also, introduce init_vuart() which is going to be called by dom0less
> generic code during guest domain construction.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

JFYI, I started to move all virtual UARTs under drivers/vuart directory
and introducing a framework for hooking vUARTs into console driver.

pl011 emulator cleanup
  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/3c635962a349afed75f47cd2559a4160ffa41106

original 'vuart' for hwdom cleanup
  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/405c86cbd6d55f5737dc9ccf9b8a8f370767e3f0

move pl011 to drivers/vuart
  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/4b5cdff118a2795278dfcc2c1b60423b46e85f27

move 'vuart' for hwdom cleanup to drivers/vuart
  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/d76c17b8056c1d500afd854a513403fc3774da51

which is followed by vUART driver framework introduction (not posted):
  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/ebc7e83650e5e3f68e5d734e5c475c6bcde626fa

These patches ^^ are not posted, since I do already have enough patches on
the mailing list which are in progress.

I did this work along w/ NS16550 emulator on x86.

IMO, it is worth delivering those patches first and then integrate SBI UART.

> ---
> This patch is independent from other RISC-V connected patch series, but it could
> be a merge conflict with some patches of other patch series.
> ---
>  xen/arch/riscv/Makefile                |  2 +
>  xen/arch/riscv/dom0less-build.c        | 30 +++++++++++++
>  xen/arch/riscv/include/asm/domain.h    |  4 ++
>  xen/arch/riscv/include/asm/kernel.h    | 21 +++++++++
>  xen/arch/riscv/include/asm/vsbi-uart.h | 58 ++++++++++++++++++++++++
>  xen/arch/riscv/vsbi-uart.c             | 62 ++++++++++++++++++++++++++
>  6 files changed, 177 insertions(+)
>  create mode 100644 xen/arch/riscv/dom0less-build.c
>  create mode 100644 xen/arch/riscv/include/asm/kernel.h
>  create mode 100644 xen/arch/riscv/include/asm/vsbi-uart.h
>  create mode 100644 xen/arch/riscv/vsbi-uart.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index d882c57528..89a1897228 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>  obj-y += aplic.o
>  obj-y += cpufeature.o
> +obj-y += dom0less-build.o
>  obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-y += entry.o
>  obj-y += intc.o
> @@ -14,6 +15,7 @@ obj-y += stubs.o
>  obj-y += time.o
>  obj-y += traps.o
>  obj-y += vm_event.o
> +obj-y += vsbi-uart.o
> 
>  $(TARGET): $(TARGET)-syms
>  	$(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/dom0less-build.c b/xen/arch/riscv/dom0less-build.c
> new file mode 100644
> index 0000000000..afce2f606d
> --- /dev/null
> +++ b/xen/arch/riscv/dom0less-build.c
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/bug.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/fdt-kernel.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +
> +#include <asm/vsbi-uart.h>
> +
> +int __init init_vuart(struct domain *d, struct kernel_info *kinfo,
> +                      const struct dt_device_node *node)
> +{
> +    int rc = -EINVAL;
> +
> +    kinfo->arch.vsbi_uart = dt_property_read_bool(node, "vsbi_uart");
> +
> +    if ( kinfo->arch.vsbi_uart )
> +    {
> +        rc = domain_vsbi_uart_init(d, NULL);
> +        if ( rc < 0 )
> +            return rc;
> +    }
> +
> +    if ( rc )
> +        panic("%s: what a domain should use as an UART?\n", __func__);
> +
> +    return rc;
> +}
> diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
> index c3d965a559..c312827d18 100644
> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -5,6 +5,8 @@
>  #include <xen/xmalloc.h>
>  #include <public/hvm/params.h>
> 
> +#include <asm/vsbi-uart.h>
> +
>  struct hvm_domain
>  {
>      uint64_t              params[HVM_NR_PARAMS];
> @@ -18,6 +20,8 @@ struct arch_vcpu {
> 
>  struct arch_domain {
>      struct hvm_domain hvm;
> +
> +    struct vsbi_uart vsbi_uart;
>  };
> 
>  #include <xen/sched.h>
> diff --git a/xen/arch/riscv/include/asm/kernel.h b/xen/arch/riscv/include/asm/kernel.h
> new file mode 100644
> index 0000000000..15c13da158
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/kernel.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef ASM__RISCV__KERNEL_H
> +#define ASM__RISCV__KERNEL_H
> +
> +struct arch_kernel_info
> +{
> +    /* Enable vsbi_uart emulation */
> +    bool vsbi_uart;
> +};
> +
> +#endif /* #ifdef ASM__RISCV__KERNEL_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/riscv/include/asm/vsbi-uart.h b/xen/arch/riscv/include/asm/vsbi-uart.h
> new file mode 100644
> index 0000000000..144e3191ff
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/vsbi-uart.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef ASM__RISCV__VSBI_UART_H
> +#define ASM__RISCV__VSBI_UART_H
> +
> +#include <xen/spinlock.h>
> +#include <xen/stdbool.h>
> +
> +#include <public/io/console.h>
> +
> +struct domain;
> +struct page_info;
> +
> +#define VSBI_UART_FIFO_SIZE 32
> +#define VSBI_UART_OUT_BUF_SIZE 128
> +
> +struct vsbi_uart_xen_backend {
> +    char in[VSBI_UART_FIFO_SIZE];
> +    char out[VSBI_UART_OUT_BUF_SIZE];
> +    XENCONS_RING_IDX in_cons, in_prod;
> +    XENCONS_RING_IDX out_prod;
> +};
> +
> +struct vsbi_uart {
> +    bool backend_in_domain;
> +    union {
> +        struct {
> +            void *ring_buf;
> +            struct page_info *ring_page;
> +        } dom;
> +        struct vsbi_uart_xen_backend *xen;
> +    } backend;
> +
> +    spinlock_t lock;
> +};
> +
> +/*
> + * TODO: we don't support an option that backend is in a domain.
> + *       At the moment, backend is ib Xen.
> + *       This structure should be implemented in the case if backend
> + *       is in a domain.
> + */
> +struct vsbi_uart_init_info {
> +};
> +
> +int domain_vsbi_uart_init(struct domain *d , struct vsbi_uart_init_info *info);
> +void domain_vsbi_uart_deinit(struct domain *d);
> +
> +#endif /* ASM__RISCV__VSBI_UART_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/riscv/vsbi-uart.c b/xen/arch/riscv/vsbi-uart.c
> new file mode 100644
> index 0000000000..5fe617ae30
> --- /dev/null
> +++ b/xen/arch/riscv/vsbi-uart.c
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/sched.h>
> +#include <xen/xmalloc.h>
> +
> +#include <asm/vsbi-uart.h>
> +
> +int domain_vsbi_uart_init(struct domain *d, struct vsbi_uart_init_info *info)
> +{
> +    int rc;
> +    struct vsbi_uart *vsbi_uart = &d->arch.vsbi_uart;
> +
> +    if ( vsbi_uart->backend.dom.ring_buf )
> +    {
> +        printk("%s: ring_buf != 0\n", __func__);
> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * info is NULL when the backend is in Xen.
> +     * info is != NULL when the backend is in a domain.
> +     */
> +    if ( info != NULL )
> +    {
> +        printk("%s: vsbi_uart backend in a domain isn't supported\n", __func__);
> +        rc = -EOPNOTSUPP;
> +        goto out;
> +    }
> +    else
> +    {
> +        vsbi_uart->backend_in_domain = false;
> +
> +        vsbi_uart->backend.xen = xzalloc(struct vsbi_uart_xen_backend);
> +        if ( vsbi_uart->backend.xen == NULL )
> +        {
> +            rc = -ENOMEM;
> +            goto out;
> +        }
> +    }
> +
> +    spin_lock_init(&vsbi_uart->lock);
> +
> +    return 0;
> +
> +out:
> +    domain_vsbi_uart_deinit(d);
> +
> +    return rc;
> +}
> +
> +void domain_vsbi_uart_deinit(struct domain *d)
> +{
> +    struct vsbi_uart *vsbi_uart = &d->arch.vsbi_uart;
> +
> +    if ( vsbi_uart->backend_in_domain )
> +        printk("%s: backed in a domain isn't supported\n", __func__);
> +    else
> +        XFREE(vsbi_uart->backend.xen);
> +}
> --
> 2.49.0
> 
> 

Thanks,
Denis