SBI extension proposal

SBI extension proposal

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

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

-- void sbi_system_shutdown()

Powers off the entire system.

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.

4. Return error code Table:
---------------------------

Here are the SBI return error codes defined.

       SBI_SUCCESS           0
       SBI_NOT_SUPPORTED    -1
       SBI_INVALID_PARAM    -2
       SBI_DENIED           -3
       SBI_INVALID_ADDRESS  -4

A mapping function between SBI error code & Linux error code should be
provided.

5. Power State
--------------

A RISC-V core can exist in any of the following power states.

enum RISCV_POWER_STATE {
        //Powered up & operational.
        RISCV_HART_ON              =  0,
        //Powered up but at reduced energy consumption. WFI instruction
can be used to achieve this state.
        RISCV_HART_STANDBY        =   1,
        //Deeper low power state. No reset required but higher wakeup
latency.
        RISCV_HART_RETENTION       =  2,
        //Powered off. Reset of the core required after power restore.
        RISCV_HART_OFF             =  3
}

TODO:
Any other power management related features or state?

6. Implementation
-------------------
Currently, SBI is implemented as a part of BBL. There is a different SBI 
implementation available in coreboot as well.

Alternatively, a separate open BSD/MIT licensed SBI project can be 
created which can be used by anybody to avoid these kind of SBI 
fragmentation in future. This project can generate both a firmware 
binary (to executed directly in M mode) or a static library that can be 
used by different boot loaders. It will also help individual boot 
loaders to either work from M or S mode without a separate SBI 
implementation.

This proposal is far from perfect and absolutely any suggestion is
welcome. Obviously, there are many other functionalities that can be
added or removed from this proposal. However, I just wanted to start
with something that is an incremental change at best to kick off the
discussion. The aim here is initiate a discussion that can lead to a 
robust SBI specification.

Looking forward to discuss other ideas as well or any feedback on this 
proposal.

Reference:
-----------
[1]
http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
[2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md


Regards,
Atish

SBI extension proposal

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

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

-- void sbi_system_shutdown()

Powers off the entire system.

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.

4. Return error code Table:
---------------------------

Here are the SBI return error codes defined.

       SBI_SUCCESS           0
       SBI_NOT_SUPPORTED    -1
       SBI_INVALID_PARAM    -2
       SBI_DENIED           -3
       SBI_INVALID_ADDRESS  -4

A mapping function between SBI error code & Linux error code should be
provided.

5. Power State
--------------

A RISC-V core can exist in any of the following power states.

enum RISCV_POWER_STATE {
        //Powered up & operational.
        RISCV_HART_ON              =  0,
        //Powered up but at reduced energy consumption. WFI instruction
can be used to achieve this state.
        RISCV_HART_STANDBY        =   1,
        //Deeper low power state. No reset required but higher wakeup
latency.
        RISCV_HART_RETENTION       =  2,
        //Powered off. Reset of the core required after power restore.
        RISCV_HART_OFF             =  3
}

TODO:
Any other power management related features or state?

6. Implementation
-------------------
Currently, SBI is implemented as a part of BBL. There is a different SBI 
implementation available in coreboot as well.

Alternatively, a separate open BSD/MIT licensed SBI project can be 
created which can be used by anybody to avoid these kind of SBI 
fragmentation in future. This project can generate both a firmware 
binary (to executed directly in M mode) or a static library that can be 
used by different boot loaders. It will also help individual boot 
loaders to either work from M or S mode without a separate SBI 
implementation.

This proposal is far from perfect and absolutely any suggestion is
welcome. Obviously, there are many other functionalities that can be
added or removed from this proposal. However, I just wanted to start
with something that is an incremental change at best to kick off the
discussion. The aim here is initiate a discussion that can lead to a 
robust SBI specification.

Looking forward to discuss other ideas as well or any feedback on this 
proposal.

Reference:
-----------
[1]
http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
[2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md


Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Olof Johansson]

One more try, this time in plain text.

On Wed, Oct 31, 2018 at 12:01 PM Olof Johansson
<olof.johansson@gmail.com> wrote:
>
> Hi,
>
> On Wed, Oct 31, 2018 at 11:23 AM Atish Patra <atish.patra@wdc.com> wrote:
>>
>> 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.
>
>
> Thanks for starting this discussion.
>
>>
>> 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.
>
>
> To clarify: There is no way a kernel can request an older version of the SBI, but with backwards compatibility that shouldn't be required.
>
>> 2. New functions:
>> -----------------
>
>
> These should have their API numbers specified by each function, including aliases between 0.1 and 0.2.
>
>>
>>
>> -- 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.
>
>
> This will quickly fall back to a one-by-one probe if one of the early count IDs are unavailable. It's likely to mostly be done at boot time anyway, and the number of functions will be relatively limited. It's also possible we'll have a sparse numbering scheme in the future. Returning a 32-bit bitmap starting at start_api_id might be a more flexible approach.
>
> Also, API numbers are 32-bit, but it takes unsigned long? Having fixed length types here would be useful, to avoid confusion with ILP32/LP64. Same goes for other functions.
>
>> -- 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.
>>
>> -- void sbi_system_shutdown()
>>
>> Powers off the entire system.
>
>
> This is a slightly weird one to put in SBI. There's usually other actions needed for _system_ level shutdown, such as external power regulators.
>
>
>> 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.
>
>
> Yeah, having a binding for description of <function,version> to SBI API ID in DT will be needed, especially once "vendor" becomes fluid (projects migrating between vendors, mixing and matching, etc).
>
> Having long-lived vendor extensions are usually a bad idea, but it's also common to need a place to do custom support or to prove out next-generation API versions.
>
> How strict guidelines might we need/want here?
>
>
>> 4. Return error code Table:
>> ---------------------------
>>
>> Here are the SBI return error codes defined.
>>
>>        SBI_SUCCESS           0
>>        SBI_NOT_SUPPORTED    -1
>>        SBI_INVALID_PARAM    -2
>>        SBI_DENIED           -3
>>        SBI_INVALID_ADDRESS  -4
>>
>> A mapping function between SBI error code & Linux error code should be
>> provided.
>
>
> This isn't Linux-specific, and what Linux maps it to is sort of out of scope for this spec.
>
> (There's no generic "failed" return code, or -EAGAIN equivalent?)
>
>> 5. Power State
>> --------------
>>
>> A RISC-V core can exist in any of the following power states.
>>
>> enum RISCV_POWER_STATE {
>>         //Powered up & operational.
>>         RISCV_HART_ON              =  0,
>>         //Powered up but at reduced energy consumption. WFI instruction
>> can be used to achieve this state.
>>         RISCV_HART_STANDBY        =   1,
>>         //Deeper low power state. No reset required but higher wakeup
>> latency.
>>         RISCV_HART_RETENTION       =  2,
>>         //Powered off. Reset of the core required after power restore.
>>         RISCV_HART_OFF             =  3
>> }
>
>
> If this table changes, we'd need a new SBI version covering the new numbers. That's probably OK -- a future version can introduce a separate power state numbering scheme if needed too.
>
>> TODO:
>> Any other power management related features or state?
>
>
> We probably also need system-level power state reporting at some point, and non-HART components (caches, memory, etc).  Likely aligned with platform specs, once those start to show up.
>
>> 6. Implementation
>> -------------------
>> Currently, SBI is implemented as a part of BBL. There is a different SBI
>> implementation available in coreboot as well.
>>
>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>> created which can be used by anybody to avoid these kind of SBI
>> fragmentation in future. This project can generate both a firmware
>> binary (to executed directly in M mode) or a static library that can be
>> used by different boot loaders. It will also help individual boot
>> loaders to either work from M or S mode without a separate SBI
>> implementation.
>
>
> Strong +1 on providing a shared place for a reference implementation that different code bases can import, and a place for vendors to upstream their vendor-specific pieces if needed.
>
>> This proposal is far from perfect and absolutely any suggestion is
>> welcome. Obviously, there are many other functionalities that can be
>> added or removed from this proposal. However, I just wanted to start
>> with something that is an incremental change at best to kick off the
>> discussion. The aim here is initiate a discussion that can lead to a
>> robust SBI specification.
>>
>> Looking forward to discuss other ideas as well or any feedback on this
>> proposal.
>>
>> Reference:
>> -----------
>> [1]
>> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
>> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
>>
>
>
> -Olof

Re: SBI extension proposal

[Olof Johansson]

One more try, this time in plain text.

On Wed, Oct 31, 2018 at 12:01 PM Olof Johansson
<olof.johansson@gmail.com> wrote:
>
> Hi,
>
> On Wed, Oct 31, 2018 at 11:23 AM Atish Patra <atish.patra@wdc.com> wrote:
>>
>> 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.
>
>
> Thanks for starting this discussion.
>
>>
>> 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.
>
>
> To clarify: There is no way a kernel can request an older version of the SBI, but with backwards compatibility that shouldn't be required.
>
>> 2. New functions:
>> -----------------
>
>
> These should have their API numbers specified by each function, including aliases between 0.1 and 0.2.
>
>>
>>
>> -- 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.
>
>
> This will quickly fall back to a one-by-one probe if one of the early count IDs are unavailable. It's likely to mostly be done at boot time anyway, and the number of functions will be relatively limited. It's also possible we'll have a sparse numbering scheme in the future. Returning a 32-bit bitmap starting at start_api_id might be a more flexible approach.
>
> Also, API numbers are 32-bit, but it takes unsigned long? Having fixed length types here would be useful, to avoid confusion with ILP32/LP64. Same goes for other functions.
>
>> -- 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.
>>
>> -- void sbi_system_shutdown()
>>
>> Powers off the entire system.
>
>
> This is a slightly weird one to put in SBI. There's usually other actions needed for _system_ level shutdown, such as external power regulators.
>
>
>> 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.
>
>
> Yeah, having a binding for description of <function,version> to SBI API ID in DT will be needed, especially once "vendor" becomes fluid (projects migrating between vendors, mixing and matching, etc).
>
> Having long-lived vendor extensions are usually a bad idea, but it's also common to need a place to do custom support or to prove out next-generation API versions.
>
> How strict guidelines might we need/want here?
>
>
>> 4. Return error code Table:
>> ---------------------------
>>
>> Here are the SBI return error codes defined.
>>
>>        SBI_SUCCESS           0
>>        SBI_NOT_SUPPORTED    -1
>>        SBI_INVALID_PARAM    -2
>>        SBI_DENIED           -3
>>        SBI_INVALID_ADDRESS  -4
>>
>> A mapping function between SBI error code & Linux error code should be
>> provided.
>
>
> This isn't Linux-specific, and what Linux maps it to is sort of out of scope for this spec.
>
> (There's no generic "failed" return code, or -EAGAIN equivalent?)
>
>> 5. Power State
>> --------------
>>
>> A RISC-V core can exist in any of the following power states.
>>
>> enum RISCV_POWER_STATE {
>>         //Powered up & operational.
>>         RISCV_HART_ON              =  0,
>>         //Powered up but at reduced energy consumption. WFI instruction
>> can be used to achieve this state.
>>         RISCV_HART_STANDBY        =   1,
>>         //Deeper low power state. No reset required but higher wakeup
>> latency.
>>         RISCV_HART_RETENTION       =  2,
>>         //Powered off. Reset of the core required after power restore.
>>         RISCV_HART_OFF             =  3
>> }
>
>
> If this table changes, we'd need a new SBI version covering the new numbers. That's probably OK -- a future version can introduce a separate power state numbering scheme if needed too.
>
>> TODO:
>> Any other power management related features or state?
>
>
> We probably also need system-level power state reporting at some point, and non-HART components (caches, memory, etc).  Likely aligned with platform specs, once those start to show up.
>
>> 6. Implementation
>> -------------------
>> Currently, SBI is implemented as a part of BBL. There is a different SBI
>> implementation available in coreboot as well.
>>
>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>> created which can be used by anybody to avoid these kind of SBI
>> fragmentation in future. This project can generate both a firmware
>> binary (to executed directly in M mode) or a static library that can be
>> used by different boot loaders. It will also help individual boot
>> loaders to either work from M or S mode without a separate SBI
>> implementation.
>
>
> Strong +1 on providing a shared place for a reference implementation that different code bases can import, and a place for vendors to upstream their vendor-specific pieces if needed.
>
>> This proposal is far from perfect and absolutely any suggestion is
>> welcome. Obviously, there are many other functionalities that can be
>> added or removed from this proposal. However, I just wanted to start
>> with something that is an incremental change at best to kick off the
>> discussion. The aim here is initiate a discussion that can lead to a
>> robust SBI specification.
>>
>> Looking forward to discuss other ideas as well or any feedback on this
>> proposal.
>>
>> Reference:
>> -----------
>> [1]
>> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
>> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
>>
>
>
> -Olof

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Atish Patra]

On 10/31/18 12:11 PM, Olof Johansson wrote:
> One more try, this time in plain text.
> 
> On Wed, Oct 31, 2018 at 12:01 PM Olof Johansson
> <olof.johansson@gmail.com> wrote:
>>
>> Hi,
>>
>> On Wed, Oct 31, 2018 at 11:23 AM Atish Patra <atish.patra@wdc.com> wrote:
>>>
>>> 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.
>>
>>
>> Thanks for starting this discussion.
>>
>>>
>>> 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.
>>
>>
>> To clarify: There is no way a kernel can request an older version of the SBI, but with backwards compatibility that shouldn't be required.
>>

Thanks for the additional clarification.
>>> 2. New functions:
>>> -----------------
>>
>>
>> These should have their API numbers specified by each function, including aliases between 0.1 and 0.2.
>>
Sure. I will update the doc in markdown format with a comparison between
0.1 and 0.2

>>>
>>>
>>> -- 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.
>>
>>
>> This will quickly fall back to a one-by-one probe if one of the early count IDs are unavailable. It's likely to mostly be done at boot time anyway, and the number of functions will be relatively limited. It's also possible we'll have a sparse numbering scheme in the future. Returning a 32-bit bitmap starting at start_api_id might be a more flexible approach.
>>

In that case, does it make more sense to have an get_feature_list() like
API instead. This can return a bitmap of all features based on API set
id (Bit[31:24]).


>> Also, API numbers are 32-bit, but it takes unsigned long? Having fixed length types here would be useful, to avoid confusion with ILP32/LP64. Same goes for other functions.
>>

Yeah. It was a typo. I will change the API numbers to unsigned long as well.

>>> -- 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.
>>>
>>> -- void sbi_system_shutdown()
>>>
>>> Powers off the entire system.
>>
>>
>> This is a slightly weird one to put in SBI. There's usually other actions needed for _system_ level shutdown, such as external power regulators.
>>

This might be because of my limited knowledge on power management.
We already have a system reset call in SBI & PSCI also had system 
shutdown function. As per my understanding, this API will provide a way
to handle all common suspend related operations on machine mode.
In case of virtual OS, it will not result in any external power regulations.

>>
>>> 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.
>>
>>
>> Yeah, having a binding for description of <function,version> to SBI API ID in DT will be needed, especially once "vendor" becomes fluid (projects migrating between vendors, mixing and matching, etc).
>>
>> Having long-lived vendor extensions are usually a bad idea, but it's also common to need a place to do custom support or to prove out next-generation API versions.
>>
>> How strict guidelines might we need/want here?
>>

Do you mean guidelines for managing different vendor specific APIs ?
Vendor specific static strings in SBI are not encouraged. I currently 
don't see any other methods than DT as you described. Any other possible 
approach is fine. But, I fear if we don't make the guidelines strict, 
insanity may prevail as different vendors choose to manage it in their 
own way. Any downside on mandating the guidelines?


>>
>>> 4. Return error code Table:
>>> ---------------------------
>>>
>>> Here are the SBI return error codes defined.
>>>
>>>         SBI_SUCCESS           0
>>>         SBI_NOT_SUPPORTED    -1
>>>         SBI_INVALID_PARAM    -2
>>>         SBI_DENIED           -3
>>>         SBI_INVALID_ADDRESS  -4
>>>
>>> A mapping function between SBI error code & Linux error code should be
>>> provided.
>>
>>
>> This isn't Linux-specific, and what Linux maps it to is sort of out of scope for this spec.
>>
Agreed. I will remove this statement.

>> (There's no generic "failed" return code, or -EAGAIN equivalent?)
>>

Correct. I will add it.

>>> 5. Power State
>>> --------------
>>>
>>> A RISC-V core can exist in any of the following power states.
>>>
>>> enum RISCV_POWER_STATE {
>>>          //Powered up & operational.
>>>          RISCV_HART_ON              =  0,
>>>          //Powered up but at reduced energy consumption. WFI instruction
>>> can be used to achieve this state.
>>>          RISCV_HART_STANDBY        =   1,
>>>          //Deeper low power state. No reset required but higher wakeup
>>> latency.
>>>          RISCV_HART_RETENTION       =  2,
>>>          //Powered off. Reset of the core required after power restore.
>>>          RISCV_HART_OFF             =  3
>>> }
>>
>>
>> If this table changes, we'd need a new SBI version covering the new numbers. That's probably OK -- a future version can introduce a separate power state numbering scheme if needed too.
>>
>>> TODO:
>>> Any other power management related features or state?
>>
>>
>> We probably also need system-level power state reporting at some point, and non-HART components (caches, memory, etc).  Likely aligned with platform specs, once those start to show up.
>>

Sure.

>>> 6. Implementation
>>> -------------------
>>> Currently, SBI is implemented as a part of BBL. There is a different SBI
>>> implementation available in coreboot as well.
>>>
>>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>>> created which can be used by anybody to avoid these kind of SBI
>>> fragmentation in future. This project can generate both a firmware
>>> binary (to executed directly in M mode) or a static library that can be
>>> used by different boot loaders. It will also help individual boot
>>> loaders to either work from M or S mode without a separate SBI
>>> implementation.
>>
>>
>> Strong +1 on providing a shared place for a reference implementation that different code bases can import, and a place for vendors to upstream their vendor-specific pieces if needed.
>>

Great. Thank you for reviewing the proposal.

Regards,
Atish
>>> This proposal is far from perfect and absolutely any suggestion is
>>> welcome. Obviously, there are many other functionalities that can be
>>> added or removed from this proposal. However, I just wanted to start
>>> with something that is an incremental change at best to kick off the
>>> discussion. The aim here is initiate a discussion that can lead to a
>>> robust SBI specification.
>>>
>>> Looking forward to discuss other ideas as well or any feedback on this
>>> proposal.
>>>
>>> Reference:
>>> -----------
>>> [1]
>>> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
>>> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
>>>
>>
>>
>> -Olof
> 

Re: SBI extension proposal

[Atish Patra]

On 10/31/18 12:11 PM, Olof Johansson wrote:
> One more try, this time in plain text.
> 
> On Wed, Oct 31, 2018 at 12:01 PM Olof Johansson
> <olof.johansson@gmail.com> wrote:
>>
>> Hi,
>>
>> On Wed, Oct 31, 2018 at 11:23 AM Atish Patra <atish.patra@wdc.com> wrote:
>>>
>>> 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.
>>
>>
>> Thanks for starting this discussion.
>>
>>>
>>> 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.
>>
>>
>> To clarify: There is no way a kernel can request an older version of the SBI, but with backwards compatibility that shouldn't be required.
>>

Thanks for the additional clarification.
>>> 2. New functions:
>>> -----------------
>>
>>
>> These should have their API numbers specified by each function, including aliases between 0.1 and 0.2.
>>
Sure. I will update the doc in markdown format with a comparison between
0.1 and 0.2

>>>
>>>
>>> -- 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.
>>
>>
>> This will quickly fall back to a one-by-one probe if one of the early count IDs are unavailable. It's likely to mostly be done at boot time anyway, and the number of functions will be relatively limited. It's also possible we'll have a sparse numbering scheme in the future. Returning a 32-bit bitmap starting at start_api_id might be a more flexible approach.
>>

In that case, does it make more sense to have an get_feature_list() like
API instead. This can return a bitmap of all features based on API set
id (Bit[31:24]).


>> Also, API numbers are 32-bit, but it takes unsigned long? Having fixed length types here would be useful, to avoid confusion with ILP32/LP64. Same goes for other functions.
>>

Yeah. It was a typo. I will change the API numbers to unsigned long as well.

>>> -- 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.
>>>
>>> -- void sbi_system_shutdown()
>>>
>>> Powers off the entire system.
>>
>>
>> This is a slightly weird one to put in SBI. There's usually other actions needed for _system_ level shutdown, such as external power regulators.
>>

This might be because of my limited knowledge on power management.
We already have a system reset call in SBI & PSCI also had system 
shutdown function. As per my understanding, this API will provide a way
to handle all common suspend related operations on machine mode.
In case of virtual OS, it will not result in any external power regulations.

>>
>>> 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.
>>
>>
>> Yeah, having a binding for description of <function,version> to SBI API ID in DT will be needed, especially once "vendor" becomes fluid (projects migrating between vendors, mixing and matching, etc).
>>
>> Having long-lived vendor extensions are usually a bad idea, but it's also common to need a place to do custom support or to prove out next-generation API versions.
>>
>> How strict guidelines might we need/want here?
>>

Do you mean guidelines for managing different vendor specific APIs ?
Vendor specific static strings in SBI are not encouraged. I currently 
don't see any other methods than DT as you described. Any other possible 
approach is fine. But, I fear if we don't make the guidelines strict, 
insanity may prevail as different vendors choose to manage it in their 
own way. Any downside on mandating the guidelines?


>>
>>> 4. Return error code Table:
>>> ---------------------------
>>>
>>> Here are the SBI return error codes defined.
>>>
>>>         SBI_SUCCESS           0
>>>         SBI_NOT_SUPPORTED    -1
>>>         SBI_INVALID_PARAM    -2
>>>         SBI_DENIED           -3
>>>         SBI_INVALID_ADDRESS  -4
>>>
>>> A mapping function between SBI error code & Linux error code should be
>>> provided.
>>
>>
>> This isn't Linux-specific, and what Linux maps it to is sort of out of scope for this spec.
>>
Agreed. I will remove this statement.

>> (There's no generic "failed" return code, or -EAGAIN equivalent?)
>>

Correct. I will add it.

>>> 5. Power State
>>> --------------
>>>
>>> A RISC-V core can exist in any of the following power states.
>>>
>>> enum RISCV_POWER_STATE {
>>>          //Powered up & operational.
>>>          RISCV_HART_ON              =  0,
>>>          //Powered up but at reduced energy consumption. WFI instruction
>>> can be used to achieve this state.
>>>          RISCV_HART_STANDBY        =   1,
>>>          //Deeper low power state. No reset required but higher wakeup
>>> latency.
>>>          RISCV_HART_RETENTION       =  2,
>>>          //Powered off. Reset of the core required after power restore.
>>>          RISCV_HART_OFF             =  3
>>> }
>>
>>
>> If this table changes, we'd need a new SBI version covering the new numbers. That's probably OK -- a future version can introduce a separate power state numbering scheme if needed too.
>>
>>> TODO:
>>> Any other power management related features or state?
>>
>>
>> We probably also need system-level power state reporting at some point, and non-HART components (caches, memory, etc).  Likely aligned with platform specs, once those start to show up.
>>

Sure.

>>> 6. Implementation
>>> -------------------
>>> Currently, SBI is implemented as a part of BBL. There is a different SBI
>>> implementation available in coreboot as well.
>>>
>>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>>> created which can be used by anybody to avoid these kind of SBI
>>> fragmentation in future. This project can generate both a firmware
>>> binary (to executed directly in M mode) or a static library that can be
>>> used by different boot loaders. It will also help individual boot
>>> loaders to either work from M or S mode without a separate SBI
>>> implementation.
>>
>>
>> Strong +1 on providing a shared place for a reference implementation that different code bases can import, and a place for vendors to upstream their vendor-specific pieces if needed.
>>

Great. Thank you for reviewing the proposal.

Regards,
Atish
>>> This proposal is far from perfect and absolutely any suggestion is
>>> welcome. Obviously, there are many other functionalities that can be
>>> added or removed from this proposal. However, I just wanted to start
>>> with something that is an incremental change at best to kick off the
>>> discussion. The aim here is initiate a discussion that can lead to a
>>> robust SBI specification.
>>>
>>> Looking forward to discuss other ideas as well or any feedback on this
>>> proposal.
>>>
>>> Reference:
>>> -----------
>>> [1]
>>> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
>>> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
>>>
>>
>>
>> -Olof
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Anup Patel]

On Thu, Nov 1, 2018 at 2:50 PM Philipp Hug <philipp@hug.cx> wrote:
>
> Hi Atish,
>
> Am Mi., 31. Okt. 2018 um 19:24 Uhr schrieb Atish Patra <atish.patra@wdc.com>:
>>
>> -- u32 sbi_get_version(void):
>> -- u32 sbi_check_api(unsigned long start_api_id, unsigned long count):
>
>
> How about putting the version information into device tree and use the compatible string? This seems more reliable than probing.
> e.g.
> firmware {
>         sbi {
>                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
>         };
> };

If it was just DT then I think having this information in DT makes
sense. In future,
we might definitely see some ACPI support in RISC-V too (just like ARM64 world).

Of course, we cannot keep adding new SBI calls for static information but we can
certainly have bare-minimum mandatory SBI calls for version and capabilities.

>
> You could also add more information about the supported features to this sbi node.
>
> I'd also suggest that all functions can return an error code and are not of type void.

Yes, I agree. With SBI v0.2, existing SBI calls from v0.1 should
return error code.

Regards,
Anup

Re: SBI extension proposal

[Anup Patel]

On Thu, Nov 1, 2018 at 2:50 PM Philipp Hug <philipp@hug.cx> wrote:
>
> Hi Atish,
>
> Am Mi., 31. Okt. 2018 um 19:24 Uhr schrieb Atish Patra <atish.patra@wdc.com>:
>>
>> -- u32 sbi_get_version(void):
>> -- u32 sbi_check_api(unsigned long start_api_id, unsigned long count):
>
>
> How about putting the version information into device tree and use the compatible string? This seems more reliable than probing.
> e.g.
> firmware {
>         sbi {
>                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
>         };
> };

If it was just DT then I think having this information in DT makes
sense. In future,
we might definitely see some ACPI support in RISC-V too (just like ARM64 world).

Of course, we cannot keep adding new SBI calls for static information but we can
certainly have bare-minimum mandatory SBI calls for version and capabilities.

>
> You could also add more information about the supported features to this sbi node.
>
> I'd also suggest that all functions can return an error code and are not of type void.

Yes, I agree. With SBI v0.2, existing SBI calls from v0.1 should
return error code.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Richard W.M. Jones]

On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> On Thu, Nov 1, 2018 at 2:50 PM Philipp Hug <philipp@hug.cx> wrote:
> >
> > Hi Atish,
> >
> > Am Mi., 31. Okt. 2018 um 19:24 Uhr schrieb Atish Patra <atish.patra@wdc.com>:
> >>
> >> -- u32 sbi_get_version(void):
> >> -- u32 sbi_check_api(unsigned long start_api_id, unsigned long count):
> >
> >
> > How about putting the version information into device tree and use the compatible string? This seems more reliable than probing.
> > e.g.
> > firmware {
> >         sbi {
> >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
> >         };
> > };
> 
> If it was just DT then I think having this information in DT makes
> sense. In future,
> we might definitely see some ACPI support in RISC-V too (just like ARM64 world).

I agree.  Please try not to make things that depend on DT, as it's a
Linux-only description which isn't suitable for other OSes and has a
poorly defined ABI.

Rich.

> Of course, we cannot keep adding new SBI calls for static information but we can
> certainly have bare-minimum mandatory SBI calls for version and capabilities.
> 
> >
> > You could also add more information about the supported features to this sbi node.
> >
> > I'd also suggest that all functions can return an error code and are not of type void.
> 
> Yes, I agree. With SBI v0.2, existing SBI calls from v0.1 should
> return error code.
> 
> Regards,
> Anup

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: SBI extension proposal

[Richard W.M. Jones]

On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> On Thu, Nov 1, 2018 at 2:50 PM Philipp Hug <philipp@hug.cx> wrote:
> >
> > Hi Atish,
> >
> > Am Mi., 31. Okt. 2018 um 19:24 Uhr schrieb Atish Patra <atish.patra@wdc.com>:
> >>
> >> -- u32 sbi_get_version(void):
> >> -- u32 sbi_check_api(unsigned long start_api_id, unsigned long count):
> >
> >
> > How about putting the version information into device tree and use the compatible string? This seems more reliable than probing.
> > e.g.
> > firmware {
> >         sbi {
> >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
> >         };
> > };
> 
> If it was just DT then I think having this information in DT makes
> sense. In future,
> we might definitely see some ACPI support in RISC-V too (just like ARM64 world).

I agree.  Please try not to make things that depend on DT, as it's a
Linux-only description which isn't suitable for other OSes and has a
poorly defined ABI.

Rich.

> Of course, we cannot keep adding new SBI calls for static information but we can
> certainly have bare-minimum mandatory SBI calls for version and capabilities.
> 
> >
> > You could also add more information about the supported features to this sbi node.
> >
> > I'd also suggest that all functions can return an error code and are not of type void.
> 
> Yes, I agree. With SBI v0.2, existing SBI calls from v0.1 should
> return error code.
> 
> Regards,
> Anup

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Philipp Hug]

Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
<rjones@redhat.com>:
>
> I agree.  Please try not to make things that depend on DT, as it's a
> Linux-only description which isn't suitable for other OSes and has a
> poorly defined ABI.
>
You don't have to depend on DT. If you implement ACPI you'd put this
information in ACPI as well as you do with other information about the
platform.
What's the advantage of putting static information into an SBI call
instead of putting it in DT, ACPI, ...?

Philipp

Re: SBI extension proposal

[Philipp Hug]

Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
<rjones@redhat.com>:
>
> I agree.  Please try not to make things that depend on DT, as it's a
> Linux-only description which isn't suitable for other OSes and has a
> poorly defined ABI.
>
You don't have to depend on DT. If you implement ACPI you'd put this
information in ACPI as well as you do with other information about the
platform.
What's the advantage of putting static information into an SBI call
instead of putting it in DT, ACPI, ...?

Philipp

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Richard W.M. Jones]

On Thu, Nov 01, 2018 at 12:03:49PM +0100, Philipp Hug wrote:
> Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
> <rjones@redhat.com>:
> >
> > I agree.  Please try not to make things that depend on DT, as it's a
> > Linux-only description which isn't suitable for other OSes and has a
> > poorly defined ABI.
> >
> You don't have to depend on DT. If you implement ACPI you'd put this
> information in ACPI as well as you do with other information about the
> platform.
> What's the advantage of putting static information into an SBI call
> instead of putting it in DT, ACPI, ...?

You only have to put it in one place.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: SBI extension proposal

[Richard W.M. Jones]

On Thu, Nov 01, 2018 at 12:03:49PM +0100, Philipp Hug wrote:
> Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
> <rjones@redhat.com>:
> >
> > I agree.  Please try not to make things that depend on DT, as it's a
> > Linux-only description which isn't suitable for other OSes and has a
> > poorly defined ABI.
> >
> You don't have to depend on DT. If you implement ACPI you'd put this
> information in ACPI as well as you do with other information about the
> platform.
> What's the advantage of putting static information into an SBI call
> instead of putting it in DT, ACPI, ...?

You only have to put it in one place.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Atish Patra]

On 11/1/18 4:25 AM, Richard W.M. Jones wrote:
> On Thu, Nov 01, 2018 at 12:03:49PM +0100, Philipp Hug wrote:
>> Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
>> <rjones@redhat.com>:
>>>
>>> I agree.  Please try not to make things that depend on DT, as it's a
>>> Linux-only description which isn't suitable for other OSes and has a
>>> poorly defined ABI.
>>>
>> You don't have to depend on DT. If you implement ACPI you'd put this
>> information in ACPI as well as you do with other information about the
>> platform.
>> What's the advantage of putting static information into an SBI call
>> instead of putting it in DT, ACPI, ...?
> 
> You only have to put it in one place.
> 
> Rich.
> 
I agree with Richard. It is better to manage it in SBI implementation. 
This version will not change in every kernel release or so. I think 
having global SBI version in SBI spec is fine.

However, we may need to put <function,version> for vendor specific APIs 
in DT/ACPI as pointed by olof earlier. That will help to avoid conflicts.

I will update the proposal such that it is clear that we are not making 
DT mandatory, ACPI is an option as well. SBI spec should independent of 
OS specific spec.


Regards,
Atish

Re: SBI extension proposal

[Atish Patra]

On 11/1/18 4:25 AM, Richard W.M. Jones wrote:
> On Thu, Nov 01, 2018 at 12:03:49PM +0100, Philipp Hug wrote:
>> Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
>> <rjones@redhat.com>:
>>>
>>> I agree.  Please try not to make things that depend on DT, as it's a
>>> Linux-only description which isn't suitable for other OSes and has a
>>> poorly defined ABI.
>>>
>> You don't have to depend on DT. If you implement ACPI you'd put this
>> information in ACPI as well as you do with other information about the
>> platform.
>> What's the advantage of putting static information into an SBI call
>> instead of putting it in DT, ACPI, ...?
> 
> You only have to put it in one place.
> 
> Rich.
> 
I agree with Richard. It is better to manage it in SBI implementation. 
This version will not change in every kernel release or so. I think 
having global SBI version in SBI spec is fine.

However, we may need to put <function,version> for vendor specific APIs 
in DT/ACPI as pointed by olof earlier. That will help to avoid conflicts.

I will update the proposal such that it is clear that we are not making 
DT mandatory, ACPI is an option as well. SBI spec should independent of 
OS specific spec.


Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: SBI extension proposal

[Karsten Merker]

On Thu, Nov 01, 2018 at 09:46:09AM +0000, Richard W.M. Jones wrote:
> On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
[...]
> > > How about putting the version information into device tree
> > > and use the compatible string?  This seems more reliable
> > > than probing.
> > > e.g.
> > > firmware {
> > >         sbi {
> > >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
> > >         };
> > > };
> > 
> > If it was just DT then I think having this information in DT makes
> > sense. In future,
> > we might definitely see some ACPI support in RISC-V too (just like ARM64 world).
> 
> I agree.  Please try not to make things that depend on DT, as it's a
> Linux-only description which isn't suitable for other OSes and has a
> poorly defined ABI.

Hello,

the notion that DT is a "Linux-only" description and not suitable
for other operating systems isn't correct.  Besides being used by
Linux, device-tree is also used by at least U-Boot, FreeBSD and
NetBSD.  Based on issues in the early days of DT one can surely
discuss about device-tree ABI stability, but since then a lot of
effort has been put into making DT ABI-stable, and in my
experience this effort has been successful.  On the other hand I
have experienced way more ACPI issues on x86-64 hardware than I
would like to remember, so I tend to assume that in the ACPI
world also not everything is nice and shining.

Regards,
Karsten
-- 
Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
Werbung sowie der Markt- oder Meinungsforschung.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Palmer Dabbelt]

On Thu, 01 Nov 2018 09:42:05 PDT (-0700), merker at debian.org wrote:
> On Thu, Nov 01, 2018 at 09:46:09AM +0000, Richard W.M. Jones wrote:
>> On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> [...]
>> > > How about putting the version information into device tree
>> > > and use the compatible string?  This seems more reliable
>> > > than probing.
>> > > e.g.
>> > > firmware {
>> > >         sbi {
>> > >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
>> > >         };
>> > > };
>> >
>> > If it was just DT then I think having this information in DT makes
>> > sense. In future,
>> > we might definitely see some ACPI support in RISC-V too (just like ARM64 world).
>>
>> I agree.  Please try not to make things that depend on DT, as it's a
>> Linux-only description which isn't suitable for other OSes and has a
>> poorly defined ABI.
>
> the notion that DT is a "Linux-only" description and not suitable
> for other operating systems isn't correct.  Besides being used by
> Linux, device-tree is also used by at least U-Boot, FreeBSD and
> NetBSD.  Based on issues in the early days of DT one can surely
> discuss about device-tree ABI stability, but since then a lot of
> effort has been put into making DT ABI-stable, and in my
> experience this effort has been successful.  On the other hand I
> have experienced way more ACPI issues on x86-64 hardware than I
> would like to remember, so I tend to assume that in the ACPI
> world also not everything is nice and shining.

I'm treating device tree as a stable interface, at least once it's written down 
as a spec (like all our other interfaces).  At SiFive we're treating it as the 
standard interface for static device discovery, so interface breaks will be a 
huge pain.

Re: SBI extension proposal

[Palmer Dabbelt]

On Thu, 01 Nov 2018 09:42:05 PDT (-0700), merker@debian.org wrote:
> On Thu, Nov 01, 2018 at 09:46:09AM +0000, Richard W.M. Jones wrote:
>> On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> [...]
>> > > How about putting the version information into device tree
>> > > and use the compatible string?  This seems more reliable
>> > > than probing.
>> > > e.g.
>> > > firmware {
>> > >         sbi {
>> > >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
>> > >         };
>> > > };
>> >
>> > If it was just DT then I think having this information in DT makes
>> > sense. In future,
>> > we might definitely see some ACPI support in RISC-V too (just like ARM64 world).
>>
>> I agree.  Please try not to make things that depend on DT, as it's a
>> Linux-only description which isn't suitable for other OSes and has a
>> poorly defined ABI.
>
> the notion that DT is a "Linux-only" description and not suitable
> for other operating systems isn't correct.  Besides being used by
> Linux, device-tree is also used by at least U-Boot, FreeBSD and
> NetBSD.  Based on issues in the early days of DT one can surely
> discuss about device-tree ABI stability, but since then a lot of
> effort has been put into making DT ABI-stable, and in my
> experience this effort has been successful.  On the other hand I
> have experienced way more ACPI issues on x86-64 hardware than I
> would like to remember, so I tend to assume that in the ACPI
> world also not everything is nice and shining.

I'm treating device tree as a stable interface, at least once it's written down 
as a spec (like all our other interfaces).  At SiFive we're treating it as the 
standard interface for static device discovery, so interface breaks will be a 
huge pain.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Olof Johansson]

On Thu, Nov 1, 2018 at 8:10 AM Atish Patra <atish.patra@wdc.com> wrote:
>
> On 11/1/18 4:25 AM, Richard W.M. Jones wrote:
> > On Thu, Nov 01, 2018 at 12:03:49PM +0100, Philipp Hug wrote:
> >> Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
> >> <rjones@redhat.com>:
> >>>
> >>> I agree.  Please try not to make things that depend on DT, as it's a
> >>> Linux-only description which isn't suitable for other OSes and has a
> >>> poorly defined ABI.
> >>>
> >> You don't have to depend on DT. If you implement ACPI you'd put this
> >> information in ACPI as well as you do with other information about the
> >> platform.
> >> What's the advantage of putting static information into an SBI call
> >> instead of putting it in DT, ACPI, ...?
> >
> > You only have to put it in one place.
> >
> > Rich.
> >
> I agree with Richard. It is better to manage it in SBI implementation.
> This version will not change in every kernel release or so. I think
> having global SBI version in SBI spec is fine.
>
> However, we may need to put <function,version> for vendor specific APIs
> in DT/ACPI as pointed by olof earlier. That will help to avoid conflicts.
>
> I will update the proposal such that it is clear that we are not making
> DT mandatory, ACPI is an option as well. SBI spec should independent of
> OS specific spec.

Please keep the ACPI mess out of it for now, there is no reason to go there.


-Olof

Re: SBI extension proposal

[Olof Johansson]

On Thu, Nov 1, 2018 at 8:10 AM Atish Patra <atish.patra@wdc.com> wrote:
>
> On 11/1/18 4:25 AM, Richard W.M. Jones wrote:
> > On Thu, Nov 01, 2018 at 12:03:49PM +0100, Philipp Hug wrote:
> >> Am Do., 1. Nov. 2018 um 10:46 Uhr schrieb Richard W.M. Jones
> >> <rjones@redhat.com>:
> >>>
> >>> I agree.  Please try not to make things that depend on DT, as it's a
> >>> Linux-only description which isn't suitable for other OSes and has a
> >>> poorly defined ABI.
> >>>
> >> You don't have to depend on DT. If you implement ACPI you'd put this
> >> information in ACPI as well as you do with other information about the
> >> platform.
> >> What's the advantage of putting static information into an SBI call
> >> instead of putting it in DT, ACPI, ...?
> >
> > You only have to put it in one place.
> >
> > Rich.
> >
> I agree with Richard. It is better to manage it in SBI implementation.
> This version will not change in every kernel release or so. I think
> having global SBI version in SBI spec is fine.
>
> However, we may need to put <function,version> for vendor specific APIs
> in DT/ACPI as pointed by olof earlier. That will help to avoid conflicts.
>
> I will update the proposal such that it is clear that we are not making
> DT mandatory, ACPI is an option as well. SBI spec should independent of
> OS specific spec.

Please keep the ACPI mess out of it for now, there is no reason to go there.


-Olof

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Anup Patel]

On Fri, Nov 2, 2018 at 8:19 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Thu, 01 Nov 2018 09:42:05 PDT (-0700), merker at debian.org wrote:
> > On Thu, Nov 01, 2018 at 09:46:09AM +0000, Richard W.M. Jones wrote:
> >> On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> > [...]
> >> > > How about putting the version information into device tree
> >> > > and use the compatible string?  This seems more reliable
> >> > > than probing.
> >> > > e.g.
> >> > > firmware {
> >> > >         sbi {
> >> > >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
> >> > >         };
> >> > > };
> >> >
> >> > If it was just DT then I think having this information in DT makes
> >> > sense. In future,
> >> > we might definitely see some ACPI support in RISC-V too (just like ARM64 world).
> >>
> >> I agree.  Please try not to make things that depend on DT, as it's a
> >> Linux-only description which isn't suitable for other OSes and has a
> >> poorly defined ABI.
> >
> > the notion that DT is a "Linux-only" description and not suitable
> > for other operating systems isn't correct.  Besides being used by
> > Linux, device-tree is also used by at least U-Boot, FreeBSD and
> > NetBSD.  Based on issues in the early days of DT one can surely
> > discuss about device-tree ABI stability, but since then a lot of
> > effort has been put into making DT ABI-stable, and in my
> > experience this effort has been successful.  On the other hand I
> > have experienced way more ACPI issues on x86-64 hardware than I
> > would like to remember, so I tend to assume that in the ACPI
> > world also not everything is nice and shining.
>
> I'm treating device tree as a stable interface, at least once it's written down
> as a spec (like all our other interfaces).  At SiFive we're treating it as the
> standard interface for static device discovery, so interface breaks will be a
> huge pain.

DT is a great HW description style (my opinion) but then there are
ACPI fans too.

I think SBI spec should be independent of DT or ACPI or any other HW
description style. In fact, a simple bare-metal programs (without DT or
ACPI support) should also be able to make complete use of SBI. This will
allow us to develop baremetal test suit for SBI calls.

Regards,
Anup

Re: SBI extension proposal

[Anup Patel]

On Fri, Nov 2, 2018 at 8:19 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Thu, 01 Nov 2018 09:42:05 PDT (-0700), merker@debian.org wrote:
> > On Thu, Nov 01, 2018 at 09:46:09AM +0000, Richard W.M. Jones wrote:
> >> On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> > [...]
> >> > > How about putting the version information into device tree
> >> > > and use the compatible string?  This seems more reliable
> >> > > than probing.
> >> > > e.g.
> >> > > firmware {
> >> > >         sbi {
> >> > >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
> >> > >         };
> >> > > };
> >> >
> >> > If it was just DT then I think having this information in DT makes
> >> > sense. In future,
> >> > we might definitely see some ACPI support in RISC-V too (just like ARM64 world).
> >>
> >> I agree.  Please try not to make things that depend on DT, as it's a
> >> Linux-only description which isn't suitable for other OSes and has a
> >> poorly defined ABI.
> >
> > the notion that DT is a "Linux-only" description and not suitable
> > for other operating systems isn't correct.  Besides being used by
> > Linux, device-tree is also used by at least U-Boot, FreeBSD and
> > NetBSD.  Based on issues in the early days of DT one can surely
> > discuss about device-tree ABI stability, but since then a lot of
> > effort has been put into making DT ABI-stable, and in my
> > experience this effort has been successful.  On the other hand I
> > have experienced way more ACPI issues on x86-64 hardware than I
> > would like to remember, so I tend to assume that in the ACPI
> > world also not everything is nice and shining.
>
> I'm treating device tree as a stable interface, at least once it's written down
> as a spec (like all our other interfaces).  At SiFive we're treating it as the
> standard interface for static device discovery, so interface breaks will be a
> huge pain.

DT is a great HW description style (my opinion) but then there are
ACPI fans too.

I think SBI spec should be independent of DT or ACPI or any other HW
description style. In fact, a simple bare-metal programs (without DT or
ACPI support) should also be able to make complete use of SBI. This will
allow us to develop baremetal test suit for SBI calls.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Chang, Abner (HPS SW/FW Technologist)]

> -----Original Message-----
> From: Anup Patel [mailto:anup at brainfault.org]
> Sent: Friday, November 02, 2018 11:27 AM
> To: Palmer Dabbelt <palmer@sifive.com>
> Cc: merker at debian.org; rjones at redhat.com; Mark Rutland
> <mark.rutland@arm.com>; Christoph Hellwig <hch@infradead.org>; Damien
> Le Moal <Damien.LeMoal@wdc.com>; Olof Johansson
> <olof.johansson@gmail.com>; Andrew Waterman <andrew@sifive.com>;
> alankao at andestech.com; philipp at hug.cx; Zong Li <zong@andestech.com>;
> Atish Patra <atish.patra@wdc.com>; Michael Clark <mjc@sifive.com>; Arnd
> Bergmann <arnd@arndb.de>; paul.walmsley at sifive.com; linux-
> riscv at lists.infradead.org; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>; vincentc at andestech.com;
> David.Abdurachmanov at cern.ch
> Subject: Re: SBI extension proposal
> 
> On Fri, Nov 2, 2018 at 8:19 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > On Thu, 01 Nov 2018 09:42:05 PDT (-0700), merker at debian.org wrote:
> > > On Thu, Nov 01, 2018 at 09:46:09AM +0000, Richard W.M. Jones wrote:
> > >> On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> > > [...]
> > >> > > How about putting the version information into device tree and
> > >> > > use the compatible string?  This seems more reliable than
> > >> > > probing.
> > >> > > e.g.
> > >> > > firmware {
> > >> > >         sbi {
> > >> > >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
> > >> > >         };
> > >> > > };
> > >> >
> > >> > If it was just DT then I think having this information in DT
> > >> > makes sense. In future, we might definitely see some ACPI support
> > >> > in RISC-V too (just like ARM64 world).
> > >>
> > >> I agree.  Please try not to make things that depend on DT, as it's
> > >> a Linux-only description which isn't suitable for other OSes and
> > >> has a poorly defined ABI.
> > >
> > > the notion that DT is a "Linux-only" description and not suitable
> > > for other operating systems isn't correct.  Besides being used by
> > > Linux, device-tree is also used by at least U-Boot, FreeBSD and
> > > NetBSD.  Based on issues in the early days of DT one can surely
> > > discuss about device-tree ABI stability, but since then a lot of
> > > effort has been put into making DT ABI-stable, and in my experience
> > > this effort has been successful.  On the other hand I have
> > > experienced way more ACPI issues on x86-64 hardware than I would
> > > like to remember, so I tend to assume that in the ACPI world also
> > > not everything is nice and shining.
> >
> > I'm treating device tree as a stable interface, at least once it's
> > written down as a spec (like all our other interfaces).  At SiFive
> > we're treating it as the standard interface for static device
> > discovery, so interface breaks will be a huge pain.
> 
> DT is a great HW description style (my opinion) but then there are ACPI fans
> too.
> 
> I think SBI spec should be independent of DT or ACPI or any other HW
> description style. In fact, a simple bare-metal programs (without DT or ACPI
> support) should also be able to make complete use of SBI. This will allow us
> to develop baremetal test suit for SBI calls.
> 
Agree.
Please also consider EFI firmware code base. There is no DT in UEFI spec for H/W devices or other information. We would like to leverage SBI spec as well, and implement SBI in EFI firmware. Having SBI version in SBI spec makes more sense to different FW frameworks.

> Regards,
> Anup

RE: SBI extension proposal

[Chang, Abner (HPS SW/FW Technologist)]

> -----Original Message-----
> From: Anup Patel [mailto:anup@brainfault.org]
> Sent: Friday, November 02, 2018 11:27 AM
> To: Palmer Dabbelt <palmer@sifive.com>
> Cc: merker@debian.org; rjones@redhat.com; Mark Rutland
> <mark.rutland@arm.com>; Christoph Hellwig <hch@infradead.org>; Damien
> Le Moal <Damien.LeMoal@wdc.com>; Olof Johansson
> <olof.johansson@gmail.com>; Andrew Waterman <andrew@sifive.com>;
> alankao@andestech.com; philipp@hug.cx; Zong Li <zong@andestech.com>;
> Atish Patra <atish.patra@wdc.com>; Michael Clark <mjc@sifive.com>; Arnd
> Bergmann <arnd@arndb.de>; paul.walmsley@sifive.com; linux-
> riscv@lists.infradead.org; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>; vincentc@andestech.com;
> David.Abdurachmanov@cern.ch
> Subject: Re: SBI extension proposal
> 
> On Fri, Nov 2, 2018 at 8:19 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >
> > On Thu, 01 Nov 2018 09:42:05 PDT (-0700), merker@debian.org wrote:
> > > On Thu, Nov 01, 2018 at 09:46:09AM +0000, Richard W.M. Jones wrote:
> > >> On Thu, Nov 01, 2018 at 03:05:51PM +0530, Anup Patel wrote:
> > > [...]
> > >> > > How about putting the version information into device tree and
> > >> > > use the compatible string?  This seems more reliable than
> > >> > > probing.
> > >> > > e.g.
> > >> > > firmware {
> > >> > >         sbi {
> > >> > >                 compatible = "riscv,sbi-r0p1", "riscv,sbi-r0p2";
> > >> > >         };
> > >> > > };
> > >> >
> > >> > If it was just DT then I think having this information in DT
> > >> > makes sense. In future, we might definitely see some ACPI support
> > >> > in RISC-V too (just like ARM64 world).
> > >>
> > >> I agree.  Please try not to make things that depend on DT, as it's
> > >> a Linux-only description which isn't suitable for other OSes and
> > >> has a poorly defined ABI.
> > >
> > > the notion that DT is a "Linux-only" description and not suitable
> > > for other operating systems isn't correct.  Besides being used by
> > > Linux, device-tree is also used by at least U-Boot, FreeBSD and
> > > NetBSD.  Based on issues in the early days of DT one can surely
> > > discuss about device-tree ABI stability, but since then a lot of
> > > effort has been put into making DT ABI-stable, and in my experience
> > > this effort has been successful.  On the other hand I have
> > > experienced way more ACPI issues on x86-64 hardware than I would
> > > like to remember, so I tend to assume that in the ACPI world also
> > > not everything is nice and shining.
> >
> > I'm treating device tree as a stable interface, at least once it's
> > written down as a spec (like all our other interfaces).  At SiFive
> > we're treating it as the standard interface for static device
> > discovery, so interface breaks will be a huge pain.
> 
> DT is a great HW description style (my opinion) but then there are ACPI fans
> too.
> 
> I think SBI spec should be independent of DT or ACPI or any other HW
> description style. In fact, a simple bare-metal programs (without DT or ACPI
> support) should also be able to make complete use of SBI. This will allow us
> to develop baremetal test suit for SBI calls.
> 
Agree.
Please also consider EFI firmware code base. There is no DT in UEFI spec for H/W devices or other information. We would like to leverage SBI spec as well, and implement SBI in EFI firmware. Having SBI version in SBI spec makes more sense to different FW frameworks.

> Regards,
> Anup
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Chang, Abner (HPS SW/FW Technologist)]

> >>> -- 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.
> >>>
> >>> -- void sbi_system_shutdown()
> >>>
> >>> Powers off the entire system.
> >>
> >>
> >> This is a slightly weird one to put in SBI. There's usually other actions
> needed for _system_ level shutdown, such as external power regulators.
> >>
> 
> This might be because of my limited knowledge on power management.
In ACPI, it defines the register block to put processor into different levels of sleep mode (suspend mode). However, this requires additional power management controller or processor CSRs on RISC-V. If we don?t consider ACPI and H/W capabilities of power MGMT on RISC-V at this moment, then SBI for processor power management may be needed.
My question is can SBI_hart_up () to execute "start" even the hart is not in Suspend or Down state? I am asking this because on UEFI we would like to have SBI for supervisor mode firmware to invoke functions provided by M-mode driver, just trying to leverage SBI_hart_up () for this intention. However, "Start" is defined as an address to entry into supervisor mode. We may need to add an additional parameter to this SBI which indicates the privilege to execute "Start" though.
Forget this if this SBI is dedicated for processor power management. We can propose new SBI for above intention.

> We already have a system reset call in SBI & PSCI also had system shutdown
Is system reset call defined in SBI spec as well? I don?t see this in Github.

> function. As per my understanding, this API will provide a way to handle all
> common suspend related operations on machine mode.
> In case of virtual OS, it will not result in any external power regulations.

RE: SBI extension proposal

[Chang, Abner (HPS SW/FW Technologist)]

> >>> -- 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.
> >>>
> >>> -- void sbi_system_shutdown()
> >>>
> >>> Powers off the entire system.
> >>
> >>
> >> This is a slightly weird one to put in SBI. There's usually other actions
> needed for _system_ level shutdown, such as external power regulators.
> >>
> 
> This might be because of my limited knowledge on power management.
In ACPI, it defines the register block to put processor into different levels of sleep mode (suspend mode). However, this requires additional power management controller or processor CSRs on RISC-V. If we don’t consider ACPI and H/W capabilities of power MGMT on RISC-V at this moment, then SBI for processor power management may be needed.
My question is can SBI_hart_up () to execute "start" even the hart is not in Suspend or Down state? I am asking this because on UEFI we would like to have SBI for supervisor mode firmware to invoke functions provided by M-mode driver, just trying to leverage SBI_hart_up () for this intention. However, "Start" is defined as an address to entry into supervisor mode. We may need to add an additional parameter to this SBI which indicates the privilege to execute "Start" though.
Forget this if this SBI is dedicated for processor power management. We can propose new SBI for above intention.

> We already have a system reset call in SBI & PSCI also had system shutdown
Is system reset call defined in SBI spec as well? I don’t see this in Github.

> function. As per my understanding, this API will provide a way to handle all
> common suspend related operations on machine mode.
> In case of virtual OS, it will not result in any external power regulations.





_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Nick Kossifidis]

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 ?

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

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

> 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?). Since you mentioned ARM, in their case
the ABI used for PSCI is the Secure Monitor Call (which is the 
alternative
to what is defined on the first paragraph of the SBI document), but it
has the same limitation. According to the documentation if an SMC comes
from a user application, we get an undefined exception. We can do better 
!


> 4. Return error code Table:
> ---------------------------
> 
> Here are the SBI return error codes defined.
> 
>       SBI_SUCCESS           0
>       SBI_NOT_SUPPORTED    -1
>       SBI_INVALID_PARAM    -2
>       SBI_DENIED           -3
>       SBI_INVALID_ADDRESS  -4
> 
> A mapping function between SBI error code & Linux error code should be
> provided.
> 

I believe it should be up to the OS to translate the SBI error codes to 
its
native ones.

> 5. Power State
> --------------
> 
> A RISC-V core can exist in any of the following power states.
> 
> enum RISCV_POWER_STATE {
>        //Powered up & operational.
>        RISCV_HART_ON              =  0,
>        //Powered up but at reduced energy consumption. WFI instruction
> can be used to achieve this state.
>        RISCV_HART_STANDBY        =   1,
>        //Deeper low power state. No reset required but higher wakeup
> latency.
>        RISCV_HART_RETENTION       =  2,
>        //Powered off. Reset of the core required after power restore.
>        RISCV_HART_OFF             =  3
> }
> 
> TODO:
> Any other power management related features or state?
> 

How about LOCKSTEP ? We could have a way of declaring a hart as a 
"mirror" of another
hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have 
an extra argument
on sbi_get/set_state when the LOCKSTEP state is used, that will get/set 
the hart that
this hart is mirroring.

> 6. Implementation
> -------------------
> Currently, SBI is implemented as a part of BBL. There is a different
> SBI implementation available in coreboot as well.
> 
> Alternatively, a separate open BSD/MIT licensed SBI project can be
> created which can be used by anybody to avoid these kind of SBI
> fragmentation in future. This project can generate both a firmware
> binary (to executed directly in M mode) or a static library that can
> be used by different boot loaders. It will also help individual boot
> loaders to either work from M or S mode without a separate SBI
> implementation.
> 

Why implement this at the boot loader level ? This is more like a 
firmware interface
than something that belongs to the bootloader. The bootloader should use 
this interface
not provide it. Also this is something that belongs to the M mode, a 
bootloader
should belong to the S mode, running the bootloader in M mode is not 
right from
a security point of view. We can provide something like "RISC-V Firmware 
Services"
that's code running on M mode after the first stage boot loader/BootROM 
and let
bootloaders use it without having to implement it. ARM does the same 
with their
ARM Trusted Firmware, their BL31 (Secure Monitor) gets loaded after the 
FSBL and
it then loads (on EL2, the equivalent to our S mode) the bootloader.

> This proposal is far from perfect and absolutely any suggestion is
> welcome. Obviously, there are many other functionalities that can be
> added or removed from this proposal. However, I just wanted to start
> with something that is an incremental change at best to kick off the
> discussion. The aim here is initiate a discussion that can lead to a
> robust SBI specification.
> 
> Looking forward to discuss other ideas as well or any feedback on this 
> proposal.
> 
> Reference:
> -----------
> [1]
> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
> 
> 
> Regards,
> Atish
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: SBI extension proposal

[Nick Kossifidis]

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 ?

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

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

> 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?). Since you mentioned ARM, in their case
the ABI used for PSCI is the Secure Monitor Call (which is the 
alternative
to what is defined on the first paragraph of the SBI document), but it
has the same limitation. According to the documentation if an SMC comes
from a user application, we get an undefined exception. We can do better 
!


> 4. Return error code Table:
> ---------------------------
> 
> Here are the SBI return error codes defined.
> 
>       SBI_SUCCESS           0
>       SBI_NOT_SUPPORTED    -1
>       SBI_INVALID_PARAM    -2
>       SBI_DENIED           -3
>       SBI_INVALID_ADDRESS  -4
> 
> A mapping function between SBI error code & Linux error code should be
> provided.
> 

I believe it should be up to the OS to translate the SBI error codes to 
its
native ones.

> 5. Power State
> --------------
> 
> A RISC-V core can exist in any of the following power states.
> 
> enum RISCV_POWER_STATE {
>        //Powered up & operational.
>        RISCV_HART_ON              =  0,
>        //Powered up but at reduced energy consumption. WFI instruction
> can be used to achieve this state.
>        RISCV_HART_STANDBY        =   1,
>        //Deeper low power state. No reset required but higher wakeup
> latency.
>        RISCV_HART_RETENTION       =  2,
>        //Powered off. Reset of the core required after power restore.
>        RISCV_HART_OFF             =  3
> }
> 
> TODO:
> Any other power management related features or state?
> 

How about LOCKSTEP ? We could have a way of declaring a hart as a 
"mirror" of another
hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have 
an extra argument
on sbi_get/set_state when the LOCKSTEP state is used, that will get/set 
the hart that
this hart is mirroring.

> 6. Implementation
> -------------------
> Currently, SBI is implemented as a part of BBL. There is a different
> SBI implementation available in coreboot as well.
> 
> Alternatively, a separate open BSD/MIT licensed SBI project can be
> created which can be used by anybody to avoid these kind of SBI
> fragmentation in future. This project can generate both a firmware
> binary (to executed directly in M mode) or a static library that can
> be used by different boot loaders. It will also help individual boot
> loaders to either work from M or S mode without a separate SBI
> implementation.
> 

Why implement this at the boot loader level ? This is more like a 
firmware interface
than something that belongs to the bootloader. The bootloader should use 
this interface
not provide it. Also this is something that belongs to the M mode, a 
bootloader
should belong to the S mode, running the bootloader in M mode is not 
right from
a security point of view. We can provide something like "RISC-V Firmware 
Services"
that's code running on M mode after the first stage boot loader/BootROM 
and let
bootloaders use it without having to implement it. ARM does the same 
with their
ARM Trusted Firmware, their BL31 (Secure Monitor) gets loaded after the 
FSBL and
it then loads (on EL2, the equivalent to our S mode) the bootloader.

> This proposal is far from perfect and absolutely any suggestion is
> welcome. Obviously, there are many other functionalities that can be
> added or removed from this proposal. However, I just wanted to start
> with something that is an incremental change at best to kick off the
> discussion. The aim here is initiate a discussion that can lead to a
> robust SBI specification.
> 
> Looking forward to discuss other ideas as well or any feedback on this 
> proposal.
> 
> Reference:
> -----------
> [1]
> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
> 
> 
> Regards,
> Atish
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Atish Patra]

On 11/1/18 11:31 PM, Chang, Abner (HPS SW/FW Technologist) wrote:
> 
>>>>> -- 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.
>>>>>
>>>>> -- void sbi_system_shutdown()
>>>>>
>>>>> Powers off the entire system.
>>>>
>>>>
>>>> This is a slightly weird one to put in SBI. There's usually other actions
>> needed for _system_ level shutdown, such as external power regulators.
>>>>
>>
>> This might be because of my limited knowledge on power management.
> In ACPI, it defines the register block to put processor into different levels of sleep mode (suspend mode). However, this requires additional power management controller or processor CSRs on RISC-V. If we don?t consider ACPI and H/W capabilities of power MGMT on RISC-V at this moment, then SBI for processor power management may be needed.
> My question is can SBI_hart_up () to execute "start" even the hart is not in Suspend or Down state? I am asking this because on UEFI we would like to have SBI for supervisor mode firmware to invoke functions provided by M-mode driver, just trying to leverage SBI_hart_up () for this intention. However, "Start" is defined as an address to entry into supervisor mode. We may need to add an additional parameter to this SBI which indicates the privilege to execute "Start" though.
> Forget this if this SBI is dedicated for processor power management. We can propose new SBI for above intention.
> 
int sbi_hart_up(unsigned long hartid, unsigned long start, unsigned
long priv)

It has already a opaque third parameter that can be used to address your 
case. Isn't it ?

>> We already have a system reset call in SBI & PSCI also had system shutdown
> Is system reset call defined in SBI spec as well? I don?t see this in Github.
> 
Sorry. I have made myself confused while writing. You are correct.


Regards,
Atish
>> function. As per my understanding, this API will provide a way to handle all
>> common suspend related operations on machine mode.
>> In case of virtual OS, it will not result in any external power regulations.
> 
> 
> 
> 
> 
> 

Re: SBI extension proposal

[Atish Patra]

On 11/1/18 11:31 PM, Chang, Abner (HPS SW/FW Technologist) wrote:
> 
>>>>> -- 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.
>>>>>
>>>>> -- void sbi_system_shutdown()
>>>>>
>>>>> Powers off the entire system.
>>>>
>>>>
>>>> This is a slightly weird one to put in SBI. There's usually other actions
>> needed for _system_ level shutdown, such as external power regulators.
>>>>
>>
>> This might be because of my limited knowledge on power management.
> In ACPI, it defines the register block to put processor into different levels of sleep mode (suspend mode). However, this requires additional power management controller or processor CSRs on RISC-V. If we don’t consider ACPI and H/W capabilities of power MGMT on RISC-V at this moment, then SBI for processor power management may be needed.
> My question is can SBI_hart_up () to execute "start" even the hart is not in Suspend or Down state? I am asking this because on UEFI we would like to have SBI for supervisor mode firmware to invoke functions provided by M-mode driver, just trying to leverage SBI_hart_up () for this intention. However, "Start" is defined as an address to entry into supervisor mode. We may need to add an additional parameter to this SBI which indicates the privilege to execute "Start" though.
> Forget this if this SBI is dedicated for processor power management. We can propose new SBI for above intention.
> 
int sbi_hart_up(unsigned long hartid, unsigned long start, unsigned
long priv)

It has already a opaque third parameter that can be used to address your 
case. Isn't it ?

>> We already have a system reset call in SBI & PSCI also had system shutdown
> Is system reset call defined in SBI spec as well? I don’t see this in Github.
> 
Sorry. I have made myself confused while writing. You are correct.


Regards,
Atish
>> function. As per my understanding, this API will provide a way to handle all
>> common suspend related operations on machine mode.
>> In case of virtual OS, it will not result in any external power regulations.
> 
> 
> 
> 
> 
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

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

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

>> -- 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 ?


  Since you mentioned ARM, in their case
> the ABI used for PSCI is the Secure Monitor Call (which is the
> alternative
> to what is defined on the first paragraph of the SBI document), but it
> has the same limitation. According to the documentation if an SMC comes
> from a user application, we get an undefined exception. We can do better
> !
> 



> 
>> 4. Return error code Table:
>> ---------------------------
>>
>> Here are the SBI return error codes defined.
>>
>>        SBI_SUCCESS           0
>>        SBI_NOT_SUPPORTED    -1
>>        SBI_INVALID_PARAM    -2
>>        SBI_DENIED           -3
>>        SBI_INVALID_ADDRESS  -4
>>
>> A mapping function between SBI error code & Linux error code should be
>> provided.
>>
> 
> I believe it should be up to the OS to translate the SBI error codes to
> its
> native ones.
> 

Correct. Olof also pointed this out. I will remove it in next version.

>> 5. Power State
>> --------------
>>
>> A RISC-V core can exist in any of the following power states.
>>
>> enum RISCV_POWER_STATE {
>>         //Powered up & operational.
>>         RISCV_HART_ON              =  0,
>>         //Powered up but at reduced energy consumption. WFI instruction
>> can be used to achieve this state.
>>         RISCV_HART_STANDBY        =   1,
>>         //Deeper low power state. No reset required but higher wakeup
>> latency.
>>         RISCV_HART_RETENTION       =  2,
>>         //Powered off. Reset of the core required after power restore.
>>         RISCV_HART_OFF             =  3
>> }
>>
>> TODO:
>> Any other power management related features or state?
>>
> 
> How about LOCKSTEP ? We could have a way of declaring a hart as a
> "mirror" of another
> hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have
> an extra argument
> on sbi_get/set_state when the LOCKSTEP state is used, that will get/set
> the hart that
> this hart is mirroring.
> 

Not sure if we need it right now. I want to keep it simple and focus on 
the bare minimum ones. We can always add exotic ones once we have 
feature/versioning in place.

>> 6. Implementation
>> -------------------
>> Currently, SBI is implemented as a part of BBL. There is a different
>> SBI implementation available in coreboot as well.
>>
>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>> created which can be used by anybody to avoid these kind of SBI
>> fragmentation in future. This project can generate both a firmware
>> binary (to executed directly in M mode) or a static library that can
>> be used by different boot loaders. It will also help individual boot
>> loaders to either work from M or S mode without a separate SBI
>> implementation.
>>
> 
> Why implement this at the boot loader level ? This is more like a
> firmware interface
> than something that belongs to the bootloader. The bootloader should use
> this interface
> not provide it.
It depends on the bootloader. For example, there are patches in coreboot 
that implements SBI. The idea with OpenSBI implementation is to avoid 
such fragmentation.

  Also this is something that belongs to the M mode, a
> bootloader
> should belong to the S mode, running the bootloader in M mode is not
> right from
> a security point of view. We can provide something like "RISC-V Firmware
> Services"
> that's code running on M mode after the first stage boot loader/BootROM
> and let
> bootloaders use it without having to implement it.

That's the idea for S mode bootloaders. However, there are folks who are 
interested in M mode ones and I understand their usecase. Thus, idea is 
to implement OpenSBI in such a way that, everybody can make use of it.

Regards,
Atish
  ARM does the same
> with their
> ARM Trusted Firmware, their BL31 (Secure Monitor) gets loaded after the
> FSBL and
> it then loads (on EL2, the equivalent to our S mode) the bootloader.
> 



>> This proposal is far from perfect and absolutely any suggestion is
>> welcome. Obviously, there are many other functionalities that can be
>> added or removed from this proposal. However, I just wanted to start
>> with something that is an incremental change at best to kick off the
>> discussion. The aim here is initiate a discussion that can lead to a
>> robust SBI specification.
>>
>> Looking forward to discuss other ideas as well or any feedback on this
>> proposal.
>>
>> Reference:
>> -----------
>> [1]
>> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
>> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
>>
>>
>> Regards,
>> Atish
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

Re: SBI extension proposal

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

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

>> -- 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 ?


  Since you mentioned ARM, in their case
> the ABI used for PSCI is the Secure Monitor Call (which is the
> alternative
> to what is defined on the first paragraph of the SBI document), but it
> has the same limitation. According to the documentation if an SMC comes
> from a user application, we get an undefined exception. We can do better
> !
> 



> 
>> 4. Return error code Table:
>> ---------------------------
>>
>> Here are the SBI return error codes defined.
>>
>>        SBI_SUCCESS           0
>>        SBI_NOT_SUPPORTED    -1
>>        SBI_INVALID_PARAM    -2
>>        SBI_DENIED           -3
>>        SBI_INVALID_ADDRESS  -4
>>
>> A mapping function between SBI error code & Linux error code should be
>> provided.
>>
> 
> I believe it should be up to the OS to translate the SBI error codes to
> its
> native ones.
> 

Correct. Olof also pointed this out. I will remove it in next version.

>> 5. Power State
>> --------------
>>
>> A RISC-V core can exist in any of the following power states.
>>
>> enum RISCV_POWER_STATE {
>>         //Powered up & operational.
>>         RISCV_HART_ON              =  0,
>>         //Powered up but at reduced energy consumption. WFI instruction
>> can be used to achieve this state.
>>         RISCV_HART_STANDBY        =   1,
>>         //Deeper low power state. No reset required but higher wakeup
>> latency.
>>         RISCV_HART_RETENTION       =  2,
>>         //Powered off. Reset of the core required after power restore.
>>         RISCV_HART_OFF             =  3
>> }
>>
>> TODO:
>> Any other power management related features or state?
>>
> 
> How about LOCKSTEP ? We could have a way of declaring a hart as a
> "mirror" of another
> hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have
> an extra argument
> on sbi_get/set_state when the LOCKSTEP state is used, that will get/set
> the hart that
> this hart is mirroring.
> 

Not sure if we need it right now. I want to keep it simple and focus on 
the bare minimum ones. We can always add exotic ones once we have 
feature/versioning in place.

>> 6. Implementation
>> -------------------
>> Currently, SBI is implemented as a part of BBL. There is a different
>> SBI implementation available in coreboot as well.
>>
>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>> created which can be used by anybody to avoid these kind of SBI
>> fragmentation in future. This project can generate both a firmware
>> binary (to executed directly in M mode) or a static library that can
>> be used by different boot loaders. It will also help individual boot
>> loaders to either work from M or S mode without a separate SBI
>> implementation.
>>
> 
> Why implement this at the boot loader level ? This is more like a
> firmware interface
> than something that belongs to the bootloader. The bootloader should use
> this interface
> not provide it.
It depends on the bootloader. For example, there are patches in coreboot 
that implements SBI. The idea with OpenSBI implementation is to avoid 
such fragmentation.

  Also this is something that belongs to the M mode, a
> bootloader
> should belong to the S mode, running the bootloader in M mode is not
> right from
> a security point of view. We can provide something like "RISC-V Firmware
> Services"
> that's code running on M mode after the first stage boot loader/BootROM
> and let
> bootloaders use it without having to implement it.

That's the idea for S mode bootloaders. However, there are folks who are 
interested in M mode ones and I understand their usecase. Thus, idea is 
to implement OpenSBI in such a way that, everybody can make use of it.

Regards,
Atish
  ARM does the same
> with their
> ARM Trusted Firmware, their BL31 (Secure Monitor) gets loaded after the
> FSBL and
> it then loads (on EL2, the equivalent to our S mode) the bootloader.
> 



>> This proposal is far from perfect and absolutely any suggestion is
>> welcome. Obviously, there are many other functionalities that can be
>> added or removed from this proposal. However, I just wanted to start
>> with something that is an incremental change at best to kick off the
>> discussion. The aim here is initiate a discussion that can lead to a
>> robust SBI specification.
>>
>> Looking forward to discuss other ideas as well or any feedback on this
>> proposal.
>>
>> Reference:
>> -----------
>> [1]
>> http://infocenter.arm.com/help/topic/com.arm.doc.den0022d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf
>> [2] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.md
>>
>>
>> Regards,
>> Atish
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Nick Kossifidis]

???? 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 ;-) If we use a bitmask for capabilities we'll be limited by
the encoding.

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

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

>> Since you mentioned ARM, in their case
>> the ABI used for PSCI is the Secure Monitor Call (which is the
>> alternative
>> to what is defined on the first paragraph of the SBI document), but it
>> has the same limitation. According to the documentation if an SMC 
>> comes
>> from a user application, we get an undefined exception. We can do 
>> better
>> !
>> 
> 
> 
> 
>> 
>>> 4. Return error code Table:
>>> ---------------------------
>>> 
>>> Here are the SBI return error codes defined.
>>> 
>>>        SBI_SUCCESS           0
>>>        SBI_NOT_SUPPORTED    -1
>>>        SBI_INVALID_PARAM    -2
>>>        SBI_DENIED           -3
>>>        SBI_INVALID_ADDRESS  -4
>>> 
>>> A mapping function between SBI error code & Linux error code should 
>>> be
>>> provided.
>>> 
>> 
>> I believe it should be up to the OS to translate the SBI error codes 
>> to
>> its
>> native ones.
>> 
> 
> Correct. Olof also pointed this out. I will remove it in next version.
> 
>>> 5. Power State
>>> --------------
>>> 
>>> A RISC-V core can exist in any of the following power states.
>>> 
>>> enum RISCV_POWER_STATE {
>>>         //Powered up & operational.
>>>         RISCV_HART_ON              =  0,
>>>         //Powered up but at reduced energy consumption. WFI 
>>> instruction
>>> can be used to achieve this state.
>>>         RISCV_HART_STANDBY        =   1,
>>>         //Deeper low power state. No reset required but higher wakeup
>>> latency.
>>>         RISCV_HART_RETENTION       =  2,
>>>         //Powered off. Reset of the core required after power 
>>> restore.
>>>         RISCV_HART_OFF             =  3
>>> }
>>> 
>>> TODO:
>>> Any other power management related features or state?
>>> 
>> 
>> How about LOCKSTEP ? We could have a way of declaring a hart as a
>> "mirror" of another
>> hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have
>> an extra argument
>> on sbi_get/set_state when the LOCKSTEP state is used, that will 
>> get/set
>> the hart that
>> this hart is mirroring.
>> 
> 
> Not sure if we need it right now. I want to keep it simple and focus
> on the bare minimum ones. We can always add exotic ones once we have
> feature/versioning in place.
> 

I agree, I mentioned LOCKSTEP because it'll need an extra argument
when setting the state. Btw I know lockstep sounds exotic at this point
but it's needed by many industries that deal with safety (automotive 
stuff,
medical devices etc). However it needs more thinking.

>>> 6. Implementation
>>> -------------------
>>> Currently, SBI is implemented as a part of BBL. There is a different
>>> SBI implementation available in coreboot as well.
>>> 
>>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>>> created which can be used by anybody to avoid these kind of SBI
>>> fragmentation in future. This project can generate both a firmware
>>> binary (to executed directly in M mode) or a static library that can
>>> be used by different boot loaders. It will also help individual boot
>>> loaders to either work from M or S mode without a separate SBI
>>> implementation.
>>> 
>> 
>> Why implement this at the boot loader level ? This is more like a
>> firmware interface
>> than something that belongs to the bootloader. The bootloader should 
>> use
>> this interface
>> not provide it.
> It depends on the bootloader. For example, there are patches in
> coreboot that implements SBI. The idea with OpenSBI implementation is
> to avoid such fragmentation.
> 
>> Also this is something that belongs to the M mode, a
>> bootloader
>> should belong to the S mode, running the bootloader in M mode is not
>> right from
>> a security point of view. We can provide something like "RISC-V 
>> Firmware
>> Services"
>> that's code running on M mode after the first stage boot 
>> loader/BootROM
>> and let
>> bootloaders use it without having to implement it.
> 
> That's the idea for S mode bootloaders. However, there are folks who
> are interested in M mode ones and I understand their usecase. Thus,
> idea is to implement OpenSBI in such a way that, everybody can make
> use of it.
> 

I guess then we can provide it as a library and also provide a reference
implementation around it. However I still believe that something like
"RISC-V Firmware Services" is more appropriate, we can structure this
in a client/server way and let bootloaders use the server part and
OS drivers/modules use the client part. This way we can also avoid
fragmentation across OSes.

Regards,
Nick

Re: SBI extension proposal

[Nick Kossifidis]

Στις 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 ;-) If we use a bitmask for capabilities we'll be limited by
the encoding.

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

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

>> Since you mentioned ARM, in their case
>> the ABI used for PSCI is the Secure Monitor Call (which is the
>> alternative
>> to what is defined on the first paragraph of the SBI document), but it
>> has the same limitation. According to the documentation if an SMC 
>> comes
>> from a user application, we get an undefined exception. We can do 
>> better
>> !
>> 
> 
> 
> 
>> 
>>> 4. Return error code Table:
>>> ---------------------------
>>> 
>>> Here are the SBI return error codes defined.
>>> 
>>>        SBI_SUCCESS           0
>>>        SBI_NOT_SUPPORTED    -1
>>>        SBI_INVALID_PARAM    -2
>>>        SBI_DENIED           -3
>>>        SBI_INVALID_ADDRESS  -4
>>> 
>>> A mapping function between SBI error code & Linux error code should 
>>> be
>>> provided.
>>> 
>> 
>> I believe it should be up to the OS to translate the SBI error codes 
>> to
>> its
>> native ones.
>> 
> 
> Correct. Olof also pointed this out. I will remove it in next version.
> 
>>> 5. Power State
>>> --------------
>>> 
>>> A RISC-V core can exist in any of the following power states.
>>> 
>>> enum RISCV_POWER_STATE {
>>>         //Powered up & operational.
>>>         RISCV_HART_ON              =  0,
>>>         //Powered up but at reduced energy consumption. WFI 
>>> instruction
>>> can be used to achieve this state.
>>>         RISCV_HART_STANDBY        =   1,
>>>         //Deeper low power state. No reset required but higher wakeup
>>> latency.
>>>         RISCV_HART_RETENTION       =  2,
>>>         //Powered off. Reset of the core required after power 
>>> restore.
>>>         RISCV_HART_OFF             =  3
>>> }
>>> 
>>> TODO:
>>> Any other power management related features or state?
>>> 
>> 
>> How about LOCKSTEP ? We could have a way of declaring a hart as a
>> "mirror" of another
>> hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have
>> an extra argument
>> on sbi_get/set_state when the LOCKSTEP state is used, that will 
>> get/set
>> the hart that
>> this hart is mirroring.
>> 
> 
> Not sure if we need it right now. I want to keep it simple and focus
> on the bare minimum ones. We can always add exotic ones once we have
> feature/versioning in place.
> 

I agree, I mentioned LOCKSTEP because it'll need an extra argument
when setting the state. Btw I know lockstep sounds exotic at this point
but it's needed by many industries that deal with safety (automotive 
stuff,
medical devices etc). However it needs more thinking.

>>> 6. Implementation
>>> -------------------
>>> Currently, SBI is implemented as a part of BBL. There is a different
>>> SBI implementation available in coreboot as well.
>>> 
>>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>>> created which can be used by anybody to avoid these kind of SBI
>>> fragmentation in future. This project can generate both a firmware
>>> binary (to executed directly in M mode) or a static library that can
>>> be used by different boot loaders. It will also help individual boot
>>> loaders to either work from M or S mode without a separate SBI
>>> implementation.
>>> 
>> 
>> Why implement this at the boot loader level ? This is more like a
>> firmware interface
>> than something that belongs to the bootloader. The bootloader should 
>> use
>> this interface
>> not provide it.
> It depends on the bootloader. For example, there are patches in
> coreboot that implements SBI. The idea with OpenSBI implementation is
> to avoid such fragmentation.
> 
>> Also this is something that belongs to the M mode, a
>> bootloader
>> should belong to the S mode, running the bootloader in M mode is not
>> right from
>> a security point of view. We can provide something like "RISC-V 
>> Firmware
>> Services"
>> that's code running on M mode after the first stage boot 
>> loader/BootROM
>> and let
>> bootloaders use it without having to implement it.
> 
> That's the idea for S mode bootloaders. However, there are folks who
> are interested in M mode ones and I understand their usecase. Thus,
> idea is to implement OpenSBI in such a way that, everybody can make
> use of it.
> 

I guess then we can provide it as a library and also provide a reference
implementation around it. However I still believe that something like
"RISC-V Firmware Services" is more appropriate, we can structure this
in a client/server way and let bootloaders use the server part and
OS drivers/modules use the client part. This way we can also avoid
fragmentation across OSes.

Regards,
Nick

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

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

>>> Since you mentioned ARM, in their case
>>> the ABI used for PSCI is the Secure Monitor Call (which is the
>>> alternative
>>> to what is defined on the first paragraph of the SBI document), but it
>>> has the same limitation. According to the documentation if an SMC
>>> comes
>>> from a user application, we get an undefined exception. We can do
>>> better
>>> !
>>>
>>
>>
>>
>>>
>>>> 4. Return error code Table:
>>>> ---------------------------
>>>>
>>>> Here are the SBI return error codes defined.
>>>>
>>>>         SBI_SUCCESS           0
>>>>         SBI_NOT_SUPPORTED    -1
>>>>         SBI_INVALID_PARAM    -2
>>>>         SBI_DENIED           -3
>>>>         SBI_INVALID_ADDRESS  -4
>>>>
>>>> A mapping function between SBI error code & Linux error code should
>>>> be
>>>> provided.
>>>>
>>>
>>> I believe it should be up to the OS to translate the SBI error codes
>>> to
>>> its
>>> native ones.
>>>
>>
>> Correct. Olof also pointed this out. I will remove it in next version.
>>
>>>> 5. Power State
>>>> --------------
>>>>
>>>> A RISC-V core can exist in any of the following power states.
>>>>
>>>> enum RISCV_POWER_STATE {
>>>>          //Powered up & operational.
>>>>          RISCV_HART_ON              =  0,
>>>>          //Powered up but at reduced energy consumption. WFI
>>>> instruction
>>>> can be used to achieve this state.
>>>>          RISCV_HART_STANDBY        =   1,
>>>>          //Deeper low power state. No reset required but higher wakeup
>>>> latency.
>>>>          RISCV_HART_RETENTION       =  2,
>>>>          //Powered off. Reset of the core required after power
>>>> restore.
>>>>          RISCV_HART_OFF             =  3
>>>> }
>>>>
>>>> TODO:
>>>> Any other power management related features or state?
>>>>
>>>
>>> How about LOCKSTEP ? We could have a way of declaring a hart as a
>>> "mirror" of another
>>> hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have
>>> an extra argument
>>> on sbi_get/set_state when the LOCKSTEP state is used, that will
>>> get/set
>>> the hart that
>>> this hart is mirroring.
>>>
>>
>> Not sure if we need it right now. I want to keep it simple and focus
>> on the bare minimum ones. We can always add exotic ones once we have
>> feature/versioning in place.
>>
> 
> I agree, I mentioned LOCKSTEP because it'll need an extra argument
> when setting the state. Btw I know lockstep sounds exotic at this point
> but it's needed by many industries that deal with safety (automotive
> stuff,
> medical devices etc). However it needs more thinking.
> 
>>>> 6. Implementation
>>>> -------------------
>>>> Currently, SBI is implemented as a part of BBL. There is a different
>>>> SBI implementation available in coreboot as well.
>>>>
>>>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>>>> created which can be used by anybody to avoid these kind of SBI
>>>> fragmentation in future. This project can generate both a firmware
>>>> binary (to executed directly in M mode) or a static library that can
>>>> be used by different boot loaders. It will also help individual boot
>>>> loaders to either work from M or S mode without a separate SBI
>>>> implementation.
>>>>
>>>
>>> Why implement this at the boot loader level ? This is more like a
>>> firmware interface
>>> than something that belongs to the bootloader. The bootloader should
>>> use
>>> this interface
>>> not provide it.
>> It depends on the bootloader. For example, there are patches in
>> coreboot that implements SBI. The idea with OpenSBI implementation is
>> to avoid such fragmentation.
>>
>>> Also this is something that belongs to the M mode, a
>>> bootloader
>>> should belong to the S mode, running the bootloader in M mode is not
>>> right from
>>> a security point of view. We can provide something like "RISC-V
>>> Firmware
>>> Services"
>>> that's code running on M mode after the first stage boot
>>> loader/BootROM
>>> and let
>>> bootloaders use it without having to implement it.
>>
>> That's the idea for S mode bootloaders. However, there are folks who
>> are interested in M mode ones and I understand their usecase. Thus,
>> idea is to implement OpenSBI in such a way that, everybody can make
>> use of it.
>>
> 
> I guess then we can provide it as a library and also provide a reference
> implementation around it. 

Yup. That is the exact idea.

However I still believe that something like
> "RISC-V Firmware Services" is more appropriate, we can structure this
> in a client/server way and let bootloaders use the server part and
> OS drivers/modules use the client part. This way we can also avoid
> fragmentation across OSes.
> 
Thanks for the idea. Let me think about it more. We have just started 
working on OpenSBI project. I will update when we have something working.

Regards,
Atish
> Regards,
> Nick
> 

Re: SBI extension proposal

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

>>> Since you mentioned ARM, in their case
>>> the ABI used for PSCI is the Secure Monitor Call (which is the
>>> alternative
>>> to what is defined on the first paragraph of the SBI document), but it
>>> has the same limitation. According to the documentation if an SMC
>>> comes
>>> from a user application, we get an undefined exception. We can do
>>> better
>>> !
>>>
>>
>>
>>
>>>
>>>> 4. Return error code Table:
>>>> ---------------------------
>>>>
>>>> Here are the SBI return error codes defined.
>>>>
>>>>         SBI_SUCCESS           0
>>>>         SBI_NOT_SUPPORTED    -1
>>>>         SBI_INVALID_PARAM    -2
>>>>         SBI_DENIED           -3
>>>>         SBI_INVALID_ADDRESS  -4
>>>>
>>>> A mapping function between SBI error code & Linux error code should
>>>> be
>>>> provided.
>>>>
>>>
>>> I believe it should be up to the OS to translate the SBI error codes
>>> to
>>> its
>>> native ones.
>>>
>>
>> Correct. Olof also pointed this out. I will remove it in next version.
>>
>>>> 5. Power State
>>>> --------------
>>>>
>>>> A RISC-V core can exist in any of the following power states.
>>>>
>>>> enum RISCV_POWER_STATE {
>>>>          //Powered up & operational.
>>>>          RISCV_HART_ON              =  0,
>>>>          //Powered up but at reduced energy consumption. WFI
>>>> instruction
>>>> can be used to achieve this state.
>>>>          RISCV_HART_STANDBY        =   1,
>>>>          //Deeper low power state. No reset required but higher wakeup
>>>> latency.
>>>>          RISCV_HART_RETENTION       =  2,
>>>>          //Powered off. Reset of the core required after power
>>>> restore.
>>>>          RISCV_HART_OFF             =  3
>>>> }
>>>>
>>>> TODO:
>>>> Any other power management related features or state?
>>>>
>>>
>>> How about LOCKSTEP ? We could have a way of declaring a hart as a
>>> "mirror" of another
>>> hart (https://en.wikipedia.org/wiki/Lockstep_(computing)). We can have
>>> an extra argument
>>> on sbi_get/set_state when the LOCKSTEP state is used, that will
>>> get/set
>>> the hart that
>>> this hart is mirroring.
>>>
>>
>> Not sure if we need it right now. I want to keep it simple and focus
>> on the bare minimum ones. We can always add exotic ones once we have
>> feature/versioning in place.
>>
> 
> I agree, I mentioned LOCKSTEP because it'll need an extra argument
> when setting the state. Btw I know lockstep sounds exotic at this point
> but it's needed by many industries that deal with safety (automotive
> stuff,
> medical devices etc). However it needs more thinking.
> 
>>>> 6. Implementation
>>>> -------------------
>>>> Currently, SBI is implemented as a part of BBL. There is a different
>>>> SBI implementation available in coreboot as well.
>>>>
>>>> Alternatively, a separate open BSD/MIT licensed SBI project can be
>>>> created which can be used by anybody to avoid these kind of SBI
>>>> fragmentation in future. This project can generate both a firmware
>>>> binary (to executed directly in M mode) or a static library that can
>>>> be used by different boot loaders. It will also help individual boot
>>>> loaders to either work from M or S mode without a separate SBI
>>>> implementation.
>>>>
>>>
>>> Why implement this at the boot loader level ? This is more like a
>>> firmware interface
>>> than something that belongs to the bootloader. The bootloader should
>>> use
>>> this interface
>>> not provide it.
>> It depends on the bootloader. For example, there are patches in
>> coreboot that implements SBI. The idea with OpenSBI implementation is
>> to avoid such fragmentation.
>>
>>> Also this is something that belongs to the M mode, a
>>> bootloader
>>> should belong to the S mode, running the bootloader in M mode is not
>>> right from
>>> a security point of view. We can provide something like "RISC-V
>>> Firmware
>>> Services"
>>> that's code running on M mode after the first stage boot
>>> loader/BootROM
>>> and let
>>> bootloaders use it without having to implement it.
>>
>> That's the idea for S mode bootloaders. However, there are folks who
>> are interested in M mode ones and I understand their usecase. Thus,
>> idea is to implement OpenSBI in such a way that, everybody can make
>> use of it.
>>
> 
> I guess then we can provide it as a library and also provide a reference
> implementation around it. 

Yup. That is the exact idea.

However I still believe that something like
> "RISC-V Firmware Services" is more appropriate, we can structure this
> in a client/server way and let bootloaders use the server part and
> OS drivers/modules use the client part. This way we can also avoid
> fragmentation across OSes.
> 
Thanks for the idea. Let me think about it more. We have just started 
working on OpenSBI project. I will update when we have something working.

Regards,
Atish
> Regards,
> Nick
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Chang, Abner (HPS SW/FW Technologist)]

> -----Original Message-----
> From: Atish Patra [mailto:atish.patra at wdc.com]
> Sent: Saturday, November 03, 2018 6:31 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>; Olof
> Johansson <olof.johansson@gmail.com>
> Cc: linux-riscv at lists.infradead.org; Palmer Dabbelt <palmer@sifive.com>;
> Andrew Waterman <andrew@sifive.com>; paul.walmsley at sifive.com;
> hch at infradead.org; zong at andestech.com; anup at brainfault.org; Damien Le
> Moal <Damien.LeMoal@wdc.com>; alankao at andestech.com; Arnd
> Bergmann <arnd@arndb.de>; mark.rutland at arm.com;
> vincentc at andestech.com; mjc at sifive.com; rjones at redhat.com; Chen,
> Gilbert <gilbert.chen@hpe.com>
> Subject: Re: SBI extension proposal
> 
> On 11/1/18 11:31 PM, Chang, Abner (HPS SW/FW Technologist) wrote:
> >
> >>>>> -- 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.
> >>>>>
> >>>>> -- void sbi_system_shutdown()
> >>>>>
> >>>>> Powers off the entire system.
> >>>>
> >>>>
> >>>> This is a slightly weird one to put in SBI. There's usually other
> >>>> actions
> >> needed for _system_ level shutdown, such as external power regulators.
> >>>>
> >>
> >> This might be because of my limited knowledge on power management.
> > In ACPI, it defines the register block to put processor into different levels of
> sleep mode (suspend mode). However, this requires additional power
> management controller or processor CSRs on RISC-V. If we don?t consider
> ACPI and H/W capabilities of power MGMT on RISC-V at this moment, then
> SBI for processor power management may be needed.
> > My question is can SBI_hart_up () to execute "start" even the hart is not in
> Suspend or Down state? I am asking this because on UEFI we would like to
> have SBI for supervisor mode firmware to invoke functions provided by M-
> mode driver, just trying to leverage SBI_hart_up () for this intention.
> However, "Start" is defined as an address to entry into supervisor mode. We
> may need to add an additional parameter to this SBI which indicates the
> privilege to execute "Start" though.
> > Forget this if this SBI is dedicated for processor power management. We
> can propose new SBI for above intention.
> >
> int sbi_hart_up(unsigned long hartid, unsigned long start, unsigned long priv)
> 
> It has already a opaque third parameter that can be used to address your
> case. Isn't it ?
According to the design you have for sbi_hart_up(), priv is prepared by caller and passed to start in supervisor mode. Means sbi_hart_up () is not necessary to understand the content of priv. The content of priv should be function-dependent and not able to be standardized. I think we can't use priv to address my case. We may need additional parameter which indicates  the privilege to execute "start". With this we can ask hart to run specific function in either m or s mode, and sbi_hart_up() will be more nice and powerful to firmware which has implementation of privilege levels during system boot.

> 
> >> We already have a system reset call in SBI & PSCI also had system
> >> shutdown
> > Is system reset call defined in SBI spec as well? I don?t see this in Github.
> >
> Sorry. I have made myself confused while writing. You are correct.
Is SBI spec of Shutdown() already freeze?  We may need parameter to indicate the shutdown type, for example
- Power off
- Cold reset
- Warm reset

> 
> 
> Regards,
> Atish
> >> function. As per my understanding, this API will provide a way to
> >> handle all common suspend related operations on machine mode.
> >> In case of virtual OS, it will not result in any external power regulations.
> >
> >
> >
> >
> >
> >

RE: SBI extension proposal

[Chang, Abner (HPS SW/FW Technologist)]

> -----Original Message-----
> From: Atish Patra [mailto:atish.patra@wdc.com]
> Sent: Saturday, November 03, 2018 6:31 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>; Olof
> Johansson <olof.johansson@gmail.com>
> Cc: linux-riscv@lists.infradead.org; Palmer Dabbelt <palmer@sifive.com>;
> Andrew Waterman <andrew@sifive.com>; paul.walmsley@sifive.com;
> hch@infradead.org; zong@andestech.com; anup@brainfault.org; Damien Le
> Moal <Damien.LeMoal@wdc.com>; alankao@andestech.com; Arnd
> Bergmann <arnd@arndb.de>; mark.rutland@arm.com;
> vincentc@andestech.com; mjc@sifive.com; rjones@redhat.com; Chen,
> Gilbert <gilbert.chen@hpe.com>
> Subject: Re: SBI extension proposal
> 
> On 11/1/18 11:31 PM, Chang, Abner (HPS SW/FW Technologist) wrote:
> >
> >>>>> -- 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.
> >>>>>
> >>>>> -- void sbi_system_shutdown()
> >>>>>
> >>>>> Powers off the entire system.
> >>>>
> >>>>
> >>>> This is a slightly weird one to put in SBI. There's usually other
> >>>> actions
> >> needed for _system_ level shutdown, such as external power regulators.
> >>>>
> >>
> >> This might be because of my limited knowledge on power management.
> > In ACPI, it defines the register block to put processor into different levels of
> sleep mode (suspend mode). However, this requires additional power
> management controller or processor CSRs on RISC-V. If we don’t consider
> ACPI and H/W capabilities of power MGMT on RISC-V at this moment, then
> SBI for processor power management may be needed.
> > My question is can SBI_hart_up () to execute "start" even the hart is not in
> Suspend or Down state? I am asking this because on UEFI we would like to
> have SBI for supervisor mode firmware to invoke functions provided by M-
> mode driver, just trying to leverage SBI_hart_up () for this intention.
> However, "Start" is defined as an address to entry into supervisor mode. We
> may need to add an additional parameter to this SBI which indicates the
> privilege to execute "Start" though.
> > Forget this if this SBI is dedicated for processor power management. We
> can propose new SBI for above intention.
> >
> int sbi_hart_up(unsigned long hartid, unsigned long start, unsigned long priv)
> 
> It has already a opaque third parameter that can be used to address your
> case. Isn't it ?
According to the design you have for sbi_hart_up(), priv is prepared by caller and passed to start in supervisor mode. Means sbi_hart_up () is not necessary to understand the content of priv. The content of priv should be function-dependent and not able to be standardized. I think we can't use priv to address my case. We may need additional parameter which indicates  the privilege to execute "start". With this we can ask hart to run specific function in either m or s mode, and sbi_hart_up() will be more nice and powerful to firmware which has implementation of privilege levels during system boot.

> 
> >> We already have a system reset call in SBI & PSCI also had system
> >> shutdown
> > Is system reset call defined in SBI spec as well? I don’t see this in Github.
> >
> Sorry. I have made myself confused while writing. You are correct.
Is SBI spec of Shutdown() already freeze?  We may need parameter to indicate the shutdown type, for example
- Power off
- Cold reset
- Warm reset

> 
> 
> Regards,
> Atish
> >> function. As per my understanding, this API will provide a way to
> >> handle all common suspend related operations on machine mode.
> >> In case of virtual OS, it will not result in any external power regulations.
> >
> >
> >
> >
> >
> >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Nick Kossifidis]

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

Re: SBI extension proposal

[Nick Kossifidis]

Στις 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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Atish Patra]

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.

Regards,
Atish

Re: SBI extension proposal

[Atish Patra]

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.

Regards,
Atish


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

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

Re: SBI extension proposal

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

SBI extension proposal

[Atish Patra]

On 11/5/18 5:55 PM, Zong Li wrote:
> 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. 

I don't have too much insight into how trigger module works or how do 
you switch to debug mode from S mode. However, if you absolutely need 
SBI calls, one single call with an argument would be the best choice.

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.
> 
I am not sure why do we need SBI calls for perf. ISA spec already have 
provisions for vendor specific perf counters via hpmcountern which you 
can access in supervisor mode just like cycle & instret.


Regards,
Atish

Re: SBI extension proposal

[Atish Patra]

On 11/5/18 5:55 PM, Zong Li wrote:
> 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. 

I don't have too much insight into how trigger module works or how do 
you switch to debug mode from S mode. However, if you absolutely need 
SBI calls, one single call with an argument would be the best choice.

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.
> 
I am not sure why do we need SBI calls for perf. ISA spec already have 
provisions for vendor specific perf counters via hpmcountern which you 
can access in supervisor mode just like cycle & instret.


Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv