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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.)

> +        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