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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.12.2025 11:24, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/sbi.h
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -14,8 +14,15 @@
>  
>  #include <xen/cpumask.h>
>  
> -#define SBI_EXT_0_1_CONSOLE_PUTCHAR          0x1
> -#define SBI_EXT_0_1_SHUTDOWN                 0x8
> +#define SBI_EXT_0_1_SET_TIMER           0x0
> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR     0x1

Why the padding adjustment when ...

> +#define SBI_EXT_0_1_CONSOLE_GETCHAR     0x2
> +#define SBI_EXT_0_1_CLEAR_IPI           0x3
> +#define SBI_EXT_0_1_SEND_IPI            0x4
> +#define SBI_EXT_0_1_REMOTE_FENCE_I      0x5
> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA   0x6
> +#define SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID  0x7

... you immediately have one that doesn't fit?

> --- a/xen/arch/riscv/vsbi/Makefile
> +++ b/xen/arch/riscv/vsbi/Makefile
> @@ -1 +1,2 @@
>  obj-y += vsbi.o
> +obj-y += vsbi-legacy-extension.o

No vsbi- prefixes please underneath vsbi/.

> --- /dev/null
> +++ b/xen/arch/riscv/vsbi/vsbi-legacy-extension.c
> @@ -0,0 +1,37 @@
> +
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/lib.h>
> +#include <xen/sched.h>
> +
> +#include <asm/processor.h>
> +#include <asm/vsbi.h>
> +
> +static int vsbi_legacy_ecall_handler(struct vcpu *vcpu, unsigned long eid,
> +                                     unsigned long fid,
> +                                     struct cpu_user_regs *regs)
> +{
> +    int ret = 0;
> +
> +    switch ( eid )
> +    {
> +    case SBI_EXT_0_1_CONSOLE_PUTCHAR:
> +        printk("%c", (char)regs->a0);

This is guest output, so shouldn't use plain printk().

> +        break;
> +
> +    case SBI_EXT_0_1_CONSOLE_GETCHAR:
> +        regs->a0 = SBI_ERR_NOT_SUPPORTED;

This will be overwritten with the return value you pass to the caller (i.e. 0),
by that caller (i.e. vsbi_handle_ecall()).

> +        break;
> +
> +    default:
> +        panic("%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
> +              __func__, fid, eid);

Please don't. domain_crash() may be okay to use here, but crashing the 
hypervisor
because of unexpected guest input isn't okay.

> +             break;

Bad indentation.

Jan