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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/6/26 10:55 AM, Jan Beulich wrote:
> On 06.01.2026 10:30, Oleksii Kurochko wrote:
> > On 1/5/26 5:26 PM, Jan Beulich wrote:
> > > On 30.12.2025 16:50, Oleksii Kurochko wrote:
> > > > 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>
> > > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Thanks.
> >
> > > Albeit with a question:
> > >
> > > > --- /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
> > > Is it clear from whatever spec it is that is ...
> > >
> > > > +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;
> > > ... implied here (it's ..._SPEC_VERSION after all) under what conditions the
> > > version would need bumping and what effects this would have on existing (e.g.
> > > migrating-in) guests? Recall that ...
> > For example, sooner or later we will want to use the SBI DBCN (Debug Console
> > Extension) for early debug output for guests, as it provides an API to work with
> > strings instead of single characters. This will require bumping the SBI version
> > to 2.0.
> I fear there's a misunderstanding here, likely on my side: Why would it be 2.0?
> Didn't you say the version is Xen controlled? If so, why not 0.3 or 1.0?
I mentioned 2.0 because SBI DBCN support starts in SBI version 2.0 (2.0-rc2, to 
be
more precise). If we use 0.3 or 1.0 instead, the guest won’t know [1][2] that it
can use the SBI DBCN extension instead of the legacy extension.

[1] https://elixir.bootlin.com/linux/v6.18.3/source/arch/riscv/kernel/sbi.c#L692
[2] 
https://elixir.bootlin.com/linux/v6.18.3/source/drivers/tty/hvc/hvc_riscv_sbi.c#L67

> Contrary to what you said previously, it now looks to me as if the version
> wasn't "Xen-controlled", but instead what we pick reflects functionality
> required by a particular spec version of a spec we do not control. That's
> "SBI version implemented by Xen" to me though, not really a "Xen-controlled"
> version.
I will update the comment above definitions of XEN_SBI_VER_MAJOR and 
XEN_SBI_VER_MINOR
to:
  /* SBI version implemented by Xen and reported to guests */

~ Oleksii

> > I don’t think this should cause any migration issues. If a guest was fully 
> > booted
> > and running with Xen SBI version 0.2, it would continue to use the legacy 
> > extension
> > for early console output (or for hvc console which is using SBI calls in Linux 
> > for
> > the moment). If the guest was still in the initialization stage (before SBI
> > extensions were probed), it would simply use the newer SBI DBCN extension 
> > instead
> > of the Legacy one.
> >
> > ~ Oleksii
> >
> > > > +    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.
> > > ... what is being said here applies to other sub-functions as well. IOW it
> > > looks to me as if the version reported needs to be a per-guest property.
> > >
> > > Jan