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
