Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Guilherme G. Piccoli]

Hi Greg / Thomas and all involved here. First, apologies for
necro-bumping this thread, but I'm working a backport of this patch to
kernel 4.15 (Ubuntu) and then I noticed we have it on stable, but only
in 4.19+.

Since the fixes tag presents an old commit (since ~3.19), I'm curious if
we have a special reason to not have it on long-term stables, like 4.9
or 4.14. It's a subtle portion of arch code, so I'm afraid I didn't see
something that prevents its backport for previous versions.

Thanks in advance,


Guilherme

Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Greg KH]

On Mon, Aug 17, 2020 at 12:36:25PM -0300, Guilherme G. Piccoli wrote:
> Hi Greg / Thomas and all involved here. First, apologies for
> necro-bumping this thread, but I'm working a backport of this patch to
> kernel 4.15 (Ubuntu) and then I noticed we have it on stable, but only
> in 4.19+.
> 
> Since the fixes tag presents an old commit (since ~3.19), I'm curious if
> we have a special reason to not have it on long-term stables, like 4.9
> or 4.14. It's a subtle portion of arch code, so I'm afraid I didn't see
> something that prevents its backport for previous versions.

What is the git commit id of this patch you are referring to, you didn't
provide any context here :(

thanks,

greg k-h

Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Guilherme G. Piccoli]

On 17/08/2020 13:21, Greg KH wrote:
> On Mon, Aug 17, 2020 at 12:36:25PM -0300, Guilherme G. Piccoli wrote:
>> Hi Greg / Thomas and all involved here. First, apologies for
>> necro-bumping this thread, but I'm working a backport of this patch to
>> kernel 4.15 (Ubuntu) and then I noticed we have it on stable, but only
>> in 4.19+.
>>
>> Since the fixes tag presents an old commit (since ~3.19), I'm curious if
>> we have a special reason to not have it on long-term stables, like 4.9
>> or 4.14. It's a subtle portion of arch code, so I'm afraid I didn't see
>> something that prevents its backport for previous versions.
> 
> What is the git commit id of this patch you are referring to, you didn't
> provide any context here :(
> 
> thanks,
> 
> greg k-h
> 

I'm sorry, I hoped the subject + thread would suffice heh

So, the mainline commit is: f8a8fe61fec8 ("x86/irq: Seperate unused
system vectors from spurious entry again") [0]. The backport to 4.19
stable tree has the following id: fc6975ee932b .

Thanks,


Guilherme


[0] http://git.kernel.org/linus/f8a8fe61fec8

Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Greg KH]

On Mon, Aug 17, 2020 at 01:43:14PM -0300, Guilherme G. Piccoli wrote:
> On 17/08/2020 13:21, Greg KH wrote:
> > On Mon, Aug 17, 2020 at 12:36:25PM -0300, Guilherme G. Piccoli wrote:
> >> Hi Greg / Thomas and all involved here. First, apologies for
> >> necro-bumping this thread, but I'm working a backport of this patch to
> >> kernel 4.15 (Ubuntu) and then I noticed we have it on stable, but only
> >> in 4.19+.
> >>
> >> Since the fixes tag presents an old commit (since ~3.19), I'm curious if
> >> we have a special reason to not have it on long-term stables, like 4.9
> >> or 4.14. It's a subtle portion of arch code, so I'm afraid I didn't see
> >> something that prevents its backport for previous versions.
> > 
> > What is the git commit id of this patch you are referring to, you didn't
> > provide any context here :(
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> I'm sorry, I hoped the subject + thread would suffice heh

There is no thread here :(

> So, the mainline commit is: f8a8fe61fec8 ("x86/irq: Seperate unused
> system vectors from spurious entry again") [0]. The backport to 4.19
> stable tree has the following id: fc6975ee932b .

Wow, over 1 1/2 years old, can you remember individual patches that long
ago?

Anyway, did you try to backport the patch to older kernels to see if it
was possible and could work?

If so, great, please feel free to submit it to the
stable@vger.kernel.org list and I will be glad to pick it up.

thanks,

greg k-h

Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Guilherme G. Piccoli]

On 17/08/2020 13:49, Greg KH wrote:
> [...]
>> I'm sorry, I hoped the subject + thread would suffice heh
> 
> There is no thread here :(
> 

Wow, that's odd. I've sent with In-Reply-To, I'd expect it'd get
threaded with the original message. Looking in lore archive [1], it
seems my first message wasn't threaded but the others were...apologies
for that, not sure what happened...


>> So, the mainline commit is: f8a8fe61fec8 ("x86/irq: Seperate unused
>> system vectors from spurious entry again") [0]. The backport to 4.19
>> stable tree has the following id: fc6975ee932b .
> 
> Wow, over 1 1/2 years old, can you remember individual patches that long
> ago?
> 
> Anyway, did you try to backport the patch to older kernels to see if it
> was possible and could work?
> 
> If so, great, please feel free to submit it to the
> stable@vger.kernel.org list and I will be glad to pick it up.
> 

I'm working on it, it is feasible. But I'm seeking here, in this
message, what is the reason it wasn't backported for pre-4.19 - is there
anything on these patches that is known to break old kernels? I'll
certainly submit my backported patch to stable 4.14 once I'm sure it's
working and there's no specific reason to not have it done before.

Thanks again!


[1]
https://lore.kernel.org/stable/a2788632-5690-932b-90de-14bd9cabedec@canonical.com/T/#mf0b26d5a7ff880cfd63bcda80136296275d3682e

Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Greg KH]

On Mon, Aug 17, 2020 at 01:59:00PM -0300, Guilherme G. Piccoli wrote:
> On 17/08/2020 13:49, Greg KH wrote:
> > [...]
> >> I'm sorry, I hoped the subject + thread would suffice heh
> > 
> > There is no thread here :(
> > 
> 
> Wow, that's odd. I've sent with In-Reply-To, I'd expect it'd get
> threaded with the original message. Looking in lore archive [1], it
> seems my first message wasn't threaded but the others were...apologies
> for that, not sure what happened...

reply to is fine, but how do you know what my email client has (hint,
not a copy of 1.5 years of history sitting around in it at the
moment...)  So there is no "thread" here as far as it is concerned...

Anyway, not a big deal, just properly quote emails in the future, that's
good to get used to no matter what :)

> >> So, the mainline commit is: f8a8fe61fec8 ("x86/irq: Seperate unused
> >> system vectors from spurious entry again") [0]. The backport to 4.19
> >> stable tree has the following id: fc6975ee932b .
> > 
> > Wow, over 1 1/2 years old, can you remember individual patches that long
> > ago?
> > 
> > Anyway, did you try to backport the patch to older kernels to see if it
> > was possible and could work?
> > 
> > If so, great, please feel free to submit it to the
> > stable@vger.kernel.org list and I will be glad to pick it up.
> > 
> 
> I'm working on it, it is feasible. But I'm seeking here, in this
> message, what is the reason it wasn't backported for pre-4.19

Try reading the stable mailing list archives, again, you are asking
about a patch 1.5 years ago.  I can't remember information about patches
sent _yesterday_ given the quantity we go through...

thanks,

greg k-h

Re: [PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Guilherme Piccoli]

On Mon, Aug 17, 2020 at 2:05 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Aug 17, 2020 at 01:59:00PM -0300, Guilherme G. Piccoli wrote:
> > On 17/08/2020 13:49, Greg KH wrote:
> > > [...]
> > >> I'm sorry, I hoped the subject + thread would suffice heh
> > >
> > > There is no thread here :(
> > >
> >
> > Wow, that's odd. I've sent with In-Reply-To, I'd expect it'd get
> > threaded with the original message. Looking in lore archive [1], it
> > seems my first message wasn't threaded but the others were...apologies
> > for that, not sure what happened...
>
> reply to is fine, but how do you know what my email client has (hint,
> not a copy of 1.5 years of history sitting around in it at the
> moment...)  So there is no "thread" here as far as it is concerned...
>
> Anyway, not a big deal, just properly quote emails in the future, that's
> good to get used to no matter what :)
>
Sure, will do - specially for super old threads like this.


> > >> So, the mainline commit is: f8a8fe61fec8 ("x86/irq: Seperate unused
> > >> system vectors from spurious entry again") [0]. The backport to 4.19
> > >> stable tree has the following id: fc6975ee932b .
> > >
> > > Wow, over 1 1/2 years old, can you remember individual patches that long
> > > ago?
> > >
> > > Anyway, did you try to backport the patch to older kernels to see if it
> > > was possible and could work?
> > >
> > > If so, great, please feel free to submit it to the
> > > stable@vger.kernel.org list and I will be glad to pick it up.
> > >
> >
> > I'm working on it, it is feasible. But I'm seeking here, in this
> > message, what is the reason it wasn't backported for pre-4.19
>
> Try reading the stable mailing list archives, again, you are asking
> about a patch 1.5 years ago.  I can't remember information about patches
> sent _yesterday_ given the quantity we go through...
>
> thanks,
>
> greg k-h

OK, thanks Greg. If Thomas or anybody involved here knows a reason to
not backport it to older kernels, please let me know - I'd really
appreciate that.
Cheers,


Guilherme

[PATCH 4.19 35/47] x86/irq: Seperate unused system vectors from spurious entry again

[Greg Kroah-Hartman]

From: Thomas Gleixner tglx@linutronix.de

commit f8a8fe61fec8006575699559ead88b0b833d5cad upstream

Quite some time ago the interrupt entry stubs for unused vectors in the
system vector range got removed and directly mapped to the spurious
interrupt vector entry point.

Sounds reasonable, but it's subtly broken. The spurious interrupt vector
entry point pushes vector number 0xFF on the stack which makes the whole
logic in __smp_spurious_interrupt() pointless.

As a consequence any spurious interrupt which comes from a vector != 0xFF
is treated as a real spurious interrupt (vector 0xFF) and not
acknowledged. That subsequently stalls all interrupt vectors of equal and
lower priority, which brings the system to a grinding halt.

This can happen because even on 64-bit the system vector space is not
guaranteed to be fully populated. A full compile time handling of the
unused vectors is not possible because quite some of them are conditonally
populated at runtime.

Bring the entry stubs back, which wastes 160 bytes if all stubs are unused,
but gains the proper handling back. There is no point to selectively spare
some of the stubs which are known at compile time as the required code in
the IDT management would be way larger and convoluted.

Do not route the spurious entries through common_interrupt and do_IRQ() as
the original code did. Route it to smp_spurious_interrupt() which evaluates
the vector number and acts accordingly now that the real vector numbers are
handed in.

Fixup the pr_warn so the actual spurious vector (0xff) is clearly
distiguished from the other vectors and also note for the vectored case
whether it was pending in the ISR or not.

 "Spurious APIC interrupt (vector 0xFF) on CPU#0, should never happen."
 "Spurious interrupt vector 0xed on CPU#1. Acked."
 "Spurious interrupt vector 0xee on CPU#1. Not pending!."

Fixes: 2414e021ac8d ("x86: Avoid building unused IRQ entry stubs")
Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Link: https://lkml.kernel.org/r/20190628111440.550568228@linutronix.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>


---
 arch/x86/entry/entry_32.S     |   24 ++++++++++++++++++++++++
 arch/x86/entry/entry_64.S     |   30 ++++++++++++++++++++++++++----
 arch/x86/include/asm/hw_irq.h |    2 ++
 arch/x86/kernel/apic/apic.c   |   33 ++++++++++++++++++++++-----------
 arch/x86/kernel/idt.c         |    3 ++-
 5 files changed, 76 insertions(+), 16 deletions(-)

--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -1098,6 +1098,30 @@ ENTRY(irq_entries_start)
     .endr
 END(irq_entries_start)
 
+#ifdef CONFIG_X86_LOCAL_APIC
+	.align 8
+ENTRY(spurious_entries_start)
+    vector=FIRST_SYSTEM_VECTOR
+    .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR)
+	pushl	$(~vector+0x80)			/* Note: always in signed byte range */
+    vector=vector+1
+	jmp	common_spurious
+	.align	8
+    .endr
+END(spurious_entries_start)
+
+common_spurious:
+	ASM_CLAC
+	addl	$-0x80, (%esp)			/* Adjust vector into the [-256, -1] range */
+	SAVE_ALL switch_stacks=1
+	ENCODE_FRAME_POINTER
+	TRACE_IRQS_OFF
+	movl	%esp, %eax
+	call	smp_spurious_interrupt
+	jmp	ret_from_intr
+ENDPROC(common_interrupt)
+#endif
+
 /*
  * the CPU automatically disables interrupts when executing an IRQ vector,
  * so IRQ-flags tracing has to follow that:
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -438,6 +438,18 @@ ENTRY(irq_entries_start)
     .endr
 END(irq_entries_start)
 
+	.align 8
+ENTRY(spurious_entries_start)
+    vector=FIRST_SYSTEM_VECTOR
+    .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR)
+	UNWIND_HINT_IRET_REGS
+	pushq	$(~vector+0x80)			/* Note: always in signed byte range */
+	jmp	common_spurious
+	.align	8
+	vector=vector+1
+    .endr
+END(spurious_entries_start)
+
 .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
 #ifdef CONFIG_DEBUG_ENTRY
 	pushq %rax
@@ -634,10 +646,20 @@ _ASM_NOKPROBE(interrupt_entry)
 
 /* Interrupt entry/exit. */
 
-	/*
-	 * The interrupt stubs push (~vector+0x80) onto the stack and
-	 * then jump to common_interrupt.
-	 */
+/*
+ * The interrupt stubs push (~vector+0x80) onto the stack and
+ * then jump to common_spurious/interrupt.
+ */
+common_spurious:
+	addq	$-0x80, (%rsp)			/* Adjust vector to [-256, -1] range */
+	call	interrupt_entry
+	UNWIND_HINT_REGS indirect=1
+	call	smp_spurious_interrupt		/* rdi points to pt_regs */
+	jmp	ret_from_intr
+END(common_spurious)
+_ASM_NOKPROBE(common_spurious)
+
+/* common_interrupt is a hotpath. Align it */
 	.p2align CONFIG_X86_L1_CACHE_SHIFT
 common_interrupt:
 	addq	$-0x80, (%rsp)			/* Adjust vector to [-256, -1] range */
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -150,6 +150,8 @@ extern char irq_entries_start[];
 #define trace_irq_entries_start irq_entries_start
 #endif
 
+extern char spurious_entries_start[];
+
 #define VECTOR_UNUSED		NULL
 #define VECTOR_SHUTDOWN		((void *)~0UL)
 #define VECTOR_RETRIGGERED	((void *)~1UL)
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2027,21 +2027,32 @@ __visible void __irq_entry smp_spurious_
 	entering_irq();
 	trace_spurious_apic_entry(vector);
 
+	inc_irq_stat(irq_spurious_count);
+
+	/*
+	 * If this is a spurious interrupt then do not acknowledge
+	 */
+	if (vector == SPURIOUS_APIC_VECTOR) {
+		/* See SDM vol 3 */
+		pr_info("Spurious APIC interrupt (vector 0xFF) on CPU#%d, should never happen.\n",
+			smp_processor_id());
+		goto out;
+	}
+
 	/*
-	 * Check if this really is a spurious interrupt and ACK it
-	 * if it is a vectored one.  Just in case...
-	 * Spurious interrupts should not be ACKed.
+	 * If it is a vectored one, verify it's set in the ISR. If set,
+	 * acknowledge it.
 	 */
 	v = apic_read(APIC_ISR + ((vector & ~0x1f) >> 1));
-	if (v & (1 << (vector & 0x1f)))
+	if (v & (1 << (vector & 0x1f))) {
+		pr_info("Spurious interrupt (vector 0x%02x) on CPU#%d. Acked\n",
+			vector, smp_processor_id());
 		ack_APIC_irq();
-
-	inc_irq_stat(irq_spurious_count);
-
-	/* see sw-dev-man vol 3, chapter 7.4.13.5 */
-	pr_info("spurious APIC interrupt through vector %02x on CPU#%d, "
-		"should never happen.\n", vector, smp_processor_id());
-
+	} else {
+		pr_info("Spurious interrupt (vector 0x%02x) on CPU#%d. Not pending!\n",
+			vector, smp_processor_id());
+	}
+out:
 	trace_spurious_apic_exit(vector);
 	exiting_irq();
 }
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -321,7 +321,8 @@ void __init idt_setup_apic_and_irq_gates
 #ifdef CONFIG_X86_LOCAL_APIC
 	for_each_clear_bit_from(i, system_vectors, NR_VECTORS) {
 		set_bit(i, system_vectors);
-		set_intr_gate(i, spurious_interrupt);
+		entry = spurious_entries_start + 8 * (i - FIRST_SYSTEM_VECTOR);
+		set_intr_gate(i, entry);
 	}
 #endif
 }