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