Fix global pointer relaxations in the RISC-V kernel

[mhorne]

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.

[mhorne]

sys/conf/ldscript.riscv

89 ↗

(On Diff #56149)

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 ↗

(On Diff #56149)

gp will have a constant value within the kernel, therefore we only load/store it when coming from userspace.

[markj]

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

sys/riscv/riscv/swtch.S

456 ↗

(On Diff #56149)

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.

[mhorne]

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 ↗

(On Diff #56149)

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.

[mhorne]

Save gp and tp during savectx().

[mhorne]

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]

sys/riscv/riscv/swtch.S

456 ↗

(On Diff #56149)

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.

[mhorne]

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]

sys/riscv/riscv/swtch.S

456 ↗

(On Diff #56149)

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 ↗

(On Diff #58276)

Do we actually need to save the FPU state here? How come?

[jhb]

sys/riscv/riscv/exception.S

49 ↗

(On Diff #58276)

This comment should be before the 'la' as the 'sd' is saving the user GP.

84 ↗

(On Diff #58276)

I would maybe do this cleanup in a separate commit FWIW.

sys/riscv/riscv/swtch.S

456 ↗

(On Diff #56149)

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.

[jhb]

sys/riscv/riscv/vm_machdep.c

86 ↗

(On Diff #58276)

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.

[mhorne]

Address jhb's comments.