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

[Anup Patel]

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

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

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

Maybe this text is not giving a clear picture ?

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

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

Clearly, the usage of shared memory is extension specific.

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

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

Regards,
Anup

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