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 <[email protected]>
---
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
