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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.05.2025 17:55, Oleksii Kurochko wrote:
> --- 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

Arm uses

obj-$(CONFIG_DOM0LESS_BOOT) += dom0less-build.init.o

Why the two differences?

> --- /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__);

Is this a reason to panic()? Isn't it possible for domains to be fine
without any UART?

> --- /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

Pointless "else" after "goto".

> +    {
> +        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:

Nit (you know what, I suppose).

> +    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__);

Is this relevant in a de-init function?

Jan