[PATCH v2 0/2] x86/apic: reset LDR in clear_local_APIC

[PATCH v2 0/2] x86/apic: reset LDR in clear_local_APIC

[Bandan Das]

v2:
   1/2: clear out the bogus initialization in bigsmp_init_apic_ldr
   2/2: reword commit message as suggested by Thomas
v1 posted at https://lkml.org/lkml/2019/8/14/1

On a 32 bit RHEL6 guest with greater than 8 cpus, the
kdump kernel hangs when calibrating apic. This happens
because when apic initializes bigsmp, it also initializes LDR
even though it probably wouldn't be used.

When booting into kdump, KVM apic incorrectly reads the stale LDR
values from the guest while building the logical destination map
even for inactive vcpus. While KVM apic can be fixed to ignore apics
that haven't been enabled, a simple guest only change can be to
just clear out the LDR.

Bandan Das (2):
  x86/apic: Do not initialize LDR and DFR for bigsmp
  x86/apic: include the LDR when clearing out apic registers

 arch/x86/kernel/apic/apic.c      |  4 ++++
 arch/x86/kernel/apic/bigsmp_32.c | 24 ++----------------------
 2 files changed, 6 insertions(+), 22 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] x86/apic: Do not initialize LDR and DFR for bigsmp

[Bandan Das]

Legacy apic init uses bigsmp for > 8 smp systems.
In these cases, PhysFlat will invariably be used and there
is no point in initializing apic LDR and DFR. Furthermore,
calculate_ldr() helper function was incorrectly setting multiple
bits in the LDR.

This was discovered with a 32 bit KVM guest loading the kdump kernel.
Owing to the multiple bits being incorrectly set in the LDR, KVM hit a
buggy "if" condition in the kvm lapic implementation that creates the
logical destination map for vcpus - it ends up overwriting and
existing valid entry and as a result, apic calibration hangs in the
guest during kdump initialization.

Note that this change isn't intended to workaround the kvm lapic bug;
bigsmp should correctly stay away from initializing LDR.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kernel/apic/bigsmp_32.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/apic/bigsmp_32.c b/arch/x86/kernel/apic/bigsmp_32.c
index afee386ff711..caedd8d60d36 100644
--- a/arch/x86/kernel/apic/bigsmp_32.c
+++ b/arch/x86/kernel/apic/bigsmp_32.c
@@ -38,32 +38,12 @@ static int bigsmp_early_logical_apicid(int cpu)
 	return early_per_cpu(x86_cpu_to_apicid, cpu);
 }
 
-static inline unsigned long calculate_ldr(int cpu)
-{
-	unsigned long val, id;
-
-	val = apic_read(APIC_LDR) & ~APIC_LDR_MASK;
-	id = per_cpu(x86_bios_cpu_apicid, cpu);
-	val |= SET_APIC_LOGICAL_ID(id);
-
-	return val;
-}
-
 /*
- * Set up the logical destination ID.
- *
- * Intel recommends to set DFR, LDR and TPR before enabling
- * an APIC.  See e.g. "AP-388 82489DX User's Manual" (Intel
- * document number 292116).  So here it goes...
+ * bigsmp enables physical destination mode
+ * and doesn't use LDR and DFR
  */
 static void bigsmp_init_apic_ldr(void)
 {
-	unsigned long val;
-	int cpu = smp_processor_id();
-
-	apic_write(APIC_DFR, APIC_DFR_FLAT);
-	val = calculate_ldr(cpu);
-	apic_write(APIC_LDR, val);
 }
 
 static void bigsmp_setup_apic_routing(void)
-- 
2.20.1

[tip: x86/urgent] x86/apic: Do not initialize LDR and DFR for bigsmp

[tip-bot2 for Bandan Das]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     bae3a8d3308ee69a7dbdf145911b18dfda8ade0d
Gitweb:        https://git.kernel.org/tip/bae3a8d3308ee69a7dbdf145911b18dfda8ade0d
Author:        Bandan Das <bsd@redhat.com>
AuthorDate:    Mon, 26 Aug 2019 06:15:12 -04:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 26 Aug 2019 20:00:56 +02:00

x86/apic: Do not initialize LDR and DFR for bigsmp

Legacy apic init uses bigsmp for smp systems with 8 and more CPUs. The
bigsmp APIC implementation uses physical destination mode, but it
nevertheless initializes LDR and DFR. The LDR even ends up incorrectly with
multiple bit being set.

This does not cause a functional problem because LDR and DFR are ignored
when physical destination mode is active, but it triggered a problem on a
32-bit KVM guest which jumps into a kdump kernel.

The multiple bits set unearthed a bug in the KVM APIC implementation. The
code which creates the logical destination map for VCPUs ignores the
disabled state of the APIC and ends up overwriting an existing valid entry
and as a result, APIC calibration hangs in the guest during kdump
initialization.

Remove the bogus LDR/DFR initialization.

This is not intended to work around the KVM APIC bug. The LDR/DFR
ininitalization is wrong on its own.

The issue goes back into the pre git history. The fixes tag is the commit
in the bitkeeper import which introduced bigsmp support in 2003.

  git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Fixes: db7b9e9f26b8 ("[PATCH] Clustered APIC setup for >8 CPU systems")
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Bandan Das <bsd@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20190826101513.5080-2-bsd@redhat.com


---
 arch/x86/kernel/apic/bigsmp_32.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/apic/bigsmp_32.c b/arch/x86/kernel/apic/bigsmp_32.c
index afee386..caedd8d 100644
--- a/arch/x86/kernel/apic/bigsmp_32.c
+++ b/arch/x86/kernel/apic/bigsmp_32.c
@@ -38,32 +38,12 @@ static int bigsmp_early_logical_apicid(int cpu)
 	return early_per_cpu(x86_cpu_to_apicid, cpu);
 }
 
-static inline unsigned long calculate_ldr(int cpu)
-{
-	unsigned long val, id;
-
-	val = apic_read(APIC_LDR) & ~APIC_LDR_MASK;
-	id = per_cpu(x86_bios_cpu_apicid, cpu);
-	val |= SET_APIC_LOGICAL_ID(id);
-
-	return val;
-}
-
 /*
- * Set up the logical destination ID.
- *
- * Intel recommends to set DFR, LDR and TPR before enabling
- * an APIC.  See e.g. "AP-388 82489DX User's Manual" (Intel
- * document number 292116).  So here it goes...
+ * bigsmp enables physical destination mode
+ * and doesn't use LDR and DFR
  */
 static void bigsmp_init_apic_ldr(void)
 {
-	unsigned long val;
-	int cpu = smp_processor_id();
-
-	apic_write(APIC_DFR, APIC_DFR_FLAT);
-	val = calculate_ldr(cpu);
-	apic_write(APIC_LDR, val);
 }
 
 static void bigsmp_setup_apic_routing(void)

[PATCH v2 2/2] x86/apic: include the LDR when clearing out apic registers

[Bandan Das]

Although apic initialization will typically clear out the LDR before
setting it, the apic cleanup code should reset the LDR.

This was discovered with a 32 bit kvm guest loading the kdump kernel.
Stale bits in the LDR exposed a bug in the kvm lapic code that creates
logical destination maps for vcpus. If multiple bits are set, kvm
could potentially overwrite a valid logical destination with an
invalid one.

Note that this fix isn't intended to paper over the kvm lapic bug;
clear_local_APIC() should correctly clear out any set bits in the LDR
when resetting apic registers.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kernel/apic/apic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index aa5495d0f478..e75f3782b915 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1179,6 +1179,10 @@ void clear_local_APIC(void)
 	apic_write(APIC_LVT0, v | APIC_LVT_MASKED);
 	v = apic_read(APIC_LVT1);
 	apic_write(APIC_LVT1, v | APIC_LVT_MASKED);
+	if (!x2apic_enabled) {
+		v = apic_read(APIC_LDR) & ~APIC_LDR_MASK;
+		apic_write(APIC_LDR, v);
+	}
 	if (maxlvt >= 4) {
 		v = apic_read(APIC_LVTPC);
 		apic_write(APIC_LVTPC, v | APIC_LVT_MASKED);
-- 
2.20.1

[tip: x86/urgent] x86/apic: Include the LDR when clearing out APIC registers

[tip-bot2 for Bandan Das]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     558682b5291937a70748d36fd9ba757fb25b99ae
Gitweb:        https://git.kernel.org/tip/558682b5291937a70748d36fd9ba757fb25b99ae
Author:        Bandan Das <bsd@redhat.com>
AuthorDate:    Mon, 26 Aug 2019 06:15:13 -04:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 26 Aug 2019 20:00:57 +02:00

x86/apic: Include the LDR when clearing out APIC registers

Although APIC initialization will typically clear out the LDR before
setting it, the APIC cleanup code should reset the LDR.

This was discovered with a 32-bit KVM guest jumping into a kdump
kernel. The stale bits in the LDR triggered a bug in the KVM APIC
implementation which caused the destination mapping for VCPUs to be
corrupted.

Note that this isn't intended to paper over the KVM APIC bug. The kernel
has to clear the LDR when resetting the APIC registers except when X2APIC
is enabled.

This lacks a Fixes tag because missing to clear LDR goes way back into pre
git history.

[ tglx: Made x2apic_enabled a function call as required ]

Signed-off-by: Bandan Das <bsd@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20190826101513.5080-3-bsd@redhat.com

---
 arch/x86/kernel/apic/apic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index aa5495d..dba2828 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1179,6 +1179,10 @@ void clear_local_APIC(void)
 	apic_write(APIC_LVT0, v | APIC_LVT_MASKED);
 	v = apic_read(APIC_LVT1);
 	apic_write(APIC_LVT1, v | APIC_LVT_MASKED);
+	if (!x2apic_enabled()) {
+		v = apic_read(APIC_LDR) & ~APIC_LDR_MASK;
+		apic_write(APIC_LDR, v);
+	}
 	if (maxlvt >= 4) {
 		v = apic_read(APIC_LVTPC);
 		apic_write(APIC_LVTPC, v | APIC_LVT_MASKED);