Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42434 )
Change subject: [WIP] x86/lapic: Set EXTINT on BSP only ......................................................................
Patch Set 5:
(6 comments)
Now that I looked deeper into it, the MultiProcessor Specification looks good, with quite nice pictures. One just needs to know which to look at (Virtual Wire Mode via Local APIC, I believe).
I still come to the same conclusion, though. We simply have to mask the interrupts on the APs (or leave their registers alone). Did anybody ever try if that fixes your issues?
https://review.coreboot.org/c/coreboot/+/42434/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42434/5//COMMIT_MSG@11 PS5, Line 11: configuration than coreboot does with this patch applied. Is this still up-to-date? I don't see how the current version could cause such. Also don't see why syslinux would expect anything wrt. to LAPICs.
https://review.coreboot.org/c/coreboot/+/42434/5/src/cpu/x86/lapic/lapic.c File src/cpu/x86/lapic/lapic.c:
https://review.coreboot.org/c/coreboot/+/42434/5/src/cpu/x86/lapic/lapic.c@7 PS5, Line 7: void do_lapic_init(void) What we (try to) do here is configuring `Virtual Wire Mode via Local APIC` as found in section 3.6.2.2 `MultiProcessor Specification 1.4`. I think that deserves a comment so that we don't need to read the whole spec everytime we want to understand it.
We could also mention what assumptions are made based on the spec:
* NMI is connected to LINT1 of all APICs. * INTR of the legacy master PIC is connected as edge-triggered to LINT0 of all APICs. * LINT0 needs to be configured as EXTINT because of the interrupt acknowlegde needs of the legacy PICs. * Interrupts on APs need to be disabled.
https://review.coreboot.org/c/coreboot/+/42434/5/src/cpu/x86/lapic/lapic.c@9 PS5, Line 9: lvt0_mask It's the same for LVT1, should we use it there too?
https://review.coreboot.org/c/coreboot/+/42434/5/src/cpu/x86/lapic/lapic.c@1... PS5, Line 13: uint32_t lvt0_val = LAPIC_LVT_REMOTE_IRR | LAPIC_SEND_PENDING; Maybe comment that this is the mask and value (well minus EXTINT but see below) used by `Example A-1 of the MultiProcessor Specification`.
https://review.coreboot.org/c/coreboot/+/42434/5/src/cpu/x86/lapic/lapic.c@3... PS5, Line 34: lvt0_val |= LAPIC_DELIVERY_MODE_FIXED | LAPIC_LVT_MASKED; I was much surprised when I learned what EXTINT means. It's about the way interrupts are acknowledged for the legacy PICs. I don't see how this part of the hardware is wired can differ for the APs.
Now it's hard to argue what values to set for disabled interrupts. But as we mimic the BSP configuration in all the other bits, why not these too? A mere
if (!boot_cpu()) lvt0_val |= LAPIC_LVT_MASKED;
would be more clear, IMHO.
Not setting anything on the APs at all might also work but I couldn't find any specification of the reset defaults for these registers.
https://review.coreboot.org/c/coreboot/+/42434/5/src/cpu/x86/lapic/lapic.c@4... PS5, Line 45: ); The spec (B.1) says
"The APs are in a halted condition with interrupts disabled. This means that the AP’s local APICs are passively monitoring the APIC bus and will react only to INIT or STARTUP interprocessor interrupts (IPIs)."
I think we should mask this one on APs too.