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. Make sbi_spec_version, sbi_fw_id, and sbi_fw_version global variables for
use in virtual SBI handling, removing redundant local declarations in
sbi_init.
3. Implement handling of SBI base extension functions, including version,
firmware ID, and machine-specific queries.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/include/asm/sbi.h | 3 ++
xen/arch/riscv/sbi.c | 8 ++--
xen/arch/riscv/vsbi/Makefile | 1 +
xen/arch/riscv/vsbi/vsbi-base-extension.c | 52 +++++++++++++++++++++++
4 files changed, 61 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/riscv/vsbi/vsbi-base-extension.c
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index e7d5d707b1..98ba872ef3 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -32,6 +32,9 @@
#define SBI_EXT_BASE_GET_IMP_ID 0x1
#define SBI_EXT_BASE_GET_IMP_VERSION 0x2
#define SBI_EXT_BASE_PROBE_EXT 0x3
+#define SBI_EXT_BASE_GET_MVENDORID 0x4
+#define SBI_EXT_BASE_GET_MARCHID 0x5
+#define SBI_EXT_BASE_GET_MIMPID 0x6
/* SBI function IDs for RFENCE extension */
#define SBI_EXT_RFENCE_REMOTE_FENCE_I 0x0
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
index 425dce44c6..97cbf84c21 100644
--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -23,7 +23,9 @@
#include <asm/processor.h>
#include <asm/sbi.h>
-static unsigned long __ro_after_init sbi_spec_version = SBI_SPEC_VERSION_DEFAULT;
+unsigned long __ro_after_init sbi_spec_version = SBI_SPEC_VERSION_DEFAULT;
+long __ro_after_init sbi_fw_id;
+long __ro_after_init sbi_fw_version;
struct sbiret sbi_ecall(unsigned long ext, unsigned long fid,
unsigned long arg0, unsigned long arg1,
@@ -313,8 +315,8 @@ int __init sbi_init(void)
if ( !sbi_spec_is_0_1() )
{
- long sbi_fw_id = sbi_get_firmware_id();
- long sbi_fw_version = sbi_get_firmware_version();
+ sbi_fw_id = sbi_get_firmware_id();
+ sbi_fw_version = sbi_get_firmware_version();
BUG_ON((sbi_fw_id < 0) || (sbi_fw_version < 0));
diff --git a/xen/arch/riscv/vsbi/Makefile b/xen/arch/riscv/vsbi/Makefile
index 4da625db9a..07ae27b99e 100644
--- a/xen/arch/riscv/vsbi/Makefile
+++ b/xen/arch/riscv/vsbi/Makefile
@@ -1,2 +1,3 @@
obj-y += vsbi.o
+obj-y += vsbi-base-extension.o
obj-y += vsbi-legacy-extension.o
diff --git a/xen/arch/riscv/vsbi/vsbi-base-extension.c b/xen/arch/riscv/vsbi/vsbi-base-extension.c
new file mode 100644
index 0000000000..88f4567cb1
--- /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;
+ 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;
+ 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);
+ 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;
+ break;
+ default:
+ panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
+ __func__, fid, eid);
+ break;
+ }
+
+ return ret;
+}
+
+VSBI_EXT_START(base, SBI_EXT_BASE, SBI_EXT_BASE, vsbi_base_ecall_handler)
+VSBI_EXT_END
--
2.52.0On 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.)
> + 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.
> + 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.
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)?
> + break;
> + default:
> + panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
> + __func__, fid, eid);
Again - inappropriate for anything controlled by guests.
Jan
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
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
© 2016 - 2025 Red Hat, Inc.