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

[Xiang W]

在 2022-06-02星期四的 18:29 +0530，Anup Patel写道：
> On Thu, Jun 2, 2022 at 2:43 PM Heinrich Schuchardt
> <heinrich.schuchardt 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 ?

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

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

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