Details

Reviewers

markj
br
jhb

Commits

rS348836: Fix global pointer relaxations in the RISC-V kernel

Summary

The gp register is intended to used by the linker as another means of
performing relaxations, and should point to the small data section (.sdata).

Currently gp is being used as the pcpu pointer within the kernel, but the more
appropriate choice for this is the tp register, which is unused.

Swap existing usage of gp with tp within the kernel, and set up gp properly
at boot with the value of __global_pointer$ for all harts.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mhorne created this revision.Apr 12 2019, 11:06 PM

mhorne added inline comments.Apr 12 2019, 11:14 PM

sys/conf/ldscript.riscv
89	0x800 allows access for addresses up to 1 page from the beginning of `.sdata` by using signed 12-bit offsets on `__global_pointer$`.
sys/riscv/riscv/exception.S
53	gp will have a constant value within the kernel, therefore we only load/store it when coming from userspace.

Do we still need to save td in cpu_fork()?

sys/riscv/riscv/swtch.S
456	Hmm, why don't we need this? For existing uses of savectx() the pcpu pointer can be determined without using the TP value in the PCB, but I don't quite see why we shouldn't save it anyway.

In D19893#428086, @markj wrote:

Do we still need to save td in cpu_fork()?

I suppose you mean tp?

I'm not sure yet. I will take a stab at trying to replicate the case that code is supposedly solving before I can answer confidently.
I suspect the code in cpu_set_user_tls() could also be problematic as is since it directly manipulates the kernel's tp.

sys/riscv/riscv/swtch.S
456	This was not saving the pcpu pointer, since that was stored in `gp` until this patch. It was simply saving an unused kernel `tp`. However, the cost of saving both of these is almost nothing, so I agree that we might as well do it.

Save gp and tp during savectx().

In D19893#428086, @markj wrote:

Do we still need to save td in cpu_fork()?

Coming back to this question: it is still a little unclear to me why we save tp and what issue it is fixing.
I can't picture the scenario where the userland's tp value changes while it is forking, BUT this seems to
be a common enough that some other architectures have a similar check (e.g. i386 saves the gs register).

@br introduced the change in r337125, so perhaps he could speak more on it.

What I would like to do is run the test suite to ensure that removing it is a safe move, however, setting
up the native toolchain has proven to be a real pain, so I'd be willing to leave it as is for now but make a
note to myself to check it again in the future.

jhb added inline comments.Jun 3 2019, 6:19 PM

sys/riscv/riscv/swtch.S
456	There's no real reason to store the kernel TP and GP in the PCB. They are not part of the per-thread state. The kernel GP is a static constant in the kernel, and the kernel TP is effectively a per-CPU constant. However, saving it in the pcb makes it easier to use for the current kgdb. OTOH, I could easily fix the kgdb riscv backend to synthesize these registers from somewhere other than the pcb, but for now stashing them in the PCB might be the simplest thing to do. The user GP and TP live in td->td_frame. As Mitchell noted earlier, the TLS code needs to not adjust $tp directly in the kernel at all. It should only change the user $tp stored in td->td_frame.

Remove modifications of pcb->pcb_tp in riscv/vm_machdep.c.

Per @jhb's comments the GP and TP are not part of the per-thread state. Therefore we should avoid updating these values in the PCB where it is not necessary, and certainly should avoid directly manipulating the tp or gp registers.

mhorne added inline comments.Jun 5 2019, 7:41 PM

sys/riscv/riscv/swtch.S
456	Thanks John, your comment gives me a little more clarity. It seems that we probably should remove these extra members from `struct pcb`, though perhaps not as part of this patch. There appears to be no in-kernel consumers of these values so kgdb would be the only thing affected (unless there's something I am missing). As of right now this patch will still save the gp and tp to the pcb in `makectx()` and `savectx()`, but I wonder if it might break kgdb anyway since I am removing the lines that save/restore `tp` during `cpu_switch()`.
sys/riscv/riscv/vm_machdep.c
86	Do we actually need to save the FPU state here? How come?

jhb accepted this revision.Jun 5 2019, 7:51 PM

jhb added inline comments.

sys/riscv/riscv/exception.S
49	This comment should be before the 'la' as the 'sd' is saving the user GP.
84	I would maybe do this cleanup in a separate commit FWIW.
sys/riscv/riscv/swtch.S
456	kgdb knows the layout of struct pcb, so it might be simplest to leave struct pcb as-is for now. All kgdb can do is fetch initial register values of gp and tp from the saved pcb after a crash, so it's fine for savectx() to save them there.

This revision is now accepted and ready to land.Jun 5 2019, 7:51 PM

jhb added inline comments.Jun 5 2019, 7:53 PM

sys/riscv/riscv/vm_machdep.c
86	The comment might be worth preserving. Note that td1 is always curthread btw. The issue is that if you are forking, you want the new thread to inherit the FPU register state from the original thread. However, the FPU register state might be dirty and not saved to the PCB. On amd64 we call fpuexit() to force any dirty FPU register state to be flushed to the PCB before copying the PCB to the new state, so this XXX should stay, and it really should be fixed.

Address jhb's comments.

This revision now requires review to proceed.Jun 7 2019, 1:10 AM

markj accepted this revision.Jun 7 2019, 5:36 PM

This revision is now accepted and ready to land.Jun 7 2019, 5:36 PM

jhb accepted this revision.Jun 7 2019, 6:57 PM

Closed by commit rS348836: Fix global pointer relaxations in the RISC-V kernel (authored by mhorne). · Explain WhyJun 9 2019, 3:43 PM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptJun 9 2019, 3:43 PM

Fix global pointer relaxations in the RISC-V kernel
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 58276

sys/conf/ldscript.riscv

sys/riscv/include/asm.h

sys/riscv/include/pcpu.h

sys/riscv/riscv/exception.S

sys/riscv/riscv/locore.S

sys/riscv/riscv/machdep.c

sys/riscv/riscv/mp_machdep.c

sys/riscv/riscv/swtch.S

sys/riscv/riscv/vm_machdep.c

Fix global pointer relaxations in the RISC-V kernelClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 58276

sys/conf/ldscript.riscv

sys/riscv/include/asm.h

sys/riscv/include/pcpu.h

sys/riscv/riscv/exception.S

sys/riscv/riscv/locore.S

sys/riscv/riscv/machdep.c

sys/riscv/riscv/mp_machdep.c

sys/riscv/riscv/swtch.S

sys/riscv/riscv/vm_machdep.c

Fix global pointer relaxations in the RISC-V kernel
ClosedPublic
Actions

Revision Contents
Changeset List