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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/22/25 8:40 AM, Jan Beulich wrote:
> On 19.12.2025 21:04, Oleksii Kurochko wrote:
> > 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 fear such a conclusion cannot be easily drawn. The parameter there is even
> "int". Anything not ASCII will remain unclear how to handle until the spec is
> changed to actually say what is intended.
Then it makes sense to add "WARN_ON(regs->a0 >> __CHAR_BIT__);" in
SBI_EXT_0_1_CONSOLE_PUTCHAR case block.

> > > > +        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.
> That contradicts the patch description saying "starting with support for
> SBI_EXT_0_1_CONSOLE_{PUT,GET}CHAR." (Still in context at the top.)
I will update the patch description. I just thought that that returning of "not 
supported"
for *_GETCHAR() could count as an implementation.

Thanks.

~ Oleksii