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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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.

~ Oleksii