Page MenuHomeFreeBSD

riscv: add SBI system reset extension
ClosedPublic

Authored by danq1222_gmail.com on Jan 18 2021, 6:55 PM.

Details

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

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

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

Could use a comment here like other SBI extensions have.

sys/riscv/riscv/sbi.c
94

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 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 inline comments.
sys/riscv/riscv/sbi.c
107

Needs a newline, and should probably print the error code

319

This is a bit ambiguous

sys/riscv/riscv/sbi.c
94

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)

sys/riscv/riscv/sbi.c
94

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);
sys/riscv/riscv/sbi.c
94

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.

This revision is now accepted and ready to land.Jan 21 2021, 8:05 AM
sys/riscv/riscv/sbi.c
102

This line looks too long, if I can count correctly

This revision was automatically updated to reflect the committed changes.