Re: [PATCH v4 1/1] xen/riscv: add RISC-V virtual SBI base extension support for guests

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

Hello Teddy,

On 12/30/25 7:02 PM, Teddy Astie wrote:
> Le 30/12/2025 à 16:52, Oleksii Kurochko a écrit :
> > Add support of virtual SBI base extension calls for RISC-V guests, delegating
> > hardware-specific queries to the underlying SBI and handling version and
> > firmware ID queries directly.
> >
> > The changes include:
> > 1. Define new SBI base extension function IDs (SBI_EXT_BASE_GET_MVENDORID,
> >      SBI_EXT_BASE_GET_MARCHID, SBI_EXT_BASE_GET_MIMPID).
> > 2. Introduce XEN_SBI_VER_MAJOR, XEN_SBI_VER_MINOR for imeplenataion of
> >      SBI_EXT_BASE_GET_SPEC_VERSION.
> > 4. Introduce SBI_XEN_IMPID to implement SBI_EXT_BASE_GET_IMP_ID.
> > 5. Implement handling of SBI base extension functions, including version,
> >      firmware ID, and machine-specific queries.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v4:
> >    - Move definition of XEN_SBI_VER_{MAJOR, MINOR} to base-extension.c.
> >    - Correct string format for FID: s/#%#lx/#%lu.
> >    - Print first EID then FID (as vsbi/core.c code does).
> > ---
> > Changes in v3:
> >    - s/XEN_SBI_IMPID/SBI_XEN_IMPID
> >    - Add ASSERT(eid == SBI_EXT_BASE) in vsbi_base_ecall_handler().
> >    - Fix code style for switch/case.
> >    - Use current instead of `vcpu` argument as it was dropped from
> >      vsbi_base_ecall_handler() prototype.
> >    - Add comments for define-s XEN_SBI_VER_{MAJOR, MINOR} and SBI_XEN_IMPID.
> > ---
> > Changes in v2:
> >    - s/vsbi-base-extension.*/base-extension.*
> >    - Introduce VCPU_SBI_IMPID, XEN_SBI_VER_MINOR and XEN_SBI_VER_MAJOR.
> >    - Return VCPU_SBI_IMPID in the case of SBI_EXT_BASE_GET_IMP_ID.
> >    - Return Xen version in the case of SBI_EXT_BASE_GET_IMP_VERSION.
> >    - Use domain_crash() instead of panic() for default case.
> >    - For SBI_EXT_BASE_GET_{MVENDORID,MARCHID,MIMPID} abd SBI_EXT_BASE_PROBE_EXT
> >      add handling of a domain is h/w or not.
> > ---
> >    xen/arch/riscv/include/asm/sbi.h     |  6 ++
> >    xen/arch/riscv/vsbi/Makefile         |  1 +
> >    xen/arch/riscv/vsbi/base-extension.c | 82 ++++++++++++++++++++++++++++
> >    3 files changed, 89 insertions(+)
> >    create mode 100644 xen/arch/riscv/vsbi/base-extension.c
> >
> > diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
> > index 751bae6d6654..79f7ff5c5501 100644
> > --- a/xen/arch/riscv/include/asm/sbi.h
> > +++ b/xen/arch/riscv/include/asm/sbi.h
> > @@ -14,6 +14,9 @@
> >    
> >    #include <xen/cpumask.h>
> >    
> > +/* SBI-defined implementation ID */
> > +#define SBI_XEN_IMPID 7
> > +
> >    #define SBI_EXT_0_1_SET_TIMER               0x0
> >    #define SBI_EXT_0_1_CONSOLE_PUTCHAR         0x1
> >    #define SBI_EXT_0_1_CONSOLE_GETCHAR         0x2
> > @@ -32,6 +35,9 @@
> >    #define SBI_EXT_BASE_GET_IMP_ID         0x1
> >    #define SBI_EXT_BASE_GET_IMP_VERSION    0x2
> >    #define SBI_EXT_BASE_PROBE_EXT          0x3
> > +#define SBI_EXT_BASE_GET_MVENDORID      0x4
> > +#define SBI_EXT_BASE_GET_MARCHID        0x5
> > +#define SBI_EXT_BASE_GET_MIMPID         0x6
> >    
> >    /* SBI function IDs for RFENCE extension */
> >    #define SBI_EXT_RFENCE_REMOTE_FENCE_I           0x0
> > diff --git a/xen/arch/riscv/vsbi/Makefile b/xen/arch/riscv/vsbi/Makefile
> > index bc5755cb13d6..8ce470f787c5 100644
> > --- a/xen/arch/riscv/vsbi/Makefile
> > +++ b/xen/arch/riscv/vsbi/Makefile
> > @@ -1,2 +1,3 @@
> > +obj-y += base-extension.o
> >    obj-y += core.o
> >    obj-y += legacy-extension.o
> > diff --git a/xen/arch/riscv/vsbi/base-extension.c 
> > b/xen/arch/riscv/vsbi/base-extension.c
> > new file mode 100644
> > index 000000000000..41a95ae188dd
> > --- /dev/null
> > +++ b/xen/arch/riscv/vsbi/base-extension.c
> > @@ -0,0 +1,82 @@
> > +
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/lib.h>
> > +#include <xen/sched.h>
> > +#include <xen/version.h>
> > +
> > +#include <asm/processor.h>
> > +#include <asm/sbi.h>
> > +#include <asm/vsbi.h>
> > +
> > +/* Xen-controlled SBI version reported to guests */
> > +#define XEN_SBI_VER_MAJOR 0
> > +#define XEN_SBI_VER_MINOR 2
> > +
> > +static int vsbi_base_ecall_handler(unsigned long eid, unsigned long fid,
> > +                                   struct cpu_user_regs *regs)
> > +{
> > +    int ret = 0;
> > +    struct sbiret sbi_ret;
> > +
> > +    ASSERT(eid == SBI_EXT_BASE);
> > +
> > +    switch ( fid )
> > +    {
> > +    case SBI_EXT_BASE_GET_SPEC_VERSION:
> > +        regs->a1 = MASK_INSR(XEN_SBI_VER_MAJOR, SBI_SPEC_VERSION_MAJOR_MASK) |
> > +                   XEN_SBI_VER_MINOR;
> > +        break;
> > +
> > +    case SBI_EXT_BASE_GET_IMP_ID:
> > +        regs->a1 = SBI_XEN_IMPID;
> > +        break;
> > +
> > +    case SBI_EXT_BASE_GET_IMP_VERSION:
> > +        regs->a1 = (xen_major_version() << 16) | xen_minor_version();
> > +        break;
> > +
> > +    case SBI_EXT_BASE_GET_MVENDORID:
> > +    case SBI_EXT_BASE_GET_MARCHID:
> > +    case SBI_EXT_BASE_GET_MIMPID:
> > +        if ( is_hardware_domain(current->domain) )
> > +        {
> > +            sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> > +            ret = sbi_ret.error;
> > +            regs->a1 = sbi_ret.value;
> > +        }
> > +        else
> > +            /*
> > +             * vSBI should present a consistent, virtualized view to guests.
> > +             * In particular, DomU-visible data must remain stable across
> > +             * migration and must not expose hardware-specific details.
> > +             *
> > +             * These register(s) must be readable in any implementation,
> > +             * but a value of 0 can be returned to indicate the field
> > +             * is not implemented.
> > +             */
> > +            regs->a1 = 0;
> > +
> > +        break;
> > +
> > +    case SBI_EXT_BASE_PROBE_EXT:
> > +        regs->a1 = vsbi_find_extension(regs->a0) ? 1 : 0;
> > +        break;
> > +
> > +    default:
> > +        /*
> > +         * TODO: domain_crash() is acceptable here while things are still under
> > +         * development.
> > +         * It shouldn't stay like this in the end though: guests should not
> > +         * be punished like this for something Xen hasn't implemented.
> > +         */
> > +        domain_crash(current->domain,
> > +                     "%s: Unsupported ecall: EID: #%#lx FID: #%lu\n",
> > +                     __func__, eid, fid);
> I think we should rather report "SBI_ERR_NOT_SUPPORTED" (-2) instead
> (eventually logging a warning in Xen) ?
The final goal is to return|SBI_ERR_NOT_SUPPORTED|.|domain_crash()| is used
only temporarily and will be kept until a usable alternative (lets say fully
supported dom0less) is available.

~ Oleksii

> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +VSBI_EXT(base, SBI_EXT_BASE, SBI_EXT_BASE, vsbi_base_ecall_handler)
> Teddy
>
>
> --
> Teddy Astie | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech