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
