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

On Fri, 2022-06-03 at 07:49 +0000, Schwarz, Konrad wrote:

From:
sig-hypervisors@... <sig-hypervisors@...>
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

From: sig-hypervisors@... <sig-hypervisors@...> 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

On Fri, Jun 3, 2022 at 12:23 AM Heinrich Schuchardt
<heinrich.schuchardt@...> wrote:

On 6/3/22 08:56, Atish Kumar Patra wrote:

On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt
<heinrich.schuchardt@...> wrote:

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.

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

On 6/3/22 08:56, Atish Kumar Patra wrote:

On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt
<heinrich.schuchardt@...> wrote:

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.

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

On Thu, Jun 2, 2022 at 2:13 AM Heinrich Schuchardt
<heinrich.schuchardt@...> wrote:

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.

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

Hi Stefano,

On Fri, Jun 3, 2022 at 2:35 AM Stefano Stabellini
<stefano.stabellini@...> wrote:

On Thu, 2 Jun 2022, Greg Favor wrote:

On Thu, Jun 2, 2022 at 1:29 PM Stefano Stabellini <stefano.stabellini@...> 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

On Thu, 2 Jun 2022, Greg Favor wrote:

On Thu, Jun 2, 2022 at 1:29 PM Stefano Stabellini <stefano.stabellini@...> 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.

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

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.

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

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 :-)

On Thu, 2 Jun 2022, Schwarz, Konrad via lists.riscv.org wrote:

Hi Anup,

From: sig-hypervisors@... <sig-hypervisors@...> 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.

On Thu, Jun 2, 2022 at 8:18 AM Anup Patel <apatel@...> wrote:

On Thu, Jun 2, 2022 at 8:30 PM Ved Shanbhogue <ved@...> 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@...> 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

On Thu, Jun 2, 2022 at 8:30 PM Ved Shanbhogue <ved@...> 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@...> 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

Mark,

In case you missed it from the agenda-email: we have the formation of
a Firmware & Platforms Services SIG on the agenda for the (joint)
Software HCs meeting today. Atish has kindly volunteered to be the
acting chair and we'll announce as soon as the draft charter is done.
The scope of the new SIG will be (at least) SBI, ACPI, UEFI and
bootloaders.

Philipp.