Details

Reviewers

mhorne
kp
jhb

Group Reviewers

riscv

Commits

rG9bae4ce661c5: riscv: add SBI system reset extension

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

danq1222_gmail.com requested review of this revision.Jan 18 2021, 6:55 PM

danq1222_gmail.com created this revision.

Run git diff with -U999999 option.

Removed not required changes to sbi.c file.

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.

mhorne added a reviewer: riscv.Jan 19 2021, 3:19 PM

kp added a subscriber: kp.Jan 19 2021, 3:34 PM

kp added inline comments.

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 added a subscriber: jrtc27.Jan 19 2021, 4:15 PM

jrtc27 added inline comments.

sys/riscv/riscv/sbi.c
107	Needs a newline, and should probably print the error code
317	This is a bit ambiguous

jrtc27 added inline comments.Jan 19 2021, 4:26 PM

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 added inline comments.Jan 19 2021, 6:10 PM

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.

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 added inline comments.Jan 20 2021, 6:30 PM

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.

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!

Update files in line with feedback.

Close comments.

sys/riscv/riscv/sbi.c
106	line removed.
107	Needs a newline, and should probably print the error code Line removed.

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.

kp accepted this revision as: kp.Jan 21 2021, 8:05 AM

This revision is now accepted and ready to land.Jan 21 2021, 8:05 AM

jrtc27 added inline comments.Jan 21 2021, 3:01 PM

sys/riscv/riscv/sbi.c
102	This line looks too long, if I can count correctly

jhb accepted this revision as: jhb.Jan 21 2021, 6:44 PM

Closed by commit rG9bae4ce661c5: riscv: add SBI system reset extension (authored by danq1222_gmail.com, committed by mhorne). · Explain WhyJan 27 2021, 11:21 PM

This revision was automatically updated to reflect the committed changes.

mhorne added a commit: rG9bae4ce661c5: riscv: add SBI system reset extension.

riscv: add SBI system reset extension
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 83012

sys/riscv/include/sbi.h

sys/riscv/riscv/sbi.c

sys/riscv/riscv/vm_machdep.c

riscv: add SBI system reset extensionClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 83012

sys/riscv/include/sbi.h

sys/riscv/riscv/sbi.c

sys/riscv/riscv/vm_machdep.c

riscv: add SBI system reset extension
ClosedPublic
Actions

Revision Contents
Changeset List