On Tue, Mar 31, 2020 at 12:13 PM Anup Patel <anup.patel@...> wrote:
>
> This patch adds SBI v0.2 compliant system reboot extension. It defines
> two functions:
> 1. sbi_reboot - A system reboot call with reboot type as parameter
> 2. sbi_shutdown - A system shutdown/poweroff call
>
> The sbi_shutdown function defined here replaces SBI v0.1 shutdown
> function.
>
> Signed-off-by: Atish Patra <atish.patra@...>
> Signed-off-by: Anup Patel <anup.patel@...>
> ---
>  riscv-sbi.adoc | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
> index 8137686..a39e362 100644
> --- a/riscv-sbi.adoc
> +++ b/riscv-sbi.adoc
> @@ -615,6 +615,70 @@ state of the hart at the time of return value verification.
>  | sbi_hart_get_status          |           2 |     0x48534D
>  |===
>
> +== System Reboot Extension, Extension ID: 0x53524254 (SRBT)
> +
> +The System Reboot Extension provides a set of functions that allow the
> +supervisor software to request system-level reboot or shutdown.
> +
> +[source, C]
> +----
> +struct sbiret sbi_system_reboot(unsigned long reboot_type)
> +----
> +
> +Reboot the system based on provided reboot type. This is a synchronous call
> +and is not expected to return if succeeds.
> +
> +The reboot_type parameter is 32 bit wide and has following possible values:
> +
> +[cols="<,>",options="header,compact"]
> +|===
> +| Value                        | Description
> +| 0x00000000                   | Cold reboot
> +| 0x00000001                   | Warm reboot
> +| 0x00000002 - 0xEFFFFFFF      | Reserved for future use
> +| 0xF0000000 - 0xFFFFFFFF      | Vendor or platform specific reboot type
> +| 0x100000000 - 2^XELN-1       | Reserved for RV64/RV128

The words are misleading, as it could indicate that the above types
only apply to RV32.

> +|===
> +
> +*Returns* one of the following possible SBI error codes through sbiret.error
> +upon failure.
> +
> +Cold reboot results in complete power cycle of the entire system while
> +warm reboot depends on SOC vendor design choices.
> +
> +[cols="<,>",options="header,compact"]
> +|===
> +| Error code                | Description
> +| SBI_ERR_INVALID_PARAM     | `reboot_type` is not valid.
> +| SBI_ERR_FAILED            | Reboot request failed for unknown reasons.
> +|===
> +
> +[source, C]
> +----
> +struct sbiret sbi_system_shutdown()
> +----
> +
> +Shutdown the system. This is a synchronous call and is not expected to
> +return if succeeds.
> +
> +*Returns* one of the following possible SBI error codes through sbiret.error
> +upon failure.
> +
> +[cols="<,>",options="header,compact"]
> +|===
> +| Error code                | Description
> +| SBI_ERR_FAILED            | The start request failed for unknown reasons.

The shutdown request

> +|===
> +
> +=== SRBT Function Listing
> +
> +[cols="<,,>",options="header,compact"]
> +|===
> +| Function Name                 | Function ID | Extension ID
> +| sbi_system_reboot                    |           0 |   0x53524254
> +| sbi_system_shutdown                  |           1 |   0x53524254
> +|===
> +
>  == Experimental SBI Extension Space, Extension IDs 0x0800000 through 0x08FFFFFF
>
>  No management.

Otherwise, looks good to me.

Regards,
Bin

Hello all,

>
> Let's continue the discussion here as it has a wider audience than
> github PR. I have also cc'd all the possible stakeholders.
>
> The other alternatives propsed so far
>
> 1. Just parse the deveice tree node in S-mode software.
>
> Pros: No additional SBI extensions are required.
> Cons: U-Boot is responsible for copying the reserved-memory node from
> the DT passed by OpenSBI and set it to the final DT if it is a
> different one. OpenSBI also need to provide the DT where the previous
> stage (FSBL/U-Boot SPL) doesn't provide a DT.
>
> IMHO, this is not a very difficult problem to solve. The U-Boot
> implementation is available here (just ~40 lines of code).
> https://patchwork.ozlabs.org/patch/1254740/
>
> 2. Trap-n-Emulate PMP CSR reads from S-mode. The details are available
> here.
>
> https://github.com/riscv/riscv-sbi-doc/pull/37#issuecomment-596944084
>
> Pros: No SBI extension or prior DTB fixup required in U-Boot.
> Cons: As PMP extensions proposals are already in place, this may need
> to be extended. Privilege spec may need to be modified to explicitly
> say that M-mode can provide PMP csr emulation.
>
>
> Any thoughts ?


The assumption that there'll always be a PMP rule for marking the
firmware's memory isn't always true. M-mode can access any region
without a matching PMP rule so basically the firmware can be anywhere
and work fine, without any PMP rule marking its region. With the current
PMP spec this is probably the best approach since it will also prevent
S-mode from ever accessing this region (S-mode will fail if there is no
matching PMP rule). Why waste a rule for the firmware's region to deny
any access from S-mode, when you can have the same result by not putting
that rule on PMP ? Even if that assumption was always true, what about
systems without PMP, systems with a different number of PMP regions, or
systems with custom PMP-equivalent solutions ? Trap-n-emulate is not
that generic (e.g. in the case of different number of PMP regions). Any
interface from S-mode to PMP or a PMP-equivalent mechanism, should be
more abstract than this IMHO.

We should also consider the security implications of exposing the PMP
configuration to a less privileged mode. It's one thing to let S-mode
know about the DRAM regions that it can't touch because the firmware is
there, and another to reveal the full PMP configuration. PMP may contain
settings that S-mode doesn't even know about, devices that are not on
the device tree for example or DRAM regions that are outside the device
tree's /memory node range (so S-mode won't touch them anyway). I'm not a
fan of security through obscurity but there are good reasons to keep PMP
configuration accessible only to M-mode (in addition to having a more
abstract API with S-mode to be able to handle the various system
configurations / implementations). We can have S-mode request M-mode to
protect specific physical regions that S-mode manages through PMP (or a
vendor-specific PMP-equivalent mechanism) but anything more than that
doesn't make much sense to me.

If we just want to prevent S-mode from touching firmware's memory we can
either exclude its region from the /memory node on the device tree, or
let the firmware modify the device tree it passes on to the next-stage
boot loader and add a reserved-memory node. I think that's the best
approach since it's the most generic one and it's standards-compliant,
plus the device-tree is platform-specific anyway, even if it's included
on the kernel blob directly without passing through OpenSBI or whatever
boot loader is there, we can always add there a reserved region, or
modify the /memory node manually.

Regards,
Nick

This patch adds SBI v0.2 compliant system reboot extension. It defines
the sbi_system_reboot function which does different things based on
reboot_type parameter:
1. shutdown    - Power-off the entire system
2. cold reboot - Power-cycle the entire system
3. warm reboot - Power-cycle only parts of system based on SOC vendor
                 design choices

The sbi_system_reboot function defined here is also an replacement of
SBI v0.1 shutdown function.

Signed-off-by: Atish Patra <atish.patra@...>
Signed-off-by: Anup Patel <anup.patel@...>
---
 riscv-sbi.adoc | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/riscv-sbi.adoc b/riscv-sbi.adoc
index 8137686..d93a5c2 100644
--- a/riscv-sbi.adoc
+++ b/riscv-sbi.adoc
@@ -615,6 +615,53 @@ state of the hart at the time of return value verification.
 | sbi_hart_get_status          |           2 |     0x48534D
 |===

+== System Reboot Extension, Extension ID: 0x53524254 (SRBT)
+
+The System Reboot Extension provides a set of functions that allow the
+supervisor software to request system-level reboot or shutdown.
+
+[source, C]
+----
+struct sbiret sbi_system_reboot(unsigned long reboot_type)
+----
+
+Reboot the system based on provided reboot type. This is a synchronous call
+and does not return if it succeeds.
+
+The reboot_type parameter is 32 bit wide and has following possible values:
+
+[cols="<,>",options="header,compact"]
+|===
+| Value                        | Description
+| 0x00000000                   | Shutdown
+| 0x00000001                   | Cold reboot
+| 0x00000002                   | Warm reboot
+| 0x00000003 - 0xEFFFFFFF      | Reserved for future use
+| 0xF0000000 - 0xFFFFFFFF      | Vendor or platform specific reboot type
+| 0x100000000 - 2^XELN-1       | Reserved and unusable on RV32
+|===
+
+Cold reboot results in complete power cycle of the entire system while
+warm reboot depends on SOC vendor design choices.
+
+*Returns* one of the following possible SBI error codes through sbiret.error
+upon failure.
+
+[cols="<,>",options="header,compact"]
+|===
+| Error code                | Description
+| SBI_ERR_INVALID_PARAM     | `reboot_type` is not valid.
+| SBI_ERR_FAILED            | Reboot request failed for unknown reasons.
+|===
+
+=== SRBT Function Listing
+
+[cols="<,,>",options="header,compact"]
+|===
+| Function Name                 | Function ID | Extension ID
+| sbi_system_reboot                    |           0 |   0x53524254
+|===
+
 == Experimental SBI Extension Space, Extension IDs 0x0800000 through 0x08FFFFFF

 No management.
--
2.17.1