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.0On 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
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
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
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
© 2016 - 2025 Red Hat, Inc.