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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 ...

> +{
> +    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.

> +        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? 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.)

> +        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.

Jan