On Thu, Aug 20, 2020 at 11:21:24AM -0700, Evan Green wrote:
I'm slightly unclear about whether interrupts are disabled at the core by this point or not. I followed native_cpu_disable() up to __cpu_disable(), up to take_cpu_down(). This is passed into a call to stop_machine_cpuslocked(), where interrupts get disabled at the core. So unless there's another path, it seems like interrupts are always disabled at the core by this point.
local_irq_disable() just does cli() which allows interrupts to trickle in to the IRR bits, and once you do sti() things would flow back for normal interrupt processing.
If interrupts are always disabled, then the comment above is a little
Disable interrupts is different from disabling LAPIC. Once you do the apic_soft_disable(), there is nothing flowing into the LAPIC except for INIT, NMI, SMI and SIPI messages.
This turns off the pipe for all other interrupts to enter LAPIC. Which is different from doing a cli().
I understand the distinction. I was mostly musing on the difference in behavior your change causes if this function is entered with interrupts enabled at the core (ie sti()). But I think it never is, so that thought is moot.
I could never repro the issue reliably on comet lake after Thomas' original fix. But I can still repro it easily on jasper lake. And this patch fixes the issue for me on that platform. Thanks for the fix.
Reviewed-by: Evan Green evgreen@chromium.org Tested-by: Evan Green evgreen@chromium.org
Thanks Evan for testing. I'll wait for thomas if he finds anything else that needs to be fixed and send a final v2 after fixing the typos and others identified by Randy.
Cheers, Ashok