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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 12/12/25 4:25 PM, 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.

I think we could still introduce the following in Xen:
   #define XEN_SBI_VERSION_MAJOR 0
   #define XEN_SBI_VERSION_MINOR 2
and pass it to the guest as:
   regs-> a1 = (XEN_SBI_VERSION_MAJOR << SBI_SPEC_VERSION_MAJOR_SHIFT) | XEN_SBI_VERSION_MINOR;

IMO, this should be sufficient because:
1. We can fully emulate the base extension in Xen without calling into OpenSBI.
    This covers the case where OpenSBI might return version 0.1,which is unlikely, as all
    boards with hypervisor extension support at least 0.2, and in practice even 2.0, while we
    report 0.2 to the guest.
2. Even if OpenSBI returns, for example, version 2.0 and we tell the guest that we support
    0.2, it should still be fine, as the base extension is at least backward compatible.

In other words, I think we should care only about what Xen supports and provide it to a
guest. Any concerns about that?

~ Oleksii