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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/18/25 3:20 PM, Jan Beulich wrote:
> On 17.12.2025 17:54, Oleksii Kurochko wrote:
> > This commit adds support for legacy SBI extensions (version 0.1) in Xen
> > for guest domains.
> >
> > The changes include:
> > 1. Define all legacy SBI extension IDs (0x0 to 0x8) for better clarity and
> >     completeness.
> > 2. Implement handling of legacy SBI extensions, starting with support for
> >     SBI_EXT_0_1_CONSOLE_{PUT,GET}CHAR.
> I can't spot any actual support for GETCHAR.
>
> > --- /dev/null
> > +++ b/xen/arch/riscv/vsbi/legacy-extension.c
> > @@ -0,0 +1,65 @@
> > +
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/console.h>
> > +#include <xen/lib.h>
> > +#include <xen/sched.h>
> > +
> > +#include <asm/processor.h>
> > +#include <asm/vsbi.h>
> > +
> > +static void vsbi_print_line(char c)
> Misleading function name? The parameter doesn't fit the name, and ...
It was called only because of ...

> > +{
> > +    struct domain *cd = current->domain;
> I guess you copied this code from somewhere, but a variable of this type and
> contents wants to be named "currd".
>
> > +    struct domain_console *cons = cd->console;
> > +
> > +    if ( !is_console_printable(c) )
> > +        return;
> > +
> > +    spin_lock(&cons->lock);
> > +    ASSERT(cons->idx < ARRAY_SIZE(cons->buf));
> > +    if ( c != '\n' )
> > +        cons->buf[cons->idx++] = c;
> > +    if ( (cons->idx == (ARRAY_SIZE(cons->buf) - 1)) || (c == '\n') )
> > +    {
> > +        cons->buf[cons->idx] = '\0';
> > +        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cons->buf);
> ... you also only print a line under certain conditions.
... this. (for the same reason as hvm_print_line() which has an argument
'uint32_t *val' but inside function working with chars because of
'char c = *val;')

I will change the name to vsbi_print_char().

> > +        cons->idx = 0;
> > +    }
> > +    spin_unlock(&cons->lock);
> > +}
> > +
> > +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:
> > +        vsbi_print_line((char)regs->a0);
> The cast isn't really needed, is it?
No, I think it could be omitted.

>   And just to double-check: The spec demands
> the upper bits to be ignored? (A link to the spec could have been useful, e.g.
> in the cover letter.)
It doesn't mention anything about upper bit for PUTCHAR:
  
https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-legacy.adoc#extension-console-putchar-eid-0x01
  (I will add a link to the spec in the cover letter.)
So I assume that they don't care about it. (IIUC your question correctly).

I also checked what OpenSBI does and it just call the following function:
  void sbi_putc(char ch)
  {
        nputs_all(&ch, 1);
  }
in the following way: sbi_putc(regs->a0);


> > +        break;
> > +
> > +    case SBI_EXT_0_1_CONSOLE_GETCHAR:
> > +        ret = SBI_ERR_NOT_SUPPORTED;
> > +        break;
> > +
> > +    default:
> > +        /*
> > +         * TODO: domain_crash() is acceptable here while things are still under
> > +         * development.
> > +         * It shouldn't stay like this in the end though: guests should not
> > +         * be punished like this for something Xen hasn't implemented.
> > +         */
> Question then is why SBI_EXT_0_1_CONSOLE_GETCHAR gets a separate case block.
Because we intentionally do not 
support|SBI_EXT_0_1_CONSOLE_GETCHAR|,|domain_crash()|
should not be called for it. Currently,|domain_crash()| is used only for 
debugging
purposes while Xen for RISC-V is still under development. In the future, the
default case should simply return|SBI_ERR_NOT_SUPPORTED|, and
the|SBI_EXT_0_1_CONSOLE_GETCHAR| case block will be removed.

Thanks.

~ Oleksii