amd64: Set GSBASE before calling init_secondary()
ClosedPublic
Actions

Authored by markj on Jul 28 2021, 6:00 PM.

Details

Reviewers

Commits

rG4b136ef259ce: amd64: Set GS.base before calling init_secondary() on APs
rGe15374508381: amd64: Set MSR_KGSBASE to 0 during AP startup

Summary

The KMSAN instrumentation requires some thread-local storage to maintain
state for function parameters and return values. This buffer is checked
as part of each function's prologue. It is provided by the KMSAN
runtime, which looks up a pointer in the current thread's structure.

init_secondary() is instrumented, but it runs without a valid %gs
descriptor for some time, so the runtime cannot safely access curthread.
To work around this, load GSBASE before calling init_secondary(). Then
the runtime can check curthread == NULL and simply return a pointer to
some dummy storage.

Also change init_secondary() to initialize kernelGSBASE to 0. I don't
see any reason why we have to set it to the pcpu pointer.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Jul 28 2021, 6:00 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 28 2021, 6:00 PM

markj requested review of this revision.Jul 28 2021, 6:00 PM

Harbormaster completed remote builds in B40728: Diff 92895.Jul 28 2021, 6:00 PM

kib added inline comments.Jul 28 2021, 6:23 PM

sys/amd64/amd64/mpboot.S
264 ↗	(On Diff #92895)	I prefer to not put the conditional code there. I do not see a harm from doing this wrmsr unconditionally, and removing corresponding wrmsr from init_secondary().
sys/x86/x86/mp_x86.c
95 ↗	(On Diff #92895)	The var is amd64-specific, am I right? Why not put it into something amd64-specific then? At least, the definition. As I understand, there is no useful amd64-only header.

markj added inline comments.Jul 28 2021, 6:58 PM

sys/amd64/amd64/mpboot.S
264 ↗	(On Diff #92895)	Ok, but the wrmsr in init_secondary() cannot be removed. It seems that loading a selector into %gs causes the base register to be cleared, so we must write MSR_GSBASE again after calling lgdt().
sys/x86/x86/mp_x86.c
95 ↗	(On Diff #92895)	Yes, it is amd64-specific, I just grouped it with `bootSTK` and `bootAP`. Alternately, I can modify i386 to use `bootpcpu`, so amd64 and i386 maintain a bit of consistency. Which do you prefer?

kib added inline comments.Jul 28 2021, 7:47 PM

sys/amd64/amd64/mpboot.S
264 ↗	(On Diff #92895)	Ok, perhaps expand this comment to say what you noted above.
sys/x86/x86/mp_x86.c
95 ↗	(On Diff #92895)	For i386, I think the policy should be to not touching it unless required.