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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.12.2025 16:50, Oleksii Kurochko wrote:
> 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. Introduce XEN_SBI_VER_MAJOR, XEN_SBI_VER_MINOR for imeplenataion of
>    SBI_EXT_BASE_GET_SPEC_VERSION.
> 4. Introduce SBI_XEN_IMPID to implement SBI_EXT_BASE_GET_IMP_ID.
> 5. Implement handling of SBI base extension functions, including version,
>    firmware ID, and machine-specific queries.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Albeit with a question:

> --- /dev/null
> +++ b/xen/arch/riscv/vsbi/base-extension.c
> @@ -0,0 +1,82 @@
> +
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +#include <xen/version.h>
> +
> +#include <asm/processor.h>
> +#include <asm/sbi.h>
> +#include <asm/vsbi.h>
> +
> +/* Xen-controlled SBI version reported to guests */
> +#define XEN_SBI_VER_MAJOR 0
> +#define XEN_SBI_VER_MINOR 2

Is it clear from whatever spec it is that is ...

> +static int vsbi_base_ecall_handler(unsigned long eid, unsigned long fid,
> +                                   struct cpu_user_regs *regs)
> +{
> +    int ret = 0;
> +    struct sbiret sbi_ret;
> +
> +    ASSERT(eid == SBI_EXT_BASE);
> +
> +    switch ( fid )
> +    {
> +    case SBI_EXT_BASE_GET_SPEC_VERSION:
> +        regs->a1 = MASK_INSR(XEN_SBI_VER_MAJOR, SBI_SPEC_VERSION_MAJOR_MASK) 
> |
> +                   XEN_SBI_VER_MINOR;
> +        break;

... implied here (it's ..._SPEC_VERSION after all) under what conditions the
version would need bumping and what effects this would have on existing (e.g.
migrating-in) guests? Recall that ...

> +    case SBI_EXT_BASE_GET_IMP_ID:
> +        regs->a1 = SBI_XEN_IMPID;
> +        break;
> +
> +    case SBI_EXT_BASE_GET_IMP_VERSION:
> +        regs->a1 = (xen_major_version() << 16) | xen_minor_version();
> +        break;
> +
> +    case SBI_EXT_BASE_GET_MVENDORID:
> +    case SBI_EXT_BASE_GET_MARCHID:
> +    case SBI_EXT_BASE_GET_MIMPID:
> +        if ( is_hardware_domain(current->domain) )
> +        {
> +            sbi_ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> +            ret = sbi_ret.error;
> +            regs->a1 = sbi_ret.value;
> +        }
> +        else
> +            /*
> +             * vSBI should present a consistent, virtualized view to guests.
> +             * In particular, DomU-visible data must remain stable across
> +             * migration and must not expose hardware-specific details.

... what is being said here applies to other sub-functions as well. IOW it
looks to me as if the version reported needs to be a per-guest property.

Jan