[PATCH v1 0/3] RISC-V: Introduce vSBI framework

[PATCH v1 0/3] RISC-V: Introduce vSBI framework

[Oleksii Kurochko]

Introduce the vSBI framework to handle extension ecall instruction calls
from a guest.

In this patch series, support is added for only a few extensions and FIDs,
just enough to boot the first guest domains and obtain logs from them. This
keeps the patch series independent from other ongoing work.

It was decided to start with support for the Legacy Extension, as it is still
supported by Linux (so we may need it anyway), and because some of its FIDs
require less functionality to implement at this stage compared to the more
modern extensions.

Oleksii Kurochko (3):
  xen/riscv: introduce vSBI extension framework
  xen/riscv: add RISC-V legacy SBI extension support for guests
  xen/riscv: add RISC-V virtual SBI base extension support for guests

 xen/arch/riscv/Makefile                     |  1 +
 xen/arch/riscv/include/asm/processor.h      |  1 +
 xen/arch/riscv/include/asm/sbi.h            | 14 +++++-
 xen/arch/riscv/include/asm/vsbi.h           | 31 ++++++++++++
 xen/arch/riscv/sbi.c                        |  8 ++--
 xen/arch/riscv/traps.c                      |  8 ++++
 xen/arch/riscv/vsbi/Makefile                |  3 ++
 xen/arch/riscv/vsbi/vsbi-base-extension.c   | 52 +++++++++++++++++++++
 xen/arch/riscv/vsbi/vsbi-legacy-extension.c | 37 +++++++++++++++
 xen/arch/riscv/vsbi/vsbi.c                  | 46 ++++++++++++++++++
 xen/arch/riscv/xen.lds.S                    |  7 +++
 11 files changed, 203 insertions(+), 5 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/vsbi.h
 create mode 100644 xen/arch/riscv/vsbi/Makefile
 create mode 100644 xen/arch/riscv/vsbi/vsbi-base-extension.c
 create mode 100644 xen/arch/riscv/vsbi/vsbi-legacy-extension.c
 create mode 100644 xen/arch/riscv/vsbi/vsbi.c

-- 
2.52.0

[PATCH v1 1/3] xen/riscv: introduce vSBI extension framework

[Oleksii Kurochko]

This commit introduces support for handling virtual SBI extensions in Xen.

The changes include:
- Added new vsbi.c and vsbi.h files to implement virtual SBI extension
  handling.
- Modified traps.c to handle CAUSE_VIRTUAL_SUPERVISOR_ECALL by calling
  vsbi_handle_ecall() when the trap originates from VS-mode.
- Updated xen.lds.S to include a new .vsbi.exts section for virtual SBI
  extension data.
- Updated Makefile to include the new vsbi/ directory in the build.
- Add hstatus register to struct cpu_user_regs as it is needed for
  a check that CAUSE_VIRTUAL_SUPERVISOR_ECALL happens from VS-mode.

The implementation allows for registration and handling of SBI
extensions via a new vsbi_ext structure and ".vsbi.exts" section,
enabling extensible virtual SBI support for RISC-V guests.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Makefile                |  1 +
 xen/arch/riscv/include/asm/processor.h |  1 +
 xen/arch/riscv/include/asm/vsbi.h      | 31 +++++++++++++++++
 xen/arch/riscv/traps.c                 |  8 +++++
 xen/arch/riscv/vsbi/Makefile           |  1 +
 xen/arch/riscv/vsbi/vsbi.c             | 46 ++++++++++++++++++++++++++
 xen/arch/riscv/xen.lds.S               |  7 ++++
 7 files changed, 95 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/vsbi.h
 create mode 100644 xen/arch/riscv/vsbi/Makefile
 create mode 100644 xen/arch/riscv/vsbi/vsbi.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index e2b8aa42c8..7bfe7024ef 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -17,6 +17,7 @@ obj-y += stubs.o
 obj-y += time.o
 obj-y += traps.o
 obj-y += vm_event.o
+obj-y += vsbi/
 
 $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
index 39696fb58d..79d02c3dd2 100644
--- a/xen/arch/riscv/include/asm/processor.h
+++ b/xen/arch/riscv/include/asm/processor.h
@@ -49,6 +49,7 @@ struct cpu_user_regs
     unsigned long t6;
     unsigned long sepc;
     unsigned long sstatus;
+    unsigned long hstatus;
     /* pointer to previous stack_cpu_regs */
     unsigned long pregs;
 };
diff --git a/xen/arch/riscv/include/asm/vsbi.h b/xen/arch/riscv/include/asm/vsbi.h
new file mode 100644
index 0000000000..984e7acf7b
--- /dev/null
+++ b/xen/arch/riscv/include/asm/vsbi.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier:  GPL-2.0-only */
+
+#ifndef ASM_RISCV_VSBI_H
+#define ASM_RISCV_VSBI_H
+
+struct regs;
+struct vcpu;
+
+struct vsbi_ext {
+    const char *name;
+    unsigned long eid_start;
+    unsigned long eid_end;
+    int (*handle)(struct vcpu *vcpu, unsigned long eid,
+                  unsigned long fid, struct cpu_user_regs *regs);
+};
+
+#define VSBI_EXT_START(ext, extid_start, extid_end, extid_handle)   \
+static const struct vsbi_ext vsbi_ext_##ext __used                  \
+__section(".vsbi.exts") = {                                         \
+    .name = #ext,                                                   \
+    .eid_start = extid_start,                                       \
+    .eid_end = extid_end,                                           \
+    .handle = extid_handle,
+
+#define VSBI_EXT_END                                                \
+};
+
+void vsbi_handle_ecall(struct vcpu *vcpu, struct cpu_user_regs *regs);
+const struct vsbi_ext *vsbi_find_extension(unsigned long ext_id);
+
+#endif
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index f061004d83..dfe1a5a112 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -15,6 +15,7 @@
 #include <asm/processor.h>
 #include <asm/riscv_encoding.h>
 #include <asm/traps.h>
+#include <asm/vsbi.h>
 
 /*
  * Initialize the trap handling.
@@ -114,6 +115,13 @@ void do_trap(struct cpu_user_regs *cpu_regs)
 
     switch ( cause )
     {
+    case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
+        if ( !(cpu_regs->hstatus & HSTATUS_SPV) )
+            panic("CAUSE_VIRTUAL_SUPERVISOR_ECALL came not from VS-mode\n");
+
+        vsbi_handle_ecall(current, cpu_regs);
+        break;
+
     case CAUSE_ILLEGAL_INSTRUCTION:
         if ( do_bug_frame(cpu_regs, pc) >= 0 )
         {
diff --git a/xen/arch/riscv/vsbi/Makefile b/xen/arch/riscv/vsbi/Makefile
new file mode 100644
index 0000000000..574c8ff78d
--- /dev/null
+++ b/xen/arch/riscv/vsbi/Makefile
@@ -0,0 +1 @@
+obj-y += vsbi.o
diff --git a/xen/arch/riscv/vsbi/vsbi.c b/xen/arch/riscv/vsbi/vsbi.c
new file mode 100644
index 0000000000..cd119ce0d6
--- /dev/null
+++ b/xen/arch/riscv/vsbi/vsbi.c
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/sched.h>
+
+#include <asm/processor.h>
+#include <asm/sbi.h>
+#include <asm/vsbi.h>
+
+extern const struct vsbi_ext _svsbi_exts[], _evsbi_exts[];
+
+const struct vsbi_ext *vsbi_find_extension(unsigned long ext_id)
+{
+    const struct vsbi_ext *vsbi_ext;
+
+    for ( vsbi_ext = _svsbi_exts; vsbi_ext != _evsbi_exts; vsbi_ext++ )
+        if ( ext_id >= vsbi_ext->eid_start &&
+             ext_id <= vsbi_ext->eid_end )
+            return vsbi_ext;
+
+    return NULL;
+}
+
+void vsbi_handle_ecall(struct vcpu *vcpu, struct cpu_user_regs *regs)
+{
+    const unsigned long eid = regs->a7;
+    const unsigned long fid = regs->a6;
+    const struct vsbi_ext *ext = vsbi_find_extension(eid);
+    int ret;
+
+    if ( ext && ext->handle )
+        ret = ext->handle(vcpu, eid, fid, regs);
+    else
+    {
+        printk("Unsupported Guest SBI EID #%#lx, FID #%lu\n", eid, regs->a1);
+        ret = SBI_ERR_NOT_SUPPORTED;
+    }
+
+    /*
+     * The ecall instruction is not part of the RISC-V C extension (compressed
+     * instructions), so it is always 4 bytes long. Therefore, it is safe to
+     * use a fixed length of 4 bytes instead of reading guest memory to
+     * determine the instruction length.
+     */
+    regs->sepc += 4;
+    regs->a0 = ret;
+}
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index edcadff90b..2967f00ac5 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -91,6 +91,13 @@ SECTIONS
 
     DT_DEV_INFO                       /* Devicetree based device info */
 
+    . = ALIGN(POINTER_ALIGN);
+    DECL_SECTION(.vsbi.exts) {
+        _svsbi_exts = .;
+        *(.vsbi.exts)
+        _evsbi_exts = .;
+    } :text
+
     . = ALIGN(PAGE_SIZE);             /* Init code and data */
     __init_begin = .;
     .init.text : {
-- 
2.52.0

Re: [PATCH v1 1/3] xen/riscv: introduce vSBI extension framework

[Jan Beulich]

On 01.12.2025 11:24, Oleksii Kurochko wrote:
> This commit introduces support for handling virtual SBI extensions in Xen.
> 
> The changes include:
> - Added new vsbi.c and vsbi.h files to implement virtual SBI extension
>   handling.
> - Modified traps.c to handle CAUSE_VIRTUAL_SUPERVISOR_ECALL by calling
>   vsbi_handle_ecall() when the trap originates from VS-mode.
> - Updated xen.lds.S to include a new .vsbi.exts section for virtual SBI
>   extension data.
> - Updated Makefile to include the new vsbi/ directory in the build.
> - Add hstatus register to struct cpu_user_regs as it is needed for
>   a check that CAUSE_VIRTUAL_SUPERVISOR_ECALL happens from VS-mode.

I can spot the check, yes, but without the field ever being set how is one
to determine whether that check actually makes sense?

> The implementation allows for registration and handling of SBI
> extensions via a new vsbi_ext structure and ".vsbi.exts" section,
> enabling extensible virtual SBI support for RISC-V guests.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/Makefile                |  1 +
>  xen/arch/riscv/include/asm/processor.h |  1 +
>  xen/arch/riscv/include/asm/vsbi.h      | 31 +++++++++++++++++
>  xen/arch/riscv/traps.c                 |  8 +++++
>  xen/arch/riscv/vsbi/Makefile           |  1 +
>  xen/arch/riscv/vsbi/vsbi.c             | 46 ++++++++++++++++++++++++++

A file named identical to the directory it lives in raises the question of
why there is such a new sub-directory. Are you expecting moree files to
appear there? How's vsbi.c then be "special" compared to the others? Do
you perhaps mean someling like "core.c" or "common.c" here?

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/vsbi.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier:  GPL-2.0-only */
> +
> +#ifndef ASM_RISCV_VSBI_H
> +#define ASM_RISCV_VSBI_H
> +
> +struct regs;

DYM struct cpu_user_regs?

> +struct vcpu;
> +
> +struct vsbi_ext {
> +    const char *name;
> +    unsigned long eid_start;
> +    unsigned long eid_end;
> +    int (*handle)(struct vcpu *vcpu, unsigned long eid,
> +                  unsigned long fid, struct cpu_user_regs *regs);

Nit: Maybe better "handler", as this isn't really a handle?

> +};
> +
> +#define VSBI_EXT_START(ext, extid_start, extid_end, extid_handle)   \

The extid_ prefixes aren't adding much value here, are they?

> +static const struct vsbi_ext vsbi_ext_##ext __used                  \
> +__section(".vsbi.exts") = {                                         \
> +    .name = #ext,                                                   \
> +    .eid_start = extid_start,                                       \
> +    .eid_end = extid_end,                                           \
> +    .handle = extid_handle,
> +
> +#define VSBI_EXT_END                                                \
> +};

There's no use here, and peeking ahead at the other two patches shows
no use where this odd split of the macros would be necessary. What is
this about?

> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -15,6 +15,7 @@
>  #include <asm/processor.h>
>  #include <asm/riscv_encoding.h>
>  #include <asm/traps.h>
> +#include <asm/vsbi.h>
>  
>  /*
>   * Initialize the trap handling.
> @@ -114,6 +115,13 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>  
>      switch ( cause )
>      {
> +    case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
> +        if ( !(cpu_regs->hstatus & HSTATUS_SPV) )
> +            panic("CAUSE_VIRTUAL_SUPERVISOR_ECALL came not from VS-mode\n");

This might more naturally want to be BUG_ON()? Assuming of course the value
in question is exclusively under hypervisor control. Otherwise panic() would
also be wrong to use here.

> --- /dev/null
> +++ b/xen/arch/riscv/vsbi/vsbi.c
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/sched.h>
> +
> +#include <asm/processor.h>
> +#include <asm/sbi.h>
> +#include <asm/vsbi.h>
> +
> +extern const struct vsbi_ext _svsbi_exts[], _evsbi_exts[];
> +
> +const struct vsbi_ext *vsbi_find_extension(unsigned long ext_id)

static?

Also, again - is the ext_ prefix adding any value here?

> +{
> +    const struct vsbi_ext *vsbi_ext;
> +
> +    for ( vsbi_ext = _svsbi_exts; vsbi_ext != _evsbi_exts; vsbi_ext++ )
> +        if ( ext_id >= vsbi_ext->eid_start &&
> +             ext_id <= vsbi_ext->eid_end )
> +            return vsbi_ext;

What if multiple entries have overlapping EID ranges?

Also at the macro definition site please clarify (by way of a comment)
that these ramnges are inclusive (especially at the upper end).

> +void vsbi_handle_ecall(struct vcpu *vcpu, struct cpu_user_regs *regs)
> +{
> +    const unsigned long eid = regs->a7;
> +    const unsigned long fid = regs->a6;
> +    const struct vsbi_ext *ext = vsbi_find_extension(eid);
> +    int ret;
> +
> +    if ( ext && ext->handle )
> +        ret = ext->handle(vcpu, eid, fid, regs);

Is a registration record NULL handler pointer actually legitimate / useful?
(If there was range overlap checking I could see a reason to permit such.)

> +    else
> +    {
> +        printk("Unsupported Guest SBI EID #%#lx, FID #%lu\n", eid, regs->a1);

Are the #-es ahead of the %-s adding value here? Is printing an ID as
decimal going to be useful, when the value is pretty much arbitrary?

> +        ret = SBI_ERR_NOT_SUPPORTED;
> +    }
> +
> +    /*
> +     * The ecall instruction is not part of the RISC-V C extension (compressed
> +     * instructions), so it is always 4 bytes long. Therefore, it is safe to
> +     * use a fixed length of 4 bytes instead of reading guest memory to
> +     * determine the instruction length.
> +     */

And ECALL is also the sole possible cause of CAUSE_VIRTUAL_SUPERVISOR_ECALL?

> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -91,6 +91,13 @@ SECTIONS
>  
>      DT_DEV_INFO                       /* Devicetree based device info */
>  
> +    . = ALIGN(POINTER_ALIGN);
> +    DECL_SECTION(.vsbi.exts) {
> +        _svsbi_exts = .;
> +        *(.vsbi.exts)
> +        _evsbi_exts = .;
> +    } :text

Isn't this read-only data? In which case it wants to move up ahead of _erodata?

Jan

Re: [PATCH v1 1/3] xen/riscv: introduce vSBI extension framework

[Oleksii Kurochko]

On 12/8/25 3:25 PM, Jan Beulich wrote:
> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>> This commit introduces support for handling virtual SBI extensions in Xen.
>>
>> The changes include:
>> - Added new vsbi.c and vsbi.h files to implement virtual SBI extension
>>    handling.
>> - Modified traps.c to handle CAUSE_VIRTUAL_SUPERVISOR_ECALL by calling
>>    vsbi_handle_ecall() when the trap originates from VS-mode.
>> - Updated xen.lds.S to include a new .vsbi.exts section for virtual SBI
>>    extension data.
>> - Updated Makefile to include the new vsbi/ directory in the build.
>> - Add hstatus register to struct cpu_user_regs as it is needed for
>>    a check that CAUSE_VIRTUAL_SUPERVISOR_ECALL happens from VS-mode.
> I can spot the check, yes, but without the field ever being set how is one
> to determine whether that check actually makes sense?

But hstatus is set automatically when a trap occurs and will be copied in
handle_trap() in entry.S. The HSTATUS_SPV bit in hstatus will be set only
when a trap originates from a guest, which is not the case now since we do not
have any guest running yet. This is why hstatus is not currently saved or
restored.

Probably, you meant that it would be better to introduce csr init function
now, something like:
static void vcpu_csr_init(struct vcpu *v)
{
     unsigned long hedeleg, hideleg, hstatus;

     hstatus = HSTATUS_SPV | HSTATUS_SPVP | HSTATUS_VTW;
     guest_regs(v)->hstatus = hstatus;
     
     ....
}
But it also make sense only for a guest, which isn't ran now.

If you think it is better to introduce saving and restoring of hstatus in
handle_trap() now, or instead drop the handling of
“case CAUSE_VIRTUAL_SUPERVISOR_ECALL:” in do_trap(), please let me know.

>
>> The implementation allows for registration and handling of SBI
>> extensions via a new vsbi_ext structure and ".vsbi.exts" section,
>> enabling extensible virtual SBI support for RISC-V guests.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>>   xen/arch/riscv/Makefile                |  1 +
>>   xen/arch/riscv/include/asm/processor.h |  1 +
>>   xen/arch/riscv/include/asm/vsbi.h      | 31 +++++++++++++++++
>>   xen/arch/riscv/traps.c                 |  8 +++++
>>   xen/arch/riscv/vsbi/Makefile           |  1 +
>>   xen/arch/riscv/vsbi/vsbi.c             | 46 ++++++++++++++++++++++++++
> A file named identical to the directory it lives in raises the question of
> why there is such a new sub-directory. Are you expecting moree files to
> appear there?

Yes, I'm expecting that and it is done in the next patches of this patch
series.

>   How's vsbi.c then be "special" compared to the others? Do
> you perhaps mean someling like "core.c" or "common.c" here?

Agree, this is more appropriate for either “core.c” or “common.c”. Both options
are fine with me. I slightly prefer using the prefix “vsbi-{core/common}.c”, but
if you think it is better to omit the prefix since the folder name already
provides that context, I’m fine with dropping it.

>
>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/vsbi.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier:  GPL-2.0-only */
>> +
>> +#ifndef ASM_RISCV_VSBI_H
>> +#define ASM_RISCV_VSBI_H
>> +
>> +struct regs;
> DYM struct cpu_user_regs?

Should be struct cpu_user_regs.

>
>> +struct vcpu;
>> +
>> +struct vsbi_ext {
>> +    const char *name;
>> +    unsigned long eid_start;
>> +    unsigned long eid_end;
>> +    int (*handle)(struct vcpu *vcpu, unsigned long eid,
>> +                  unsigned long fid, struct cpu_user_regs *regs);
> Nit: Maybe better "handler", as this isn't really a handle?

It could be handler, I am okay with that.

>
>> +};
>> +
>> +#define VSBI_EXT_START(ext, extid_start, extid_end, extid_handle)   \
> The extid_ prefixes aren't adding much value here, are they?

Agree, not to much sense to have extid_ prefix here, lets drop it.

>
>> +static const struct vsbi_ext vsbi_ext_##ext __used                  \
>> +__section(".vsbi.exts") = {                                         \
>> +    .name = #ext,                                                   \
>> +    .eid_start = extid_start,                                       \
>> +    .eid_end = extid_end,                                           \
>> +    .handle = extid_handle,
>> +
>> +#define VSBI_EXT_END                                                \
>> +};
> There's no use here, and peeking ahead at the other two patches shows
> no use where this odd split of the macros would be necessary. What is
> this about?

I thought this was the common approach, similar to DT_DEVICE, where we have
DT_DEVICE_START and DT_DEVICE_END. There may be no need for it right now, but
perhaps we will eventually want similar behavior for VSBI_EXT_START.

If you think it is better to drop VSBI_EXT_END for now, I’m okay with that,
and can just use VSBI_EXT instead of VSBI_EXT_START.

>
>> --- a/xen/arch/riscv/traps.c
>> +++ b/xen/arch/riscv/traps.c
>> @@ -15,6 +15,7 @@
>>   #include <asm/processor.h>
>>   #include <asm/riscv_encoding.h>
>>   #include <asm/traps.h>
>> +#include <asm/vsbi.h>
>>   
>>   /*
>>    * Initialize the trap handling.
>> @@ -114,6 +115,13 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>   
>>       switch ( cause )
>>       {
>> +    case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
>> +        if ( !(cpu_regs->hstatus & HSTATUS_SPV) )
>> +            panic("CAUSE_VIRTUAL_SUPERVISOR_ECALL came not from VS-mode\n");
> This might more naturally want to be BUG_ON()? Assuming of course the value
> in question is exclusively under hypervisor control. Otherwise panic() would
> also be wrong to use here.

Only hypervisor can access ->hstatus (of course, hart is changing it when a trap
happens, for example).
BUG_ON() is a good option for me.

>
>> --- /dev/null
>> +++ b/xen/arch/riscv/vsbi/vsbi.c
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#include <xen/sched.h>
>> +
>> +#include <asm/processor.h>
>> +#include <asm/sbi.h>
>> +#include <asm/vsbi.h>
>> +
>> +extern const struct vsbi_ext _svsbi_exts[], _evsbi_exts[];
>> +
>> +const struct vsbi_ext *vsbi_find_extension(unsigned long ext_id)
> static?

It could be use not in vsbi.c (for example, in the next patches it is used for
SBI_EXT_BASE_PROBE_EXT), so it shouldn't be static.

>
> Also, again - is the ext_ prefix adding any value here?

Not really, I guess.

>
>> +{
>> +    const struct vsbi_ext *vsbi_ext;
>> +
>> +    for ( vsbi_ext = _svsbi_exts; vsbi_ext != _evsbi_exts; vsbi_ext++ )
>> +        if ( ext_id >= vsbi_ext->eid_start &&
>> +             ext_id <= vsbi_ext->eid_end )
>> +            return vsbi_ext;
> What if multiple entries have overlapping EID ranges?

Good question, I wasn't able to find that EID is always unique in SBI spec,
but, at the same time, if to look at all available extensions and their id(s),
they are always unique, so I expect that they will be always unique, otherwise,
it won't be possible which extension should be used.

>
> Also at the macro definition site please clarify (by way of a comment)
> that these ramnges are inclusive (especially at the upper end).

I will add the following above VSBI_EXT_START:
   /* Ranges (start and end) are inclusive within an extension */

>
>> +void vsbi_handle_ecall(struct vcpu *vcpu, struct cpu_user_regs *regs)
>> +{
>> +    const unsigned long eid = regs->a7;
>> +    const unsigned long fid = regs->a6;
>> +    const struct vsbi_ext *ext = vsbi_find_extension(eid);
>> +    int ret;
>> +
>> +    if ( ext && ext->handle )
>> +        ret = ext->handle(vcpu, eid, fid, regs);
> Is a registration record NULL handler pointer actually legitimate / useful?
> (If there was range overlap checking I could see a reason to permit such.)

it is a good question, I think ext->handle = NULL should be impossible. At
least, at the moment I can't come up with the case where it is possible and
what will be a use case. I will drop ext->handle check.

>
>> +    else
>> +    {
>> +        printk("Unsupported Guest SBI EID #%#lx, FID #%lu\n", eid, regs->a1);
> Are the #-es ahead of the %-s adding value here?

It is how SBI spec writes them. For example,
  9. Hart State Management Extension (EID #0x48534D "HSM") . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 26
  9.1. Function: Hart start (FID #0) . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 27

So I thought that it would help to find stuff faster.

>   Is printing an ID as
> decimal going to be useful, when the value is pretty much arbitrary?

It seems like FID (in comparison with EID) is always in sequence and
start from 0, but I don't see that SBI spec guarantees that.

But in the same side for old extension they used hexdecimal for FID, but
again it is in sequence:
5. Legacy Extensions (EIDs #0x00 - #0x0F) . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 16
5.1. Extension: Set Timer (EID #0x00) . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 16
5.2. Extension: Console Putchar (EID #0x01) . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 16

>
>> +        ret = SBI_ERR_NOT_SUPPORTED;
>> +    }
>> +
>> +    /*
>> +     * The ecall instruction is not part of the RISC-V C extension (compressed
>> +     * instructions), so it is always 4 bytes long. Therefore, it is safe to
>> +     * use a fixed length of 4 bytes instead of reading guest memory to
>> +     * determine the instruction length.
>> +     */
> And ECALL is also the sole possible cause of CAUSE_VIRTUAL_SUPERVISOR_ECALL?

I think yes, in Trap Cause Codes paragraph in RISC-V spec is mentioned the following:
   Furthermore, environment calls from VS-mode are assigned cause 10,
   whereas those from HS-mode or S-mode use cause 9 as usual.

So it is explicitly tells that environemt calls, so ECALL.

>
>> --- a/xen/arch/riscv/xen.lds.S
>> +++ b/xen/arch/riscv/xen.lds.S
>> @@ -91,6 +91,13 @@ SECTIONS
>>   
>>       DT_DEV_INFO                       /* Devicetree based device info */
>>   
>> +    . = ALIGN(POINTER_ALIGN);
>> +    DECL_SECTION(.vsbi.exts) {
>> +        _svsbi_exts = .;
>> +        *(.vsbi.exts)
>> +        _evsbi_exts = .;
>> +    } :text
> Isn't this read-only data? In which case it wants to move up ahead of _erodata?

It is. I will move it to recommended place.

Thanks.

~ Oleksii

Re: [PATCH v1 1/3] xen/riscv: introduce vSBI extension framework

[Jan Beulich]

On 10.12.2025 18:03, Oleksii Kurochko wrote:
> On 12/8/25 3:25 PM, Jan Beulich wrote:
>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>> This commit introduces support for handling virtual SBI extensions in Xen.
>>>
>>> The changes include:
>>> - Added new vsbi.c and vsbi.h files to implement virtual SBI extension
>>>    handling.
>>> - Modified traps.c to handle CAUSE_VIRTUAL_SUPERVISOR_ECALL by calling
>>>    vsbi_handle_ecall() when the trap originates from VS-mode.
>>> - Updated xen.lds.S to include a new .vsbi.exts section for virtual SBI
>>>    extension data.
>>> - Updated Makefile to include the new vsbi/ directory in the build.
>>> - Add hstatus register to struct cpu_user_regs as it is needed for
>>>    a check that CAUSE_VIRTUAL_SUPERVISOR_ECALL happens from VS-mode.
>> I can spot the check, yes, but without the field ever being set how is one
>> to determine whether that check actually makes sense?
> 
> But hstatus is set automatically when a trap occurs and will be copied in
> handle_trap() in entry.S.

Just that entry.S isn't even touched by this series. Did you perhaps omit an
important part of the change?

> If you think it is better to introduce saving and restoring of hstatus in
> handle_trap() now, or instead drop the handling of
> “case CAUSE_VIRTUAL_SUPERVISOR_ECALL:” in do_trap(), please let me know.

I think what I said above is quite clear: When you introduce a field that's
supposed to be filled upon entry to the hypervisor, the entry code wants
updating accordingly.

>>> ---
>>>   xen/arch/riscv/Makefile                |  1 +
>>>   xen/arch/riscv/include/asm/processor.h |  1 +
>>>   xen/arch/riscv/include/asm/vsbi.h      | 31 +++++++++++++++++
>>>   xen/arch/riscv/traps.c                 |  8 +++++
>>>   xen/arch/riscv/vsbi/Makefile           |  1 +
>>>   xen/arch/riscv/vsbi/vsbi.c             | 46 ++++++++++++++++++++++++++
>> A file named identical to the directory it lives in raises the question of
>> why there is such a new sub-directory. Are you expecting moree files to
>> appear there?
> 
> Yes, I'm expecting that and it is done in the next patches of this patch
> series.
> 
>>   How's vsbi.c then be "special" compared to the others? Do
>> you perhaps mean someling like "core.c" or "common.c" here?
> 
> Agree, this is more appropriate for either “core.c” or “common.c”. Both options
> are fine with me. I slightly prefer using the prefix “vsbi-{core/common}.c”, but
> if you think it is better to omit the prefix since the folder name already
> provides that context, I’m fine with dropping it.

Yes, I'm actually quite heavily opposed to such redundant prefixes. They obfuscate
things, and they get in the way of name completion features in shells and alike.

>>> +static const struct vsbi_ext vsbi_ext_##ext __used                  \
>>> +__section(".vsbi.exts") = {                                         \
>>> +    .name = #ext,                                                   \
>>> +    .eid_start = extid_start,                                       \
>>> +    .eid_end = extid_end,                                           \
>>> +    .handle = extid_handle,
>>> +
>>> +#define VSBI_EXT_END                                                \
>>> +};
>> There's no use here, and peeking ahead at the other two patches shows
>> no use where this odd split of the macros would be necessary. What is
>> this about?
> 
> I thought this was the common approach, similar to DT_DEVICE, where we have
> DT_DEVICE_START and DT_DEVICE_END. There may be no need for it right now, but
> perhaps we will eventually want similar behavior for VSBI_EXT_START.

For DT_DEVICE_{START,END} there at least is a reason to have a split like
this. (I would very much like for that to be done without such a split, though.)

> If you think it is better to drop VSBI_EXT_END for now, I’m okay with that,
> and can just use VSBI_EXT instead of VSBI_EXT_START.

Yes please. If and when the need arises, it can be introduced, or (as per above)
a better solution be found.

>>> --- a/xen/arch/riscv/traps.c
>>> +++ b/xen/arch/riscv/traps.c
>>> @@ -15,6 +15,7 @@
>>>   #include <asm/processor.h>
>>>   #include <asm/riscv_encoding.h>
>>>   #include <asm/traps.h>
>>> +#include <asm/vsbi.h>
>>>   
>>>   /*
>>>    * Initialize the trap handling.
>>> @@ -114,6 +115,13 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>>   
>>>       switch ( cause )
>>>       {
>>> +    case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
>>> +        if ( !(cpu_regs->hstatus & HSTATUS_SPV) )
>>> +            panic("CAUSE_VIRTUAL_SUPERVISOR_ECALL came not from VS-mode\n");
>> This might more naturally want to be BUG_ON()? Assuming of course the value
>> in question is exclusively under hypervisor control. Otherwise panic() would
>> also be wrong to use here.
> 
> Only hypervisor can access ->hstatus (of course, hart is changing it when a trap
> happens, for example).
> BUG_ON() is a good option for me.

Just to clarify: "can access" != "under control". There's also the possibility
that a guest could do something causing the hardware to raise a
CAUSE_VIRTUAL_SUPERVISOR_ECALL trap without setting HSTATUS_SPV. That was the
underlying question here.

>>> --- /dev/null
>>> +++ b/xen/arch/riscv/vsbi/vsbi.c
>>> @@ -0,0 +1,46 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#include <xen/sched.h>
>>> +
>>> +#include <asm/processor.h>
>>> +#include <asm/sbi.h>
>>> +#include <asm/vsbi.h>
>>> +
>>> +extern const struct vsbi_ext _svsbi_exts[], _evsbi_exts[];
>>> +
>>> +const struct vsbi_ext *vsbi_find_extension(unsigned long ext_id)
>> static?
> 
> It could be use not in vsbi.c (for example, in the next patches it is used for
> SBI_EXT_BASE_PROBE_EXT), so it shouldn't be static.

Okay. In RISC-V that's okay as long as it's not subject to Misra scanning. Yet
still introducing a non-static function without callers from other CUs may
warrant a remark in the description. Once RISC-V becomes subject to Misra scans,
such will be problematic, after all.

>> Also, again - is the ext_ prefix adding any value here?
> 
> Not really, I guess.

Maybe, to still distinguish from "fid", use "eid" here then?

>>> +{
>>> +    const struct vsbi_ext *vsbi_ext;
>>> +
>>> +    for ( vsbi_ext = _svsbi_exts; vsbi_ext != _evsbi_exts; vsbi_ext++ )
>>> +        if ( ext_id >= vsbi_ext->eid_start &&
>>> +             ext_id <= vsbi_ext->eid_end )
>>> +            return vsbi_ext;
>> What if multiple entries have overlapping EID ranges?
> 
> Good question, I wasn't able to find that EID is always unique in SBI spec,
> but, at the same time, if to look at all available extensions and their id(s),
> they are always unique, so I expect that they will be always unique, otherwise,
> it won't be possible which extension should be used.

Then should there be a build-time (or if that's not easily possible, boot-
time) check?

>>> +void vsbi_handle_ecall(struct vcpu *vcpu, struct cpu_user_regs *regs)
>>> +{
>>> +    const unsigned long eid = regs->a7;
>>> +    const unsigned long fid = regs->a6;
>>> +    const struct vsbi_ext *ext = vsbi_find_extension(eid);
>>> +    int ret;
>>> +
>>> +    if ( ext && ext->handle )
>>> +        ret = ext->handle(vcpu, eid, fid, regs);
>> Is a registration record NULL handler pointer actually legitimate / useful?
>> (If there was range overlap checking I could see a reason to permit such.)
> 
> it is a good question, I think ext->handle = NULL should be impossible. At
> least, at the moment I can't come up with the case where it is possible and
> what will be a use case. I will drop ext->handle check.
> 
>>
>>> +    else
>>> +    {
>>> +        printk("Unsupported Guest SBI EID #%#lx, FID #%lu\n", eid, regs->a1);
>> Are the #-es ahead of the %-s adding value here?
> 
> It is how SBI spec writes them. For example,
>   9. Hart State Management Extension (EID #0x48534D "HSM") . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 26
>   9.1. Function: Hart start (FID #0) . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 27
> 
> So I thought that it would help to find stuff faster.

Okay. Maybe mention such in the description?

Jan

Re: [PATCH v1 1/3] xen/riscv: introduce vSBI extension framework

[Oleksii Kurochko]

On 12/11/25 10:23 AM, Jan Beulich wrote:
> On 10.12.2025 18:03, Oleksii Kurochko wrote:
>> On 12/8/25 3:25 PM, Jan Beulich wrote:
>>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>>> This commit introduces support for handling virtual SBI extensions in Xen.
>>>>
>>>> The changes include:
>>>> - Added new vsbi.c and vsbi.h files to implement virtual SBI extension
>>>>     handling.
>>>> - Modified traps.c to handle CAUSE_VIRTUAL_SUPERVISOR_ECALL by calling
>>>>     vsbi_handle_ecall() when the trap originates from VS-mode.
>>>> - Updated xen.lds.S to include a new .vsbi.exts section for virtual SBI
>>>>     extension data.
>>>> - Updated Makefile to include the new vsbi/ directory in the build.
>>>> - Add hstatus register to struct cpu_user_regs as it is needed for
>>>>     a check that CAUSE_VIRTUAL_SUPERVISOR_ECALL happens from VS-mode.
>>> I can spot the check, yes, but without the field ever being set how is one
>>> to determine whether that check actually makes sense?
>> But hstatus is set automatically when a trap occurs and will be copied in
>> handle_trap() in entry.S.
> Just that entry.S isn't even touched by this series. Did you perhaps omit an
> important part of the change?

Yes, it was omitted. I planned to introduce it as part of a larger update to
entry.S when jump (giving control) to guest support is implemented in the hypervisor.
Considering what is written here...

>
>> If you think it is better to introduce saving and restoring of hstatus in
>> handle_trap() now, or instead drop the handling of
>> “case CAUSE_VIRTUAL_SUPERVISOR_ECALL:” in do_trap(), please let me know.
> I think what I said above is quite clear: When you introduce a field that's
> supposed to be filled upon entry to the hypervisor, the entry code wants
> updating accordingly.

... I will prepare a patch that at least introduces the hstatus-related updates
in handle_trap() in entry.S.

>>>> --- a/xen/arch/riscv/traps.c
>>>> +++ b/xen/arch/riscv/traps.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <asm/processor.h>
>>>>    #include <asm/riscv_encoding.h>
>>>>    #include <asm/traps.h>
>>>> +#include <asm/vsbi.h>
>>>>    
>>>>    /*
>>>>     * Initialize the trap handling.
>>>> @@ -114,6 +115,13 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>>>    
>>>>        switch ( cause )
>>>>        {
>>>> +    case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
>>>> +        if ( !(cpu_regs->hstatus & HSTATUS_SPV) )
>>>> +            panic("CAUSE_VIRTUAL_SUPERVISOR_ECALL came not from VS-mode\n");
>>> This might more naturally want to be BUG_ON()? Assuming of course the value
>>> in question is exclusively under hypervisor control. Otherwise panic() would
>>> also be wrong to use here.
>> Only hypervisor can access ->hstatus (of course, hart is changing it when a trap
>> happens, for example).
>> BUG_ON() is a good option for me.
> Just to clarify: "can access" != "under control". There's also the possibility
> that a guest could do something causing the hardware to raise a
> CAUSE_VIRTUAL_SUPERVISOR_ECALL trap without setting HSTATUS_SPV. That was the
> underlying question here.

It is impossible for a guest to do something like that, because when the guest
is running it is in VS or VU mode, and when a trap is taken into HS mode the
virtualization mode V is set to 0 and ,in addition, hstatus.SPV and
sstatus.SPP are set according to the table:


Previous Mode SPV SPP
    U-mode      0   0
    HS-mode     0   1
    VU-mode     1   0
    VS-mode     1   1

(the panic() message should use “guest mode” instead of “VS mode”)

>
>>>> --- /dev/null
>>>> +++ b/xen/arch/riscv/vsbi/vsbi.c
>>>> @@ -0,0 +1,46 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +
>>>> +#include <xen/sched.h>
>>>> +
>>>> +#include <asm/processor.h>
>>>> +#include <asm/sbi.h>
>>>> +#include <asm/vsbi.h>
>>>> +
>>>> +extern const struct vsbi_ext _svsbi_exts[], _evsbi_exts[];
>>>> +
>>>> +const struct vsbi_ext *vsbi_find_extension(unsigned long ext_id)
>>> static?
>> It could be use not in vsbi.c (for example, in the next patches it is used for
>> SBI_EXT_BASE_PROBE_EXT), so it shouldn't be static.
> Okay. In RISC-V that's okay as long as it's not subject to Misra scanning. Yet
> still introducing a non-static function without callers from other CUs may
> warrant a remark in the description. Once RISC-V becomes subject to Misra scans,
> such will be problematic, after all.

I will add such a remark in the commit description.

>
>>> Also, again - is the ext_ prefix adding any value here?
>> Not really, I guess.
> Maybe, to still distinguish from "fid", use "eid" here then?

Makes sense to use eid. I will apply this change.


>
>>>> +{
>>>> +    const struct vsbi_ext *vsbi_ext;
>>>> +
>>>> +    for ( vsbi_ext = _svsbi_exts; vsbi_ext != _evsbi_exts; vsbi_ext++ )
>>>> +        if ( ext_id >= vsbi_ext->eid_start &&
>>>> +             ext_id <= vsbi_ext->eid_end )
>>>> +            return vsbi_ext;
>>> What if multiple entries have overlapping EID ranges?
>> Good question, I wasn't able to find that EID is always unique in SBI spec,
>> but, at the same time, if to look at all available extensions and their id(s),
>> they are always unique, so I expect that they will be always unique, otherwise,
>> it won't be possible which extension should be used.
> Then should there be a build-time (or if that's not easily possible, boot-
> time) check?

Considering that the .vsbi.ext section is filled dynamically, I think it would
be quite difficult to perform a build-time check without writing an additional
script to parse the .vsbi.ext entries and verify that there are no overlaps,
which seems excessive.

A boot-time check is much easier.

>
>>>> +void vsbi_handle_ecall(struct vcpu *vcpu, struct cpu_user_regs *regs)
>>>> +{
>>>> +    const unsigned long eid = regs->a7;
>>>> +    const unsigned long fid = regs->a6;
>>>> +    const struct vsbi_ext *ext = vsbi_find_extension(eid);
>>>> +    int ret;
>>>> +
>>>> +    if ( ext && ext->handle )
>>>> +        ret = ext->handle(vcpu, eid, fid, regs);
>>> Is a registration record NULL handler pointer actually legitimate / useful?
>>> (If there was range overlap checking I could see a reason to permit such.)
>> it is a good question, I think ext->handle = NULL should be impossible. At
>> least, at the moment I can't come up with the case where it is possible and
>> what will be a use case. I will drop ext->handle check.
>>
>>>> +    else
>>>> +    {
>>>> +        printk("Unsupported Guest SBI EID #%#lx, FID #%lu\n", eid, regs->a1);
>>> Are the #-es ahead of the %-s adding value here?
>> It is how SBI spec writes them. For example,
>>    9. Hart State Management Extension (EID #0x48534D "HSM") . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 26
>>    9.1. Function: Hart start (FID #0) . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 27
>>
>> So I thought that it would help to find stuff faster.
> Okay. Maybe mention such in the description?

Sure, but I think a comment above printk() will be enough.

~ Oleksii

[PATCH v1 2/3] xen/riscv: add RISC-V legacy SBI extension support for guests

[Oleksii Kurochko]

This commit adds support for legacy SBI extensions (version 0.1) in Xen
for guest domains.

The changes include:
1. Define all legacy SBI extension IDs (0x0 to 0x8) for better clarity and
   completeness.
2. Implement handling of legacy SBI extensions, starting with support for
   SBI_EXT_0_1_CONSOLE_{PUT,GET}CHAR.

The implementation uses the existing virtual SBI framework to handle legacy
SBI ecalls, ensuring compatibility with older SBI specifications in
RISC-V guests.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/sbi.h            | 11 ++++--
 xen/arch/riscv/vsbi/Makefile                |  1 +
 xen/arch/riscv/vsbi/vsbi-legacy-extension.c | 37 +++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/riscv/vsbi/vsbi-legacy-extension.c

diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index ade24a572d..e7d5d707b1 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -14,8 +14,15 @@
 
 #include <xen/cpumask.h>
 
-#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
-#define SBI_EXT_0_1_SHUTDOWN			0x8
+#define SBI_EXT_0_1_SET_TIMER           0x0
+#define SBI_EXT_0_1_CONSOLE_PUTCHAR     0x1
+#define SBI_EXT_0_1_CONSOLE_GETCHAR     0x2
+#define SBI_EXT_0_1_CLEAR_IPI           0x3
+#define SBI_EXT_0_1_SEND_IPI            0x4
+#define SBI_EXT_0_1_REMOTE_FENCE_I      0x5
+#define SBI_EXT_0_1_REMOTE_SFENCE_VMA   0x6
+#define SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID  0x7
+#define SBI_EXT_0_1_SHUTDOWN            0x8
 
 #define SBI_EXT_BASE                    0x10
 #define SBI_EXT_RFENCE                  0x52464E43
diff --git a/xen/arch/riscv/vsbi/Makefile b/xen/arch/riscv/vsbi/Makefile
index 574c8ff78d..4da625db9a 100644
--- a/xen/arch/riscv/vsbi/Makefile
+++ b/xen/arch/riscv/vsbi/Makefile
@@ -1 +1,2 @@
 obj-y += vsbi.o
+obj-y += vsbi-legacy-extension.o
diff --git a/xen/arch/riscv/vsbi/vsbi-legacy-extension.c b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
new file mode 100644
index 0000000000..39d65931b1
--- /dev/null
+++ b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
@@ -0,0 +1,37 @@
+
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/lib.h>
+#include <xen/sched.h>
+
+#include <asm/processor.h>
+#include <asm/vsbi.h>
+
+static int vsbi_legacy_ecall_handler(struct vcpu *vcpu, unsigned long eid,
+                                     unsigned long fid,
+                                     struct cpu_user_regs *regs)
+{
+    int ret = 0;
+
+    switch ( eid )
+    {
+    case SBI_EXT_0_1_CONSOLE_PUTCHAR:
+        printk("%c", (char)regs->a0);
+        break;
+
+    case SBI_EXT_0_1_CONSOLE_GETCHAR:
+        regs->a0 = SBI_ERR_NOT_SUPPORTED;
+        break;
+
+    default:
+        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
+              __func__, fid, eid);
+		break;
+    }
+
+    return ret;
+}
+
+VSBI_EXT_START(legacy, SBI_EXT_0_1_SET_TIMER, SBI_EXT_0_1_SHUTDOWN,
+               vsbi_legacy_ecall_handler)
+VSBI_EXT_END
-- 
2.52.0

Re: [PATCH v1 2/3] xen/riscv: add RISC-V legacy SBI extension support for guests

[Jan Beulich]

On 01.12.2025 11:24, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/sbi.h
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -14,8 +14,15 @@
>  
>  #include <xen/cpumask.h>
>  
> -#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
> -#define SBI_EXT_0_1_SHUTDOWN			0x8
> +#define SBI_EXT_0_1_SET_TIMER           0x0
> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR     0x1

Why the padding adjustment when ...

> +#define SBI_EXT_0_1_CONSOLE_GETCHAR     0x2
> +#define SBI_EXT_0_1_CLEAR_IPI           0x3
> +#define SBI_EXT_0_1_SEND_IPI            0x4
> +#define SBI_EXT_0_1_REMOTE_FENCE_I      0x5
> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA   0x6
> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID  0x7

... you immediately have one that doesn't fit?

> --- a/xen/arch/riscv/vsbi/Makefile
> +++ b/xen/arch/riscv/vsbi/Makefile
> @@ -1 +1,2 @@
>  obj-y += vsbi.o
> +obj-y += vsbi-legacy-extension.o

No vsbi- prefixes please underneath vsbi/.

> --- /dev/null
> +++ b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
> @@ -0,0 +1,37 @@
> +
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +
> +#include <asm/processor.h>
> +#include <asm/vsbi.h>
> +
> +static int vsbi_legacy_ecall_handler(struct vcpu *vcpu, unsigned long eid,
> +                                     unsigned long fid,
> +                                     struct cpu_user_regs *regs)
> +{
> +    int ret = 0;
> +
> +    switch ( eid )
> +    {
> +    case SBI_EXT_0_1_CONSOLE_PUTCHAR:
> +        printk("%c", (char)regs->a0);

This is guest output, so shouldn't use plain printk().

> +        break;
> +
> +    case SBI_EXT_0_1_CONSOLE_GETCHAR:
> +        regs->a0 = SBI_ERR_NOT_SUPPORTED;

This will be overwritten with the return value you pass to the caller (i.e. 0),
by that caller (i.e. vsbi_handle_ecall()).

> +        break;
> +
> +    default:
> +        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
> +              __func__, fid, eid);

Please don't. domain_crash() may be okay to use here, but crashing the hypervisor
because of unexpected guest input isn't okay.

> +		break;

Bad indentation.

Jan

Re: [PATCH v1 2/3] xen/riscv: add RISC-V legacy SBI extension support for guests

[Oleksii Kurochko]

On 12/8/25 4:05 PM, Jan Beulich wrote:
> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/sbi.h
>> +++ b/xen/arch/riscv/include/asm/sbi.h
>> @@ -14,8 +14,15 @@
>>   
>>   #include <xen/cpumask.h>
>>   
>> -#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
>> -#define SBI_EXT_0_1_SHUTDOWN			0x8
>> +#define SBI_EXT_0_1_SET_TIMER           0x0
>> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR     0x1
> Why the padding adjustment when ...
>
>> +#define SBI_EXT_0_1_CONSOLE_GETCHAR     0x2
>> +#define SBI_EXT_0_1_CLEAR_IPI           0x3
>> +#define SBI_EXT_0_1_SEND_IPI            0x4
>> +#define SBI_EXT_0_1_REMOTE_FENCE_I      0x5
>> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA   0x6
>> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID  0x7
> ... you immediately have one that doesn't fit?

IDK, the padding adjustment shouldn't be done in this way.
I will correct it.

>> --- /dev/null
>> +++ b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
>> @@ -0,0 +1,37 @@
>> +
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#include <xen/lib.h>
>> +#include <xen/sched.h>
>> +
>> +#include <asm/processor.h>
>> +#include <asm/vsbi.h>
>> +
>> +static int vsbi_legacy_ecall_handler(struct vcpu *vcpu, unsigned long eid,
>> +                                     unsigned long fid,
>> +                                     struct cpu_user_regs *regs)
>> +{
>> +    int ret = 0;
>> +
>> +    switch ( eid )
>> +    {
>> +    case SBI_EXT_0_1_CONSOLE_PUTCHAR:
>> +        printk("%c", (char)regs->a0);
> This is guest output, so shouldn't use plain printk().

I think that I don't know what should be used instead. Could you suggest me something
or point to the code in other arch-s?

Or do you mean that guest_printk() should be used?

>> +        break;
>> +
>> +    case SBI_EXT_0_1_CONSOLE_GETCHAR:
>> +        regs->a0 = SBI_ERR_NOT_SUPPORTED;
> This will be overwritten with the return value you pass to the caller (i.e. 0),
> by that caller (i.e. vsbi_handle_ecall()).

Oh, thanks. It should be "ret = SBI_ERR_NOT_SUPPORTED;" here.


>
>> +        break;
>> +
>> +    default:
>> +        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
>> +              __func__, fid, eid);
> Please don't. domain_crash() may be okay to use here, but crashing the hypervisor
> because of unexpected guest input isn't okay.

|domain_crash()| is better. I also considered just returning|SBI_ERR_NOT_SUPPORTED|,
but it wasn’t too convenient for debugging which FID/EID the guest was called,
so I started using|panic()| instead.

Thanks.

~ Oleksii

Re: [PATCH v1 2/3] xen/riscv: add RISC-V legacy SBI extension support for guests

[Jan Beulich]

On 11.12.2025 11:29, Oleksii Kurochko wrote:
> On 12/8/25 4:05 PM, Jan Beulich wrote:
>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
>>> @@ -0,0 +1,37 @@
>>> +
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#include <xen/lib.h>
>>> +#include <xen/sched.h>
>>> +
>>> +#include <asm/processor.h>
>>> +#include <asm/vsbi.h>
>>> +
>>> +static int vsbi_legacy_ecall_handler(struct vcpu *vcpu, unsigned long eid,
>>> +                                     unsigned long fid,
>>> +                                     struct cpu_user_regs *regs)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    switch ( eid )
>>> +    {
>>> +    case SBI_EXT_0_1_CONSOLE_PUTCHAR:
>>> +        printk("%c", (char)regs->a0);
>> This is guest output, so shouldn't use plain printk().
> 
> I think that I don't know what should be used instead. Could you suggest me something
> or point to the code in other arch-s?
> 
> Or do you mean that guest_printk() should be used?

No direct replacement will do what you want, as they all prefix something to the
string passed (which isn't what you want). You may need to buffer characters and
emit them in batches (full lines unless overly long). For x86 see hvm_print_line(),
but I think Arm also has something like this.

>>> +    default:
>>> +        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
>>> +              __func__, fid, eid);
>> Please don't. domain_crash() may be okay to use here, but crashing the hypervisor
>> because of unexpected guest input isn't okay.
> 
> |domain_crash()| is better. I also considered just returning|SBI_ERR_NOT_SUPPORTED|,
> but it wasn’t too convenient for debugging which FID/EID the guest was called,
> so I started using|panic()| instead.

FTAOD - domain_crash() is acceptable here while things are still under development.
It shouldn't stay like this in the end though: Guests should not be punished like
this for something Xen hasn't implemented.

Jan

Re: [PATCH v1 2/3] xen/riscv: add RISC-V legacy SBI extension support for guests

[Oleksii Kurochko]

On 12/11/25 12:02 PM, Jan Beulich wrote:
> On 11.12.2025 11:29, Oleksii Kurochko wrote:
>> On 12/8/25 4:05 PM, Jan Beulich wrote:
>>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
>>>> @@ -0,0 +1,37 @@
>>>> +
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +
>>>> +#include <xen/lib.h>
>>>> +#include <xen/sched.h>
>>>> +
>>>> +#include <asm/processor.h>
>>>> +#include <asm/vsbi.h>
>>>> +
>>>> +static int vsbi_legacy_ecall_handler(struct vcpu *vcpu, unsigned long eid,
>>>> +                                     unsigned long fid,
>>>> +                                     struct cpu_user_regs *regs)
>>>> +{
>>>> +    int ret = 0;
>>>> +
>>>> +    switch ( eid )
>>>> +    {
>>>> +    case SBI_EXT_0_1_CONSOLE_PUTCHAR:
>>>> +        printk("%c", (char)regs->a0);
>>> This is guest output, so shouldn't use plain printk().
>> I think that I don't know what should be used instead. Could you suggest me something
>> or point to the code in other arch-s?
>>
>> Or do you mean that guest_printk() should be used?
> No direct replacement will do what you want, as they all prefix something to the
> string passed (which isn't what you want). You may need to buffer characters and
> emit them in batches (full lines unless overly long). For x86 see hvm_print_line(),
> but I think Arm also has something like this.

I don’t recall anything like that for ARM. The only thing related to character
buffering that I remember is in vpl011_write_data_xen()
(https://elixir.bootlin.com/xen/v4.21.0/source/xen/arch/arm/vpl011.c#L76), but it
does not use the buf declared in struct domain_console. Instead, it provides a
separate structure for vpl011:
     struct vpl011_xen_backend {
         char in[SBSA_UART_FIFO_SIZE];
         char out[SBSA_UART_OUT_BUF_SIZE];
         XENCONS_RING_IDX in_cons, in_prod;
         XENCONS_RING_IDX out_prod;
     };

I don’t see that ARM uses struct domain_console.

By the way, I can’t find a counterpart of hvm_print_line() for reading a character(s).
Is domain_console->buf intended to be used only for writing characters?


>
>>>> +    default:
>>>> +        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
>>>> +              __func__, fid, eid);
>>> Please don't. domain_crash() may be okay to use here, but crashing the hypervisor
>>> because of unexpected guest input isn't okay.
>> |domain_crash()| is better. I also considered just returning|SBI_ERR_NOT_SUPPORTED|,
>> but it wasn’t too convenient for debugging which FID/EID the guest was called,
>> so I started using|panic()| instead.
> FTAOD - domain_crash() is acceptable here while things are still under development.
> It shouldn't stay like this in the end though: Guests should not be punished like
> this for something Xen hasn't implemented.

Agree, I will create a task in my Xen's repo to not forget to drop panic()/domain_crash().

~ Oleksii

Re: [PATCH v1 2/3] xen/riscv: add RISC-V legacy SBI extension support for guests

[Jan Beulich]

On 11.12.2025 13:52, Oleksii Kurochko wrote:
> On 12/11/25 12:02 PM, Jan Beulich wrote:
>> On 11.12.2025 11:29, Oleksii Kurochko wrote:
>>> On 12/8/25 4:05 PM, Jan Beulich wrote:
>>>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
>>>>> @@ -0,0 +1,37 @@
>>>>> +
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +
>>>>> +#include <xen/lib.h>
>>>>> +#include <xen/sched.h>
>>>>> +
>>>>> +#include <asm/processor.h>
>>>>> +#include <asm/vsbi.h>
>>>>> +
>>>>> +static int vsbi_legacy_ecall_handler(struct vcpu *vcpu, unsigned long eid,
>>>>> +                                     unsigned long fid,
>>>>> +                                     struct cpu_user_regs *regs)
>>>>> +{
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    switch ( eid )
>>>>> +    {
>>>>> +    case SBI_EXT_0_1_CONSOLE_PUTCHAR:
>>>>> +        printk("%c", (char)regs->a0);
>>>> This is guest output, so shouldn't use plain printk().
>>> I think that I don't know what should be used instead. Could you suggest me something
>>> or point to the code in other arch-s?
>>>
>>> Or do you mean that guest_printk() should be used?
>> No direct replacement will do what you want, as they all prefix something to the
>> string passed (which isn't what you want). You may need to buffer characters and
>> emit them in batches (full lines unless overly long). For x86 see hvm_print_line(),
>> but I think Arm also has something like this.
> 
> I don’t recall anything like that for ARM. The only thing related to character
> buffering that I remember is in vpl011_write_data_xen()
> (https://elixir.bootlin.com/xen/v4.21.0/source/xen/arch/arm/vpl011.c#L76), but it
> does not use the buf declared in struct domain_console. Instead, it provides a
> separate structure for vpl011:
>      struct vpl011_xen_backend {
>          char in[SBSA_UART_FIFO_SIZE];
>          char out[SBSA_UART_OUT_BUF_SIZE];
>          XENCONS_RING_IDX in_cons, in_prod;
>          XENCONS_RING_IDX out_prod;
>      };
> 
> I don’t see that ARM uses struct domain_console.
> 
> By the way, I can’t find a counterpart of hvm_print_line() for reading a character(s).
> Is domain_console->buf intended to be used only for writing characters?

I don't think there's any particular intention, but of course you can use it
only for one of the two.

Jan

[PATCH v1 3/3] xen/riscv: add RISC-V virtual SBI base extension support for guests

[Oleksii Kurochko]

Add support of virtual SBI base extension calls for RISC-V guests, delegating
hardware-specific queries to the underlying SBI and handling version and
firmware ID queries directly.

The changes include:
1. Define new SBI base extension function IDs (SBI_EXT_BASE_GET_MVENDORID,
   SBI_EXT_BASE_GET_MARCHID, SBI_EXT_BASE_GET_MIMPID).
2. Make sbi_spec_version, sbi_fw_id, and sbi_fw_version global variables for
   use in virtual SBI handling, removing redundant local declarations in
   sbi_init.
3. Implement handling of SBI base extension functions, including version,
   firmware ID, and machine-specific queries.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/sbi.h          |  3 ++
 xen/arch/riscv/sbi.c                      |  8 ++--
 xen/arch/riscv/vsbi/Makefile              |  1 +
 xen/arch/riscv/vsbi/vsbi-base-extension.c | 52 +++++++++++++++++++++++
 4 files changed, 61 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/riscv/vsbi/vsbi-base-extension.c

diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index e7d5d707b1..98ba872ef3 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -32,6 +32,9 @@
 #define SBI_EXT_BASE_GET_IMP_ID         0x1
 #define SBI_EXT_BASE_GET_IMP_VERSION    0x2
 #define SBI_EXT_BASE_PROBE_EXT          0x3
+#define SBI_EXT_BASE_GET_MVENDORID      0x4
+#define SBI_EXT_BASE_GET_MARCHID        0x5
+#define SBI_EXT_BASE_GET_MIMPID         0x6
 
 /* SBI function IDs for RFENCE extension */
 #define SBI_EXT_RFENCE_REMOTE_FENCE_I           0x0
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
index 425dce44c6..97cbf84c21 100644
--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -23,7 +23,9 @@
 #include <asm/processor.h>
 #include <asm/sbi.h>
 
-static unsigned long __ro_after_init sbi_spec_version = SBI_SPEC_VERSION_DEFAULT;
+unsigned long __ro_after_init sbi_spec_version = SBI_SPEC_VERSION_DEFAULT;
+long __ro_after_init sbi_fw_id;
+long __ro_after_init sbi_fw_version;
 
 struct sbiret sbi_ecall(unsigned long ext, unsigned long fid,
                         unsigned long arg0, unsigned long arg1,
@@ -313,8 +315,8 @@ int __init sbi_init(void)
 
     if ( !sbi_spec_is_0_1() )
     {
-        long sbi_fw_id = sbi_get_firmware_id();
-        long sbi_fw_version = sbi_get_firmware_version();
+        sbi_fw_id = sbi_get_firmware_id();
+        sbi_fw_version = sbi_get_firmware_version();
 
         BUG_ON((sbi_fw_id < 0) || (sbi_fw_version < 0));
 
diff --git a/xen/arch/riscv/vsbi/Makefile b/xen/arch/riscv/vsbi/Makefile
index 4da625db9a..07ae27b99e 100644
--- a/xen/arch/riscv/vsbi/Makefile
+++ b/xen/arch/riscv/vsbi/Makefile
@@ -1,2 +1,3 @@
 obj-y += vsbi.o
+obj-y += vsbi-base-extension.o
 obj-y += vsbi-legacy-extension.o
diff --git a/xen/arch/riscv/vsbi/vsbi-base-extension.c b/xen/arch/riscv/vsbi/vsbi-base-extension.c
new file mode 100644
index 0000000000..88f4567cb1
--- /dev/null
+++ b/xen/arch/riscv/vsbi/vsbi-base-extension.c
@@ -0,0 +1,52 @@
+
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/lib.h>
+#include <xen/sched.h>
+
+#include <asm/processor.h>
+#include <asm/sbi.h>
+#include <asm/vsbi.h>
+
+extern unsigned long __ro_after_init sbi_spec_version;
+extern long __ro_after_init sbi_fw_id;
+extern long __ro_after_init sbi_fw_version;
+
+static int vsbi_base_ecall_handler(struct vcpu *vcpu, unsigned long eid,
+                                   unsigned long fid,
+                                   struct cpu_user_regs *regs)
+{
+    int ret = 0;
+    struct sbiret sbi_ret;
+
+    switch ( fid ) {
+    case SBI_EXT_BASE_GET_SPEC_VERSION:
+        regs->a1 = sbi_spec_version;
+        break;
+    case SBI_EXT_BASE_GET_IMP_ID:
+        regs->a1 = sbi_fw_id;
+        break;
+    case SBI_EXT_BASE_GET_IMP_VERSION:
+        regs->a1 = sbi_fw_version;
+        break;
+    case SBI_EXT_BASE_GET_MVENDORID:
+    case SBI_EXT_BASE_GET_MARCHID:
+    case SBI_EXT_BASE_GET_MIMPID:
+        sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
+        ret = sbi_ret.error;
+        regs->a1 = sbi_ret.value;
+        break;
+    case SBI_EXT_BASE_PROBE_EXT:
+        regs->a1 = vsbi_find_extension(regs->a0) ? 1 : 0;
+        break;
+    default:
+        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
+              __func__, fid, eid);
+        break;
+    }
+
+    return ret;
+}
+
+VSBI_EXT_START(base, SBI_EXT_BASE, SBI_EXT_BASE, vsbi_base_ecall_handler)
+VSBI_EXT_END
-- 
2.52.0

Re: [PATCH v1 3/3] xen/riscv: add RISC-V virtual SBI base extension support for guests

[Jan Beulich]

On 01.12.2025 11:24, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/vsbi/vsbi-base-extension.c
> @@ -0,0 +1,52 @@
> +
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +
> +#include <asm/processor.h>
> +#include <asm/sbi.h>
> +#include <asm/vsbi.h>
> +
> +extern unsigned long __ro_after_init sbi_spec_version;
> +extern long __ro_after_init sbi_fw_id;
> +extern long __ro_after_init sbi_fw_version;
> +
> +static int vsbi_base_ecall_handler(struct vcpu *vcpu, unsigned long eid,
> +                                   unsigned long fid,
> +                                   struct cpu_user_regs *regs)
> +{
> +    int ret = 0;
> +    struct sbiret sbi_ret;
> +
> +    switch ( fid ) {
> +    case SBI_EXT_BASE_GET_SPEC_VERSION:
> +        regs->a1 = sbi_spec_version;

Wouldn't this need to be the minimum of what firmware supports and what Xen
supports / knows about? (Assuming backward compatibility among the spec
versions of course.)

> +        break;
> +    case SBI_EXT_BASE_GET_IMP_ID:
> +        regs->a1 = sbi_fw_id;
> +        break;
> +    case SBI_EXT_BASE_GET_IMP_VERSION:
> +        regs->a1 = sbi_fw_version;

Same concern here, but see also below.

> +        break;
> +    case SBI_EXT_BASE_GET_MVENDORID:
> +    case SBI_EXT_BASE_GET_MARCHID:
> +    case SBI_EXT_BASE_GET_MIMPID:
> +        sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);

This may be okay to do for the hardware domain, but hardly for DomU-s.

Same concern for SBI_EXT_BASE_GET_IMP_ID.

> +        ret = sbi_ret.error;
> +        regs->a1 = sbi_ret.value;
> +        break;
> +    case SBI_EXT_BASE_PROBE_EXT:
> +        regs->a1 = vsbi_find_extension(regs->a0) ? 1 : 0;

At least for hwdom doesn't this also need combining virtual and
underlying physical lookup, if for some extensions you may pass the
requests down to the physical one (as done above)?

> +        break;
> +    default:
> +        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
> +              __func__, fid, eid);

Again - inappropriate for anything controlled by guests.

Jan

Re: [PATCH v1 3/3] xen/riscv: add RISC-V virtual SBI base extension support for guests

[Oleksii Kurochko]

On 12/8/25 4:15 PM, Jan Beulich wrote:
> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>> --- /dev/null
>> +++ b/xen/arch/riscv/vsbi/vsbi-base-extension.c
>> @@ -0,0 +1,52 @@
>> +
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#include <xen/lib.h>
>> +#include <xen/sched.h>
>> +
>> +#include <asm/processor.h>
>> +#include <asm/sbi.h>
>> +#include <asm/vsbi.h>
>> +
>> +extern unsigned long __ro_after_init sbi_spec_version;
>> +extern long __ro_after_init sbi_fw_id;
>> +extern long __ro_after_init sbi_fw_version;
>> +
>> +static int vsbi_base_ecall_handler(struct vcpu *vcpu, unsigned long eid,
>> +                                   unsigned long fid,
>> +                                   struct cpu_user_regs *regs)
>> +{
>> +    int ret = 0;
>> +    struct sbiret sbi_ret;
>> +
>> +    switch ( fid ) {
>> +    case SBI_EXT_BASE_GET_SPEC_VERSION:
>> +        regs->a1 = sbi_spec_version;
> Wouldn't this need to be the minimum of what firmware supports and what Xen
> supports / knows about? (Assuming backward compatibility among the spec
> versions of course.)

The base extension is mandatory (according to the spec), and based on some Linux
commits from contributors to the OpenSBI spec, it is also intended to allow
backward compatibility and probing of future extensions (although I was not able
to find this explicitly stated in the spec).

However, none of this guarantees that everything else is backward compatible.
For example, the entire v0.1 SBI has been moved to the legacy extension, which
is now an optional extension. This is technically a backwards-incompatible
change because the legacy extension is optional, and v0.1 of the SBI does not
allow probing.

Regarding what should be written to|regs->a1|, I think you are right: it should
be the minimum of what the firmware provides and what Xen supports. Otherwise,
if|sbi_spec_version| is set to 2.0 and we return 2.0 to the guest, the guest might
try to probe the DBGN (which Xen does not currently support) extension and use
it instead of the legacy extension for the early console.


>> +        break;
>> +    case SBI_EXT_BASE_GET_IMP_ID:
>> +        regs->a1 = sbi_fw_id;
>> +        break;
>> +    case SBI_EXT_BASE_GET_IMP_VERSION:
>> +        regs->a1 = sbi_fw_version;
> Same concern here, but see also below.

For SBI_EXT_BASE_GET_IMP_ID, I think we want to return XEN id which is according
to OpenSBI spec is 7.

Something similar for SBI_EXT_BASE_GET_IMP_VERSION, maybe we want to return Xen
version code (XEN_FULLVERSION).

>
>> +        break;
>> +    case SBI_EXT_BASE_GET_MVENDORID:
>> +    case SBI_EXT_BASE_GET_MARCHID:
>> +    case SBI_EXT_BASE_GET_MIMPID:
>> +        sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> This may be okay to do for the hardware domain, but hardly for DomU-s.

I don’t see an issue with returning the vendor, microarchitecture, and
processor ID. This is essentially what other hypervisors do.

What would be better to return? Returning 0 could be an option, and according
to the RISC-V spec:
   This register must be readable in any implementation, but a value of 0 can
   be returned to indicate the field is not implemented.

So returning 0 would simply indicate that the field is not provided for case
of DomUs, and provide it for hardware domain.

Would it be better?

>
> Same concern for SBI_EXT_BASE_GET_IMP_ID.
>
>> +        ret = sbi_ret.error;
>> +        regs->a1 = sbi_ret.value;
>> +        break;
>> +    case SBI_EXT_BASE_PROBE_EXT:
>> +        regs->a1 = vsbi_find_extension(regs->a0) ? 1 : 0;
> At least for hwdom doesn't this also need combining virtual and
> underlying physical lookup, if for some extensions you may pass the
> requests down to the physical one (as done above)?

I think I understand your intention, but I am not 100% sure that we need to
perform a physical lookup. There may be implementation-specific cases where
a call is emulated by the hypervisor instead of being passthroughed to
OpenSBI.
In other words, it could be the case that an extension is fully emulated
without requiring support for the corresponding physical extension.

~ Oleksii

Re: [PATCH v1 3/3] xen/riscv: add RISC-V virtual SBI base extension support for guests

[Oleksii Kurochko]

On 12/12/25 4:25 PM, Oleksii Kurochko wrote:
> On 12/8/25 4:15 PM, Jan Beulich wrote:
>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/vsbi/vsbi-base-extension.c
>>> @@ -0,0 +1,52 @@
>>> +
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#include <xen/lib.h>
>>> +#include <xen/sched.h>
>>> +
>>> +#include <asm/processor.h>
>>> +#include <asm/sbi.h>
>>> +#include <asm/vsbi.h>
>>> +
>>> +extern unsigned long __ro_after_init sbi_spec_version;
>>> +extern long __ro_after_init sbi_fw_id;
>>> +extern long __ro_after_init sbi_fw_version;
>>> +
>>> +static int vsbi_base_ecall_handler(struct vcpu *vcpu, unsigned long 
>>> eid,
>>> +                                   unsigned long fid,
>>> +                                   struct cpu_user_regs *regs)
>>> +{
>>> +    int ret = 0;
>>> +    struct sbiret sbi_ret;
>>> +
>>> +    switch ( fid ) {
>>> +    case SBI_EXT_BASE_GET_SPEC_VERSION:
>>> +        regs->a1 = sbi_spec_version;
>> Wouldn't this need to be the minimum of what firmware supports and 
>> what Xen
>> supports / knows about? (Assuming backward compatibility among the spec
>> versions of course.)
>
> The base extension is mandatory (according to the spec), and based on 
> some Linux
> commits from contributors to the OpenSBI spec, it is also intended to 
> allow
> backward compatibility and probing of future extensions (although I 
> was not able
> to find this explicitly stated in the spec).
>
> However, none of this guarantees that everything else is backward 
> compatible.
> For example, the entire v0.1 SBI has been moved to the legacy 
> extension, which
> is now an optional extension. This is technically a 
> backwards-incompatible
> change because the legacy extension is optional, and v0.1 of the SBI 
> does not
> allow probing.
>
> Regarding what should be written to|regs->a1|, I think you are right: 
> it should
> be the minimum of what the firmware provides and what Xen supports. 
> Otherwise,
> if|sbi_spec_version| is set to 2.0 and we return 2.0 to the guest, the 
> guest might
> try to probe the DBGN (which Xen does not currently support) extension 
> and use
> it instead of the legacy extension for the early console.

I think we could still introduce the following in Xen:
   #define XEN_SBI_VERSION_MAJOR 0
   #define XEN_SBI_VERSION_MINOR 2
and pass it to the guest as:
   regs-> a1 = (XEN_SBI_VERSION_MAJOR << SBI_SPEC_VERSION_MAJOR_SHIFT) | XEN_SBI_VERSION_MINOR;

IMO, this should be sufficient because:
1. We can fully emulate the base extension in Xen without calling into OpenSBI.
    This covers the case where OpenSBI might return version 0.1,which is unlikely, as all
    boards with hypervisor extension support at least 0.2, and in practice even 2.0, while we
    report 0.2 to the guest.
2. Even if OpenSBI returns, for example, version 2.0 and we tell the guest that we support
    0.2, it should still be fine, as the base extension is at least backward compatible.

In other words, I think we should care only about what Xen supports and provide it to a
guest. Any concerns about that?

~ Oleksii

Re: [PATCH v1 3/3] xen/riscv: add RISC-V virtual SBI base extension support for guests

[Jan Beulich]

On 12.12.2025 16:25, Oleksii Kurochko wrote:
> On 12/8/25 4:15 PM, Jan Beulich wrote:
>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/vsbi/vsbi-base-extension.c
>>> @@ -0,0 +1,52 @@
>>> +
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#include <xen/lib.h>
>>> +#include <xen/sched.h>
>>> +
>>> +#include <asm/processor.h>
>>> +#include <asm/sbi.h>
>>> +#include <asm/vsbi.h>
>>> +
>>> +extern unsigned long __ro_after_init sbi_spec_version;
>>> +extern long __ro_after_init sbi_fw_id;
>>> +extern long __ro_after_init sbi_fw_version;
>>> +
>>> +static int vsbi_base_ecall_handler(struct vcpu *vcpu, unsigned long eid,
>>> +                                   unsigned long fid,
>>> +                                   struct cpu_user_regs *regs)
>>> +{
>>> +    int ret = 0;
>>> +    struct sbiret sbi_ret;
>>> +
>>> +    switch ( fid ) {
>>> +    case SBI_EXT_BASE_GET_SPEC_VERSION:
>>> +        regs->a1 = sbi_spec_version;
>> Wouldn't this need to be the minimum of what firmware supports and what Xen
>> supports / knows about? (Assuming backward compatibility among the spec
>> versions of course.)
> 
> The base extension is mandatory (according to the spec), and based on some Linux
> commits from contributors to the OpenSBI spec, it is also intended to allow
> backward compatibility and probing of future extensions (although I was not able
> to find this explicitly stated in the spec).
> 
> However, none of this guarantees that everything else is backward compatible.
> For example, the entire v0.1 SBI has been moved to the legacy extension, which
> is now an optional extension. This is technically a backwards-incompatible
> change because the legacy extension is optional, and v0.1 of the SBI does not
> allow probing.
> 
> Regarding what should be written to|regs->a1|, I think you are right: it should
> be the minimum of what the firmware provides and what Xen supports. Otherwise,
> if|sbi_spec_version| is set to 2.0 and we return 2.0 to the guest, the guest might
> try to probe the DBGN (which Xen does not currently support) extension and use
> it instead of the legacy extension for the early console.
> 
> 
>>> +        break;
>>> +    case SBI_EXT_BASE_GET_IMP_ID:
>>> +        regs->a1 = sbi_fw_id;
>>> +        break;
>>> +    case SBI_EXT_BASE_GET_IMP_VERSION:
>>> +        regs->a1 = sbi_fw_version;
>> Same concern here, but see also below.
> 
> For SBI_EXT_BASE_GET_IMP_ID, I think we want to return XEN id which is according
> to OpenSBI spec is 7.
> 
> Something similar for SBI_EXT_BASE_GET_IMP_VERSION, maybe we want to return Xen
> version code (XEN_FULLVERSION).
> 
>>
>>> +        break;
>>> +    case SBI_EXT_BASE_GET_MVENDORID:
>>> +    case SBI_EXT_BASE_GET_MARCHID:
>>> +    case SBI_EXT_BASE_GET_MIMPID:
>>> +        sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
>> This may be okay to do for the hardware domain, but hardly for DomU-s.
> 
> I don’t see an issue with returning the vendor, microarchitecture, and
> processor ID. This is essentially what other hypervisors do.
> 
> What would be better to return? Returning 0 could be an option, and according
> to the RISC-V spec:
>    This register must be readable in any implementation, but a value of 0 can
>    be returned to indicate the field is not implemented.
> 
> So returning 0 would simply indicate that the field is not provided for case
> of DomUs, and provide it for hardware domain.
> 
> Would it be better?
> 
>>
>> Same concern for SBI_EXT_BASE_GET_IMP_ID.
>>
>>> +        ret = sbi_ret.error;
>>> +        regs->a1 = sbi_ret.value;
>>> +        break;
>>> +    case SBI_EXT_BASE_PROBE_EXT:
>>> +        regs->a1 = vsbi_find_extension(regs->a0) ? 1 : 0;
>> At least for hwdom doesn't this also need combining virtual and
>> underlying physical lookup, if for some extensions you may pass the
>> requests down to the physical one (as done above)?
> 
> I think I understand your intention, but I am not 100% sure that we need to
> perform a physical lookup. There may be implementation-specific cases where
> a call is emulated by the hypervisor instead of being passthroughed to
> OpenSBI.
> In other words, it could be the case that an extension is fully emulated
> without requiring support for the corresponding physical extension.

I don't have sufficient RISC-V knowledge to further comment on this. My main
concern is that we have to present (a) a consistent picture to both hwdom
and DomU-s while (b) presenting a properly virtualized view to DomU-s (i.e.
abstracting away hardware implementation details). In particular for DomU-s
you will already now need to think of what happens if a guest is migrated:
Data returned from vSBI probably shouldn't change across migration, or else
you may confuse the guest.

Jan

Re: [PATCH v1 3/3] xen/riscv: add RISC-V virtual SBI base extension support for guests

[Oleksii Kurochko]

On 12/15/25 9:20 AM, Jan Beulich wrote:
> On 12.12.2025 16:25, Oleksii Kurochko wrote:
>> On 12/8/25 4:15 PM, Jan Beulich wrote:
>>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/riscv/vsbi/vsbi-base-extension.c
>>>> @@ -0,0 +1,52 @@
>>>> +
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +
>>>> +#include <xen/lib.h>
>>>> +#include <xen/sched.h>
>>>> +
>>>> +#include <asm/processor.h>
>>>> +#include <asm/sbi.h>
>>>> +#include <asm/vsbi.h>
>>>> +
>>>> +extern unsigned long __ro_after_init sbi_spec_version;
>>>> +extern long __ro_after_init sbi_fw_id;
>>>> +extern long __ro_after_init sbi_fw_version;
>>>> +
>>>> +static int vsbi_base_ecall_handler(struct vcpu *vcpu, unsigned long eid,
>>>> +                                   unsigned long fid,
>>>> +                                   struct cpu_user_regs *regs)
>>>> +{
>>>> +    int ret = 0;
>>>> +    struct sbiret sbi_ret;
>>>> +
>>>> +    switch ( fid ) {
>>>> +    case SBI_EXT_BASE_GET_SPEC_VERSION:
>>>> +        regs->a1 = sbi_spec_version;
>>> Wouldn't this need to be the minimum of what firmware supports and what Xen
>>> supports / knows about? (Assuming backward compatibility among the spec
>>> versions of course.)
>> The base extension is mandatory (according to the spec), and based on some Linux
>> commits from contributors to the OpenSBI spec, it is also intended to allow
>> backward compatibility and probing of future extensions (although I was not able
>> to find this explicitly stated in the spec).
>>
>> However, none of this guarantees that everything else is backward compatible.
>> For example, the entire v0.1 SBI has been moved to the legacy extension, which
>> is now an optional extension. This is technically a backwards-incompatible
>> change because the legacy extension is optional, and v0.1 of the SBI does not
>> allow probing.
>>
>> Regarding what should be written to|regs->a1|, I think you are right: it should
>> be the minimum of what the firmware provides and what Xen supports. Otherwise,
>> if|sbi_spec_version| is set to 2.0 and we return 2.0 to the guest, the guest might
>> try to probe the DBGN (which Xen does not currently support) extension and use
>> it instead of the legacy extension for the early console.
>>
>>
>>>> +        break;
>>>> +    case SBI_EXT_BASE_GET_IMP_ID:
>>>> +        regs->a1 = sbi_fw_id;
>>>> +        break;
>>>> +    case SBI_EXT_BASE_GET_IMP_VERSION:
>>>> +        regs->a1 = sbi_fw_version;
>>> Same concern here, but see also below.
>> For SBI_EXT_BASE_GET_IMP_ID, I think we want to return XEN id which is according
>> to OpenSBI spec is 7.
>>
>> Something similar for SBI_EXT_BASE_GET_IMP_VERSION, maybe we want to return Xen
>> version code (XEN_FULLVERSION).
>>
>>>> +        break;
>>>> +    case SBI_EXT_BASE_GET_MVENDORID:
>>>> +    case SBI_EXT_BASE_GET_MARCHID:
>>>> +    case SBI_EXT_BASE_GET_MIMPID:
>>>> +        sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
>>> This may be okay to do for the hardware domain, but hardly for DomU-s.
>> I don’t see an issue with returning the vendor, microarchitecture, and
>> processor ID. This is essentially what other hypervisors do.
>>
>> What would be better to return? Returning 0 could be an option, and according
>> to the RISC-V spec:
>>     This register must be readable in any implementation, but a value of 0 can
>>     be returned to indicate the field is not implemented.
>>
>> So returning 0 would simply indicate that the field is not provided for case
>> of DomUs, and provide it for hardware domain.
>>
>> Would it be better?
>>
>>> Same concern for SBI_EXT_BASE_GET_IMP_ID.
>>>
>>>> +        ret = sbi_ret.error;
>>>> +        regs->a1 = sbi_ret.value;
>>>> +        break;
>>>> +    case SBI_EXT_BASE_PROBE_EXT:
>>>> +        regs->a1 = vsbi_find_extension(regs->a0) ? 1 : 0;
>>> At least for hwdom doesn't this also need combining virtual and
>>> underlying physical lookup, if for some extensions you may pass the
>>> requests down to the physical one (as done above)?
>> I think I understand your intention, but I am not 100% sure that we need to
>> perform a physical lookup. There may be implementation-specific cases where
>> a call is emulated by the hypervisor instead of being passthroughed to
>> OpenSBI.
>> In other words, it could be the case that an extension is fully emulated
>> without requiring support for the corresponding physical extension.
> I don't have sufficient RISC-V knowledge to further comment on this. My main
> concern is that we have to present (a) a consistent picture to both hwdom
> and DomU-s while (b) presenting a properly virtualized view to DomU-s (i.e.
> abstracting away hardware implementation details). In particular for DomU-s
> you will already now need to think of what happens if a guest is migrated:
> Data returned from vSBI probably shouldn't change across migration, or else
> you may confuse the guest.

Okay, now I see more sense in applying your suggestions.

I’ve changed the implementation for all EIDs except|SBI_*_PROBE_EXT| for now.
It still seems unnecessary to perform a physical lookup. In particular, doing a
physical lookup for hwdom in the following way:
     case SBI_EXT_BASE_PROBE_EXT:
         regs.a1 = vsbi_find_extension(regs->a0) ? 1 : 0;

         if ( regs->a1 && is_hardware_domain(vcpu->domain) )
         {
             sbi_ret = sbi_ecall(SBI_EXT_BASE,
                                 SBI_EXT_BASE_PROBE_EXT,
                                 regs->a0, 0, 0, 0, 0, 0);
             regs->a1 = !sbi_ret.error && sbi_ret.value;
         }

         break;

would only result in extra SBI ecall traps into Xen. Eventually, this would lead
to adding support for a new|VSBI_EXT(...)|, which, in the hwdom case, would simply
forward the call to SBI. Once such a|VSBI_EXT()| is provided,|vsbi_find_extension() |would handle everything, and there would be no need for the|is_hardware_domain() |check anymore.

In other words, this approach might help identify which extensions would be useful
to implement in Xen for hwdom. However, I don’t see much value in this kind of
detection logic. If an extension is needed for hwdom, it seems simpler to just
provide a|VSBI_EXT(...)| implementation for it.
That said, I’m generally okay with keeping the|is_hardware_domain()| handling for
|SBI_EXT_BASE_PROBE_EXT|. Just let me know if you still think it would be useful.

Thanks.

~ Oleksii