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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.12.2025 10:29, Oleksii Kurochko wrote:
> 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.

Well, no, or at least not longer term: Like you may not ASSERT() or BUG_ON() on
guest controlled data, you may not WARN_ON() on such.

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

It counts as an implementation, but calling such "support" goes too far imo.

Jan