SBI Debug Console Extension Proposal (Draft v1)

SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

Hi All,

Below is the draft proposal for SBI Debug Console Extension.

Please review it and provide feedback.

Thanks,
Anup

Debug Console Extension (EID #0x4442434E "DBCN")
================================================

The debug console extension defines a generic mechanism for boot-time
early prints from supervisor-mode software which allows users to catch
boot-time issues in supervisor-mode software.

This extension replaces legacy console putchar (EID #0x01) extension
and it is better in following ways:
1) It follows the new calling convention defined for SBI v1.0
   (or higher).
2) It is based on a shared memory area between SBI implementation
   and supervisor-mode software so multiple characters can be
   printed using a single SBI call.

The supervisor-mode software must set the shared memory area before
printing characters on the debug console. Also, all HARTs share the
same shared memory area so only one HART needs to set it at boot-time.

Function: Set Console Area (FID #0)
-----------------------------------

struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
                                         unsigned long size)

Set the shared memory area specified by `addr_div_by_2` and `size`
parameters. The `addr_div_by_4` parameter is base address of the
shared memory area right shifted by 2 whereas `size` parameter is
the size of shared memory area in bytes.

The shared memory area should be normal cacheable memory for the
supervisor-mode software. Also, the shared memory area is global
across all HARTs so SBI implementation must ensure atomicity in
setting the shared memory area.

Errors:
SBI_SUCCESS                - Shared memory area set successfully.
SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
                          `addr_div_by_2` and `size` parameters
                          is not normal cacheable memory or not
                          accessible to supervisor-mode software.

Function: Console Puts (FID #1)
-------------------------------

struct sbiret sbi_debug_console_puts(unsigned long area_offset,
                                     unsigned long num_chars)

Print the string specified by `area_offset` and `num_chars` on
the debug console. The `area_offset` parameter is the start of
string in the shard memory area whereas `num_chars` parameter
is the number of characters (or bytes) in the string.

This is a blocking SBI call and will only return after printing
all characters of the string.

Errors:
SBI_SUCCESS                - Characters printed successfully.
SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
                          `area_offset`) or end of the string
                          (i.e. `area_offset + num_chars`) is
                          outside shared memory area.

[sig-hypervisors] SBI Debug Console Extension Proposal (Draft v1)

[Dylan Reid]

On Wed, Jun 01, 2022 at 09:47:32PM +0530, Anup Patel wrote:
> Hi All,
> 
> Below is the draft proposal for SBI Debug Console Extension.
> 
> Please review it and provide feedback.
> 
> Thanks,
> Anup
> 
> Debug Console Extension (EID #0x4442434E "DBCN")
> ================================================
> 
> The debug console extension defines a generic mechanism for boot-time
> early prints from supervisor-mode software which allows users to catch
> boot-time issues in supervisor-mode software.
> 
> This extension replaces legacy console putchar (EID #0x01) extension
> and it is better in following ways:

Thanks, it will be nice to drop putchar.

> 1) It follows the new calling convention defined for SBI v1.0
>    (or higher).
> 2) It is based on a shared memory area between SBI implementation
>    and supervisor-mode software so multiple characters can be
>    printed using a single SBI call.
> 
> The supervisor-mode software must set the shared memory area before
> printing characters on the debug console. Also, all HARTs share the
> same shared memory area so only one HART needs to set it at boot-time.
> 
> Function: Set Console Area (FID #0)
> -----------------------------------
> 
> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
>                                          unsigned long size)
> 
> Set the shared memory area specified by `addr_div_by_2` and `size`
> parameters. The `addr_div_by_4` parameter is base address of the
> shared memory area right shifted by 2 whereas `size` parameter is
> the size of shared memory area in bytes.
> 
> The shared memory area should be normal cacheable memory for the
> supervisor-mode software. Also, the shared memory area is global
> across all HARTs so SBI implementation must ensure atomicity in
> setting the shared memory area.
> 
> Errors:
> SBI_SUCCESS                - Shared memory area set successfully.
> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
>                           `addr_div_by_2` and `size` parameters
>                           is not normal cacheable memory or not
>                           accessible to supervisor-mode software.
> 
> Function: Console Puts (FID #1)
> -------------------------------
> 
> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
>                                      unsigned long num_chars)

What is the motivation for `area_offset`? Will the supervisor use
different offsets for different harts?

What are the advantages and disadvantages of the offset vs. using a ring
buffer for example?

> 
> Print the string specified by `area_offset` and `num_chars` on
> the debug console. The `area_offset` parameter is the start of
> string in the shard memory area whereas `num_chars` parameter
> is the number of characters (or bytes) in the string.
> 
> This is a blocking SBI call and will only return after printing
> all characters of the string.
> 
> Errors:
> SBI_SUCCESS                - Characters printed successfully.
> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
>                           `area_offset`) or end of the string
>                           (i.e. `area_offset + num_chars`) is
>                           outside shared memory area.
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#101): https://lists.riscv.org/g/sig-hypervisors/message/101
> Mute This Topic: https://lists.riscv.org/mt/91480123/6320247
> Group Owner: sig-hypervisors+owner at lists.riscv.org
> Unsubscribe: https://lists.riscv.org/g/sig-hypervisors/leave/11085586/6320247/1694467148/xyzzy [dylan at rivosinc.com]
> -=-=-=-=-=-=-=-=-=-=-=-
> 
> 

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Heinrich Schuchardt]

On 6/1/22 18:17, Anup Patel wrote:
> Hi All,
> 
> Below is the draft proposal for SBI Debug Console Extension.
> 
> Please review it and provide feedback.
> 
> Thanks,
> Anup
> 
> Debug Console Extension (EID #0x4442434E "DBCN")
> ================================================
> 
> The debug console extension defines a generic mechanism for boot-time
> early prints from supervisor-mode software which allows users to catch
> boot-time issues in supervisor-mode software.
> 
> This extension replaces legacy console putchar (EID #0x01) extension
> and it is better in following ways:

Thanks for starting to close this gap.

> 1) It follows the new calling convention defined for SBI v1.0
>     (or higher).
> 2) It is based on a shared memory area between SBI implementation
>     and supervisor-mode software so multiple characters can be
>     printed using a single SBI call.

I miss a discussion of the conflicts that can arise if the configuration 
of the serial console is changed by the caller.

Do we need an ecall that closes the SBI console to further access?

> 
> The supervisor-mode software must set the shared memory area before
> printing characters on the debug console. Also, all HARTs share the
> same shared memory area so only one HART needs to set it at boot-time.

Isn't it M-mode software that has to program the MMU to allow all harts 
in M-mode and S-mode access to the memory area? What is the S-mode 
software to do about the memory area prior to calling 
sbi_debug_console_set_area()?

> 
> Function: Set Console Area (FID #0)
> -----------------------------------
> 
> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
>                                           unsigned long size)

The console output is needed on the very start of the S-mode software, 
before setting up anything.

Can we avoid this extra function?

Can we simply assume that the caller of sbi_debug_console_puts() 
provides a physical address pointer to a memory area that is suitable?

> 
> Set the shared memory area specified by `addr_div_by_2` and `size`

%s/addr_div_by_2/addr_div_by_4/

> parameters. The `addr_div_by_4` parameter is base address of the

%s/is base/is the base/

> shared memory area right shifted by 2 whereas `size` parameter is
> the size of shared memory area in bytes.

Why shifting the address? I would prefer to keep it simple for the 
caller. If the alignment is not suitable, return an error.

But why is an alignment needed here at all? And why 4 aligned?

> 
> The shared memory area should be normal cacheable memory for the
> supervisor-mode software. Also, the shared memory area is global
> across all HARTs so SBI implementation must ensure atomicity in
> setting the shared memory area.
> 
> Errors:
> SBI_SUCCESS                - Shared memory area set successfully.
> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
>                            `addr_div_by_2` and `size` parameters
>                            is not normal cacheable memory or not
>                            accessible to supervisor-mode software.
> 
> Function: Console Puts (FID #1)
> -------------------------------
> 
> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
>                                       unsigned long num_chars)

I would prefer to simply pass a physical address pointer here with no 
requirements on alignment. And no prior SBI call.

Do we need num_chars? Are we expecting to provide binary output? Using 
0x00 as terminator would be adequate in most cases.

What is the requirement on the console? Does it have to support 8bit 
output to allow for UTF-8?

Do we make any assumptions about encoding?

How would we handle a console set up to 7bit + parity if a character > 
0x7f is sent?

> 
> Print the string specified by `area_offset` and `num_chars` on
> the debug console. The `area_offset` parameter is the start of
> string in the shard memory area whereas `num_chars` parameter

%s/shard/shared/

> is the number of characters (or bytes) in the string.
> 
> This is a blocking SBI call and will only return after printing
> all characters of the string.
> 
> Errors:
> SBI_SUCCESS                - Characters printed successfully.
> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
>                            `area_offset`) or end of the string
>                            (i.e. `area_offset + num_chars`) is
>                            outside shared memory area.

There could be other reasons of failures:

* set up of the UART failed in OpenSBI
* no UART defined in the device-tree
* ...

So let us add SBI_ERR_FAILED to the list.

Best regards

Heinrich

SBI Debug Console Extension Proposal (Draft v1)

[Heiko Stübner]

Hi Anup,

Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel:
> Hi All,
> 
> Below is the draft proposal for SBI Debug Console Extension.
> 
> Please review it and provide feedback.
> 
> Thanks,
> Anup
> 
> Debug Console Extension (EID #0x4442434E "DBCN")
> ================================================
> 
> The debug console extension defines a generic mechanism for boot-time
> early prints from supervisor-mode software which allows users to catch
> boot-time issues in supervisor-mode software.
> 
> This extension replaces legacy console putchar (EID #0x01) extension
> and it is better in following ways:
> 1) It follows the new calling convention defined for SBI v1.0
>    (or higher).
> 2) It is based on a shared memory area between SBI implementation
>    and supervisor-mode software so multiple characters can be
>    printed using a single SBI call.
> 
> The supervisor-mode software must set the shared memory area before
> printing characters on the debug console. Also, all HARTs share the
> same shared memory area so only one HART needs to set it at boot-time.
> 
> Function: Set Console Area (FID #0)
> -----------------------------------
> 
> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
>                                          unsigned long size)
> 
> Set the shared memory area specified by `addr_div_by_2` and `size`

typo in the "div_by_2" (not 4 like below and in the function itself) ?


> parameters. The `addr_div_by_4` parameter is base address of the
> shared memory area right shifted by 2 whereas `size` parameter is
> the size of shared memory area in bytes.
> 
> The shared memory area should be normal cacheable memory for the
> supervisor-mode software. Also, the shared memory area is global
> across all HARTs so SBI implementation must ensure atomicity in
> setting the shared memory area.
> 
> Errors:
> SBI_SUCCESS                - Shared memory area set successfully.
> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
>                           `addr_div_by_2` and `size` parameters
>                           is not normal cacheable memory or not
>                           accessible to supervisor-mode software.
> 
> Function: Console Puts (FID #1)
> -------------------------------
> 
> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
>                                      unsigned long num_chars)
> 
> Print the string specified by `area_offset` and `num_chars` on
> the debug console. The `area_offset` parameter is the start of
> string in the shard memory area whereas `num_chars` parameter
> is the number of characters (or bytes) in the string.
> 
> This is a blocking SBI call and will only return after printing
> all characters of the string.
> 
> Errors:
> SBI_SUCCESS                - Characters printed successfully.
> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
>                           `area_offset`) or end of the string
>                           (i.e. `area_offset + num_chars`) is
>                           outside shared memory area.

This will vastly reduce the number of needed ecalls when outputting
characters, so this will probably improve performance quite a bit :-)


I guess I still would like to have an _additional_ single-character
putc call. As mentioned in the other thread [0], especially on consumer
hardware [where there is no elaborate debug infrastructure] this can
be a very handy debugging tool even in the earliest stages of a
booting kernel (both before relocation and even inside the startup
assembly).

I.e. just doing a
	li a7, 1
	li a6, 0
	li a0, 36
	ecall

in any kernel assembly will just output a "$" character right now, without
needing any preparation at all - same with using the current 
sbi_console_putchar() directly in c-code.

This _can_ be very helpful in some cases, so I guess it would be nice
to keep such a functionality around also in the new spec.


Thanks
Heiko


[0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html

SBI Debug Console Extension Proposal (Draft v1)

[Xiang W]

? 2022-06-01???? 21:47 +0530?Anup Patel???
> Hi All,
> 
> Below is the draft proposal for SBI Debug Console Extension.
> 
> Please review it and provide feedback.
> 
> Thanks,
> Anup
The use of these two APIs is too complicated, and the supervisor-mode
software needs to create a data structure to manage the Console Area.
I recommend using the simpler interface:

struct sbiret sbi_debug_console_puts(unsigned long addr,
                                     unsigned long num_chars)

Regards,
Xiang W
> 
> Debug Console Extension (EID #0x4442434E "DBCN")
> ================================================
> 
> The debug console extension defines a generic mechanism for boot-time
> early prints from supervisor-mode software which allows users to catch
> boot-time issues in supervisor-mode software.
> 
> This extension replaces legacy console putchar (EID #0x01) extension
> and it is better in following ways:
> 1) It follows the new calling convention defined for SBI v1.0
> ?? (or higher).
> 2) It is based on a shared memory area between SBI implementation
> ?? and supervisor-mode software so multiple characters can be
> ?? printed using a single SBI call.
> 
> The supervisor-mode software must set the shared memory area before
> printing characters on the debug console. Also, all HARTs share the
> same shared memory area so only one HART needs to set it at boot-time.
> 
> Function: Set Console Area (FID #0)
> -----------------------------------
> 
> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> ???????????????????????????????????????? unsigned long size)
> 
> Set the shared memory area specified by `addr_div_by_2` and `size`
> parameters. The `addr_div_by_4` parameter is base address of the
> shared memory area right shifted by 2 whereas `size` parameter is
> the size of shared memory area in bytes.
> 
> The shared memory area should be normal cacheable memory for the
> supervisor-mode software. Also, the shared memory area is global
> across all HARTs so SBI implementation must ensure atomicity in
> setting the shared memory area.
> 
> Errors:
> SBI_SUCCESS??????????????? - Shared memory area set successfully.
> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> ????????????????????????? `addr_div_by_2` and `size` parameters
> ????????????????????????? is not normal cacheable memory or not
> ????????????????????????? accessible to supervisor-mode software.
> 
> Function: Console Puts (FID #1)
> -------------------------------
> 
> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> ???????????????????????????????????? unsigned long num_chars)
> 
> Print the string specified by `area_offset` and `num_chars` on
> the debug console. The `area_offset` parameter is the start of
> string in the shard memory area whereas `num_chars` parameter
> is the number of characters (or bytes) in the string.
> 
> This is a blocking SBI call and will only return after printing
> all characters of the string.
> 
> Errors:
> SBI_SUCCESS??????????????? - Characters printed successfully.
> SBI_ERR_INVALID_ADDRESS??? - The start of the string (i.e.
> ????????????????????????? `area_offset`) or end of the string
> ????????????????????????? (i.e. `area_offset + num_chars`) is
> ????????????????????????? outside shared memory area.
> 

SBI Debug Console Extension Proposal (Draft v1)

[Xiang W]

? 2022-06-01???? 21:47 +0530?Anup Patel???
> Hi All,
> 
> Below is the draft proposal for SBI Debug Console Extension.
> 
> Please review it and provide feedback.
> 
> Thanks,
> Anup
> 
> Debug Console Extension (EID #0x4442434E "DBCN")
> ================================================
> 
> The debug console extension defines a generic mechanism for boot-time
> early prints from supervisor-mode software which allows users to catch
> boot-time issues in supervisor-mode software.
> 
> This extension replaces legacy console putchar (EID #0x01) extension
> and it is better in following ways:
> 1) It follows the new calling convention defined for SBI v1.0
> ?? (or higher).
> 2) It is based on a shared memory area between SBI implementation
> ?? and supervisor-mode software so multiple characters can be
> ?? printed using a single SBI call.
> 
> The supervisor-mode software must set the shared memory area before
> printing characters on the debug console. Also, all HARTs share the
> same shared memory area so only one HART needs to set it at boot-time.
> 
> Function: Set Console Area (FID #0)
> -----------------------------------
> 
> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> ???????????????????????????????????????? unsigned long size)
> 
> Set the shared memory area specified by `addr_div_by_2` and `size`
> parameters. The `addr_div_by_4` parameter is base address of the
> shared memory area right shifted by 2 whereas `size` parameter is
> the size of shared memory area in bytes.
> 
> The shared memory area should be normal cacheable memory for the
> supervisor-mode software. Also, the shared memory area is global
> across all HARTs so SBI implementation must ensure atomicity in
> setting the shared memory area.
> 
> Errors:
> SBI_SUCCESS??????????????? - Shared memory area set successfully.
> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> ????????????????????????? `addr_div_by_2` and `size` parameters
> ????????????????????????? is not normal cacheable memory or not
> ????????????????????????? accessible to supervisor-mode software.
> 

Shared memory can be a single extension. The three interfaces are as follows

/* The supervisor hands a piece of physical memory to sbi for shared memory */
struct sbiret sbi_shared_memory_extrend(unsigned long addr, unsigned long size);


/* The supervisor applies for a piece of physical memory from sbi */
struct sbiret sbi_shared_memory_alloc(unsigned long size);


/* The supervisor notifies sbi to release the requested memory */
struct sbiret sbi_shared_memory_free(unsigned long addr);


> Function: Console Puts (FID #1)
> -------------------------------
> 
> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> ???????????????????????????????????? unsigned long num_chars)
> 
> Print the string specified by `area_offset` and `num_chars` on
> the debug console. The `area_offset` parameter is the start of
> string in the shard memory area whereas `num_chars` parameter
> is the number of characters (or bytes) in the string.
> 
> This is a blocking SBI call and will only return after printing
> all characters of the string.
> 
> Errors:
> SBI_SUCCESS??????????????? - Characters printed successfully.
> SBI_ERR_INVALID_ADDRESS??? - The start of the string (i.e.
> ????????????????????????? `area_offset`) or end of the string
> ????????????????????????? (i.e. `area_offset + num_chars`) is
> ????????????????????????? outside shared memory area.
> 

[RISC-V] [tech-unixplatformspec] [sig-hypervisors] SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Wed, Jun 1, 2022 at 11:51 PM Dylan Reid <dylan@rivosinc.com> wrote:
>
> On Wed, Jun 01, 2022 at 09:47:32PM +0530, Anup Patel wrote:
> > Hi All,
> >
> > Below is the draft proposal for SBI Debug Console Extension.
> >
> > Please review it and provide feedback.
> >
> > Thanks,
> > Anup
> >
> > Debug Console Extension (EID #0x4442434E "DBCN")
> > ================================================
> >
> > The debug console extension defines a generic mechanism for boot-time
> > early prints from supervisor-mode software which allows users to catch
> > boot-time issues in supervisor-mode software.
> >
> > This extension replaces legacy console putchar (EID #0x01) extension
> > and it is better in following ways:
>
> Thanks, it will be nice to drop putchar.
>
> > 1) It follows the new calling convention defined for SBI v1.0
> >    (or higher).
> > 2) It is based on a shared memory area between SBI implementation
> >    and supervisor-mode software so multiple characters can be
> >    printed using a single SBI call.
> >
> > The supervisor-mode software must set the shared memory area before
> > printing characters on the debug console. Also, all HARTs share the
> > same shared memory area so only one HART needs to set it at boot-time.
> >
> > Function: Set Console Area (FID #0)
> > -----------------------------------
> >
> > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >                                          unsigned long size)
> >
> > Set the shared memory area specified by `addr_div_by_2` and `size`
> > parameters. The `addr_div_by_4` parameter is base address of the
> > shared memory area right shifted by 2 whereas `size` parameter is
> > the size of shared memory area in bytes.
> >
> > The shared memory area should be normal cacheable memory for the
> > supervisor-mode software. Also, the shared memory area is global
> > across all HARTs so SBI implementation must ensure atomicity in
> > setting the shared memory area.
> >
> > Errors:
> > SBI_SUCCESS                - Shared memory area set successfully.
> > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >                           `addr_div_by_2` and `size` parameters
> >                           is not normal cacheable memory or not
> >                           accessible to supervisor-mode software.
> >
> > Function: Console Puts (FID #1)
> > -------------------------------
> >
> > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >                                      unsigned long num_chars)
>
> What is the motivation for `area_offset`? Will the supervisor use
> different offsets for different harts?

There are variety of ways in which supervisor software can use
tarea_offset:
1) Use lock to serialize access to shared memory and always
use fixed offset (maybe zero) from all HARTs
2) No lock to protect shared memory and instead each HART
will use separate offsets to print

In addition to above, bare-metal test code (or assembly sources)
can have pre-populated strings (i.e. "PASS", "FAIL", "ERROR", etc)
in shared memory and simply use different offsets to print different
strings.

>
> What are the advantages and disadvantages of the offset vs. using a ring
> buffer for example?

Mandating a ring on shared memory will make things complicated
for bare metal test code (or assembly sources). Also, there is no
scheduler in M-mode firmware to have worker thread for consuming
bytes from a ring.

The "area_offset" is relatively more flexible in this case because it
allows sophisticated supervisor software to create ring on shared
memory without the SBI implementation knowing about it where:
1) The head & tail will be maintained by supervisor software
2) supervisor software will have a consumer thread to consume
bytes from ring and print using puts()

>
> >
> > Print the string specified by `area_offset` and `num_chars` on
> > the debug console. The `area_offset` parameter is the start of
> > string in the shard memory area whereas `num_chars` parameter
> > is the number of characters (or bytes) in the string.
> >
> > This is a blocking SBI call and will only return after printing
> > all characters of the string.
> >
> > Errors:
> > SBI_SUCCESS                - Characters printed successfully.
> > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >                           `area_offset`) or end of the string
> >                           (i.e. `area_offset + num_chars`) is
> >                           outside shared memory area.
> >
> >
> >
> >
> >
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#1709): https://lists.riscv.org/g/tech-unixplatformspec/message/1709
> Mute This Topic: https://lists.riscv.org/mt/91482999/1774265
> Group Owner: tech-unixplatformspec+owner at lists.riscv.org
> Unsubscribe: https://lists.riscv.org/g/tech-unixplatformspec/unsub [apatel at ventanamicro.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

Regards,
Anup

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 6/1/22 18:17, Anup Patel wrote:
> > Hi All,
> >
> > Below is the draft proposal for SBI Debug Console Extension.
> >
> > Please review it and provide feedback.
> >
> > Thanks,
> > Anup
> >
> > Debug Console Extension (EID #0x4442434E "DBCN")
> > ================================================
> >
> > The debug console extension defines a generic mechanism for boot-time
> > early prints from supervisor-mode software which allows users to catch
> > boot-time issues in supervisor-mode software.
> >
> > This extension replaces legacy console putchar (EID #0x01) extension
> > and it is better in following ways:
>
> Thanks for starting to close this gap.
>
> > 1) It follows the new calling convention defined for SBI v1.0
> >     (or higher).
> > 2) It is based on a shared memory area between SBI implementation
> >     and supervisor-mode software so multiple characters can be
> >     printed using a single SBI call.
>
> I miss a discussion of the conflicts that can arise if the configuration
> of the serial console is changed by the caller.
>
> Do we need an ecall that closes the SBI console to further access?

Usually, the serial port related code in M-mode firmware only uses
status and data registers so for most serial ports support the M-mode
firmware will adapt to serial port configuration changes.

In fact, this is why we never had a special SBI call for serial port
reconfiguration in legacy SBI v0.1 as well.

In case of virtualization, the serial port (or console) is emulated so
the special SBI call is not useful for virtualization.

>
> >
> > The supervisor-mode software must set the shared memory area before
> > printing characters on the debug console. Also, all HARTs share the
> > same shared memory area so only one HART needs to set it at boot-time.
>
> Isn't it M-mode software that has to program the MMU to allow all harts
> in M-mode and S-mode access to the memory area? What is the S-mode
> software to do about the memory area prior to calling
> sbi_debug_console_set_area()?

Actually, it's the S-mode software which is voluntarily giving a portion of
its memory to be used as shared memory. The proposal only mandates
that whatever memory is selected by S-mode software should be a
regular cacheable memory (not IO memory). Also, if Svpbmt is available
then S-mode software should only use memory type 0 in the PTEs.

>
> >
> > Function: Set Console Area (FID #0)
> > -----------------------------------
> >
> > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >                                           unsigned long size)
>
> The console output is needed on the very start of the S-mode software,
> before setting up anything.
>
> Can we avoid this extra function?
>
> Can we simply assume that the caller of sbi_debug_console_puts()
> provides a physical address pointer to a memory area that is suitable?

Theoretically, we can avoid the extra function to set shared memory area
but it will complicate things in future when we have supervisor software
encrypting it's own memory (using special ISA support) because in this
case supervisor software will have to unprotect memory every time the
sbi_debug_console_puts() is called and protect it again after the call.

>
> >
> > Set the shared memory area specified by `addr_div_by_2` and `size`
>
> %s/addr_div_by_2/addr_div_by_4/

Okay, I will update.

>
> > parameters. The `addr_div_by_4` parameter is base address of the
>
> %s/is base/is the base/

Okay, I will update.

>
> > shared memory area right shifted by 2 whereas `size` parameter is
> > the size of shared memory area in bytes.
>
> Why shifting the address? I would prefer to keep it simple for the
> caller. If the alignment is not suitable, return an error.
>
> But why is an alignment needed here at all? And why 4 aligned?

For RV32 S-mode, the physical address space is 34bits wide but
"unsigned long" is 32bits wide. This is because Sv32 PTEs allow
34bits of PPN. In fact, even instructions such as HFENCE.GVMA
have this "address right shift by 2" requirement based on this rationale.

>
> >
> > The shared memory area should be normal cacheable memory for the
> > supervisor-mode software. Also, the shared memory area is global
> > across all HARTs so SBI implementation must ensure atomicity in
> > setting the shared memory area.
> >
> > Errors:
> > SBI_SUCCESS                - Shared memory area set successfully.
> > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >                            `addr_div_by_2` and `size` parameters
> >                            is not normal cacheable memory or not
> >                            accessible to supervisor-mode software.
> >
> > Function: Console Puts (FID #1)
> > -------------------------------
> >
> > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >                                       unsigned long num_chars)
>
> I would prefer to simply pass a physical address pointer here with no
> requirements on alignment. And no prior SBI call.
>
> Do we need num_chars? Are we expecting to provide binary output? Using
> 0x00 as terminator would be adequate in most cases.

Bare-metal tests (or assembly sources) can print sub-strings from
a large per-populated string in shared memory. Assuming that string
is always terminated by 0x00 in sbi_debug_console_puts() will break
this flexibility.

>
> What is the requirement on the console? Does it have to support 8bit
> output to allow for UTF-8?

We need to clarify this. Suggestions ?

>
> Do we make any assumptions about encoding?

Same as above, this needs more clarification. Suggestions ?

I am of the opinion to keep such encoding related assumptions to be
minimal.

>
> How would we handle a console set up to 7bit + parity if a character >
> 0x7f is sent?

I would consider this to be part of the clarification we add for encoding.

>
> >
> > Print the string specified by `area_offset` and `num_chars` on
> > the debug console. The `area_offset` parameter is the start of
> > string in the shard memory area whereas `num_chars` parameter
>
> %s/shard/shared/

Okay, I will update.

>
> > is the number of characters (or bytes) in the string.
> >
> > This is a blocking SBI call and will only return after printing
> > all characters of the string.
> >
> > Errors:
> > SBI_SUCCESS                - Characters printed successfully.
> > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >                            `area_offset`) or end of the string
> >                            (i.e. `area_offset + num_chars`) is
> >                            outside shared memory area.
>
> There could be other reasons of failures:
>
> * set up of the UART failed in OpenSBI
> * no UART defined in the device-tree
> * ...
>
> So let us add SBI_ERR_FAILED to the list.

Okay, I will add.

>
> Best regards
>
> Heinrich

Regards,
Anup

SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Thu, Jun 2, 2022 at 12:02 AM Heiko St?bner <heiko@sntech.de> wrote:
>
> Hi Anup,
>
> Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel:
> > Hi All,
> >
> > Below is the draft proposal for SBI Debug Console Extension.
> >
> > Please review it and provide feedback.
> >
> > Thanks,
> > Anup
> >
> > Debug Console Extension (EID #0x4442434E "DBCN")
> > ================================================
> >
> > The debug console extension defines a generic mechanism for boot-time
> > early prints from supervisor-mode software which allows users to catch
> > boot-time issues in supervisor-mode software.
> >
> > This extension replaces legacy console putchar (EID #0x01) extension
> > and it is better in following ways:
> > 1) It follows the new calling convention defined for SBI v1.0
> >    (or higher).
> > 2) It is based on a shared memory area between SBI implementation
> >    and supervisor-mode software so multiple characters can be
> >    printed using a single SBI call.
> >
> > The supervisor-mode software must set the shared memory area before
> > printing characters on the debug console. Also, all HARTs share the
> > same shared memory area so only one HART needs to set it at boot-time.
> >
> > Function: Set Console Area (FID #0)
> > -----------------------------------
> >
> > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >                                          unsigned long size)
> >
> > Set the shared memory area specified by `addr_div_by_2` and `size`
>
> typo in the "div_by_2" (not 4 like below and in the function itself) ?
>
>
> > parameters. The `addr_div_by_4` parameter is base address of the
> > shared memory area right shifted by 2 whereas `size` parameter is
> > the size of shared memory area in bytes.
> >
> > The shared memory area should be normal cacheable memory for the
> > supervisor-mode software. Also, the shared memory area is global
> > across all HARTs so SBI implementation must ensure atomicity in
> > setting the shared memory area.
> >
> > Errors:
> > SBI_SUCCESS                - Shared memory area set successfully.
> > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >                           `addr_div_by_2` and `size` parameters
> >                           is not normal cacheable memory or not
> >                           accessible to supervisor-mode software.
> >
> > Function: Console Puts (FID #1)
> > -------------------------------
> >
> > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >                                      unsigned long num_chars)
> >
> > Print the string specified by `area_offset` and `num_chars` on
> > the debug console. The `area_offset` parameter is the start of
> > string in the shard memory area whereas `num_chars` parameter
> > is the number of characters (or bytes) in the string.
> >
> > This is a blocking SBI call and will only return after printing
> > all characters of the string.
> >
> > Errors:
> > SBI_SUCCESS                - Characters printed successfully.
> > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >                           `area_offset`) or end of the string
> >                           (i.e. `area_offset + num_chars`) is
> >                           outside shared memory area.
>
> This will vastly reduce the number of needed ecalls when outputting
> characters, so this will probably improve performance quite a bit :-)
>
>
> I guess I still would like to have an _additional_ single-character
> putc call. As mentioned in the other thread [0], especially on consumer
> hardware [where there is no elaborate debug infrastructure] this can
> be a very handy debugging tool even in the earliest stages of a
> booting kernel (both before relocation and even inside the startup
> assembly).
>
> I.e. just doing a
>         li a7, 1
>         li a6, 0
>         li a0, 36
>         ecall
>
> in any kernel assembly will just output a "$" character right now, without
> needing any preparation at all - same with using the current
> sbi_console_putchar() directly in c-code.
>
> This _can_ be very helpful in some cases, so I guess it would be nice
> to keep such a functionality around also in the new spec.

You can easily create multiple pre-populated strings using ".asciiz" in
assembly sources. Just set the base address of pre-populated strings
once on boot hart and print from anywhere using usual 4-5 instruction
(similar to what posted above).

>
>
> Thanks
> Heiko
>
>
> [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html
>
>
>

Regards,
Anup

SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Thu, Jun 2, 2022 at 9:14 AM Xiang W <wxjstz@126.com> wrote:
>
> ? 2022-06-01???? 21:47 +0530?Anup Patel???
> > Hi All,
> >
> > Below is the draft proposal for SBI Debug Console Extension.
> >
> > Please review it and provide feedback.
> >
> > Thanks,
> > Anup
> The use of these two APIs is too complicated, and the supervisor-mode
> software needs to create a data structure to manage the Console Area.
> I recommend using the simpler interface:
>
> struct sbiret sbi_debug_console_puts(unsigned long addr,
>                                      unsigned long num_chars)

Heinrich has a similar suggestion so please see my response to
his comment.

Regards,
Anup

>
> Regards,
> Xiang W
> >
> > Debug Console Extension (EID #0x4442434E "DBCN")
> > ================================================
> >
> > The debug console extension defines a generic mechanism for boot-time
> > early prints from supervisor-mode software which allows users to catch
> > boot-time issues in supervisor-mode software.
> >
> > This extension replaces legacy console putchar (EID #0x01) extension
> > and it is better in following ways:
> > 1) It follows the new calling convention defined for SBI v1.0
> >    (or higher).
> > 2) It is based on a shared memory area between SBI implementation
> >    and supervisor-mode software so multiple characters can be
> >    printed using a single SBI call.
> >
> > The supervisor-mode software must set the shared memory area before
> > printing characters on the debug console. Also, all HARTs share the
> > same shared memory area so only one HART needs to set it at boot-time.
> >
> > Function: Set Console Area (FID #0)
> > -----------------------------------
> >
> > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >                                          unsigned long size)
> >
> > Set the shared memory area specified by `addr_div_by_2` and `size`
> > parameters. The `addr_div_by_4` parameter is base address of the
> > shared memory area right shifted by 2 whereas `size` parameter is
> > the size of shared memory area in bytes.
> >
> > The shared memory area should be normal cacheable memory for the
> > supervisor-mode software. Also, the shared memory area is global
> > across all HARTs so SBI implementation must ensure atomicity in
> > setting the shared memory area.
> >
> > Errors:
> > SBI_SUCCESS                - Shared memory area set successfully.
> > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >                           `addr_div_by_2` and `size` parameters
> >                           is not normal cacheable memory or not
> >                           accessible to supervisor-mode software.
> >
> > Function: Console Puts (FID #1)
> > -------------------------------
> >
> > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >                                      unsigned long num_chars)
> >
> > Print the string specified by `area_offset` and `num_chars` on
> > the debug console. The `area_offset` parameter is the start of
> > string in the shard memory area whereas `num_chars` parameter
> > is the number of characters (or bytes) in the string.
> >
> > This is a blocking SBI call and will only return after printing
> > all characters of the string.
> >
> > Errors:
> > SBI_SUCCESS                - Characters printed successfully.
> > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >                           `area_offset`) or end of the string
> >                           (i.e. `area_offset + num_chars`) is
> >                           outside shared memory area.
> >
>
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

SBI Debug Console Extension Proposal (Draft v1)

[Heiko Stübner]

Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel:
> On Thu, Jun 2, 2022 at 12:02 AM Heiko St?bner <heiko@sntech.de> wrote:
> >
> > Hi Anup,
> >
> > Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel:
> > > Hi All,
> > >
> > > Below is the draft proposal for SBI Debug Console Extension.
> > >
> > > Please review it and provide feedback.
> > >
> > > Thanks,
> > > Anup
> > >
> > > Debug Console Extension (EID #0x4442434E "DBCN")
> > > ================================================
> > >
> > > The debug console extension defines a generic mechanism for boot-time
> > > early prints from supervisor-mode software which allows users to catch
> > > boot-time issues in supervisor-mode software.
> > >
> > > This extension replaces legacy console putchar (EID #0x01) extension
> > > and it is better in following ways:
> > > 1) It follows the new calling convention defined for SBI v1.0
> > >    (or higher).
> > > 2) It is based on a shared memory area between SBI implementation
> > >    and supervisor-mode software so multiple characters can be
> > >    printed using a single SBI call.
> > >
> > > The supervisor-mode software must set the shared memory area before
> > > printing characters on the debug console. Also, all HARTs share the
> > > same shared memory area so only one HART needs to set it at boot-time.
> > >
> > > Function: Set Console Area (FID #0)
> > > -----------------------------------
> > >
> > > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> > >                                          unsigned long size)
> > >
> > > Set the shared memory area specified by `addr_div_by_2` and `size`
> >
> > typo in the "div_by_2" (not 4 like below and in the function itself) ?
> >
> >
> > > parameters. The `addr_div_by_4` parameter is base address of the
> > > shared memory area right shifted by 2 whereas `size` parameter is
> > > the size of shared memory area in bytes.
> > >
> > > The shared memory area should be normal cacheable memory for the
> > > supervisor-mode software. Also, the shared memory area is global
> > > across all HARTs so SBI implementation must ensure atomicity in
> > > setting the shared memory area.
> > >
> > > Errors:
> > > SBI_SUCCESS                - Shared memory area set successfully.
> > > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> > >                           `addr_div_by_2` and `size` parameters
> > >                           is not normal cacheable memory or not
> > >                           accessible to supervisor-mode software.
> > >
> > > Function: Console Puts (FID #1)
> > > -------------------------------
> > >
> > > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> > >                                      unsigned long num_chars)
> > >
> > > Print the string specified by `area_offset` and `num_chars` on
> > > the debug console. The `area_offset` parameter is the start of
> > > string in the shard memory area whereas `num_chars` parameter
> > > is the number of characters (or bytes) in the string.
> > >
> > > This is a blocking SBI call and will only return after printing
> > > all characters of the string.
> > >
> > > Errors:
> > > SBI_SUCCESS                - Characters printed successfully.
> > > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> > >                           `area_offset`) or end of the string
> > >                           (i.e. `area_offset + num_chars`) is
> > >                           outside shared memory area.
> >
> > This will vastly reduce the number of needed ecalls when outputting
> > characters, so this will probably improve performance quite a bit :-)
> >
> >
> > I guess I still would like to have an _additional_ single-character
> > putc call. As mentioned in the other thread [0], especially on consumer
> > hardware [where there is no elaborate debug infrastructure] this can
> > be a very handy debugging tool even in the earliest stages of a
> > booting kernel (both before relocation and even inside the startup
> > assembly).
> >
> > I.e. just doing a
> >         li a7, 1
> >         li a6, 0
> >         li a0, 36
> >         ecall
> >
> > in any kernel assembly will just output a "$" character right now, without
> > needing any preparation at all - same with using the current
> > sbi_console_putchar() directly in c-code.
> >
> > This _can_ be very helpful in some cases, so I guess it would be nice
> > to keep such a functionality around also in the new spec.
> 
> You can easily create multiple pre-populated strings using ".asciiz" in
> assembly sources. Just set the base address of pre-populated strings
> once on boot hart and print from anywhere using usual 4-5 instruction
> (similar to what posted above).

ok, sounds like a plan as well :-)


> > [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html
> >
> >
> >
> 
> Regards,
> Anup
> 

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Heinrich Schuchardt]

On 6/2/22 10:44, Anup Patel wrote:
> On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 6/1/22 18:17, Anup Patel wrote:
>>> Hi All,
>>>
>>> Below is the draft proposal for SBI Debug Console Extension.
>>>
>>> Please review it and provide feedback.
>>>
>>> Thanks,
>>> Anup
>>>
>>> Debug Console Extension (EID #0x4442434E "DBCN")
>>> ================================================
>>>
>>> The debug console extension defines a generic mechanism for boot-time
>>> early prints from supervisor-mode software which allows users to catch
>>> boot-time issues in supervisor-mode software.
>>>
>>> This extension replaces legacy console putchar (EID #0x01) extension
>>> and it is better in following ways:
>>
>> Thanks for starting to close this gap.
>>
>>> 1) It follows the new calling convention defined for SBI v1.0
>>>      (or higher).
>>> 2) It is based on a shared memory area between SBI implementation
>>>      and supervisor-mode software so multiple characters can be
>>>      printed using a single SBI call.
>>
>> I miss a discussion of the conflicts that can arise if the configuration
>> of the serial console is changed by the caller.
>>
>> Do we need an ecall that closes the SBI console to further access?
> 
> Usually, the serial port related code in M-mode firmware only uses
> status and data registers so for most serial ports support the M-mode
> firmware will adapt to serial port configuration changes.
> 
> In fact, this is why we never had a special SBI call for serial port
> reconfiguration in legacy SBI v0.1 as well.
> 
> In case of virtualization, the serial port (or console) is emulated so
> the special SBI call is not useful for virtualization.
> 
>>
>>>
>>> The supervisor-mode software must set the shared memory area before
>>> printing characters on the debug console. Also, all HARTs share the
>>> same shared memory area so only one HART needs to set it at boot-time.
>>
>> Isn't it M-mode software that has to program the MMU to allow all harts
>> in M-mode and S-mode access to the memory area? What is the S-mode
>> software to do about the memory area prior to calling
>> sbi_debug_console_set_area()?
> 
> Actually, it's the S-mode software which is voluntarily giving a portion of
> its memory to be used as shared memory. The proposal only mandates
> that whatever memory is selected by S-mode software should be a
> regular cacheable memory (not IO memory). Also, if Svpbmt is available
> then S-mode software should only use memory type 0 in the PTEs.
> 
>>
>>>
>>> Function: Set Console Area (FID #0)
>>> -----------------------------------
>>>
>>> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
>>>                                            unsigned long size)
>>
>> The console output is needed on the very start of the S-mode software,
>> before setting up anything.
>>
>> Can we avoid this extra function?
>>
>> Can we simply assume that the caller of sbi_debug_console_puts()
>> provides a physical address pointer to a memory area that is suitable?
> 
> Theoretically, we can avoid the extra function to set shared memory area
> but it will complicate things in future when we have supervisor software
> encrypting it's own memory (using special ISA support) because in this
> case supervisor software will have to unprotect memory every time the
> sbi_debug_console_puts() is called and protect it again after the call.

Currently this function is just a nop(). It is not needed in this 
revision of the extension.

The function might be called repeatedly by different threads with 
different values. How do you want to keep track of all of these 
different areas?

Memory shared between different security realms will arise in many 
different scenarios. As this is not console specific it should be in a 
separate extension. That extension should be defined once we have 
clarity about how security realms are managed.

> 
>>
>>>
>>> Set the shared memory area specified by `addr_div_by_2` and `size`
>>
>> %s/addr_div_by_2/addr_div_by_4/
> 
> Okay, I will update.
> 
>>
>>> parameters. The `addr_div_by_4` parameter is base address of the
>>
>> %s/is base/is the base/
> 
> Okay, I will update.
> 
>>
>>> shared memory area right shifted by 2 whereas `size` parameter is
>>> the size of shared memory area in bytes.
>>
>> Why shifting the address? I would prefer to keep it simple for the
>> caller. If the alignment is not suitable, return an error.
>>
>> But why is an alignment needed here at all? And why 4 aligned?
> 
> For RV32 S-mode, the physical address space is 34bits wide but
> "unsigned long" is 32bits wide. This is because Sv32 PTEs allow
> 34bits of PPN. In fact, even instructions such as HFENCE.GVMA
> have this "address right shift by 2" requirement based on this rationale.
> 
>>
>>>
>>> The shared memory area should be normal cacheable memory for the
>>> supervisor-mode software. Also, the shared memory area is global
>>> across all HARTs so SBI implementation must ensure atomicity in
>>> setting the shared memory area.
>>>
>>> Errors:
>>> SBI_SUCCESS                - Shared memory area set successfully.
>>> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
>>>                             `addr_div_by_2` and `size` parameters
>>>                             is not normal cacheable memory or not
>>>                             accessible to supervisor-mode software.
>>>
>>> Function: Console Puts (FID #1)
>>> -------------------------------
>>>
>>> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
>>>                                        unsigned long num_chars)
>>
>> I would prefer to simply pass a physical address pointer here with no
>> requirements on alignment. And no prior SBI call.

sbi_debug_console_set_area() might be called with different values by 
different threads. An offset is ambiguous as it does not define to which 
of the different shared areas it relates. Please, use a pointer.

>>
>> Do we need num_chars? Are we expecting to provide binary output? Using
>> 0x00 as terminator would be adequate in most cases.
> 
> Bare-metal tests (or assembly sources) can print sub-strings from
> a large per-populated string in shared memory. Assuming that string
> is always terminated by 0x00 in sbi_debug_console_puts() will break
> this flexibility.

OK

> 
>>
>> What is the requirement on the console? Does it have to support 8bit
>> output to allow for UTF-8?
> 
> We need to clarify this. Suggestions ?

The platform specification would be the right place to require 8-bit 
support for the console.

> 
>>
>> Do we make any assumptions about encoding?
> 
> Same as above, this needs more clarification. Suggestions ?

We should add to the platform specification that UTF-8 output is assumed 
on the serial console.

> 
> I am of the opinion to keep such encoding related assumptions to be
> minimal.
> 
>>
>> How would we handle a console set up to 7bit + parity if a character >
>> 0x7f is sent?
> 
> I would consider this to be part of the clarification we add for encoding.

We should state if extra bits are ignored or the bytes are not send.
The easiest thing is to just ignore the extra bits. So let't state this 
here.

Best regards

Heinrich

> 
>>
>>>
>>> Print the string specified by `area_offset` and `num_chars` on
>>> the debug console. The `area_offset` parameter is the start of
>>> string in the shard memory area whereas `num_chars` parameter
>>
>> %s/shard/shared/
> 
> Okay, I will update.
> 
>>
>>> is the number of characters (or bytes) in the string.
>>>
>>> This is a blocking SBI call and will only return after printing
>>> all characters of the string.
>>>
>>> Errors:
>>> SBI_SUCCESS                - Characters printed successfully.
>>> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
>>>                             `area_offset`) or end of the string
>>>                             (i.e. `area_offset + num_chars`) is
>>>                             outside shared memory area.
>>
>> There could be other reasons of failures:
>>
>> * set up of the UART failed in OpenSBI
>> * no UART defined in the device-tree
>> * ...
>>
>> So let us add SBI_ERR_FAILED to the list.
> 
> Okay, I will add.
> 
>>
>> Best regards
>>
>> Heinrich
> 
> Regards,
> Anup

SBI Debug Console Extension Proposal (Draft v1)

[Heiko Stübner]

Am Donnerstag, 2. Juni 2022, 10:50:56 CEST schrieb Heiko St?bner:
> Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel:
> > On Thu, Jun 2, 2022 at 12:02 AM Heiko St?bner <heiko@sntech.de> wrote:
> > >
> > > Hi Anup,
> > >
> > > Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel:
> > > > Hi All,
> > > >
> > > > Below is the draft proposal for SBI Debug Console Extension.
> > > >
> > > > Please review it and provide feedback.
> > > >
> > > > Thanks,
> > > > Anup
> > > >
> > > > Debug Console Extension (EID #0x4442434E "DBCN")
> > > > ================================================
> > > >
> > > > The debug console extension defines a generic mechanism for boot-time
> > > > early prints from supervisor-mode software which allows users to catch
> > > > boot-time issues in supervisor-mode software.
> > > >
> > > > This extension replaces legacy console putchar (EID #0x01) extension
> > > > and it is better in following ways:
> > > > 1) It follows the new calling convention defined for SBI v1.0
> > > >    (or higher).
> > > > 2) It is based on a shared memory area between SBI implementation
> > > >    and supervisor-mode software so multiple characters can be
> > > >    printed using a single SBI call.
> > > >
> > > > The supervisor-mode software must set the shared memory area before
> > > > printing characters on the debug console. Also, all HARTs share the
> > > > same shared memory area so only one HART needs to set it at boot-time.
> > > >
> > > > Function: Set Console Area (FID #0)
> > > > -----------------------------------
> > > >
> > > > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> > > >                                          unsigned long size)
> > > >
> > > > Set the shared memory area specified by `addr_div_by_2` and `size`
> > >
> > > typo in the "div_by_2" (not 4 like below and in the function itself) ?
> > >
> > >
> > > > parameters. The `addr_div_by_4` parameter is base address of the
> > > > shared memory area right shifted by 2 whereas `size` parameter is
> > > > the size of shared memory area in bytes.
> > > >
> > > > The shared memory area should be normal cacheable memory for the
> > > > supervisor-mode software. Also, the shared memory area is global
> > > > across all HARTs so SBI implementation must ensure atomicity in
> > > > setting the shared memory area.
> > > >
> > > > Errors:
> > > > SBI_SUCCESS                - Shared memory area set successfully.
> > > > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> > > >                           `addr_div_by_2` and `size` parameters
> > > >                           is not normal cacheable memory or not
> > > >                           accessible to supervisor-mode software.
> > > >
> > > > Function: Console Puts (FID #1)
> > > > -------------------------------
> > > >
> > > > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> > > >                                      unsigned long num_chars)
> > > >
> > > > Print the string specified by `area_offset` and `num_chars` on
> > > > the debug console. The `area_offset` parameter is the start of
> > > > string in the shard memory area whereas `num_chars` parameter
> > > > is the number of characters (or bytes) in the string.
> > > >
> > > > This is a blocking SBI call and will only return after printing
> > > > all characters of the string.
> > > >
> > > > Errors:
> > > > SBI_SUCCESS                - Characters printed successfully.
> > > > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> > > >                           `area_offset`) or end of the string
> > > >                           (i.e. `area_offset + num_chars`) is
> > > >                           outside shared memory area.
> > >
> > > This will vastly reduce the number of needed ecalls when outputting
> > > characters, so this will probably improve performance quite a bit :-)
> > >
> > >
> > > I guess I still would like to have an _additional_ single-character
> > > putc call. As mentioned in the other thread [0], especially on consumer
> > > hardware [where there is no elaborate debug infrastructure] this can
> > > be a very handy debugging tool even in the earliest stages of a
> > > booting kernel (both before relocation and even inside the startup
> > > assembly).
> > >
> > > I.e. just doing a
> > >         li a7, 1
> > >         li a6, 0
> > >         li a0, 36
> > >         ecall
> > >
> > > in any kernel assembly will just output a "$" character right now, without
> > > needing any preparation at all - same with using the current
> > > sbi_console_putchar() directly in c-code.
> > >
> > > This _can_ be very helpful in some cases, so I guess it would be nice
> > > to keep such a functionality around also in the new spec.
> > 
> > You can easily create multiple pre-populated strings using ".asciiz" in
> > assembly sources. Just set the base address of pre-populated strings
> > once on boot hart and print from anywhere using usual 4-5 instruction
> > (similar to what posted above).
> 
> ok, sounds like a plan as well :-)

though, how does that relate to the time before MMU setup?

I.e. in response to Heinrich's mail you talk about svpbmt, so I guess you
expect virtual memory there, so what is the expected value-type before
the mmu is setup in S-mode ?


> > > [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html
> > >
> > >
> > >
> > 
> > Regards,
> > Anup
> > 
> 
> 

[sig-hypervisors] SBI Debug Console Extension Proposal (Draft v1)

[Schwarz, Konrad]

Hi Anup,

> From: sig-hypervisors at lists.riscv.org <sig-hypervisors@lists.riscv.org> On Behalf Of Anup Patel via
> lists.riscv.org
> Subject: [sig-hypervisors] SBI Debug Console Extension Proposal (Draft v1)
> 
> Below is the draft proposal for SBI Debug Console Extension.

Here are my thoughts:

* Guest memory access: I think this would be the first SBI extension to require access to
  guest memory. This needs to be considered carefully, but I think the higher bandwidth afforded by
  the interface is useful enough to allow this.

* API:
	* Currently, only a write interface is provided. It would be much better to have a
	  read/write interface.

        Benefits of this would be to allow a hypervisor to control an OS, e.g., for testing purposes
        or to automate installation tasks.  Inter-guest communication could also be realized
        via such an interface.

	* As the relationship between SBI and OS is the same as OS and user process, an interface
	  in the style of e.g., Unix IO is possible.

        * Global shared memory buffer design and alternatives:

        The API should have per read() or write() parameters for buffer address and length.
	  This makes it easy for different parts of the OS kernel to output strings directly,
	  without requiring thread synchronization of the shared buffer used by the present proposal.
	  A single buffer (i.e., shared memory) will in most SW architectures require an
	  extra copying of the formatted output string into the shared memory region, which
	  would be avoided using per read()/write() call parameters.
        The exact same argument applies to the SBI implementation: a multi-hart machine
	  utilizing a single shared memory block to communicate with S-mode software
        will require m-mode thread synchronization when accessing the block.
        Having only a single shared memory block will lead to scaling problems
        on high hart count machines.

        I see no advantages of the proposed design to dedicate a block of memory to
	  I/O in advance (and no modern API does this).
        The SBI implementation will still need to be prepared to handle
        access faults on each read and write call, there is no amortization of one-time costs
	  regarding address translation or permission checking that I see.  In the H-extension,
        the guest memory access instructions are as effective as possible, for M-mode code,
        I'm sure that efficient access to S-mode virtual addresses is possible as well.
        (Or the convention can be made that addresses are specified as guest-physical,
        which should be avoided if efficient alternatives are available in RISC-V.
        Note that the draft proposal does not speak of this at all).

	  * Multiple device support: a parameter should be allocated to allow the OS to
	  select which of possibly several output devices to utilize, like the "file
	  descriptor" parameter of the POSIX read()/write() interface.

	  This raises the question of binding these file descriptors to physical devices,
        but a start could be made in analogy to Unix (again), where devices 0, 1, 2 are
        assigned to standard in, standard out, and standard error, and in many cases
        will be attached to the same physical device, and that are pre-opened when the
	  OS starts. As in Unix, the semantics of these would be roughly defined,
        but an OS could write out its boot logging strings to 2, and drive an interactive
        console process (such as a shell) from 0 and 1.

	  The question of binding string-valued names to file descriptor (open()) and
	  closing them, etc., could be deferred to a later date and possibly
        be implementation defined.

        * Asynchronicity and flow control: Unix solves this (poorly) via select()
        or the SIGIO signal.  If SBI introduces an interface for this, this will need to
        co-exist with the OS scheduling that is largely driven by the interrupt controller,
	  which is a hard thing to do.

        I think that the SBI interface should
	  be best effort, copying all data from the client immediately (as Unix does):
        when buffer space in the SBI implementation is exhausted,
        it should return short counts; if the SBI client is prepared to wait, a blocking
	  flush call could be added, which returns when some measure space is
        available in the SBI implementation's output buffers (or when input is available).
        
        The existing Unix interfaces for this can again be used as a guide; they
        should be mature enough to cover all relevant cases.

        For highest performance, an S-mode interrupt could be synthesized by the
	  SBI implementation when buffer space is available.

With best regards,
Konrad Schwarz

Siemens AG
T CED SES-DE
Otto-Hahn-Ring 6
81739 Munich, Germany
Phone: +49 (89) 7805-22579
Fax.: +49 (89) 636-33045
Mobile: +49 (1522) 8864636
mailto:konrad.schwarz at siemens.com
www.siemens.com

Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Jim Hagemann Snabe; Managing Board: Roland Busch, Chairman, President and Chief Executive Officer; Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese; Registered offices: Berlin and Munich, Germany; Commercial registries: Berlin-Charlottenburg, HRB 12300, Munich, HRB 6684; WEEE-Reg.-No. DE 23691322

SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Thu, Jun 2, 2022 at 10:08 AM Xiang W <wxjstz@126.com> wrote:
>
> ? 2022-06-01???? 21:47 +0530?Anup Patel???
> > Hi All,
> >
> > Below is the draft proposal for SBI Debug Console Extension.
> >
> > Please review it and provide feedback.
> >
> > Thanks,
> > Anup
> >
> > Debug Console Extension (EID #0x4442434E "DBCN")
> > ================================================
> >
> > The debug console extension defines a generic mechanism for boot-time
> > early prints from supervisor-mode software which allows users to catch
> > boot-time issues in supervisor-mode software.
> >
> > This extension replaces legacy console putchar (EID #0x01) extension
> > and it is better in following ways:
> > 1) It follows the new calling convention defined for SBI v1.0
> >    (or higher).
> > 2) It is based on a shared memory area between SBI implementation
> >    and supervisor-mode software so multiple characters can be
> >    printed using a single SBI call.
> >
> > The supervisor-mode software must set the shared memory area before
> > printing characters on the debug console. Also, all HARTs share the
> > same shared memory area so only one HART needs to set it at boot-time.
> >
> > Function: Set Console Area (FID #0)
> > -----------------------------------
> >
> > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >                                          unsigned long size)
> >
> > Set the shared memory area specified by `addr_div_by_2` and `size`
> > parameters. The `addr_div_by_4` parameter is base address of the
> > shared memory area right shifted by 2 whereas `size` parameter is
> > the size of shared memory area in bytes.
> >
> > The shared memory area should be normal cacheable memory for the
> > supervisor-mode software. Also, the shared memory area is global
> > across all HARTs so SBI implementation must ensure atomicity in
> > setting the shared memory area.
> >
> > Errors:
> > SBI_SUCCESS                - Shared memory area set successfully.
> > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >                           `addr_div_by_2` and `size` parameters
> >                           is not normal cacheable memory or not
> >                           accessible to supervisor-mode software.
> >
>
> Shared memory can be a single extension. The three interfaces are as follows
>
> /* The supervisor hands a piece of physical memory to sbi for shared memory */
> struct sbiret sbi_shared_memory_extrend(unsigned long addr, unsigned long size);
>
>
> /* The supervisor applies for a piece of physical memory from sbi */
> struct sbiret sbi_shared_memory_alloc(unsigned long size);
>
>
> /* The supervisor notifies sbi to release the requested memory */
> struct sbiret sbi_shared_memory_free(unsigned long addr);

Clearly, if we have a separate SBI extension to manage shared memory
then we will end-up with such memory management calls. Memory allocators
are generally hard to get it right and this also adds lot of bookkeeping and
state management in SBI implementations (e.g. OpenSBI, KVM RISC-V,
and various hypervisors).

Further, some of the SBI extensions (such as Steal Time Accounting) will
have separate shared memory for each HART whereas some (such as
Debug Console) will have global shared memory for all HARTs.

For clean and modular SBI implementations, I would recommend that
each SBI extension define its own shared memory setup API.

Regards,
Anup

>
>
> > Function: Console Puts (FID #1)
> > -------------------------------
> >
> > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >                                      unsigned long num_chars)
> >
> > Print the string specified by `area_offset` and `num_chars` on
> > the debug console. The `area_offset` parameter is the start of
> > string in the shard memory area whereas `num_chars` parameter
> > is the number of characters (or bytes) in the string.
> >
> > This is a blocking SBI call and will only return after printing
> > all characters of the string.
> >
> > Errors:
> > SBI_SUCCESS                - Characters printed successfully.
> > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >                           `area_offset`) or end of the string
> >                           (i.e. `area_offset + num_chars`) is
> >                           outside shared memory area.
> >
>
>

SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Thu, Jun 2, 2022 at 2:58 PM Heiko St?bner <heiko@sntech.de> wrote:
>
> Am Donnerstag, 2. Juni 2022, 10:50:56 CEST schrieb Heiko St?bner:
> > Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel:
> > > On Thu, Jun 2, 2022 at 12:02 AM Heiko St?bner <heiko@sntech.de> wrote:
> > > >
> > > > Hi Anup,
> > > >
> > > > Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel:
> > > > > Hi All,
> > > > >
> > > > > Below is the draft proposal for SBI Debug Console Extension.
> > > > >
> > > > > Please review it and provide feedback.
> > > > >
> > > > > Thanks,
> > > > > Anup
> > > > >
> > > > > Debug Console Extension (EID #0x4442434E "DBCN")
> > > > > ================================================
> > > > >
> > > > > The debug console extension defines a generic mechanism for boot-time
> > > > > early prints from supervisor-mode software which allows users to catch
> > > > > boot-time issues in supervisor-mode software.
> > > > >
> > > > > This extension replaces legacy console putchar (EID #0x01) extension
> > > > > and it is better in following ways:
> > > > > 1) It follows the new calling convention defined for SBI v1.0
> > > > >    (or higher).
> > > > > 2) It is based on a shared memory area between SBI implementation
> > > > >    and supervisor-mode software so multiple characters can be
> > > > >    printed using a single SBI call.
> > > > >
> > > > > The supervisor-mode software must set the shared memory area before
> > > > > printing characters on the debug console. Also, all HARTs share the
> > > > > same shared memory area so only one HART needs to set it at boot-time.
> > > > >
> > > > > Function: Set Console Area (FID #0)
> > > > > -----------------------------------
> > > > >
> > > > > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> > > > >                                          unsigned long size)
> > > > >
> > > > > Set the shared memory area specified by `addr_div_by_2` and `size`
> > > >
> > > > typo in the "div_by_2" (not 4 like below and in the function itself) ?
> > > >
> > > >
> > > > > parameters. The `addr_div_by_4` parameter is base address of the
> > > > > shared memory area right shifted by 2 whereas `size` parameter is
> > > > > the size of shared memory area in bytes.
> > > > >
> > > > > The shared memory area should be normal cacheable memory for the
> > > > > supervisor-mode software. Also, the shared memory area is global
> > > > > across all HARTs so SBI implementation must ensure atomicity in
> > > > > setting the shared memory area.
> > > > >
> > > > > Errors:
> > > > > SBI_SUCCESS                - Shared memory area set successfully.
> > > > > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> > > > >                           `addr_div_by_2` and `size` parameters
> > > > >                           is not normal cacheable memory or not
> > > > >                           accessible to supervisor-mode software.
> > > > >
> > > > > Function: Console Puts (FID #1)
> > > > > -------------------------------
> > > > >
> > > > > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> > > > >                                      unsigned long num_chars)
> > > > >
> > > > > Print the string specified by `area_offset` and `num_chars` on
> > > > > the debug console. The `area_offset` parameter is the start of
> > > > > string in the shard memory area whereas `num_chars` parameter
> > > > > is the number of characters (or bytes) in the string.
> > > > >
> > > > > This is a blocking SBI call and will only return after printing
> > > > > all characters of the string.
> > > > >
> > > > > Errors:
> > > > > SBI_SUCCESS                - Characters printed successfully.
> > > > > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> > > > >                           `area_offset`) or end of the string
> > > > >                           (i.e. `area_offset + num_chars`) is
> > > > >                           outside shared memory area.
> > > >
> > > > This will vastly reduce the number of needed ecalls when outputting
> > > > characters, so this will probably improve performance quite a bit :-)
> > > >
> > > >
> > > > I guess I still would like to have an _additional_ single-character
> > > > putc call. As mentioned in the other thread [0], especially on consumer
> > > > hardware [where there is no elaborate debug infrastructure] this can
> > > > be a very handy debugging tool even in the earliest stages of a
> > > > booting kernel (both before relocation and even inside the startup
> > > > assembly).
> > > >
> > > > I.e. just doing a
> > > >         li a7, 1
> > > >         li a6, 0
> > > >         li a0, 36
> > > >         ecall
> > > >
> > > > in any kernel assembly will just output a "$" character right now, without
> > > > needing any preparation at all - same with using the current
> > > > sbi_console_putchar() directly in c-code.
> > > >
> > > > This _can_ be very helpful in some cases, so I guess it would be nice
> > > > to keep such a functionality around also in the new spec.
> > >
> > > You can easily create multiple pre-populated strings using ".asciiz" in
> > > assembly sources. Just set the base address of pre-populated strings
> > > once on boot hart and print from anywhere using usual 4-5 instruction
> > > (similar to what posted above).
> >
> > ok, sounds like a plan as well :-)
>
> though, how does that relate to the time before MMU setup?
>
> I.e. in response to Heinrich's mail you talk about svpbmt, so I guess you
> expect virtual memory there, so what is the expected value-type before
> the mmu is setup in S-mode ?

The memory type should be 0 (i.e. PMA) for the shared memory between
SBI implementation and supervisor software. Before MMU setup, the
memory type is by default 0 (i.e. PMA) so we don't have to mandate
any memory type for MMU disabled case.

We only have issue on systems with Svpmbt where supervisor software
can potentially map the shared memory as non-cacheable or IO (memory
type != 0) using PTE memory type bits.

In addition to above, a SBI implementation must ensure that the shared
memory address provided by supervisor software is a regular memory
(not MMIO device). This can be easily achieved in OpenSBI, KVM RISC-V,
and various hypervisors.

Regards,
Anup

>
>
> > > > [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html
> > > >
> > > >
> > > >
> > >
> > > Regards,
> > > Anup
> > >
> >
> >
>
>
>
>

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Thu, Jun 2, 2022 at 2:43 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 6/2/22 10:44, Anup Patel wrote:
> > On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 6/1/22 18:17, Anup Patel wrote:
> >>> Hi All,
> >>>
> >>> Below is the draft proposal for SBI Debug Console Extension.
> >>>
> >>> Please review it and provide feedback.
> >>>
> >>> Thanks,
> >>> Anup
> >>>
> >>> Debug Console Extension (EID #0x4442434E "DBCN")
> >>> ================================================
> >>>
> >>> The debug console extension defines a generic mechanism for boot-time
> >>> early prints from supervisor-mode software which allows users to catch
> >>> boot-time issues in supervisor-mode software.
> >>>
> >>> This extension replaces legacy console putchar (EID #0x01) extension
> >>> and it is better in following ways:
> >>
> >> Thanks for starting to close this gap.
> >>
> >>> 1) It follows the new calling convention defined for SBI v1.0
> >>>      (or higher).
> >>> 2) It is based on a shared memory area between SBI implementation
> >>>      and supervisor-mode software so multiple characters can be
> >>>      printed using a single SBI call.
> >>
> >> I miss a discussion of the conflicts that can arise if the configuration
> >> of the serial console is changed by the caller.
> >>
> >> Do we need an ecall that closes the SBI console to further access?
> >
> > Usually, the serial port related code in M-mode firmware only uses
> > status and data registers so for most serial ports support the M-mode
> > firmware will adapt to serial port configuration changes.
> >
> > In fact, this is why we never had a special SBI call for serial port
> > reconfiguration in legacy SBI v0.1 as well.
> >
> > In case of virtualization, the serial port (or console) is emulated so
> > the special SBI call is not useful for virtualization.
> >
> >>
> >>>
> >>> The supervisor-mode software must set the shared memory area before
> >>> printing characters on the debug console. Also, all HARTs share the
> >>> same shared memory area so only one HART needs to set it at boot-time.
> >>
> >> Isn't it M-mode software that has to program the MMU to allow all harts
> >> in M-mode and S-mode access to the memory area? What is the S-mode
> >> software to do about the memory area prior to calling
> >> sbi_debug_console_set_area()?
> >
> > Actually, it's the S-mode software which is voluntarily giving a portion of
> > its memory to be used as shared memory. The proposal only mandates
> > that whatever memory is selected by S-mode software should be a
> > regular cacheable memory (not IO memory). Also, if Svpbmt is available
> > then S-mode software should only use memory type 0 in the PTEs.
> >
> >>
> >>>
> >>> Function: Set Console Area (FID #0)
> >>> -----------------------------------
> >>>
> >>> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >>>                                            unsigned long size)
> >>
> >> The console output is needed on the very start of the S-mode software,
> >> before setting up anything.
> >>
> >> Can we avoid this extra function?
> >>
> >> Can we simply assume that the caller of sbi_debug_console_puts()
> >> provides a physical address pointer to a memory area that is suitable?
> >
> > Theoretically, we can avoid the extra function to set shared memory area
> > but it will complicate things in future when we have supervisor software
> > encrypting it's own memory (using special ISA support) because in this
> > case supervisor software will have to unprotect memory every time the
> > sbi_debug_console_puts() is called and protect it again after the call.
>
> Currently this function is just a nop(). It is not needed in this
> revision of the extension.
>
> The function might be called repeatedly by different threads with
> different values. How do you want to keep track of all of these
> different areas?

The shared memory area in case of this SBI extension will be shared
across all HARTs so the SBI implementation will ensure atomicity
in setting/reading shared memory coordinates. This way multiple
HARTs can call the sbi_debug_console_set_area() but only the
last call will be in effect.

Following text in the proposal explains above:
"The supervisor-mode software must set the shared memory area before
printing characters on the debug console. Also, all HARTs share the
same shared memory area so only one HART needs to set it at boot-time."

Maybe this text is not giving a clear picture ?

>
> Memory shared between different security realms will arise in many
> different scenarios. As this is not console specific it should be in a
> separate extension. That extension should be defined once we have
> clarity about how security realms are managed.

In the case of this extension, the shared memory is global across all
HARTs and it mostly contains bytes to be printed. In case of steal
time accounting, the shared memory is separate for each HART and
it is a well-defined data structure.

Clearly, the usage of shared memory is extension specific.

The VirtIO devices are a good (and time tested) example of shared
memory based approaches. Over there as well, the shared memory
(i.e. various VirtIO rings) are setup by Guest and there is no central
pool of shared memory (i.e no dedicated VirtIO device managing
shared memory).

Similar to VirtIO world, we should let SBI extension define its own
shared memory usage and API.

Regards,
Anup

>
> >
> >>
> >>>
> >>> Set the shared memory area specified by `addr_div_by_2` and `size`
> >>
> >> %s/addr_div_by_2/addr_div_by_4/
> >
> > Okay, I will update.
> >
> >>
> >>> parameters. The `addr_div_by_4` parameter is base address of the
> >>
> >> %s/is base/is the base/
> >
> > Okay, I will update.
> >
> >>
> >>> shared memory area right shifted by 2 whereas `size` parameter is
> >>> the size of shared memory area in bytes.
> >>
> >> Why shifting the address? I would prefer to keep it simple for the
> >> caller. If the alignment is not suitable, return an error.
> >>
> >> But why is an alignment needed here at all? And why 4 aligned?
> >
> > For RV32 S-mode, the physical address space is 34bits wide but
> > "unsigned long" is 32bits wide. This is because Sv32 PTEs allow
> > 34bits of PPN. In fact, even instructions such as HFENCE.GVMA
> > have this "address right shift by 2" requirement based on this rationale.
> >
> >>
> >>>
> >>> The shared memory area should be normal cacheable memory for the
> >>> supervisor-mode software. Also, the shared memory area is global
> >>> across all HARTs so SBI implementation must ensure atomicity in
> >>> setting the shared memory area.
> >>>
> >>> Errors:
> >>> SBI_SUCCESS                - Shared memory area set successfully.
> >>> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >>>                             `addr_div_by_2` and `size` parameters
> >>>                             is not normal cacheable memory or not
> >>>                             accessible to supervisor-mode software.
> >>>
> >>> Function: Console Puts (FID #1)
> >>> -------------------------------
> >>>
> >>> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >>>                                        unsigned long num_chars)
> >>
> >> I would prefer to simply pass a physical address pointer here with no
> >> requirements on alignment. And no prior SBI call.
>
> sbi_debug_console_set_area() might be called with different values by
> different threads. An offset is ambiguous as it does not define to which
> of the different shared areas it relates. Please, use a pointer.
>
> >>
> >> Do we need num_chars? Are we expecting to provide binary output? Using
> >> 0x00 as terminator would be adequate in most cases.
> >
> > Bare-metal tests (or assembly sources) can print sub-strings from
> > a large per-populated string in shared memory. Assuming that string
> > is always terminated by 0x00 in sbi_debug_console_puts() will break
> > this flexibility.
>
> OK
>
> >
> >>
> >> What is the requirement on the console? Does it have to support 8bit
> >> output to allow for UTF-8?
> >
> > We need to clarify this. Suggestions ?
>
> The platform specification would be the right place to require 8-bit
> support for the console.
>
> >
> >>
> >> Do we make any assumptions about encoding?
> >
> > Same as above, this needs more clarification. Suggestions ?
>
> We should add to the platform specification that UTF-8 output is assumed
> on the serial console.
>
> >
> > I am of the opinion to keep such encoding related assumptions to be
> > minimal.
> >
> >>
> >> How would we handle a console set up to 7bit + parity if a character >
> >> 0x7f is sent?
> >
> > I would consider this to be part of the clarification we add for encoding.
>
> We should state if extra bits are ignored or the bytes are not send.
> The easiest thing is to just ignore the extra bits. So let't state this
> here.
>
> Best regards
>
> Heinrich
>
> >
> >>
> >>>
> >>> Print the string specified by `area_offset` and `num_chars` on
> >>> the debug console. The `area_offset` parameter is the start of
> >>> string in the shard memory area whereas `num_chars` parameter
> >>
> >> %s/shard/shared/
> >
> > Okay, I will update.
> >
> >>
> >>> is the number of characters (or bytes) in the string.
> >>>
> >>> This is a blocking SBI call and will only return after printing
> >>> all characters of the string.
> >>>
> >>> Errors:
> >>> SBI_SUCCESS                - Characters printed successfully.
> >>> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >>>                             `area_offset`) or end of the string
> >>>                             (i.e. `area_offset + num_chars`) is
> >>>                             outside shared memory area.
> >>
> >> There could be other reasons of failures:
> >>
> >> * set up of the UART failed in OpenSBI
> >> * no UART defined in the device-tree
> >> * ...
> >>
> >> So let us add SBI_ERR_FAILED to the list.
> >
> > Okay, I will add.
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >
> > Regards,
> > Anup
>

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@rivosinc.com> wrote:
>
> Should we keep this simple in the SBI - only have register based inputs - to send and receive 1 byte in each call?
> Keeping it a simple out_byte or in_byte - a serial port like interface seems the simplest and most secure.

The legacy SBI v0.1 putchar() and getchar() are byte-level send/receive calls.

This is very slow for virtualized world particularly KVM RISC-V because each
SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will
forward it to user-space QEMU or KVMTOOL. This means each early print
character using SBI v0.1 putchar() will go all the way to host user-space and
come back. This is horribly slow for KVM Guest. This becomes further slower
for nested virtualization.

The MMIO based early prints are further worse because we have at least two
MMIO traps for every character where each MMIO exits to host user-space.

The shared memory based SBI puts() drastically reduces the number of SBI
traps hence reducing boot time with early prints enabled.

> I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory.

The VirtIO based para-virt devices rely heavily on shared memory so I think
it is possible to address security concerns related to shared memory.

> The API as defined does not say whether the address is a virtual address or a physical address.

It is a physical address. I will clarify this in Draft v2.

> If it is a virtual address then the SBI call will need to use a MPRV load/store to gather this data and will also need to deal with page fault, access faults, etc. that may occur on such accesses.

Yes, MPRV (or HLV/HSV) based load/store have performance issues
particularly due to page faults. These are prevalent in some of the
legacy SBI v0.1 calls. With SBI v0.2 (or higher), we have tried to
ensure that we don't use virtual addresses as parameter in newer SBI
calls.

> Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart.

The goal of the shared memory based SBI call for early prints is to
minimize the number of traps which in-turn helps virtualization to
drastically reduce boot-time.

Regards,
Anup

>
> regards
> ved
>
>
>
> On Thu, Jun 2, 2022 at 7:43 AM Anup Patel <apatel@ventanamicro.com> wrote:
>>
>> On Thu, Jun 2, 2022 at 2:58 PM Heiko St?bner <heiko@sntech.de> wrote:
>> >
>> > Am Donnerstag, 2. Juni 2022, 10:50:56 CEST schrieb Heiko St?bner:
>> > > Am Donnerstag, 2. Juni 2022, 10:47:58 CEST schrieb Anup Patel:
>> > > > On Thu, Jun 2, 2022 at 12:02 AM Heiko St?bner <heiko@sntech.de> wrote:
>> > > > >
>> > > > > Hi Anup,
>> > > > >
>> > > > > Am Mittwoch, 1. Juni 2022, 18:17:32 CEST schrieb Anup Patel:
>> > > > > > Hi All,
>> > > > > >
>> > > > > > Below is the draft proposal for SBI Debug Console Extension.
>> > > > > >
>> > > > > > Please review it and provide feedback.
>> > > > > >
>> > > > > > Thanks,
>> > > > > > Anup
>> > > > > >
>> > > > > > Debug Console Extension (EID #0x4442434E "DBCN")
>> > > > > > ================================================
>> > > > > >
>> > > > > > The debug console extension defines a generic mechanism for boot-time
>> > > > > > early prints from supervisor-mode software which allows users to catch
>> > > > > > boot-time issues in supervisor-mode software.
>> > > > > >
>> > > > > > This extension replaces legacy console putchar (EID #0x01) extension
>> > > > > > and it is better in following ways:
>> > > > > > 1) It follows the new calling convention defined for SBI v1.0
>> > > > > >    (or higher).
>> > > > > > 2) It is based on a shared memory area between SBI implementation
>> > > > > >    and supervisor-mode software so multiple characters can be
>> > > > > >    printed using a single SBI call.
>> > > > > >
>> > > > > > The supervisor-mode software must set the shared memory area before
>> > > > > > printing characters on the debug console. Also, all HARTs share the
>> > > > > > same shared memory area so only one HART needs to set it at boot-time.
>> > > > > >
>> > > > > > Function: Set Console Area (FID #0)
>> > > > > > -----------------------------------
>> > > > > >
>> > > > > > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
>> > > > > >                                          unsigned long size)
>> > > > > >
>> > > > > > Set the shared memory area specified by `addr_div_by_2` and `size`
>> > > > >
>> > > > > typo in the "div_by_2" (not 4 like below and in the function itself) ?
>> > > > >
>> > > > >
>> > > > > > parameters. The `addr_div_by_4` parameter is base address of the
>> > > > > > shared memory area right shifted by 2 whereas `size` parameter is
>> > > > > > the size of shared memory area in bytes.
>> > > > > >
>> > > > > > The shared memory area should be normal cacheable memory for the
>> > > > > > supervisor-mode software. Also, the shared memory area is global
>> > > > > > across all HARTs so SBI implementation must ensure atomicity in
>> > > > > > setting the shared memory area.
>> > > > > >
>> > > > > > Errors:
>> > > > > > SBI_SUCCESS                - Shared memory area set successfully.
>> > > > > > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
>> > > > > >                           `addr_div_by_2` and `size` parameters
>> > > > > >                           is not normal cacheable memory or not
>> > > > > >                           accessible to supervisor-mode software.
>> > > > > >
>> > > > > > Function: Console Puts (FID #1)
>> > > > > > -------------------------------
>> > > > > >
>> > > > > > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
>> > > > > >                                      unsigned long num_chars)
>> > > > > >
>> > > > > > Print the string specified by `area_offset` and `num_chars` on
>> > > > > > the debug console. The `area_offset` parameter is the start of
>> > > > > > string in the shard memory area whereas `num_chars` parameter
>> > > > > > is the number of characters (or bytes) in the string.
>> > > > > >
>> > > > > > This is a blocking SBI call and will only return after printing
>> > > > > > all characters of the string.
>> > > > > >
>> > > > > > Errors:
>> > > > > > SBI_SUCCESS                - Characters printed successfully.
>> > > > > > SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
>> > > > > >                           `area_offset`) or end of the string
>> > > > > >                           (i.e. `area_offset + num_chars`) is
>> > > > > >                           outside shared memory area.
>> > > > >
>> > > > > This will vastly reduce the number of needed ecalls when outputting
>> > > > > characters, so this will probably improve performance quite a bit :-)
>> > > > >
>> > > > >
>> > > > > I guess I still would like to have an _additional_ single-character
>> > > > > putc call. As mentioned in the other thread [0], especially on consumer
>> > > > > hardware [where there is no elaborate debug infrastructure] this can
>> > > > > be a very handy debugging tool even in the earliest stages of a
>> > > > > booting kernel (both before relocation and even inside the startup
>> > > > > assembly).
>> > > > >
>> > > > > I.e. just doing a
>> > > > >         li a7, 1
>> > > > >         li a6, 0
>> > > > >         li a0, 36
>> > > > >         ecall
>> > > > >
>> > > > > in any kernel assembly will just output a "$" character right now, without
>> > > > > needing any preparation at all - same with using the current
>> > > > > sbi_console_putchar() directly in c-code.
>> > > > >
>> > > > > This _can_ be very helpful in some cases, so I guess it would be nice
>> > > > > to keep such a functionality around also in the new spec.
>> > > >
>> > > > You can easily create multiple pre-populated strings using ".asciiz" in
>> > > > assembly sources. Just set the base address of pre-populated strings
>> > > > once on boot hart and print from anywhere using usual 4-5 instruction
>> > > > (similar to what posted above).
>> > >
>> > > ok, sounds like a plan as well :-)
>> >
>> > though, how does that relate to the time before MMU setup?
>> >
>> > I.e. in response to Heinrich's mail you talk about svpbmt, so I guess you
>> > expect virtual memory there, so what is the expected value-type before
>> > the mmu is setup in S-mode ?
>>
>> The memory type should be 0 (i.e. PMA) for the shared memory between
>> SBI implementation and supervisor software. Before MMU setup, the
>> memory type is by default 0 (i.e. PMA) so we don't have to mandate
>> any memory type for MMU disabled case.
>>
>> We only have issue on systems with Svpmbt where supervisor software
>> can potentially map the shared memory as non-cacheable or IO (memory
>> type != 0) using PTE memory type bits.
>>
>> In addition to above, a SBI implementation must ensure that the shared
>> memory address provided by supervisor software is a regular memory
>> (not MMIO device). This can be easily achieved in OpenSBI, KVM RISC-V,
>> and various hypervisors.
>>
>> Regards,
>> Anup
>>
>> >
>> >
>> > > > > [0] http://lists.infradead.org/pipermail/opensbi/2022-June/002796.html
>> > > > >
>> > > > >
>> > > > >
>> > > >
>> > > > Regards,
>> > > > Anup
>> > > >
>> > >
>> > >
>> >
>> >
>> >
>> >
>>
>>
>> -=-=-=-=-=-=-=-=-=-=-=-
>> Links: You receive all messages sent to this group.
>> View/Reply Online (#1721): https://lists.riscv.org/g/tech-unixplatformspec/message/1721
>> Mute This Topic: https://lists.riscv.org/mt/91480122/6317211
>> Group Owner: tech-unixplatformspec+owner at lists.riscv.org
>> Unsubscribe: https://lists.riscv.org/g/tech-unixplatformspec/unsub [ved at rivosinc.com]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>
>>

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Ved Shanbhogue]

On Thu, Jun 02, 2022 at 07:52:55PM +0530, Anup Patel wrote:
>On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@rivosinc.com> wrote:
>This is very slow for virtualized world particularly KVM RISC-V because each
>SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will
>forward it to user-space QEMU or KVMTOOL. This means each early print
>character using SBI v0.1 putchar() will go all the way to host user-space and
>come back. This is horribly slow for KVM Guest. This becomes further slower
>for nested virtualization.
>
Since this is for debug and really early phase debug till enough of the guest
boots up to use a VFIO based char driver provided by the VMM, I am not sure
that the slowness matters.
Even this SBI call I expected the VMM to intercept and if the VMM has all 
emulation in the user space VMM - e.g. the console tty is open by the user
space VMM, I am not sure this would avoid that trip to user space. 
If the motivation is primarily VM debug then perhaps a standardized set of
hypercalls implemented by KVM makes more sense than SBI calls that would
need to be built into the M-mode firmware? 

>
>> I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory.
>
>The VirtIO based para-virt devices rely heavily on shared memory so I think
>it is possible to address security concerns related to shared memory.

Yes, and now that I understand the motivation better why dont we define this 
as a hypercall/pv-ops interface to a VMM than a SBI call to the M-mode 
firmware and needing to build a virt-io like framework in firmware.

>
>> The API as defined does not say whether the address is a virtual address or a physical address.
>
>It is a physical address. I will clarify this in Draft v2.
>
Thanks. I was not sure since we had all the discussion about Svpbmt.

>
>> Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart.
>
>The goal of the shared memory based SBI call for early prints is to
>minimize the number of traps which in-turn helps virtualization to
>drastically reduce boot-time.
>

Understand that better now. But if that is the main motivation then
I am not understanding why we would want to push all of this into
M-mode firmware vs. defining a set of standardized pv-ops to be used
by guest OSes.

regards
ved

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

On Thu, Jun 2, 2022 at 8:30 PM Ved Shanbhogue <ved@rivosinc.com> wrote:
>
> On Thu, Jun 02, 2022 at 07:52:55PM +0530, Anup Patel wrote:
> >On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@rivosinc.com> wrote:
> >This is very slow for virtualized world particularly KVM RISC-V because each
> >SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will
> >forward it to user-space QEMU or KVMTOOL. This means each early print
> >character using SBI v0.1 putchar() will go all the way to host user-space and
> >come back. This is horribly slow for KVM Guest. This becomes further slower
> >for nested virtualization.
> >
> Since this is for debug and really early phase debug till enough of the guest
> boots up to use a VFIO based char driver provided by the VMM, I am not sure
> that the slowness matters.
> Even this SBI call I expected the VMM to intercept and if the VMM has all
> emulation in the user space VMM - e.g. the console tty is open by the user
> space VMM, I am not sure this would avoid that trip to user space.
> If the motivation is primarily VM debug then perhaps a standardized set of
> hypercalls implemented by KVM makes more sense than SBI calls that would
> need to be built into the M-mode firmware?

We have a requirement in the OS-A platform spec to mandate a particular type
of UART for early prints but there were objections on selecting one type of UART
over another.

This SBI debug console extension is also a way to avoid standardizing a
particular type of UART in OS-A platform spec.

>
> >
> >> I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory.
> >
> >The VirtIO based para-virt devices rely heavily on shared memory so I think
> >it is possible to address security concerns related to shared memory.
>
> Yes, and now that I understand the motivation better why dont we define this
> as a hypercall/pv-ops interface to a VMM than a SBI call to the M-mode
> firmware and needing to build a virt-io like framework in firmware.

The VirtIO framework has rings in shared memory but over here it is just
shared memory without any formatted data structure.

>
> >
> >> The API as defined does not say whether the address is a virtual address or a physical address.
> >
> >It is a physical address. I will clarify this in Draft v2.
> >
> Thanks. I was not sure since we had all the discussion about Svpbmt.
>
> >
> >> Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart.
> >
> >The goal of the shared memory based SBI call for early prints is to
> >minimize the number of traps which in-turn helps virtualization to
> >drastically reduce boot-time.
> >
>
> Understand that better now. But if that is the main motivation then
> I am not understanding why we would want to push all of this into
> M-mode firmware vs. defining a set of standardized pv-ops to be used
> by guest OSes.

The SBI debug console also provides a standard way of early prints for
supervisor software running natively (directly under M-mode) irrespective
of the type of UART/Console available on the console so it's not just
for a virtualized world. Although, the virtualized world has an added
performance advantage due to reduced traps.

Regards,
Anup

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Xiang W]

? 2022-06-02???? 18:29 +0530?Anup Patel???
> On Thu, Jun 2, 2022 at 2:43 PM Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> > 
> > On 6/2/22 10:44, Anup Patel wrote:
> > > On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > > > 
> > > > On 6/1/22 18:17, Anup Patel wrote:
> > > > > Hi All,
> > > > > 
> > > > > Below is the draft proposal for SBI Debug Console Extension.
> > > > > 
> > > > > Please review it and provide feedback.
> > > > > 
> > > > > Thanks,
> > > > > Anup
> > > > > 
> > > > > Debug Console Extension (EID #0x4442434E "DBCN")
> > > > > ================================================
> > > > > 
> > > > > The debug console extension defines a generic mechanism for boot-time
> > > > > early prints from supervisor-mode software which allows users to catch
> > > > > boot-time issues in supervisor-mode software.
> > > > > 
> > > > > This extension replaces legacy console putchar (EID #0x01) extension
> > > > > and it is better in following ways:
> > > > 
> > > > Thanks for starting to close this gap.
> > > > 
> > > > > 1) It follows the new calling convention defined for SBI v1.0
> > > > > ???? (or higher).
> > > > > 2) It is based on a shared memory area between SBI implementation
> > > > > ???? and supervisor-mode software so multiple characters can be
> > > > > ???? printed using a single SBI call.
> > > > 
> > > > I miss a discussion of the conflicts that can arise if the configuration
> > > > of the serial console is changed by the caller.
> > > > 
> > > > Do we need an ecall that closes the SBI console to further access?
> > > 
> > > Usually, the serial port related code in M-mode firmware only uses
> > > status and data registers so for most serial ports support the M-mode
> > > firmware will adapt to serial port configuration changes.
> > > 
> > > In fact, this is why we never had a special SBI call for serial port
> > > reconfiguration in legacy SBI v0.1 as well.
> > > 
> > > In case of virtualization, the serial port (or console) is emulated so
> > > the special SBI call is not useful for virtualization.
> > > 
> > > > 
> > > > > 
> > > > > The supervisor-mode software must set the shared memory area before
> > > > > printing characters on the debug console. Also, all HARTs share the
> > > > > same shared memory area so only one HART needs to set it at boot-time.
> > > > 
> > > > Isn't it M-mode software that has to program the MMU to allow all harts
> > > > in M-mode and S-mode access to the memory area? What is the S-mode
> > > > software to do about the memory area prior to calling
> > > > sbi_debug_console_set_area()?
> > > 
> > > Actually, it's the S-mode software which is voluntarily giving a portion of
> > > its memory to be used as shared memory. The proposal only mandates
> > > that whatever memory is selected by S-mode software should be a
> > > regular cacheable memory (not IO memory). Also, if Svpbmt is available
> > > then S-mode software should only use memory type 0 in the PTEs.
> > > 
> > > > 
> > > > > 
> > > > > Function: Set Console Area (FID #0)
> > > > > -----------------------------------
> > > > > 
> > > > > struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> > > > > ?????????????????????????????????????????? unsigned long size)
> > > > 
> > > > The console output is needed on the very start of the S-mode software,
> > > > before setting up anything.
> > > > 
> > > > Can we avoid this extra function?
> > > > 
> > > > Can we simply assume that the caller of sbi_debug_console_puts()
> > > > provides a physical address pointer to a memory area that is suitable?
> > > 
> > > Theoretically, we can avoid the extra function to set shared memory area
> > > but it will complicate things in future when we have supervisor software
> > > encrypting it's own memory (using special ISA support) because in this
> > > case supervisor software will have to unprotect memory every time the
> > > sbi_debug_console_puts() is called and protect it again after the call.
> > 
> > Currently this function is just a nop(). It is not needed in this
> > revision of the extension.
> > 
> > The function might be called repeatedly by different threads with
> > different values. How do you want to keep track of all of these
> > different areas?
> 
> The shared memory area in case of this SBI extension will be shared
> across all HARTs so the SBI implementation will ensure atomicity
> in setting/reading shared memory coordinates. This way multiple
> HARTs can call the sbi_debug_console_set_area() but only the
> last call will be in effect.
> 
> Following text in the proposal explains above:
> "The supervisor-mode software must set the shared memory area before
> printing characters on the debug console. Also, all HARTs share the
> same shared memory area so only one HART needs to set it at boot-time."
> 
> Maybe this text is not giving a clear picture ?

There is a strict calling order between the two APIs, which?
adds to the complexity. If we pass the physical address directly
to sbi_debug_console_puts will circumvent these problems.

I would like to know the necessity of setting up shared memory first.

Regards,
Xiang W
> 
> > 
> > Memory shared between different security realms will arise in many
> > different scenarios. As this is not console specific it should be in a
> > separate extension. That extension should be defined once we have
> > clarity about how security realms are managed.
> 
> In the case of this extension, the shared memory is global across all
> HARTs and it mostly contains bytes to be printed. In case of steal
> time accounting, the shared memory is separate for each HART and
> it is a well-defined data structure.
> 
> Clearly, the usage of shared memory is extension specific.
> 
> The VirtIO devices are a good (and time tested) example of shared
> memory based approaches. Over there as well, the shared memory
> (i.e. various VirtIO rings) are setup by Guest and there is no central
> pool of shared memory (i.e no dedicated VirtIO device managing
> shared memory).
> 
> Similar to VirtIO world, we should let SBI extension define its own
> shared memory usage and API.
> 
> Regards,
> Anup
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > Set the shared memory area specified by `addr_div_by_2` and `size`
> > > > 
> > > > %s/addr_div_by_2/addr_div_by_4/
> > > 
> > > Okay, I will update.
> > > 
> > > > 
> > > > > parameters. The `addr_div_by_4` parameter is base address of the
> > > > 
> > > > %s/is base/is the base/
> > > 
> > > Okay, I will update.
> > > 
> > > > 
> > > > > shared memory area right shifted by 2 whereas `size` parameter is
> > > > > the size of shared memory area in bytes.
> > > > 
> > > > Why shifting the address? I would prefer to keep it simple for the
> > > > caller. If the alignment is not suitable, return an error.
> > > > 
> > > > But why is an alignment needed here at all? And why 4 aligned?
> > > 
> > > For RV32 S-mode, the physical address space is 34bits wide but
> > > "unsigned long" is 32bits wide. This is because Sv32 PTEs allow
> > > 34bits of PPN. In fact, even instructions such as HFENCE.GVMA
> > > have this "address right shift by 2" requirement based on this rationale.
> > > 
> > > > 
> > > > > 
> > > > > The shared memory area should be normal cacheable memory for the
> > > > > supervisor-mode software. Also, the shared memory area is global
> > > > > across all HARTs so SBI implementation must ensure atomicity in
> > > > > setting the shared memory area.
> > > > > 
> > > > > Errors:
> > > > > SBI_SUCCESS??????????????? - Shared memory area set successfully.
> > > > > SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> > > > > ??????????????????????????? `addr_div_by_2` and `size` parameters
> > > > > ??????????????????????????? is not normal cacheable memory or not
> > > > > ??????????????????????????? accessible to supervisor-mode software.
> > > > > 
> > > > > Function: Console Puts (FID #1)
> > > > > -------------------------------
> > > > > 
> > > > > struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> > > > > ?????????????????????????????????????? unsigned long num_chars)
> > > > 
> > > > I would prefer to simply pass a physical address pointer here with no
> > > > requirements on alignment. And no prior SBI call.
> > 
> > sbi_debug_console_set_area() might be called with different values by
> > different threads. An offset is ambiguous as it does not define to which
> > of the different shared areas it relates. Please, use a pointer.
> > 
> > > > 
> > > > Do we need num_chars? Are we expecting to provide binary output? Using
> > > > 0x00 as terminator would be adequate in most cases.
> > > 
> > > Bare-metal tests (or assembly sources) can print sub-strings from
> > > a large per-populated string in shared memory. Assuming that string
> > > is always terminated by 0x00 in sbi_debug_console_puts() will break
> > > this flexibility.
> > 
> > OK
> > 
> > > 
> > > > 
> > > > What is the requirement on the console? Does it have to support 8bit
> > > > output to allow for UTF-8?
> > > 
> > > We need to clarify this. Suggestions ?
> > 
> > The platform specification would be the right place to require 8-bit
> > support for the console.
> > 
> > > 
> > > > 
> > > > Do we make any assumptions about encoding?
> > > 
> > > Same as above, this needs more clarification. Suggestions ?
> > 
> > We should add to the platform specification that UTF-8 output is assumed
> > on the serial console.
> > 
> > > 
> > > I am of the opinion to keep such encoding related assumptions to be
> > > minimal.
> > > 
> > > > 
> > > > How would we handle a console set up to 7bit + parity if a character >
> > > > 0x7f is sent?
> > > 
> > > I would consider this to be part of the clarification we add for encoding.
> > 
> > We should state if extra bits are ignored or the bytes are not send.
> > The easiest thing is to just ignore the extra bits. So let't state this
> > here.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > 
> > > > 
> > > > > 
> > > > > Print the string specified by `area_offset` and `num_chars` on
> > > > > the debug console. The `area_offset` parameter is the start of
> > > > > string in the shard memory area whereas `num_chars` parameter
> > > > 
> > > > %s/shard/shared/
> > > 
> > > Okay, I will update.
> > > 
> > > > 
> > > > > is the number of characters (or bytes) in the string.
> > > > > 
> > > > > This is a blocking SBI call and will only return after printing
> > > > > all characters of the string.
> > > > > 
> > > > > Errors:
> > > > > SBI_SUCCESS??????????????? - Characters printed successfully.
> > > > > SBI_ERR_INVALID_ADDRESS??? - The start of the string (i.e.
> > > > > ??????????????????????????? `area_offset`) or end of the string
> > > > > ??????????????????????????? (i.e. `area_offset + num_chars`) is
> > > > > ??????????????????????????? outside shared memory area.
> > > > 
> > > > There could be other reasons of failures:
> > > > 
> > > > * set up of the UART failed in OpenSBI
> > > > * no UART defined in the device-tree
> > > > * ...
> > > > 
> > > > So let us add SBI_ERR_FAILED to the list.
> > > 
> > > Okay, I will add.
> > > 
> > > > 
> > > > Best regards
> > > > 
> > > > Heinrich
> > > 
> > > Regards,
> > > Anup
> > 
> 

[RISC-V][tech-os-a-see] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Atish Kumar Patra]

On Thu, Jun 2, 2022 at 8:18 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Thu, Jun 2, 2022 at 8:30 PM Ved Shanbhogue <ved@rivosinc.com> wrote:
> >
> > On Thu, Jun 02, 2022 at 07:52:55PM +0530, Anup Patel wrote:
> > >On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@rivosinc.com> wrote:
> > >This is very slow for virtualized world particularly KVM RISC-V because each
> > >SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will
> > >forward it to user-space QEMU or KVMTOOL. This means each early print
> > >character using SBI v0.1 putchar() will go all the way to host user-space and
> > >come back. This is horribly slow for KVM Guest. This becomes further slower
> > >for nested virtualization.
> > >
> > Since this is for debug and really early phase debug till enough of the guest
> > boots up to use a VFIO based char driver provided by the VMM, I am not sure
> > that the slowness matters.
> > Even this SBI call I expected the VMM to intercept and if the VMM has all
> > emulation in the user space VMM - e.g. the console tty is open by the user
> > space VMM, I am not sure this would avoid that trip to user space.
> > If the motivation is primarily VM debug then perhaps a standardized set of
> > hypercalls implemented by KVM makes more sense than SBI calls that would
> > need to be built into the M-mode firmware?
>
> We have a requirement in the OS-A platform spec to mandate a particular type
> of UART for early prints but there were objections on selecting one type of UART
> over another.
>
> This SBI debug console extension is also a way to avoid standardizing a
> particular type of UART in OS-A platform spec.
>
> >
> > >
> > >> I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory.
> > >
> > >The VirtIO based para-virt devices rely heavily on shared memory so I think
> > >it is possible to address security concerns related to shared memory.
> >
> > Yes, and now that I understand the motivation better why dont we define this
> > as a hypercall/pv-ops interface to a VMM than a SBI call to the M-mode
> > firmware and needing to build a virt-io like framework in firmware.
>
> The VirtIO framework has rings in shared memory but over here it is just
> shared memory without any formatted data structure.
>
> >
> > >
> > >> The API as defined does not say whether the address is a virtual address or a physical address.
> > >
> > >It is a physical address. I will clarify this in Draft v2.
> > >
> > Thanks. I was not sure since we had all the discussion about Svpbmt.
> >
> > >
> > >> Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart.
> > >
> > >The goal of the shared memory based SBI call for early prints is to
> > >minimize the number of traps which in-turn helps virtualization to
> > >drastically reduce boot-time.
> > >
> >
> > Understand that better now. But if that is the main motivation then
> > I am not understanding why we would want to push all of this into
> > M-mode firmware vs. defining a set of standardized pv-ops to be used
> > by guest OSes.
>
> The SBI debug console also provides a standard way of early prints for
> supervisor software running natively (directly under M-mode) irrespective
> of the type of UART/Console available on the console so it's not just
> for a virtualized world. Although, the virtualized world has an added
> performance advantage due to reduced traps.
>

It's also helpful in early board bringup and early debugging as
pointed out by Heiko as well.
To address some of the concerns raised above, how about putting some
restrictions on sbi_debug_console_set_area ?

1. Instead of any size, restrict the shared memory region to a fixed
size (256 byte should be enough for early prints) ?
It is still a common area across all harts. Thus, atomicity needs to
be ensured by the M-mode or HS mode.
We may explore setting up a per-heart area to avoid synchronization
issues but platforms with large hart counts may have to
share multiple pages depending on the page size.

2. The shared memory region should be set up only once during a boot
cycle. Any further invocation of sbi_debug_console_set_area
should return appropriate errors.

If the per heart shared memory region approach is preferred, the above
restriction applies once per hart.

> Regards,
> Anup
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#31): https://lists.riscv.org/g/tech-os-a-see/message/31
> Mute This Topic: https://lists.riscv.org/mt/91498450/1774178
> Group Owner: tech-os-a-see+owner at lists.riscv.org
> Unsubscribe: https://lists.riscv.org/g/tech-os-a-see/leave/11230180/1774178/2121601819/xyzzy [atishp at rivosinc.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

[sig-hypervisors] SBI Debug Console Extension Proposal (Draft v1)

[Stefano Stabellini]

On Thu, 2 Jun 2022, Schwarz, Konrad via lists.riscv.org wrote:
> Hi Anup,
> 
> > From: sig-hypervisors at lists.riscv.org <sig-hypervisors@lists.riscv.org> On Behalf Of Anup Patel via
> > lists.riscv.org
> > Subject: [sig-hypervisors] SBI Debug Console Extension Proposal (Draft v1)
> > 
> > Below is the draft proposal for SBI Debug Console Extension.
> 
> Here are my thoughts:
> 
> * Guest memory access: I think this would be the first SBI extension to require access to
>   guest memory. This needs to be considered carefully, but I think the higher bandwidth afforded by
>   the interface is useful enough to allow this.

> * API:
> 	* Currently, only a write interface is provided. It would be much better to have a
> 	  read/write interface.
> 
>         Benefits of this would be to allow a hypervisor to control an OS, e.g., for testing purposes
>         or to automate installation tasks.  Inter-guest communication could also be realized
>         via such an interface.
> 

This could be done. As an example Xen provides the hypercall
HYPERVISOR_console_io. The first parameter is the operation:
CONSOLEIO_write or CONSOLEIO_read.

The interface in RISC-V spec language would be something along these
lines:

struct sbiret sbi_debug_console_io(unsigned long operation, /* read or write */
                                   unsigned long address,
                                   unsigned long num_chars)

There is no memory area pre-registration required, however appropriate
checks on the validity of the address provided should always be done in
the implementation.

The good thing about getting rid of the pre-registration is that
multiple threads could make concurrent sbi_debug_console_io requests on
different CPUs.

I think it might be a good idea in the guest-side implementation (e.g.
Linux or Zephyr) to choose a specific memory area for this. However, it
doesn't have to be part of the interface. I think it should be an
implementation detail.

The firmware/hypervisor-side implementation of sbi_debug_console_io
can deal with any addresses provided as long as they are valid.

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Stefano Stabellini]

On Thu, 2 Jun 2022, Vedvyas Shanbhogue via lists.riscv.org wrote:
> > > Based on discussion it did not seem like it needs to be much fancier than
> > > this as this is for early OS/VMM code till it has enough functionality to
> > > directly interact with a uart.
> > 
> > The goal of the shared memory based SBI call for early prints is to
> > minimize the number of traps which in-turn helps virtualization to
> > drastically reduce boot-time.
> > 
> 
> Understand that better now. But if that is the main motivation then
> I am not understanding why we would want to push all of this into
> M-mode firmware vs. defining a set of standardized pv-ops to be used
> by guest OSes.

That is because it is useful to have debug console output when porting a
hypervisor or baremetal code to a new board.

Of course, if a hypervisor is already available for the board, then it
would be just as easy to use a paravirtualized interface, e.g. Xen's
HYPERVISOR_console_io hypercall. But somebody has to port the
hypervisor first :-)

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Ved Shanbhogue]

>Of course, if a hypervisor is already available for the board, then it
>would be just as easy to use a paravirtualized interface, e.g. Xen's
>HYPERVISOR_console_io hypercall. But somebody has to port the
>hypervisor first :-)
>
Of course a console is needed. But I was questioning the need for something much
more elaborate than a putchar/getchar interface. I understand its needed to port
the hypervisor but I undersatnd it would be needed for very early parts of the 
hypervisor where it does not even have the ability to master a uart on its own.

regards
ved

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Stefano Stabellini]

On Thu, 2 Jun 2022, Vedvyas Shanbhogue via lists.riscv.org wrote:
> > Of course, if a hypervisor is already available for the board, then it
> > would be just as easy to use a paravirtualized interface, e.g. Xen's
> > HYPERVISOR_console_io hypercall. But somebody has to port the
> > hypervisor first :-)
> > 
> Of course a console is needed. But I was questioning the need for something
> much
> more elaborate than a putchar/getchar interface. I understand its needed to
> port
> the hypervisor but I undersatnd it would be needed for very early parts of the
> hypervisor where it does not even have the ability to master a uart on its
> own.

Yeah, I think you are right that it is not a must-have feature but a
nice-to-have feature.

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Ved Shanbhogue]

On Thu, Jun 02, 2022 at 01:50:01PM -0700, Greg Favor wrote:
>On Thu, Jun 2, 2022 at 1:29 PM Stefano Stabellini <
>stefano.stabellini at xilinx.com> wrote:
>
>> On Thu, 2 Jun 2022, Vedvyas Shanbhogue via lists.riscv.org wrote:
>> > Of course a console is needed. But I was questioning the need for
>> something much
>> > more elaborate than a putchar/getchar interface. I understand its needed
>> to port
>> > the hypervisor but I undersatnd it would be needed for very early parts
>> of the
>> > hypervisor where it does not even have the ability to master a uart on
>> its own.
>>
>> Yeah, I think you are right that it is not a must-have feature but a
>> nice-to-have feature.
>
>
>Even though we all (me included) tend to think that slowing down something
>like this will be inconsequential to the time to boot and/or who cares how
>fast or slow is boot time, in the past I have found that to be a risky
>presumption in certain production environments where boot time matters
>(especially with frequent standing up of VMs from scratch).  So a
>worthwhile sanity check question would be 1) what percentage of early boot
>time is spent writing to the console with per-string SBI calls, and 2) by
>what order of magnitude does that percentage inflate due to SBI calls for
>every individual character.
>
I think the extension was proposed to be a Debug console extension.

In a production environment if 10's of VMs were booting having them emit
to the physical console - where all of those outputs will get interspersed
seems to be not so useful. In a production environment would we expect the
VMM to provide a console service and not need SBI calls into M-mode firmare
to emit VM output to a physical console?

I was thinking this was about early boot debug and not so much about needing
a console at early boot. In most production deployments there may not be 
someone sitting at a terminal watching prints and so it may make more sense
for VM console outputs to be logged into a persistent file somewhere for example.
Or be held in the VM itself - e.g. a kernel ring buffer that someone may want
to display using a tool like dmesg when needed but otherwise does not get 
emitted chattily to a physical console.

Hope I am understanding this right.

regards
ved

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Stefano Stabellini]

On Thu, 2 Jun 2022, Greg Favor wrote:
> On Thu, Jun 2, 2022 at 1:29 PM Stefano Stabellini <stefano.stabellini@xilinx.com> wrote:
>       On Thu, 2 Jun 2022, Vedvyas Shanbhogue via lists.riscv.org wrote:
>       > Of course a console is needed. But I was questioning the need for something much
>       > more elaborate than a putchar/getchar interface. I understand its needed to port
>       > the hypervisor but I undersatnd it would be needed for very early parts of the
>       > hypervisor where it does not even have the ability to master a uart on its own.
> 
>       Yeah, I think you are right that it is not a must-have feature but a
>       nice-to-have feature.
> 
> 
> Even though we all (me included) tend to think that slowing down something like this will be inconsequential to the time to boot and/or who
> cares how fast or slow is boot?time, in the past I have found that to be a risky presumption in certain production environments where boot
> time matters (especially with frequent standing up of VMs from scratch).? So a worthwhile sanity check question would be 1) what percentage
> of early boot time is spent writing to the console with per-string SBI calls, and 2) by what order of magnitude does that percentage
> inflate due to SBI calls for every individual character.

The numbers you asked will help but I just wanted to add that in my
opinion a per-character putchar interface is not suitable for production
due to the reasons you mentioned. I would compile it out in non-DEBUG
builds.

On the other hand the per-string print interface might be usable in
production, even not just early boot. However, of course, a proper UART
driver would be even better.

This is why I think it is nice to have: it expands the potential uses of
the SBI console interface, and it is simple and easy to use. At the same
time it is not a must-have because you could get away with just putchat
in early boot for DEBUG builds, and nothing + UART driver in non-DEBUG
builds.

That is why I am in favor of this addition, although I recognize it is
not critical.

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Anup Patel]

Hi Stefano,

On Fri, Jun 3, 2022 at 2:35 AM Stefano Stabellini
<stefano.stabellini@xilinx.com> wrote:
>
> On Thu, 2 Jun 2022, Greg Favor wrote:
> > On Thu, Jun 2, 2022 at 1:29 PM Stefano Stabellini <stefano.stabellini@xilinx.com> wrote:
> >       On Thu, 2 Jun 2022, Vedvyas Shanbhogue via lists.riscv.org wrote:
> >       > Of course a console is needed. But I was questioning the need for something much
> >       > more elaborate than a putchar/getchar interface. I understand its needed to port
> >       > the hypervisor but I undersatnd it would be needed for very early parts of the
> >       > hypervisor where it does not even have the ability to master a uart on its own.
> >
> >       Yeah, I think you are right that it is not a must-have feature but a
> >       nice-to-have feature.
> >
> >
> > Even though we all (me included) tend to think that slowing down something like this will be inconsequential to the time to boot and/or who
> > cares how fast or slow is boot time, in the past I have found that to be a risky presumption in certain production environments where boot
> > time matters (especially with frequent standing up of VMs from scratch).  So a worthwhile sanity check question would be 1) what percentage
> > of early boot time is spent writing to the console with per-string SBI calls, and 2) by what order of magnitude does that percentage
> > inflate due to SBI calls for every individual character.
>
> The numbers you asked will help but I just wanted to add that in my
> opinion a per-character putchar interface is not suitable for production
> due to the reasons you mentioned. I would compile it out in non-DEBUG
> builds.
>
> On the other hand the per-string print interface might be usable in
> production, even not just early boot. However, of course, a proper UART
> driver would be even better.
>
> This is why I think it is nice to have: it expands the potential uses of
> the SBI console interface, and it is simple and easy to use. At the same
> time it is not a must-have because you could get away with just putchat
> in early boot for DEBUG builds, and nothing + UART driver in non-DEBUG
> builds.
>
> That is why I am in favor of this addition, although I recognize it is
> not critical.

We have legacy SBI v0.1 calls where most of them are replaced by
newer SBI v0.2 (or higher) calls. Only the SBI v0.1 putchar() does not
have a replacement. This putchar() was being used in various cases
for early prints from OSes and bootloaders.

The SBI debug console extension is an attempt to replace the legacy
SBI v0.1 putchar(). The use of shared memory in this proposal is
motivated by the need to allow users to print multiple characters in
one SBI call.

The legacy SBI v0.1 also had a getchar() call which is deprecated and
does not need any newer SBI call replacing it because it is expected
all platforms (including virtual platforms emulated by hypervisors) will
have a proper interrupt driver console for supervisor software. The
proposed SBI debug console extension only tries to provide a solution
for early prints.

Regards,
Anup

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Atish Kumar Patra]

On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 6/2/22 10:44, Anup Patel wrote:
> > On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 6/1/22 18:17, Anup Patel wrote:
> >>> Hi All,
> >>>
> >>> Below is the draft proposal for SBI Debug Console Extension.
> >>>
> >>> Please review it and provide feedback.
> >>>
> >>> Thanks,
> >>> Anup
> >>>
> >>> Debug Console Extension (EID #0x4442434E "DBCN")
> >>> ================================================
> >>>
> >>> The debug console extension defines a generic mechanism for boot-time
> >>> early prints from supervisor-mode software which allows users to catch
> >>> boot-time issues in supervisor-mode software.
> >>>
> >>> This extension replaces legacy console putchar (EID #0x01) extension
> >>> and it is better in following ways:
> >>
> >> Thanks for starting to close this gap.
> >>
> >>> 1) It follows the new calling convention defined for SBI v1.0
> >>>      (or higher).
> >>> 2) It is based on a shared memory area between SBI implementation
> >>>      and supervisor-mode software so multiple characters can be
> >>>      printed using a single SBI call.
> >>
> >> I miss a discussion of the conflicts that can arise if the configuration
> >> of the serial console is changed by the caller.
> >>
> >> Do we need an ecall that closes the SBI console to further access?
> >
> > Usually, the serial port related code in M-mode firmware only uses
> > status and data registers so for most serial ports support the M-mode
> > firmware will adapt to serial port configuration changes.
> >
> > In fact, this is why we never had a special SBI call for serial port
> > reconfiguration in legacy SBI v0.1 as well.
> >
> > In case of virtualization, the serial port (or console) is emulated so
> > the special SBI call is not useful for virtualization.
> >
> >>
> >>>
> >>> The supervisor-mode software must set the shared memory area before
> >>> printing characters on the debug console. Also, all HARTs share the
> >>> same shared memory area so only one HART needs to set it at boot-time.
> >>
> >> Isn't it M-mode software that has to program the MMU to allow all harts
> >> in M-mode and S-mode access to the memory area? What is the S-mode
> >> software to do about the memory area prior to calling
> >> sbi_debug_console_set_area()?
> >
> > Actually, it's the S-mode software which is voluntarily giving a portion of
> > its memory to be used as shared memory. The proposal only mandates
> > that whatever memory is selected by S-mode software should be a
> > regular cacheable memory (not IO memory). Also, if Svpbmt is available
> > then S-mode software should only use memory type 0 in the PTEs.
> >
> >>
> >>>
> >>> Function: Set Console Area (FID #0)
> >>> -----------------------------------
> >>>
> >>> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >>>                                            unsigned long size)
> >>
> >> The console output is needed on the very start of the S-mode software,
> >> before setting up anything.
> >>
> >> Can we avoid this extra function?
> >>
> >> Can we simply assume that the caller of sbi_debug_console_puts()
> >> provides a physical address pointer to a memory area that is suitable?
> >
> > Theoretically, we can avoid the extra function to set shared memory area
> > but it will complicate things in future when we have supervisor software
> > encrypting it's own memory (using special ISA support) because in this
> > case supervisor software will have to unprotect memory every time the
> > sbi_debug_console_puts() is called and protect it again after the call.
>
> Currently this function is just a nop(). It is not needed in this
> revision of the extension.
>
> The function might be called repeatedly by different threads with
> different values. How do you want to keep track of all of these
> different areas?
>
> Memory shared between different security realms will arise in many
> different scenarios. As this is not console specific it should be in a
> separate extension. That extension should be defined once we have
> clarity about how security realms are managed.
>

I would like to understand more about the security concerns you raised.
Can you please elaborate on this ?

As you pointed out, there are other possible use cases(e.g STA, PMU)
for shared memory between
S & M mode or VS & HS mode. It would be good to address these concerns right now
instead of discussing those in each individual extension context.

> >
> >>
> >>>
> >>> Set the shared memory area specified by `addr_div_by_2` and `size`
> >>
> >> %s/addr_div_by_2/addr_div_by_4/
> >
> > Okay, I will update.
> >
> >>
> >>> parameters. The `addr_div_by_4` parameter is base address of the
> >>
> >> %s/is base/is the base/
> >
> > Okay, I will update.
> >
> >>
> >>> shared memory area right shifted by 2 whereas `size` parameter is
> >>> the size of shared memory area in bytes.
> >>
> >> Why shifting the address? I would prefer to keep it simple for the
> >> caller. If the alignment is not suitable, return an error.
> >>
> >> But why is an alignment needed here at all? And why 4 aligned?
> >
> > For RV32 S-mode, the physical address space is 34bits wide but
> > "unsigned long" is 32bits wide. This is because Sv32 PTEs allow
> > 34bits of PPN. In fact, even instructions such as HFENCE.GVMA
> > have this "address right shift by 2" requirement based on this rationale.
> >
> >>
> >>>
> >>> The shared memory area should be normal cacheable memory for the
> >>> supervisor-mode software. Also, the shared memory area is global
> >>> across all HARTs so SBI implementation must ensure atomicity in
> >>> setting the shared memory area.
> >>>
> >>> Errors:
> >>> SBI_SUCCESS                - Shared memory area set successfully.
> >>> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >>>                             `addr_div_by_2` and `size` parameters
> >>>                             is not normal cacheable memory or not
> >>>                             accessible to supervisor-mode software.
> >>>
> >>> Function: Console Puts (FID #1)
> >>> -------------------------------
> >>>
> >>> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >>>                                        unsigned long num_chars)
> >>
> >> I would prefer to simply pass a physical address pointer here with no
> >> requirements on alignment. And no prior SBI call.
>
> sbi_debug_console_set_area() might be called with different values by
> different threads. An offset is ambiguous as it does not define to which
> of the different shared areas it relates. Please, use a pointer.
>
> >>
> >> Do we need num_chars? Are we expecting to provide binary output? Using
> >> 0x00 as terminator would be adequate in most cases.
> >
> > Bare-metal tests (or assembly sources) can print sub-strings from
> > a large per-populated string in shared memory. Assuming that string
> > is always terminated by 0x00 in sbi_debug_console_puts() will break
> > this flexibility.
>
> OK
>
> >
> >>
> >> What is the requirement on the console? Does it have to support 8bit
> >> output to allow for UTF-8?
> >
> > We need to clarify this. Suggestions ?
>
> The platform specification would be the right place to require 8-bit
> support for the console.
>
> >
> >>
> >> Do we make any assumptions about encoding?
> >
> > Same as above, this needs more clarification. Suggestions ?
>
> We should add to the platform specification that UTF-8 output is assumed
> on the serial console.
>
> >
> > I am of the opinion to keep such encoding related assumptions to be
> > minimal.
> >
> >>
> >> How would we handle a console set up to 7bit + parity if a character >
> >> 0x7f is sent?
> >
> > I would consider this to be part of the clarification we add for encoding.
>
> We should state if extra bits are ignored or the bytes are not send.
> The easiest thing is to just ignore the extra bits. So let't state this
> here.
>
> Best regards
>
> Heinrich
>
> >
> >>
> >>>
> >>> Print the string specified by `area_offset` and `num_chars` on
> >>> the debug console. The `area_offset` parameter is the start of
> >>> string in the shard memory area whereas `num_chars` parameter
> >>
> >> %s/shard/shared/
> >
> > Okay, I will update.
> >
> >>
> >>> is the number of characters (or bytes) in the string.
> >>>
> >>> This is a blocking SBI call and will only return after printing
> >>> all characters of the string.
> >>>
> >>> Errors:
> >>> SBI_SUCCESS                - Characters printed successfully.
> >>> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >>>                             `area_offset`) or end of the string
> >>>                             (i.e. `area_offset + num_chars`) is
> >>>                             outside shared memory area.
> >>
> >> There could be other reasons of failures:
> >>
> >> * set up of the UART failed in OpenSBI
> >> * no UART defined in the device-tree
> >> * ...
> >>
> >> So let us add SBI_ERR_FAILED to the list.
> >
> > Okay, I will add.
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >
> > Regards,
> > Anup
>
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#1717): https://lists.riscv.org/g/tech-unixplatformspec/message/1717
> Mute This Topic: https://lists.riscv.org/mt/91480122/1774178
> Group Owner: tech-unixplatformspec+owner at lists.riscv.org
> Unsubscribe: https://lists.riscv.org/g/tech-unixplatformspec/unsub [atishp at rivosinc.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Heinrich Schuchardt]

On 6/3/22 08:56, Atish Kumar Patra wrote:
> On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 6/2/22 10:44, Anup Patel wrote:
>>> On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> On 6/1/22 18:17, Anup Patel wrote:
>>>>> Hi All,
>>>>>
>>>>> Below is the draft proposal for SBI Debug Console Extension.
>>>>>
>>>>> Please review it and provide feedback.
>>>>>
>>>>> Thanks,
>>>>> Anup
>>>>>
>>>>> Debug Console Extension (EID #0x4442434E "DBCN")
>>>>> ================================================
>>>>>
>>>>> The debug console extension defines a generic mechanism for boot-time
>>>>> early prints from supervisor-mode software which allows users to catch
>>>>> boot-time issues in supervisor-mode software.
>>>>>
>>>>> This extension replaces legacy console putchar (EID #0x01) extension
>>>>> and it is better in following ways:
>>>>
>>>> Thanks for starting to close this gap.
>>>>
>>>>> 1) It follows the new calling convention defined for SBI v1.0
>>>>>       (or higher).
>>>>> 2) It is based on a shared memory area between SBI implementation
>>>>>       and supervisor-mode software so multiple characters can be
>>>>>       printed using a single SBI call.
>>>>
>>>> I miss a discussion of the conflicts that can arise if the configuration
>>>> of the serial console is changed by the caller.
>>>>
>>>> Do we need an ecall that closes the SBI console to further access?
>>>
>>> Usually, the serial port related code in M-mode firmware only uses
>>> status and data registers so for most serial ports support the M-mode
>>> firmware will adapt to serial port configuration changes.
>>>
>>> In fact, this is why we never had a special SBI call for serial port
>>> reconfiguration in legacy SBI v0.1 as well.
>>>
>>> In case of virtualization, the serial port (or console) is emulated so
>>> the special SBI call is not useful for virtualization.
>>>
>>>>
>>>>>
>>>>> The supervisor-mode software must set the shared memory area before
>>>>> printing characters on the debug console. Also, all HARTs share the
>>>>> same shared memory area so only one HART needs to set it at boot-time.
>>>>
>>>> Isn't it M-mode software that has to program the MMU to allow all harts
>>>> in M-mode and S-mode access to the memory area? What is the S-mode
>>>> software to do about the memory area prior to calling
>>>> sbi_debug_console_set_area()?
>>>
>>> Actually, it's the S-mode software which is voluntarily giving a portion of
>>> its memory to be used as shared memory. The proposal only mandates
>>> that whatever memory is selected by S-mode software should be a
>>> regular cacheable memory (not IO memory). Also, if Svpbmt is available
>>> then S-mode software should only use memory type 0 in the PTEs.
>>>
>>>>
>>>>>
>>>>> Function: Set Console Area (FID #0)
>>>>> -----------------------------------
>>>>>
>>>>> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
>>>>>                                             unsigned long size)
>>>>
>>>> The console output is needed on the very start of the S-mode software,
>>>> before setting up anything.
>>>>
>>>> Can we avoid this extra function?
>>>>
>>>> Can we simply assume that the caller of sbi_debug_console_puts()
>>>> provides a physical address pointer to a memory area that is suitable?
>>>
>>> Theoretically, we can avoid the extra function to set shared memory area
>>> but it will complicate things in future when we have supervisor software
>>> encrypting it's own memory (using special ISA support) because in this
>>> case supervisor software will have to unprotect memory every time the
>>> sbi_debug_console_puts() is called and protect it again after the call.
>>
>> Currently this function is just a nop(). It is not needed in this
>> revision of the extension.
>>
>> The function might be called repeatedly by different threads with
>> different values. How do you want to keep track of all of these
>> different areas?
>>
>> Memory shared between different security realms will arise in many
>> different scenarios. As this is not console specific it should be in a
>> separate extension. That extension should be defined once we have
>> clarity about how security realms are managed.
>>
> 
> I would like to understand more about the security concerns you raised.
> Can you please elaborate on this ?

I have no security concerns. My thought was that it might be better to 
manage shared memory in a separate extension for all use cases.

Best regards

Heinrich

> 
> As you pointed out, there are other possible use cases(e.g STA, PMU)
> for shared memory between
> S & M mode or VS & HS mode. It would be good to address these concerns right now
> instead of discussing those in each individual extension context.
> 
>>>
>>>>
>>>>>
>>>>> Set the shared memory area specified by `addr_div_by_2` and `size`
>>>>
>>>> %s/addr_div_by_2/addr_div_by_4/
>>>
>>> Okay, I will update.
>>>
>>>>
>>>>> parameters. The `addr_div_by_4` parameter is base address of the
>>>>
>>>> %s/is base/is the base/
>>>
>>> Okay, I will update.
>>>
>>>>
>>>>> shared memory area right shifted by 2 whereas `size` parameter is
>>>>> the size of shared memory area in bytes.
>>>>
>>>> Why shifting the address? I would prefer to keep it simple for the
>>>> caller. If the alignment is not suitable, return an error.
>>>>
>>>> But why is an alignment needed here at all? And why 4 aligned?
>>>
>>> For RV32 S-mode, the physical address space is 34bits wide but
>>> "unsigned long" is 32bits wide. This is because Sv32 PTEs allow
>>> 34bits of PPN. In fact, even instructions such as HFENCE.GVMA
>>> have this "address right shift by 2" requirement based on this rationale.
>>>
>>>>
>>>>>
>>>>> The shared memory area should be normal cacheable memory for the
>>>>> supervisor-mode software. Also, the shared memory area is global
>>>>> across all HARTs so SBI implementation must ensure atomicity in
>>>>> setting the shared memory area.
>>>>>
>>>>> Errors:
>>>>> SBI_SUCCESS                - Shared memory area set successfully.
>>>>> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
>>>>>                              `addr_div_by_2` and `size` parameters
>>>>>                              is not normal cacheable memory or not
>>>>>                              accessible to supervisor-mode software.
>>>>>
>>>>> Function: Console Puts (FID #1)
>>>>> -------------------------------
>>>>>
>>>>> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
>>>>>                                         unsigned long num_chars)
>>>>
>>>> I would prefer to simply pass a physical address pointer here with no
>>>> requirements on alignment. And no prior SBI call.
>>
>> sbi_debug_console_set_area() might be called with different values by
>> different threads. An offset is ambiguous as it does not define to which
>> of the different shared areas it relates. Please, use a pointer.
>>
>>>>
>>>> Do we need num_chars? Are we expecting to provide binary output? Using
>>>> 0x00 as terminator would be adequate in most cases.
>>>
>>> Bare-metal tests (or assembly sources) can print sub-strings from
>>> a large per-populated string in shared memory. Assuming that string
>>> is always terminated by 0x00 in sbi_debug_console_puts() will break
>>> this flexibility.
>>
>> OK
>>
>>>
>>>>
>>>> What is the requirement on the console? Does it have to support 8bit
>>>> output to allow for UTF-8?
>>>
>>> We need to clarify this. Suggestions ?
>>
>> The platform specification would be the right place to require 8-bit
>> support for the console.
>>
>>>
>>>>
>>>> Do we make any assumptions about encoding?
>>>
>>> Same as above, this needs more clarification. Suggestions ?
>>
>> We should add to the platform specification that UTF-8 output is assumed
>> on the serial console.
>>
>>>
>>> I am of the opinion to keep such encoding related assumptions to be
>>> minimal.
>>>
>>>>
>>>> How would we handle a console set up to 7bit + parity if a character >
>>>> 0x7f is sent?
>>>
>>> I would consider this to be part of the clarification we add for encoding.
>>
>> We should state if extra bits are ignored or the bytes are not send.
>> The easiest thing is to just ignore the extra bits. So let't state this
>> here.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>
>>>>>
>>>>> Print the string specified by `area_offset` and `num_chars` on
>>>>> the debug console. The `area_offset` parameter is the start of
>>>>> string in the shard memory area whereas `num_chars` parameter
>>>>
>>>> %s/shard/shared/
>>>
>>> Okay, I will update.
>>>
>>>>
>>>>> is the number of characters (or bytes) in the string.
>>>>>
>>>>> This is a blocking SBI call and will only return after printing
>>>>> all characters of the string.
>>>>>
>>>>> Errors:
>>>>> SBI_SUCCESS                - Characters printed successfully.
>>>>> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
>>>>>                              `area_offset`) or end of the string
>>>>>                              (i.e. `area_offset + num_chars`) is
>>>>>                              outside shared memory area.
>>>>
>>>> There could be other reasons of failures:
>>>>
>>>> * set up of the UART failed in OpenSBI
>>>> * no UART defined in the device-tree
>>>> * ...
>>>>
>>>> So let us add SBI_ERR_FAILED to the list.
>>>
>>> Okay, I will add.
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>
>>> Regards,
>>> Anup
>>
>>
>>
>> -=-=-=-=-=-=-=-=-=-=-=-
>> Links: You receive all messages sent to this group.
>> View/Reply Online (#1717): https://lists.riscv.org/g/tech-unixplatformspec/message/1717
>> Mute This Topic: https://lists.riscv.org/mt/91480122/1774178
>> Group Owner: tech-unixplatformspec+owner at lists.riscv.org
>> Unsubscribe: https://lists.riscv.org/g/tech-unixplatformspec/unsub [atishp at rivosinc.com]
>> -=-=-=-=-=-=-=-=-=-=-=-
>>
>>

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Atish Kumar Patra]

On Fri, Jun 3, 2022 at 12:23 AM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 6/3/22 08:56, Atish Kumar Patra wrote:
> > On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 6/2/22 10:44, Anup Patel wrote:
> >>> On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> On 6/1/22 18:17, Anup Patel wrote:
> >>>>> Hi All,
> >>>>>
> >>>>> Below is the draft proposal for SBI Debug Console Extension.
> >>>>>
> >>>>> Please review it and provide feedback.
> >>>>>
> >>>>> Thanks,
> >>>>> Anup
> >>>>>
> >>>>> Debug Console Extension (EID #0x4442434E "DBCN")
> >>>>> ================================================
> >>>>>
> >>>>> The debug console extension defines a generic mechanism for boot-time
> >>>>> early prints from supervisor-mode software which allows users to catch
> >>>>> boot-time issues in supervisor-mode software.
> >>>>>
> >>>>> This extension replaces legacy console putchar (EID #0x01) extension
> >>>>> and it is better in following ways:
> >>>>
> >>>> Thanks for starting to close this gap.
> >>>>
> >>>>> 1) It follows the new calling convention defined for SBI v1.0
> >>>>>       (or higher).
> >>>>> 2) It is based on a shared memory area between SBI implementation
> >>>>>       and supervisor-mode software so multiple characters can be
> >>>>>       printed using a single SBI call.
> >>>>
> >>>> I miss a discussion of the conflicts that can arise if the configuration
> >>>> of the serial console is changed by the caller.
> >>>>
> >>>> Do we need an ecall that closes the SBI console to further access?
> >>>
> >>> Usually, the serial port related code in M-mode firmware only uses
> >>> status and data registers so for most serial ports support the M-mode
> >>> firmware will adapt to serial port configuration changes.
> >>>
> >>> In fact, this is why we never had a special SBI call for serial port
> >>> reconfiguration in legacy SBI v0.1 as well.
> >>>
> >>> In case of virtualization, the serial port (or console) is emulated so
> >>> the special SBI call is not useful for virtualization.
> >>>
> >>>>
> >>>>>
> >>>>> The supervisor-mode software must set the shared memory area before
> >>>>> printing characters on the debug console. Also, all HARTs share the
> >>>>> same shared memory area so only one HART needs to set it at boot-time.
> >>>>
> >>>> Isn't it M-mode software that has to program the MMU to allow all harts
> >>>> in M-mode and S-mode access to the memory area? What is the S-mode
> >>>> software to do about the memory area prior to calling
> >>>> sbi_debug_console_set_area()?
> >>>
> >>> Actually, it's the S-mode software which is voluntarily giving a portion of
> >>> its memory to be used as shared memory. The proposal only mandates
> >>> that whatever memory is selected by S-mode software should be a
> >>> regular cacheable memory (not IO memory). Also, if Svpbmt is available
> >>> then S-mode software should only use memory type 0 in the PTEs.
> >>>
> >>>>
> >>>>>
> >>>>> Function: Set Console Area (FID #0)
> >>>>> -----------------------------------
> >>>>>
> >>>>> struct sbiret sbi_debug_console_set_area(unsigned long addr_div_by_4,
> >>>>>                                             unsigned long size)
> >>>>
> >>>> The console output is needed on the very start of the S-mode software,
> >>>> before setting up anything.
> >>>>
> >>>> Can we avoid this extra function?
> >>>>
> >>>> Can we simply assume that the caller of sbi_debug_console_puts()
> >>>> provides a physical address pointer to a memory area that is suitable?
> >>>
> >>> Theoretically, we can avoid the extra function to set shared memory area
> >>> but it will complicate things in future when we have supervisor software
> >>> encrypting it's own memory (using special ISA support) because in this
> >>> case supervisor software will have to unprotect memory every time the
> >>> sbi_debug_console_puts() is called and protect it again after the call.
> >>
> >> Currently this function is just a nop(). It is not needed in this
> >> revision of the extension.
> >>
> >> The function might be called repeatedly by different threads with
> >> different values. How do you want to keep track of all of these
> >> different areas?
> >>
> >> Memory shared between different security realms will arise in many
> >> different scenarios. As this is not console specific it should be in a
> >> separate extension. That extension should be defined once we have
> >> clarity about how security realms are managed.
> >>
> >
> > I would like to understand more about the security concerns you raised.
> > Can you please elaborate on this ?
>
> I have no security concerns. My thought was that it might be better to
> manage shared memory in a separate extension for all use cases.
>

Ahh I see. Sorry I misunderstood your comment.
The designated shared memory for debug console is shared across
the platforms while STA or PMU use case will have per cpu shared memory.

I agree that all the concerns related to shared memory can be discussed
together and all the common constraints can be described at one place.

But defining a separate extension for the shared memory may be problematic
as we need to manage the extension dependencies.

> Best regards
>
> Heinrich
>
> >
> > As you pointed out, there are other possible use cases(e.g STA, PMU)
> > for shared memory between
> > S & M mode or VS & HS mode. It would be good to address these concerns right now
> > instead of discussing those in each individual extension context.
> >
> >>>
> >>>>
> >>>>>
> >>>>> Set the shared memory area specified by `addr_div_by_2` and `size`
> >>>>
> >>>> %s/addr_div_by_2/addr_div_by_4/
> >>>
> >>> Okay, I will update.
> >>>
> >>>>
> >>>>> parameters. The `addr_div_by_4` parameter is base address of the
> >>>>
> >>>> %s/is base/is the base/
> >>>
> >>> Okay, I will update.
> >>>
> >>>>
> >>>>> shared memory area right shifted by 2 whereas `size` parameter is
> >>>>> the size of shared memory area in bytes.
> >>>>
> >>>> Why shifting the address? I would prefer to keep it simple for the
> >>>> caller. If the alignment is not suitable, return an error.
> >>>>
> >>>> But why is an alignment needed here at all? And why 4 aligned?
> >>>
> >>> For RV32 S-mode, the physical address space is 34bits wide but
> >>> "unsigned long" is 32bits wide. This is because Sv32 PTEs allow
> >>> 34bits of PPN. In fact, even instructions such as HFENCE.GVMA
> >>> have this "address right shift by 2" requirement based on this rationale.
> >>>
> >>>>
> >>>>>
> >>>>> The shared memory area should be normal cacheable memory for the
> >>>>> supervisor-mode software. Also, the shared memory area is global
> >>>>> across all HARTs so SBI implementation must ensure atomicity in
> >>>>> setting the shared memory area.
> >>>>>
> >>>>> Errors:
> >>>>> SBI_SUCCESS                - Shared memory area set successfully.
> >>>>> SBI_ERR_INVALID_ADDRESS - The shared memory area pointed by
> >>>>>                              `addr_div_by_2` and `size` parameters
> >>>>>                              is not normal cacheable memory or not
> >>>>>                              accessible to supervisor-mode software.
> >>>>>
> >>>>> Function: Console Puts (FID #1)
> >>>>> -------------------------------
> >>>>>
> >>>>> struct sbiret sbi_debug_console_puts(unsigned long area_offset,
> >>>>>                                         unsigned long num_chars)
> >>>>
> >>>> I would prefer to simply pass a physical address pointer here with no
> >>>> requirements on alignment. And no prior SBI call.
> >>
> >> sbi_debug_console_set_area() might be called with different values by
> >> different threads. An offset is ambiguous as it does not define to which
> >> of the different shared areas it relates. Please, use a pointer.
> >>
> >>>>
> >>>> Do we need num_chars? Are we expecting to provide binary output? Using
> >>>> 0x00 as terminator would be adequate in most cases.
> >>>
> >>> Bare-metal tests (or assembly sources) can print sub-strings from
> >>> a large per-populated string in shared memory. Assuming that string
> >>> is always terminated by 0x00 in sbi_debug_console_puts() will break
> >>> this flexibility.
> >>
> >> OK
> >>
> >>>
> >>>>
> >>>> What is the requirement on the console? Does it have to support 8bit
> >>>> output to allow for UTF-8?
> >>>
> >>> We need to clarify this. Suggestions ?
> >>
> >> The platform specification would be the right place to require 8-bit
> >> support for the console.
> >>
> >>>
> >>>>
> >>>> Do we make any assumptions about encoding?
> >>>
> >>> Same as above, this needs more clarification. Suggestions ?
> >>
> >> We should add to the platform specification that UTF-8 output is assumed
> >> on the serial console.
> >>
> >>>
> >>> I am of the opinion to keep such encoding related assumptions to be
> >>> minimal.
> >>>
> >>>>
> >>>> How would we handle a console set up to 7bit + parity if a character >
> >>>> 0x7f is sent?
> >>>
> >>> I would consider this to be part of the clarification we add for encoding.
> >>
> >> We should state if extra bits are ignored or the bytes are not send.
> >> The easiest thing is to just ignore the extra bits. So let't state this
> >> here.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>>
> >>>>>
> >>>>> Print the string specified by `area_offset` and `num_chars` on
> >>>>> the debug console. The `area_offset` parameter is the start of
> >>>>> string in the shard memory area whereas `num_chars` parameter
> >>>>
> >>>> %s/shard/shared/
> >>>
> >>> Okay, I will update.
> >>>
> >>>>
> >>>>> is the number of characters (or bytes) in the string.
> >>>>>
> >>>>> This is a blocking SBI call and will only return after printing
> >>>>> all characters of the string.
> >>>>>
> >>>>> Errors:
> >>>>> SBI_SUCCESS                - Characters printed successfully.
> >>>>> SBI_ERR_INVALID_ADDRESS    - The start of the string (i.e.
> >>>>>                              `area_offset`) or end of the string
> >>>>>                              (i.e. `area_offset + num_chars`) is
> >>>>>                              outside shared memory area.
> >>>>
> >>>> There could be other reasons of failures:
> >>>>
> >>>> * set up of the UART failed in OpenSBI
> >>>> * no UART defined in the device-tree
> >>>> * ...
> >>>>
> >>>> So let us add SBI_ERR_FAILED to the list.
> >>>
> >>> Okay, I will add.
> >>>
> >>>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>
> >>> Regards,
> >>> Anup
> >>
> >>
> >>
> >> -=-=-=-=-=-=-=-=-=-=-=-
> >> Links: You receive all messages sent to this group.
> >> View/Reply Online (#1717): https://lists.riscv.org/g/tech-unixplatformspec/message/1717
> >> Mute This Topic: https://lists.riscv.org/mt/91480122/1774178
> >> Group Owner: tech-unixplatformspec+owner at lists.riscv.org
> >> Unsubscribe: https://lists.riscv.org/g/tech-unixplatformspec/unsub [atishp at rivosinc.com]
> >> -=-=-=-=-=-=-=-=-=-=-=-
> >>
> >>
>

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Schwarz, Konrad]

> From: sig-hypervisors at lists.riscv.org <sig-hypervisors@lists.riscv.org> On Behalf Of Anup Patel via
> lists.riscv.org
> Sent: Friday, June 3, 2022 5:01

> We have legacy SBI v0.1 calls where most of them are replaced by
> newer SBI v0.2 (or higher) calls. Only the SBI v0.1 putchar() does not
> have a replacement. This putchar() was being used in various cases
> for early prints from OSes and bootloaders.
> 
> The SBI debug console extension is an attempt to replace the legacy
> SBI v0.1 putchar(). The use of shared memory in this proposal is
> motivated by the need to allow users to print multiple characters in
> one SBI call.

As I pointed out in an earlier message, the design using a shared memory block
is a hindrance.  Instead, each call should provide a pointer to the (virtual)
address of the character buffer.

> The legacy SBI v0.1 also had a getchar() call which is deprecated and
> does not need any newer SBI call replacing it because it is expected
> all platforms (including virtual platforms emulated by hypervisors) will
> have a proper interrupt driver console for supervisor software. The
> proposed SBI debug console extension only tries to provide a solution
> for early prints.

Consider the case where we have more VMs than physical UARTs -- this
will actually be the norm in most deployments.

(To counter the argument that the hypervisor can provide virtual UARTs
for its guests, note that a dedicated para-virtualized interface is much more
efficient than trapping individual memory accesses to simulated HW registers).

By making the interface a bit richer, as discussed in my earlier proposal,
a hypervisor can cover a much larger set of use cases. 


-- 
Konrad

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Alistair Francis]

On Fri, 2022-06-03 at 07:49 +0000, Schwarz, Konrad wrote:
> 
> > From:
> > sig-hypervisors at lists.riscv.org?<sig-hypervisors@lists.riscv.org>
> > On Behalf Of Anup Patel via
> > lists.riscv.org
> > Sent: Friday, June 3, 2022 5:01
> 
> > We have legacy SBI v0.1 calls where most of them are replaced by
> > newer SBI v0.2 (or higher) calls. Only the SBI v0.1 putchar() does
> > not
> > have a replacement. This putchar() was being used in various cases
> > for early prints from OSes and bootloaders.
> > 
> > The SBI debug console extension is an attempt to replace the legacy
> > SBI v0.1 putchar(). The use of shared memory in this proposal is
> > motivated by the need to allow users to print multiple characters
> > in
> > one SBI call.
> 
> As I pointed out in an earlier message, the design using a shared
> memory block
> is a hindrance.? Instead, each call should provide a pointer to the
> (virtual)
> address of the character buffer.

I agree

With the current design there is no way for the supervisor software to
reclaim the memory once it has shared it with OpenSBI. We can add a
close() call, but it seems simpler and safer to instead just pass an
address and size to print.

> 
> > The legacy SBI v0.1 also had a getchar() call which is deprecated
> > and
> > does not need any newer SBI call replacing it because it is
> > expected
> > all platforms (including virtual platforms emulated by hypervisors)
> > will
> > have a proper interrupt driver console for supervisor software. The
> > proposed SBI debug console extension only tries to provide a
> > solution
> > for early prints.
> 
> Consider the case where we have more VMs than physical UARTs -- this
> will actually be the norm in most deployments.

In this case you should use virtIO or some other more powerful
interface

> 
> (To counter the argument that the hypervisor can provide virtual
> UARTs
> for its guests, note that a dedicated para-virtualized interface is
> much more
> efficient than trapping individual memory accesses to simulated HW
> registers).

Agreed, but tha seems up to the Hypervisor to implement an effient
virtual UART implementation. For example the Xen HYPERVISOR_console_io
that Stefano mentioned earlier. Plus then we can leverage existing
implementations insted of inveting a another virtual serial device.

> 
> By making the interface a bit richer, as discussed in my earlier
> proposal,
> a hypervisor can cover a much larger set of use cases. 

Which we also then need to support in M-mode firmware. The more complex
the firmware becomes the more exposed it is.

Alistair

> 
> 
> -- 
> Konrad
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#1747):
> https://lists.riscv.org/g/tech-unixplatformspec/message/1747
> Mute This Topic: https://lists.riscv.org/mt/91507495/1782774
> Group Owner: tech-unixplatformspec+owner at lists.riscv.org
> Unsubscribe:
> https://lists.riscv.org/g/tech-unixplatformspec/unsub?[alistair.francis at wdc.com
> ]
> -=-=-=-=-=-=-=-=-=-=-=-
> 
> 

[sig-hypervisors] [RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Schwarz, Konrad]

> -----Original Message-----
> From: Alistair Francis <Alistair.Francis@wdc.com>
> Sent: Friday, June 3, 2022 10:47
> > Consider the case where we have more VMs than physical UARTs -- this
> > will actually be the norm in most deployments.
> 
> In this case you should use virtIO or some other more powerful
> interface
> 
> >
> > (To counter the argument that the hypervisor can provide virtual
> > UARTs
> > for its guests, note that a dedicated para-virtualized interface is
> > much more
> > efficient than trapping individual memory accesses to simulated HW
> > registers).
> 
> Agreed, but tha seems up to the Hypervisor to implement an effient
> virtual UART implementation. For example the Xen HYPERVISOR_console_io
> that Stefano mentioned earlier. Plus then we can leverage existing
> implementations insted of inveting a another virtual serial device.

But this makes it incumbent on each hypervisor to provide such an
interface, and requires each OS to provide drivers for each
hypervisor it knows about.  You then have an N:M situation,
which is bad for the RISC-V platform.

> > By making the interface a bit richer, as discussed in my earlier
> > proposal,
> > a hypervisor can cover a much larger set of use cases.
> 
> Which we also then need to support in M-mode firmware. The more complex
> the firmware becomes the more exposed it is.

The same argument applies to hypervisors. What fundamental difference
between M-mode and HS-mode software is there that makes the risk
so much different?

An implementation wishing to minimize its responsibilities can
always return run-time errors for functionality it does not
wish to support.

-- 
Konrad

[RISC-V] [tech-unixplatformspec] SBI Debug Console Extension Proposal (Draft v1)

[Darius Rad]

On Wed, Jun 01, 2022 at 09:47:32PM +0530, Anup Patel wrote:
> 
> Debug Console Extension (EID #0x4442434E "DBCN")
> ================================================
> 
> The debug console extension defines a generic mechanism for boot-time
> early prints from supervisor-mode software which allows users to catch
> boot-time issues in supervisor-mode software.
> 
> This extension replaces legacy console putchar (EID #0x01) extension
> and it is better in following ways:
> 1) It follows the new calling convention defined for SBI v1.0
>    (or higher).
> 2) It is based on a shared memory area between SBI implementation
>    and supervisor-mode software so multiple characters can be
>    printed using a single SBI call.
> 
> The supervisor-mode software must set the shared memory area before
> printing characters on the debug console. Also, all HARTs share the
> same shared memory area so only one HART needs to set it at boot-time.
> 

It seems to me that the rationale and summary of the extension is
insufficient, and non-normative commentary is lacking.  The ensuing mailing
list discussion suggests that there are additional requirements and
assumptions that influenced the design of the interface, but are not
mentioned here.  It is important to document such information, in order to
have a constructive discussion now, and also to preserve such information
for readers in the future who may not have the benefit of reading the
mailing list discussions.

// darius