Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Continue the WIP.
Handle NMIs and #MC.
Misc bug fixes.
Missed items:
- wrmsr_early_safe()
- save/restore pcb
- xen upcall ipi (?)
This should be functionally complete with the one minor missed XEN ipi handler code.
The only testing I did is the boot in simics. There is no hardware available in stores with FRED. It seems that bochs recently added the FRED support, but I did not tried it yet.
I had noted this elsewhere (Github #1748) is it really a good idea to pass around struct trapframe * so much? In particular I notice for normal interrupts it ends up in 3-4 stack frames before reaching intr_event_handle() which then dumps it into curthread->td_intr_frame. Most uses retrieve it from curthread->td_intr_frame.
If instead it was placed in curthread->td_intr_frame by the assembly-language bridge functions many places would avoid the hassle of touching it. This is particularly troublesome for cascading PICs which also need to correct curthread->td_intr_nesting_level before calling intr_event_handle(). This would significantly improve processor accounting by reducing the in-interrupt time where curthread->td_intr_nesting_level == 0. Since basically none of the cascading PICs deal with this issue processor accounting is rather poor on non-x86.
I'm afraid I don't know that much about FRED to make a proper review of the FRED bits, some comments about the Xen side. Let me know if you would like me to prepare a patch to switch Xen to use lapic_ipi_alloc()like the HyperV code does, that would remove the need for Xen specific checks in trap_fred_extint().
Regarding hardware, I've heard that ASUS NUC 16 with Panther Lake CPUs are or should be available very soon, that has FRED support and it's a good platforms for development. On the "tall" models you can fit a RS232 bracket and connect it to the motherboard using a DB9 connector to get serial output.
| sys/amd64/amd64/machdep.c | ||
|---|---|---|
| 209 | Nit: won't it be more natural for this to be a boolean, and then using a local int variable to fetch the tunable value below? ie: int opt_fred = 1;
TUNABLE_INT_FETCH("hw.fred", &opt_fred);
fred = !!opt_fred; | |
| sys/amd64/amd64/trap.c | ||
| 1378 | I could move the Xen vector to use lapic_ipi_alloc() instead, which will make all this less cumbersome to deal with? That would however change the priority of the Xen interrupt, which might have other side effects. On newer Xen versions we can allocate a different Xen interrupt per-CPU, and we no longer need a common vector reserved on all CPUs. This makes it easier to implement just like a normal interrupt. Any guest that has FRED support will also have the capacity to allocate per-CPU interrupts, maybe we could use that to avoid having to reserve a global interrupt when using FRED. Albeit we would still need to global interrupt for older Xen versions, IIRC AWS still has Xen versions that don't support per-CPU vector allocation. | |
| sys/x86/x86/local_apic.c | ||
| 606 | You can possibly set all the IDT vectors in the same conditional block, ahead of the lapic_enable(); call? However, instead of having to gate all calls to setidt() with if (!fred) { setid(...); } won't it be less intrusive to make setid(); a no-op when FRED is enabled, so that you don't have to adjust all callers? | |
| sys/amd64/amd64/machdep.c | ||
|---|---|---|
| 209 | All other controls for x86 are int. I might consider converting all of them as an independent change. | |
| sys/amd64/amd64/trap.c | ||
| 1378 | As I replied in the Xen review, please do this. | |
| sys/x86/x86/local_apic.c | ||
| 606 | I did what you suggested, moving the check for fred into amd64 setidt(). Formally this check is not needed at all, but I prefer to not touch idt for fred case. | |