riscv: add SBI system reset extension

[danq1222_gmail.com]

Summary

Version 0.3-rc0 of the RISCV-SBI spec
introduced a new extension - sbi_system_reset.
This function is a replacement of sbi_shutdown
function in v0.1.

Update SBI code to use the new replacement
extension, and fall back to the legacy one, when necessary.

Test Plan

• Build OpenSBI from source
• Boot to multiuser in QEMU with OpenSBI built from source,

and with older OpenSBI versions.

[danq1222_gmail.com]

Run git diff with -U999999 option.

[danq1222_gmail.com]

Removed not required changes to sbi.c file.

[mhorne]

Looks great overall, just a couple of notes.

Also, OpenSBI v0.9 was released yesterday, which contains the SRST extension. I will update the FreeBSD port this week.

sys/riscv/include/sbi.h

109

I don't think there's a need to spell these constants in hex.

210–222

Could use a comment here like other SBI extensions have.

sys/riscv/riscv/sbi.c

94–105

As a follow-up change, this function could be extended to handle other reset types.

RB_POWERCYCLE implies a warm reboot, and RB_DUMP implies a system failure (panic). The lack of these three flags indicates a regular reboot.

106

sys/riscv/riscv/vm_machdep.c

113

Nice, this will cause cpu_reset to behave correctly when the extension is present.

[kp]

sys/riscv/riscv/sbi.c

107

It may be worth falling back to SBI_SHUTDOWN if this fails.

Happily that should only require moving that call out of the 'else' clause.

[jrtc27]

sys/riscv/riscv/sbi.c

107

Needs a newline, and should probably print the error code

317

This is a bit ambiguous

[jrtc27]

sys/riscv/riscv/sbi.c

94–105

Generally drivers just check RB_POWEROFF and RB_HALT. RB_POWERCYCLE is a power cycle, which if anything would be a cold reboot, but is only used by the IPMI driver at the moment.

So normally you want:

if ((howto & RB_POWEROFF) != 0) {
    halt
} else if ((howto & RB_HALT) == 0) {
    reboot
}

(see the ACPI driver)

[mhorne]

sys/riscv/riscv/sbi.c

94–105

Hmmm, agreed. My comment about RB_POWERCYCLE is incorrect. It remains to be seen how useful SBI_SRST_REASON_SYSTEM_FAILURE is.

[danq1222_gmail.com]

Feedback.

sys/riscv/riscv/sbi.c

107

It may be worth falling back to SBI_SHUTDOWN if this fails.

Happily that should only require moving that call out of the 'else' clause.

Like the below?

if (ret.error != SBI_SUCCESS) {
	printf("SBI System Reset Type Error %lu\n", reset_type);
	(void)SBI_CALL0(SBI_SHUTDOWN, 0);
}

107

Needs a newline, and should probably print the error code

I am thinking of the reset_type as error code. Could this be OK?
Can't think of anything else.

printf("SBI System Reset Type Error %lu\n", reset_type);

[kp]

sys/riscv/riscv/sbi.c

94–105

It's not unheard of for embedded system to care. A reasonably likely use case for something like this is to inform the boot loader (or on RISC-V M-mode) that the current image failed to start correctly so it can try another one.

107

I'd just do:

if (has_srst_extension) {
		(void)SBI_CALL2(SBI_EXT_ID_SRST, SBI_SRST_SYSTEM_RESET, reset_type,
			reset_reason);
}
(void)SBI_CALL0(SBI_SHUTDOWN, 0);

I would expect SBI_SRST_SYSTEM_RESET to not return, so if it does we can try SBI_SHUTDOWN instead.

[danq1222_gmail.com]

danq added inline comments.

sys/riscv/riscv/sbi.c

107

I'd just do:

if (has_srst_extension) {
		(void)SBI_CALL2(SBI_EXT_ID_SRST, SBI_SRST_SYSTEM_RESET, reset_type,
			reset_reason);
}
(void)SBI_CALL0(SBI_SHUTDOWN, 0);

I would expect SBI_SRST_SYSTEM_RESET to not return, so if it does we can try SBI_SHUTDOWN instead.

I see, thanks!

[danq1222_gmail.com]

Update files in line with feedback.

[danq1222_gmail.com]

Close comments.

sys/riscv/riscv/sbi.c

106

line removed.

107

Needs a newline, and should probably print the error code

Line removed.

[danq1222_gmail.com]

Mark additional inline comments.

sys/riscv/riscv/sbi.c

107

Needs a newline, and should probably print the error code

I am thinking of the reset_type as error code. Could this be OK?
Can't think of anything else.

printf("SBI System Reset Type Error %lu\n", reset_type);

Line removed.

[jrtc27]

sys/riscv/riscv/sbi.c

102

This line looks too long, if I can count correctly