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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan