Re: [PATCH v2 05/12] x86: rework arch_local_irq_restore() to not use popf
From: Mark Rutland <mark.rutland@arm.com>
Date: 2020-12-09 18:16:26
Also in:
lkml, xen-devel
Subsystem:
the rest, x86 architecture (32-bit and 64-bit) · Maintainers:
Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen
On Fri, Nov 20, 2020 at 12:59:43PM +0100, Peter Zijlstra wrote:
On Fri, Nov 20, 2020 at 12:46:23PM +0100, Juergen Gross wrote:quoted
+static __always_inline void arch_local_irq_restore(unsigned long flags) +{ + if (!arch_irqs_disabled_flags(flags)) + arch_local_irq_enable(); +}If someone were to write horrible code like: local_irq_disable(); local_irq_save(flags); local_irq_enable(); local_irq_restore(flags); we'd be up some creek without a paddle... now I don't _think_ we have genius code like that, but I'd feel saver if we can haz an assertion in there somewhere...
I've cobbled that together locally (i'll post it momentarily), and gave it a spin on both arm64 and x86, whereupon it exploded at boot time on x86. In arch/x86/kernel/apic/io_apic.c's timer_irq_works() we do: local_irq_save(flags); local_irq_enable(); [ trigger an IRQ here ] local_irq_restore(flags); ... and in check_timer() we call that a number of times after either a local_irq_save() or local_irq_disable(), eventually trailing with a local_irq_disable() that will balance things up before calling local_irq_restore(). I guess that timer_irq_works() should instead do: local_irq_save(flags); local_irq_enable(); ... local_irq_disable(); local_irq_restore(flags); ... assuming we consider that legitimate? With that, and all the calls to local_irq_disable() in check_timer() removed (diff below) I get a clean boot under QEMU with the assertion hacked in and DEBUG_LOCKDEP enabled. Thanks Mark. ---->8----
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 7b3c7e0d4a09..e79e665a3aeb 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c@@ -1631,6 +1631,7 @@ static int __init timer_irq_works(void) else delay_without_tsc(); + local_irq_disable(); local_irq_restore(flags); /*
@@ -2191,7 +2192,6 @@ static inline void __init check_timer(void) goto out; } panic_if_irq_remap("timer doesn't work through Interrupt-remapped IO-APIC"); - local_irq_disable(); clear_IO_APIC_pin(apic1, pin1); if (!no_pin1) apic_printk(APIC_QUIET, KERN_ERR "..MP-BIOS bug: "
@@ -2215,7 +2215,6 @@ static inline void __init check_timer(void) /* * Cleanup, just in case ... */ - local_irq_disable(); legacy_pic->mask(0); clear_IO_APIC_pin(apic2, pin2); apic_printk(APIC_QUIET, KERN_INFO "....... failed.\n");
@@ -2232,7 +2231,6 @@ static inline void __init check_timer(void) apic_printk(APIC_QUIET, KERN_INFO "..... works.\n"); goto out; } - local_irq_disable(); legacy_pic->mask(0); apic_write(APIC_LVT0, APIC_LVT_MASKED | APIC_DM_FIXED | cfg->vector); apic_printk(APIC_QUIET, KERN_INFO "..... failed.\n");
@@ -2251,7 +2249,6 @@ static inline void __init check_timer(void) apic_printk(APIC_QUIET, KERN_INFO "..... works.\n"); goto out; } - local_irq_disable(); apic_printk(APIC_QUIET, KERN_INFO "..... failed :(.\n"); if (apic_is_x2apic_enabled()) apic_printk(APIC_QUIET, KERN_INFO
_______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization