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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/8/25 4:05 PM, Jan Beulich wrote:
> 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?
IDK, the padding adjustment shouldn't be done in this way.
I will correct it.

> > --- /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().
I think that I don't know what should be used instead. Could you suggest me 
something
or point to the code in other arch-s?

Or do you mean that guest_printk() should be used?

> > +        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()).
Oh, thanks. It should be "ret = SBI_ERR_NOT_SUPPORTED;" here.


> > +        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.
|domain_crash()| is better. I also considered just 
returning|SBI_ERR_NOT_SUPPORTED|,
but it wasn’t too convenient for debugging which FID/EID the guest was called,
so I started using|panic()| instead.

Thanks.

~ Oleksii