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