xen/riscv: add initialization support for virtual SBI UART (vSBI UART)

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

Posted by Oleksii Kurochko 7 months, 1 week ago

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

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

Posted by dmkhn@proton.me 7 months ago

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

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

Posted by Oleksii Kurochko 7 months ago

Hi Denis,

On 5/16/25 9:50 PM, dmkhn@proton.me wrote:
> 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.

Agree, it makes sense. But If it will take a lot of time to upstream/merge then I prefer this patch
go first to not block RISC-V upstreaming.

Anyway, I will look at your changes tomorrow.

Thanks.

~ Oleksii

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

Posted by Jan Beulich 7 months, 1 week ago

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

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

Posted by Oleksii Kurochko 7 months ago

On 5/15/25 12:08 PM, Jan Beulich wrote:
> 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?

Missed that. I have to understand why Arm uses *.init.o, but likely I should do the same for RISC-V.

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

Good point.
I think that it is possible to leave without UART. At this stage,
of development I need UART for debug, so automatically the panic() was added.
But I agree, it should be dropped.

>> +    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?

Probably not, it was just added to not forget to update domain_vsbi_uart_deinit().

Thanks.

~ Oleksii

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

Posted by Jan Beulich 7 months ago

On 21.05.2025 13:40, Oleksii Kurochko wrote:
> On 5/15/25 12:08 PM, Jan Beulich wrote:
>> 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?
> 
> Missed that. I have to understand why Arm uses *.init.o, but likely I should do the same for RISC-V.

There's likely nothing Arm-specific in this. When all functions in a file
are __init and all data is __initdata / __initconst, we prefer to move
the remaining data (string literals being the most prominent example) to
.init.* as well. That's (basically) what the *.o -> *.init.o build step
does.

Jan