SBI extension proposal

[zongbox@gmail.com (Zong Li)]

Atish Patra <atish.patra@wdc.com> ? 2018?11?6? ?? ??2:52???
>
> On 11/5/18 5:51 AM, Nick Kossifidis wrote:
> > ???? 2018-11-03 02:00, Atish Patra ??????:
> >> On 11/2/18 4:45 PM, Nick Kossifidis wrote:
> >>> ???? 2018-11-03 01:12, Atish Patra ??????:
> >>>> On 11/2/18 8:25 AM, Nick Kossifidis wrote:
> >>>>> Hello Atish and thanks for bringing this up,
> >>>>>
> >>>>> ???? 2018-10-31 20:23, Atish Patra ??????:
> >>>>>> Here is a proposal to make SBI a flexible and extensible interface.
> >>>>>> It is based on the foundation policy of RISC-V i.e. modularity and
> >>>>>> openness. It is designed in such a way that it introduces very few
> >>>>>> new
> >>>>>> mandatory SBI APIs that are absolutely required to maintain
> >>>>>> backward
> >>>>>> compatibility. Everything else is optional so that it remains an
> >>>>>> open
> >>>>>> standard yet robust.
> >>>>>>
> >>>>>> 1. Introduction:
> >>>>>> ----------------
> >>>>>> The current RISC-V SBI only defines a few mandatory functions such
> >>>>>> as
> >>>>>> inter-processor interrupts (IPI) interface, reprogramming timer,
> >>>>>> serial
> >>>>>> console and memory barrier instructions. The existing SBI
> >>>>>> documentation
> >>>>>> can be found here [1]. Many important functionalities such as power
> >>>>>> management/cpu-hotplug are not yet defined due to difficulties in
> >>>>>> accommodating modifications without breaking the backward
> >>>>>> compatibility
> >>>>>> with the current interface.
> >>>>>>
> >>>>>> Its design is inspired by Power State Coordination Interface (PSCI)
> >>>>>> from
> >>>>>> ARM world. However, it adds only two new mandatory SBI calls
> >>>>>> providing
> >>>>>> version information and supported APIs, unlike PSCI where a
> >>>>>> significant
> >>>>>> number of functions are mandatory. The version of the existing SBI
> >>>>>> will
> >>>>>> be defined as a minimum version(0.1) which will always be backward
> >>>>>> compatible. Similarly, any Linux kernel with newer feature will
> >>>>>> fall
> >>>>>> back if an older version of SBI does not support the updated
> >>>>>> capabilities. Both the operating system and SEE can be implemented
> >>>>>> to
> >>>>>> be two way backward compatible.
> >>>>>>
> >>>>>> 2. New functions:
> >>>>>> -----------------
> >>>>>>
> >>>>>> -- u32 sbi_get_version(void):
> >>>>>>
> >>>>>> Returns the current SBI version implemented by the firmware.
> >>>>>> version: uint32: Bits[31:16] Major Version
> >>>>>>                 Bits[15:0] Minor Version
> >>>>>>
> >>>>>> The existing SBI version can be 0.1. The proposed version will be
> >>>>>> at
> >>>>>> 0.2
> >>>>>> A different major version may indicate possible incompatible
> >>>>>> functions.
> >>>>>> A different minor version must be compatible with each other even
> >>>>>> if
> >>>>>> they have a higher number of features.
> >>>>>>
> >>>>>> -- u32 sbi_check_api(unsigned long start_api_id, unsigned long
> >>>>>> count):
> >>>>>>
> >>>>>> Accepts a start_api_id as an argument and returns if start_api_id
> >>>>>> to
> >>>>>> (start_api_id + count - 1) are supported or not.
> >>>>>> The API numbering scheme is described in section 3.
> >>>>>>
> >>>>>> A count is introduced so that a range of APIs can be checked at one
> >>>>>> SBI
> >>>>>> call to minimize the M-mode traps.
> >>>>>>
> >>>>>
> >>>>> Is this really needed ? We can determine the SBI version from
> >>>>> sbi_get_version, why
> >>>>> include an SBI call to perform a check that can easily be performed
> >>>>> by
> >>>>> the caller
> >>>>> instead ?
> >>>>>
> >>>>
> >>>> This API is still in discussion. Probably, an API returning bitmask
> >>>> of
> >>>> all the features make more sense ?
> >>>>
> >>>> However, I feel a separate API is required from sbi_get_version as
> >>>> there vendors can choose not to implement some of the features that
> >>>> is
> >>>> specified by a version as most of the API set are optional.
> >>>>
> >>>
> >>> We can always rely on the SBI_NOT_SUPPORTED error code you've already
> >>> included ;-)
> >>
> >>
> >> That's what sbi_check_api will return if a single API or set of API is
> >> not supported. We potentially can return it from individual function
> >> calls but having a separate API helps in avoiding making those calls
> >> in first place from supervisor level.
> >>
> >> If we use a bitmask for capabilities we'll be limited by
> >>> the encoding.
> >>>
> >> That was the reason sbi_check_api was introduced. But looking back, I
> >> wonder if we ever have more than 24 SBI APIs to deal with!!
> >>
> >>
> >>>>>> -- int sbi_hart_up(unsigned long hartid, unsigned long start,
> >>>>>> unsigned
> >>>>>> long priv)
> >>>>>>
> >>>>>> Brings up "hartid" either during initial boot or after a
> >>>>>> sbi_hart_down
> >>>>>> SBI call.
> >>>>>>
> >>>>>> "start" points to a runtime-specified address where a hart can
> >>>>>> enter
> >>>>>> into supervisor mode. This must be a physical address.
> >>>>>>
> >>>>>> "priv" is a private data that caller can use to pass information
> >>>>>> about
> >>>>>> execution context.
> >>>>>>
> >>>>>> Return the appropriate SBI error code.
> >>>>>>
> >>>>>> -- int sbi_hart_suspend(u32 state, unsigned long resume_entry,
> >>>>>> unsigned
> >>>>>> long priv)
> >>>>>>
> >>>>>> Suspends the calling hart to a particular power state. Suspended
> >>>>>> hart
> >>>>>> will automatically wake-up based on some wakeup events at
> >>>>>> resume_entry
> >>>>>> physical address.
> >>>>>>
> >>>>>> "priv" is a private data that caller can use to pass information
> >>>>>> about
> >>>>>> execution context. The SBI implementation must save a copy so that
> >>>>>> caller can reuse while restoring hart from suspend.
> >>>>>>
> >>>>>> Return the appropriate SBI error code.
> >>>>>>
> >>>>>> -- int sbi_hart_down()
> >>>>>>
> >>>>>> It powers off the hart and will be used in cpu-hotplug.
> >>>>>> Only individual hart can remove itself from supervisor mode. It can
> >>>>>> be
> >>>>>> moved to normal state only by sbi_hart_up function.
> >>>>>>
> >>>>>> Return the appropriate SBI error code.
> >>>>>>
> >>>>>> -- u32 sbi_hart_state(unsigned long hartid)
> >>>>>>
> >>>>>> Returns the RISCV_POWER_STATE for a specific hartid. This will help
> >>>>>> make
> >>>>>> kexec like functionality more robust.
> >>>>>>
> >>>>>
> >>>>> Instead of the above I believe it would be cleaner and simpler to
> >>>>> have
> >>>>> an sbi_get_hart_state and an sbi_set_hart_state call. This way we
> >>>>> can
> >>>>> better handle state transitions and, handle ON/OFF/STANDBY/RETENTION
> >>>>> and any other state we come up with, without adding extra sbi calls.
> >>>>>
> >>>>
> >>>> When do you want to use sbi_set_hart_state ?
> >>>> The power states will be modified as a part of sbi_hart_down or
> >>>> sbi_shutdown/suspend calls anyways.
> >>>>
> >>>
> >>> The idea is to have sbi_set_hart_state instead of
> >>> sbi_hart_down/shutdown/suspend/etc.
> >>> Instead of having different calls for different states we just have
> >>> two
> >>> calls, one
> >>> to get the state and one to set it. This way we have fewer calls and
> >>> if
> >>> we add a
> >>> new state we can add it without having to add a new call.
> >>>
> >> Ahh I see it now. IMHO, having explicit names makes more sense.
> >>
> >>
> >>>>>> -- void sbi_system_shutdown()
> >>>>>>
> >>>>>> Powers off the entire system.
> >>>>>>
> >>>>>
> >>>>> Don't we already have that ? What's the difference between
> >>>>> sbi_system_shutdown
> >>>>> and sbi_shutdown ? Does it make sense to use sbi_shutdown and leave
> >>>>> the
> >>>>> system
> >>>>> on with all the harts down ? Maybe we can just say that sbi_shutdown
> >>>>> also
> >>>>> powers down the system and rename it to sbi_system_shutdown with the
> >>>>> same
> >>>>> function ID.
> >>>>>
> >>>>
> >>>> Yeah. That's a better.
> >>>>
> >>>>
> >>>>>> 3. SBI API ID numbering scheme:
> >>>>>> ------------------------------
> >>>>>> An API Set is a set of SBI APIs which collectively implement some
> >>>>>> kind of feature/functionality.
> >>>>>>
> >>>>>> Let's say SBI API ID is u32  then
> >>>>>> Bit[31:24] =  API Set Number
> >>>>>> Bit[23:0] = API Number within API Set
> >>>>>>
> >>>>>> Here are few API Sets for SBI v0.2:
> >>>>>> 1. Base APIs
> >>>>>> API Set Number: 0x0
> >>>>>> Description: Base APIs mandatory for any SBI version
> >>>>>>
> >>>>>> 2. HART PM APIs
> >>>>>> API Set Number: 0x1
> >>>>>> Description: Hart UP/Down/Suspend APIs for per-Hart
> >>>>>> power management
> >>>>>>
> >>>>>> 3. System PM APIs
> >>>>>> API Set Number; 0x2
> >>>>>> Description: System Shutdown/Reboot/Suspend for system-level
> >>>>>> power management
> >>>>>>
> >>>>>> 4. Vendor APIs
> >>>>>> API Set Number: 0xff
> >>>>>> Description: Vendor specific APIs.
> >>>>>> There is a possibility that different vendors can choose to assign
> >>>>>> same API numbers for different functionality. In that case, vendor
> >>>>>> specific strings in Device Tree can be used to verify if a specific
> >>>>>> API belongs to the intended vendor or not.
> >>>>>>
> >>>>>
> >>>>> I understand the rationale behind this but I believe it's better to
> >>>>> call them services or functions instead of APIs, they are not sets
> >>>>> of APIs the way I see it. Also since we are moving that path why
> >>>>> call
> >>>>> it SBI and restrict it only to be used by the supervisor ? I'd love
> >>>>> to
> >>>>> have another category for the secure monitor calls for example,
> >>>>> but on the software architecture that we are discussing on the TEE
> >>>>> group, such calls can also originate from U mode or HS/HU modes.
> >>>>> The same goes for vendor specific calls, there are lots of use case
> >>>>> scenarios where vendors would like to be able to call their stuff
> >>>>> from U mode for example (e.g. a user space library talking to a
> >>>>> smart card directly).
> >>>>>
> >>>>> I suggest we rename it from SBI to something more generic that is
> >>>>> not
> >>>>> defined by who is calling it but by what we are calling, which in
> >>>>> this
> >>>>> case is the firmware (FBI?).
> >>>>
> >>>> possible conflict with a very famous law enforcement agency :) :).
> >>>>
> >>>> Jokes aside, renaming to something more generic is a good idea. But
> >>>> it
> >>>> will probably break a ton of documentation/spec in RISC-V which I
> >>>> don't want to deal with right now. May be in future versions ?
> >>>>
> >>>
> >>> I get that updating the documentation can be a pain, I can help with
> >>> that if you want (and I promise I'll stay away from law enforcement
> >>> agencies !). I suggest we do the renaming as early as possible
> >>> because we are going to carry it for a long time instead and it'll
> >>> only get harder to change later on.
> >>>
> >> Thanks. I guess we need Palmer/Andrew to pitch in first on this before
> >> we decide anything.
> >>
> >
> > I just noticed that on privilege spec, SBI stands for System Binary
> > Interface,
> > not Supervisor Binary Interface as mentioned here:
> > https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
> >
> > "System" is broader than "supervisor" since it doesn't imply the mode
> > where
> > the calls originate from. On the other hand it doesn't say anything
> > about
> > who is calling or who it calls, it's very broad, it can be an interface
> > between anything.
> >
> I guess you found a typo in privileged spec.
>
> It is mentioned as supervisor binary interface (SBI) in chapter 1
> (https://github.com/riscv/riscv-isa-manual/blob/master/src/intro.tex#L126)
> while system binary interface (SBI) in Chapter 4.
> (https://github.com/riscv/riscv-isa-manual/blob/master/src/supervisor.tex#L9)
>
> I have filed a bug in github for now.
>

Which API set is suitable for trigger module use? We need to invoke a
SBI call to set trigger module. We can use one SBI call for setting
mcontrol, icount, itrigger and etrigger(decide which types inside this
SBI), or four SBI calls for them respectively. On the other hand, I
saw the perf branch in riscv-pk repository, maybe we should consider
it together? But actually, to be exact, one is debugging use, and
another one is tracing use.