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.

The implementation uses the existing virtual SBI framework to handle legacy
SBI ecalls, ensuring compatibility with older SBI specifications in
RISC-V guests.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - s/vsbi-legacy-extension.*/legacy-extension.*
 - Correct padding for SBI_EXT_0_1_* macros.
 - Set ret = SBI_ERR_NOT_SUPPORTED; in the case of vsbi_legacy_ecall_handler()
   instead of regs->a0 = ... as regs->a0 will be overwritten in
   vsbi_handle_ecall().
 - Use domain_crash() instead of panic() in vsbi_legacy_ecall_handler() and
   add TODO.
 - Use newly introduced VSBI_EXT macros instead of VSBI_EXT_{START,END}.
 - Introduce vsbi_print_line() and use it instead of plain printk() in the
   handler of SBI_EXT_0_1_CONSOLE_PUTCHAR.
---
 xen/arch/riscv/include/asm/sbi.h       | 11 ++++-
 xen/arch/riscv/vsbi/Makefile           |  1 +
 xen/arch/riscv/vsbi/legacy-extension.c | 65 ++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/riscv/vsbi/legacy-extension.c

diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index ade24a572d..751bae6d66 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -14,8 +14,15 @@
 
 #include <xen/cpumask.h>
 
-#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
-#define SBI_EXT_0_1_SHUTDOWN			0x8
+#define SBI_EXT_0_1_SET_TIMER               0x0
+#define SBI_EXT_0_1_CONSOLE_PUTCHAR         0x1
+#define SBI_EXT_0_1_CONSOLE_GETCHAR         0x2
+#define SBI_EXT_0_1_CLEAR_IPI               0x3
+#define SBI_EXT_0_1_SEND_IPI                0x4
+#define SBI_EXT_0_1_REMOTE_FENCE_I          0x5
+#define SBI_EXT_0_1_REMOTE_SFENCE_VMA       0x6
+#define SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID  0x7
+#define SBI_EXT_0_1_SHUTDOWN                0x8
 
 #define SBI_EXT_BASE                    0x10
 #define SBI_EXT_RFENCE                  0x52464E43
diff --git a/xen/arch/riscv/vsbi/Makefile b/xen/arch/riscv/vsbi/Makefile
index 820eb10ac2..bc5755cb13 100644
--- a/xen/arch/riscv/vsbi/Makefile
+++ b/xen/arch/riscv/vsbi/Makefile
@@ -1 +1,2 @@
 obj-y += core.o
+obj-y += legacy-extension.o
diff --git a/xen/arch/riscv/vsbi/legacy-extension.c b/xen/arch/riscv/vsbi/legacy-extension.c
new file mode 100644
index 0000000000..75280de3bc
--- /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)
+{
+    struct domain *cd = current->domain;
+    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);
+        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);
+        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.
+         */
+        domain_crash(vcpu->domain,
+                     "%s: Unsupported ecall: FID: #%lx, EID: #%lx\n",
+                     __func__, fid, eid);
+        break;
+    }
+
+    return ret;
+}
+
+VSBI_EXT(legacy, SBI_EXT_0_1_SET_TIMER, SBI_EXT_0_1_SHUTDOWN,
+         vsbi_legacy_ecall_handler);
-- 
2.52.0

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

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

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.

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

Jan

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

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