Some fixes for kern.kstack_pages on arm64/riscv
ClosedPublic
Actions

Authored by kib on Oct 10 2023, 12:06 AM.

Details

Reviewers

andrew
markj
jhb
manu

Commits

rG6aa641b71d0d: arm64, risvc: warn about ignored kstack_pages for thread0
rG39cddbd7a07c: arm64: do not disable the kern.kstack_pages tunable on arm64
rGac63f7534d01: arm64, riscv: Use KSTACK_PAGES for the thread0 kstack size designator
rG4095e0bcb9e8: arm64 locore.S: fix typos

Summary

arm64 locore.S: fix typos

arm64, riscv: Use KSTACK_PAGES for the thread0 kstack size designator

instead of kstack_pages. Although it is correct right now to use
kstack_pages on amd64 since the kern.kstack_pages tunable is not
functional on arm64, this is too fragile and wrong on riscv.

arm64: do not disable the kern.kstack_pages tunable on arm64

Add a comment explaining what is not quite correct with arm64 and riscv.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib created this revision.Oct 10 2023, 12:06 AM

Herald added a reviewer: manu. · View Herald TranscriptOct 10 2023, 12:06 AM

Herald added subscribers: riscv, emaste, imp. · View Herald Transcript

kib requested review of this revision.Oct 10 2023, 12:06 AM

Ahh, it looks sane to me ;)

andrew added a subscriber: brooks.Oct 10 2023, 6:19 AM

andrew added inline comments.

sys/arm64/arm64/machdep.c
383 ↗	(On Diff #128479)	@brooks changed this in e1b610f76caa09bbc3910477da878ea5078c6569

kib marked an inline comment as done.Oct 10 2023, 10:39 AM

kib added inline comments.

sys/arm64/arm64/machdep.c
383 ↗	(On Diff #128479)	So include of opt_kstack_pages.h needs to be added.

include "opt_kstack_pages.h"

markj added inline comments.Oct 10 2023, 1:41 PM

sys/arm64/arm64/machdep.c
384 ↗	(On Diff #128498)	You might go further and make the stack size a parameter to init_proc0(). In particular, you could add a `kern_stack_size` field to struct arm64_bootparams and riscv_bootparams, set in locore. Then init_proc0() will make fewer assumptions about locore behaviour.
sys/kern/subr_param.c
175 ↗	(On Diff #128498)

andrew added inline comments.Oct 10 2023, 3:02 PM

sys/arm64/arm64/machdep.c
384 ↗	(On Diff #128498)	We could allocate the stack in initarm like we do on arm so it would be correctly sized.

jhb added inline comments.Oct 10 2023, 5:03 PM

sys/arm64/arm64/machdep.c
384 ↗	(On Diff #128498)	I think this is perhaps the best fix: allocate the real stack in C and switch to it in asm before calling mi_startup the way we do for other architectures.

kib added inline comments.Oct 11 2023, 3:06 AM

sys/arm64/arm64/machdep.c
384 ↗	(On Diff #128498)	I agree of course, but that would be a change of different size. Esp. taking into account some mitigations set up for arm64 stack. Proposed change is minimal and fixes the bug at hands. Next it could be improved from there.

markj accepted this revision.Oct 11 2023, 1:41 PM

markj added inline comments.

sys/arm64/arm64/machdep.c
384 ↗	(On Diff #128498)	IMO the real bug is that machdep.c and locore.S are independently hard-coding the initial stack size. My proposal makes locore.S be the source of truth, so a similar bug cannot arise again.

This revision is now accepted and ready to land.Oct 11 2023, 1:41 PM

kib marked an inline comment as done.Oct 11 2023, 6:33 PM

jhb added inline comments.Oct 11 2023, 6:49 PM

sys/arm64/arm64/machdep.c
384 ↗	(On Diff #128498)	I think it would indeed be better for locore to pass along its notion of the stack size, and that we should use that if we reuse locore's stack rather than allocating a new one. Probably that can be a followup to @kib's series though?

jhb accepted this revision.Oct 11 2023, 6:49 PM

I'd rather limit the use of KSTACK_PAGES as much as possible since it's prone to misuse, but this is fine if we're not going willing to fix the init_proc0 interface right now.