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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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