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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/18/25 3:32 PM, Jan Beulich wrote:
> On 17.12.2025 17:54, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/sbi.h
> > +++ b/xen/arch/riscv/include/asm/sbi.h
> > @@ -14,6 +14,10 @@
> >   
> >   #include <xen/cpumask.h>
> >   
> > +#define XEN_SBI_VER_MAJOR 0
> > +#define XEN_SBI_VER_MINOR 2
> > +#define XEN_SBI_IMPID 7
> Are these numbers part of the spec (sorry, lack of a reference makes me wonder,
> plus if that were the case, I'd kind of expect the names to be SBI_XEN_..., not
> XEN_SBI_...)?
XEN_SBI_IMPID is a number defined by the SBI specification:
  
https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-base.adoc#sbi-implementation-ids

XEN_SBI_VER_MAJOR and XEN_SBI_VER_MINOR somehow also is a part of the spec, 
there is
no such defines explicitly, but it is real numbers of the SBI version.

I will rename the defines accordingly:
 - s/XEN_SBI_VER_MAJOR/SBI_XEN_VER_MAJOR
 - s/XEN_SBI_VER_MINOR/SBI_XEN_VER_MINOR
 - s/XEN_SBI_IMPID/SBI_XEN_IMPID

> > --- /dev/null
> > +++ b/xen/arch/riscv/vsbi/base-extension.c
> > @@ -0,0 +1,71 @@
> > +
> > +/* 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>
> > +
> > +static int vsbi_base_ecall_handler(struct vcpu *vcpu, 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 ) {
> Nit: Brace placement.
>
> > +    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 = XEN_SBI_IMPID;
> > +        break;
> > +    case SBI_EXT_BASE_GET_IMP_VERSION:
> > +        regs->a1 = (xen_major_version() << 16) | xen_minor_version();
> > +        break;
> Along those lines here - are we free to use an arbitrary layout (shifting major 
> by
> 16 bits), or is this mandated by the spec? At least in the latter case, the 16 
> will
> want to gain a #define.
I would say that we are free to use an arbitrary layout. The specification says:
  The encoding of this version number is specific to the SBI implementation.
(https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-base.adoc#function-get-sbi-implementation-version-fid-2)

So this fully depends on how a specific SBI implementation decides to encode the
version. For Xen, I simply copied the approach used by OpenSBI:
/**
 * OpenSBI 32-bit version with:
 * 1. upper 16-bits as major number
 * 2. lower 16-bits as minor number
 */
#define OPENSBI_VERSION ((OPENSBI_VERSION_MAJOR << 16) | \
                         (OPENSBI_VERSION_MINOR))

Therefore, I think it is fine to keep Xen’s implementation as it is now, without
introducing additional defines.

Thanks.

~ Oleksii