amd64 efi rt: handle #BP

[kib]

Summary

PR: 282860

[kib]

Add some debugging

[markj]

sys/amd64/amd64/trap.c

613 ↗

(On Diff #146797)

I think the check has to come before kdb_trap().

[kib]

Reorder checks. Explain why I did not wanted to do it initially.

[markj]

I suppose this is ok, but it'd be nice to understand the problem a bit better. Do all EFI calls trap, or just GetTime? Perhaps we should have a dedicated flag for catching this, instead of overloading TDF_NOFAULTING.

[markj]

Also, isn't loader susceptible to the same problem?

[imp]

Loader runs in efi context as a process. Any traps are handled by the FW in handlers they set up, no?

Also I don't think the loader gets time or reboots the system... though it does call the variable runtime routines

[kib]

Reformulating what @imp said, loader is executing in EFI boot services environment, while EFI RT is EFI runtime services client. The switch of the environment is supposed to pass ownership of the machine to OS, and also the rt code must be relocated. The later is known to be often buggy. This is why I implemented onfault handling for RT calls.

Executing INT3 from RT code seems to be a deliberate action, sort of assert(). I am not sure it is productive to try to debug the BIOS.

[kib]

Mark rt call region with TDP_EFIRT. It allows to not depend on the implementation detail of disabling page faults in the region.

[markj]

sys/amd64/amd64/trap.c

608 ↗

(On Diff #146809)

Maybe we should skip this path if frame->tf_rip >= KERNBASE.

[kib]

sys/amd64/amd64/trap.c

608 ↗

(On Diff #146809)

I am not sure. Right now the fact that RT mappings are all fit into userspace portion of VA is an implementation detail, and there is some desire to allow for (almost) arbitrary mappings. This would be a small obstacle, of course, but pcb_onfault is set very late, so tf_rip check would not improve the situation with kdb.

[imp]

I'd meant to click 'Accept Changes' but got busy, for what it's worth.

sys/amd64/amd64/trap.c

608 ↗

(On Diff #146809)

I think we should not skip mark's proposal. When we're calling EFIRT, we have the FW's map in place. When that map is in place, KERNBASE isn't necessarily meaningful (though I think that it might be in the current map). When we fully support LinuxBoot, for example, we'll need to have mappings that are effectively arbitrary (though to date, I've not seen any KERBASE or bigger addresses). I'm curious when we'd see it when TDP_EFIRT is set....

[kib]

sys/amd64/amd64/trap.c

608 ↗

(On Diff #146809)

Sorry, I do not quite follow the text. Initially it seems that you are pro adding the restriction for %rip to the check. But the later part sounds as vaguely debating it.

As I noted above, the difference between presence and absence of the check right now is for coverage of dozen or less instructions in the asm trampoline between setting pcb_onfault, and the moment when the transfer to the EFI code is really made (and returning/fault catching section).