On 6/2/22 10:44, Anup Patel wrote:

On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
<heinrich.schuchardt@...> 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

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

On Thu, Jun 2, 2022 at 9:14 AM Xiang W <wxjstz@...> 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@...
http://lists.infradead.org/mailman/listinfo/opensbi

On Thu, Jun 2, 2022 at 12:02 AM Heiko Stübner <heiko@...> 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

On Wed, Jun 1, 2022 at 11:59 PM Heinrich Schuchardt
<heinrich.schuchardt@...> 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

On Wed, Jun 1, 2022 at 11:51 PM Dylan Reid <dylan@...> 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.

Regards,
Anup

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

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

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.

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.

Hi All,

I am pleased to inform that 45 days of public review period has ended
for the RISC-V UEFI Protocol Spec. Feedbacks during review period are
addressed.

We will go through the remaining process as per the "Ratification Ready" row in
https://docs.google.com/presentation/d/1nQ5uFb39KA6gvUi5SReWfIQSiRN7hp6z7ZPfctE4mKk/edit#slide=id.ga0a994c3c8_0_6

The latest spec for the final ratification stage is available at,
https://github.com/riscv-non-isa/riscv-uefi/releases/download/1.0.0/RISCV_UEFI_PROTOCOL-spec.pdf

Updated Status checklist can be found at:
https://docs.google.com/spreadsheets/d/1EHRXPGZnqUxiBoBHN9Vc7Svkoi2RPZITqyld5IAunDE/edit#gid=1029126936

Thanks!
Sunil

Several modifications to the WDT spec were proposed, and seemingly agreed in this discussion thread.  Is there any plan to actually update the spec?  Should I try to learn enough git-foo to generate a pull request?

Any plans to try to get this ratified in the foreseeable future?

I'm currently trying to finalize a WDT implementation spec, and would like to align with the official spec as much as possible.

Thanks,
Phil

On Tue, Apr 12, 2022 at 09:43:43AM -0700, Aaron Durbin wrote:

On Fri, Apr 8, 2022 at 3:45 AM <darius@...> wrote:

On Mon, Apr 04, 2022 at 08:49:28AM -0600, Aaron Durbin wrote:

On Fri, Mar 25, 2022 at 7:25 AM <darius@...> wrote:

- The RISC-V Privileged Architecture treats the SEE as "concrete
instances of components implementing the interfaces", whereas the
proposed charter is somewhat more vague. Obviously that

definition is

what we are discussing here, but generally it should be clear

whether

the SEE is (1) one or a set of interfaces, (2) a reference or

abstract

implementation, or (3) any one of a number of specific

implementations.

It would be (3). I'll adjust to try to make that clearer. Feel free to
provide specific suggestions as well to the prose.

Maybe I am missing some nuance, but what I keep going back to is that it
seems the very thing you are describe is exactly the SBI as defined in the
RISC-V Privileged Architecture. Unfortunate terminology aside, to me it
seems like the ideal term is SBI and is clearly described in the Privileged
Architecture.

A specification typically defines the interfaces, as the concrete instance
is up to each implementation. I guess I don't understand how we would
define the *environment* (SEE) if not by the *interface* (SBI), and if we
are defining the *interface*, is that not SBI (per the priv spec
definition)?

I think I answered your original question incorrectly (or I misunderstood
what (3) was suggesting. I was suggesting (3) could be achieved by
adhering/using (1).

W.r.t. to SBI vs SEE, I appreciate the attention to detail, Darius. You are
correct that the current privilege spec calls these abstract concepts out
in that specific way. FWIW, the HBI and HEE as currently detailed in the
specification are not reality in how the H extension has been defined.

I haven't been following the H extension closely, and was not aware the
HBI/HEE terms have suffered the same fate. This is one of those things
that makes it hard to convince people to take RISC-V seriously; when we
can't even be consistent with terms that we invent.

How things have progressed in RISC-V has caused a naming a collision. We
have the abstract SBI concept from the current incarnation of the ISA
manual, and we have the SBI specification. The latter does fully define the
interfaces a supervisor implementation requires in the world of an OS-A
rich OS. That includes other entities, e.g. UEFI, DeviceTree bindings,
ACPI, etc in addition to some pieces of the SBI specification.

So what should we call this? SBI is already overloaded. Would could tack on
an 'I' for interface and make it SEEI, but there isn't a pure 'SBI' name
that one can take advantage of because that ship has already sailed.

I was hoping that since the SBI specification had not been ratified that it
could be renamed to be more consistent with prior usage of that term. But
if, as you say, that ship has sailed, then I'll drop it.

SEEI seems reasonable, or perhaps SEI for Supervisor Environment Interface.
I don't have a strong opinion about the term that is used, other than it
*not* be SEE, so we don't perpetuate this bad habit of reusing specific
terms with explicitly different meanings. And that we define the term
clearly at whatever point we start using it (i.e., in the charter, if the
charter uses it).

- SBI is used in the RISC-V Privileged Architecture as the entirety
of the interface between the SEE and the operating system.
Separately, we have this RISC-V thing called SBI (of which OpenSBI
is an implementation) which encapsulates some, but not all, of
that functionality. For example, memory mapped peripherals are
not part of the OpenSBI type of SBI, but appear to be covered by
the other style SBI.

It's an unfortunate reuse of terms that mean different things. It's also
abstract in that current implementations/usage does not rely on SBI spec

as

its only dependency in booting the kernel.

I don't follow that second sentence, could you elaborate?

The kernels don't only use SBI, as in the SBI specification, to boot and
run for an OS-A machine. As noted above other specs and dependencies exist
beyond SBI when SBI mean 'SBI specification'.

I think I understand what you are saying now, and I think that was roughly
the same point I was trying to make, in the context of advocating for a
single consistent use of the term SBI (for one usage only). In any case, I
guess we're just accepting that the term SBI is a confusing mess.

The proposed charter seems to be unclear on this point. It says "focus
will be on the interfaces between the SEE and the hosted environment",
which suggests the lower interface to me. However, it also refers to
binary compatible operating systems, which suggests the upper

interface.

The target of SEE is for entities building binary compatible operating
systems. i.e. One could take a distribution from any provider and boot

said

distribution on a compatible RISC-V implementation. I adjusted the

verbiage

some to hopefully make that clearer w.r.t. intent.

Maybe I'm missing something, because I am envisioning that the SEE *is* the
firmware or hosted environment. I don't understand what these interfaces
would be between.

SEE is the supervisor's environment and how the supervisor interacts w/ the
firmware or hosted environment is what we are attempting to nail down and
define. I don't think there are any interfaces between or if there are then
that would be out of the scope of this endeavor.

Perhaps instead of "The focus will be on the interfaces between the SEE and
the hosted environment", use "The focus will be on the interfaces between
the operating system and the SEE (through which the operating system may,
for example, access services provided by the firmware or otherwise)".

// darius

Add requirement for preserving the PCIe ID routing as described in issue:
https://github.com/riscv/riscv-platform-specs/issues/81

Signed-off-by: Mayuresh Chitale <mchitale@...>
---
riscv-platform-spec.adoc | 2 ++
1 file changed, 2 insertions(+)

diff --git a/riscv-platform-spec.adoc b/riscv-platform-spec.adoc
index e06500d..289163c 100644
--- a/riscv-platform-spec.adoc
+++ b/riscv-platform-spec.adoc
@@ -825,6 +825,8 @@ supported PCIe domains and map the ECAM I/O region for each domain.
* Platform software must configure ECAM I/O regions such that the effective
memory attributes are that of a PMA I/O region (i.e. strongly-ordered,
non-cacheable, non-idempotent).
+* If the platform software (for e.g OS) re-enumerates the PCIe topology then it
+is required that the underlying fabric routing is always correctly preserved.

===== PCIe Memory Space
Platforms are required to map PCIe address space directly in the system address
--
2.17.1

Currently the SBI specification defines how to hand device-trees from the SEE to the S-mode firmware.

In the context of Trusted Firmware A a document has been developed describing what a more generic handover structure may look like that will also encompass ACPI tables and additional information like TPM measurements.

https://developer.arm.com/documentation/den0135/a

As probably EDK II and U-Boot will adopt parsing this structure it would make sense to discuss if the same can be used in the RISC-V world too.

Best regards

Heinrich