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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/15/25 9:20 AM, Jan Beulich wrote:
> On 12.12.2025 16:25, Oleksii Kurochko wrote:
> > On 12/8/25 4:15 PM, Jan Beulich wrote:
> > > On 01.12.2025 11:24, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/vsbi/vsbi-base-extension.c
> > > > @@ -0,0 +1,52 @@
> > > > +
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +
> > > > +#include <xen/lib.h>
> > > > +#include <xen/sched.h>
> > > > +
> > > > +#include <asm/processor.h>
> > > > +#include <asm/sbi.h>
> > > > +#include <asm/vsbi.h>
> > > > +
> > > > +extern unsigned long __ro_after_init sbi_spec_version;
> > > > +extern long __ro_after_init sbi_fw_id;
> > > > +extern long __ro_after_init sbi_fw_version;
> > > > +
> > > > +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;
> > > > +
> > > > +    switch ( fid ) {
> > > > +    case SBI_EXT_BASE_GET_SPEC_VERSION:
> > > > +        regs->a1 = sbi_spec_version;
> > > Wouldn't this need to be the minimum of what firmware supports and what Xen
> > > supports / knows about? (Assuming backward compatibility among the spec
> > > versions of course.)
> > The base extension is mandatory (according to the spec), and based on some Linux
> > commits from contributors to the OpenSBI spec, it is also intended to allow
> > backward compatibility and probing of future extensions (although I was not able
> > to find this explicitly stated in the spec).
> >
> > However, none of this guarantees that everything else is backward compatible.
> > For example, the entire v0.1 SBI has been moved to the legacy extension, which
> > is now an optional extension. This is technically a backwards-incompatible
> > change because the legacy extension is optional, and v0.1 of the SBI does not
> > allow probing.
> >
> > Regarding what should be written to|regs->a1|, I think you are right: it should
> > be the minimum of what the firmware provides and what Xen supports. Otherwise,
> > if|sbi_spec_version| is set to 2.0 and we return 2.0 to the guest, the guest 
> > might
> > try to probe the DBGN (which Xen does not currently support) extension and use
> > it instead of the legacy extension for the early console.
> >
> >
> > > > +        break;
> > > > +    case SBI_EXT_BASE_GET_IMP_ID:
> > > > +        regs->a1 = sbi_fw_id;
> > > > +        break;
> > > > +    case SBI_EXT_BASE_GET_IMP_VERSION:
> > > > +        regs->a1 = sbi_fw_version;
> > > Same concern here, but see also below.
> > For SBI_EXT_BASE_GET_IMP_ID, I think we want to return XEN id which is according
> > to OpenSBI spec is 7.
> >
> > Something similar for SBI_EXT_BASE_GET_IMP_VERSION, maybe we want to return Xen
> > version code (XEN_FULLVERSION).
> >
> > > > +        break;
> > > > +    case SBI_EXT_BASE_GET_MVENDORID:
> > > > +    case SBI_EXT_BASE_GET_MARCHID:
> > > > +    case SBI_EXT_BASE_GET_MIMPID:
> > > > +        sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> > > This may be okay to do for the hardware domain, but hardly for DomU-s.
> > I don’t see an issue with returning the vendor, microarchitecture, and
> > processor ID. This is essentially what other hypervisors do.
> >
> > What would be better to return? Returning 0 could be an option, and according
> > to the RISC-V spec:
> >     This register must be readable in any implementation, but a value of 0 can
> >     be returned to indicate the field is not implemented.
> >
> > So returning 0 would simply indicate that the field is not provided for case
> > of DomUs, and provide it for hardware domain.
> >
> > Would it be better?
> >
> > > Same concern for SBI_EXT_BASE_GET_IMP_ID.
> > >
> > > > +        ret = sbi_ret.error;
> > > > +        regs->a1 = sbi_ret.value;
> > > > +        break;
> > > > +    case SBI_EXT_BASE_PROBE_EXT:
> > > > +        regs->a1 = vsbi_find_extension(regs->a0) ? 1 : 0;
> > > At least for hwdom doesn't this also need combining virtual and
> > > underlying physical lookup, if for some extensions you may pass the
> > > requests down to the physical one (as done above)?
> > I think I understand your intention, but I am not 100% sure that we need to
> > perform a physical lookup. There may be implementation-specific cases where
> > a call is emulated by the hypervisor instead of being passthroughed to
> > OpenSBI.
> > In other words, it could be the case that an extension is fully emulated
> > without requiring support for the corresponding physical extension.
> I don't have sufficient RISC-V knowledge to further comment on this. My main
> concern is that we have to present (a) a consistent picture to both hwdom
> and DomU-s while (b) presenting a properly virtualized view to DomU-s (i.e.
> abstracting away hardware implementation details). In particular for DomU-s
> you will already now need to think of what happens if a guest is migrated:
> Data returned from vSBI probably shouldn't change across migration, or else
> you may confuse the guest.
Okay, now I see more sense in applying your suggestions.

I’ve changed the implementation for all EIDs except|SBI_*_PROBE_EXT| for now.
It still seems unnecessary to perform a physical lookup. In particular, doing a
physical lookup for hwdom in the following way:
    case SBI_EXT_BASE_PROBE_EXT:
        regs.a1 = vsbi_find_extension(regs->a0) ? 1 : 0;

        if ( regs->a1 && is_hardware_domain(vcpu->domain) )
        {
            sbi_ret = sbi_ecall(SBI_EXT_BASE,
                                SBI_EXT_BASE_PROBE_EXT,
                                regs->a0, 0, 0, 0, 0, 0);
            regs->a1 = !sbi_ret.error && sbi_ret.value;
        }

        break;

would only result in extra SBI ecall traps into Xen. Eventually, this would lead
to adding support for a new|VSBI_EXT(...)|, which, in the hwdom case, would 
simply
forward the call to SBI. Once such a|VSBI_EXT()| is 
provided,|vsbi_find_extension() |would handle everything, and there would be no 
need for the|is_hardware_domain() |check anymore.

In other words, this approach might help identify which extensions would be 
useful
to implement in Xen for hwdom. However, I don’t see much value in this kind of
detection logic. If an extension is needed for hwdom, it seems simpler to just
provide a|VSBI_EXT(...)| implementation for it.
That said, I’m generally okay with keeping the|is_hardware_domain()| handling 
for
|SBI_EXT_BASE_PROBE_EXT|. Just let me know if you still think it would be 
useful.

Thanks.

~ Oleksii