[Xen-devel] [PATCH 0/3] x86: (largely) LAPIC related cleanup

Jan Beulich posted 3 patches 4 years, 7 months ago
Only 0 patches received!
[Xen-devel] [PATCH 0/3] x86: (largely) LAPIC related cleanup
Posted by Jan Beulich 4 years, 7 months ago
The latter two patches are derived from Linux ones, which caught
my attention. The first one is simply some extra code reduction
potential I noticed while evaluating whether those Linux changes
are applicable to our tree.

1: x86: drop CONFIG_X86_MCE_THERMAL
2: x86/apic: include the LDR when clearing out APIC registers
3: x86/apic: do not initialize LDR and DFR for bigsmp

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/3] x86: drop CONFIG_X86_MCE_THERMAL
Posted by Jan Beulich 4 years, 7 months ago
There's no point having this if it's not exposed through Kconfig.

Take the liberty and also drop an unnecessary "return" in context.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -189,19 +189,15 @@ void clear_local_APIC(void)
         v = apic_read(APIC_LVTPC);
         apic_write(APIC_LVTPC, v | APIC_LVT_MASKED);
     }
-
-/* lets not touch this if we didn't frob it */
-#ifdef CONFIG_X86_MCE_THERMAL
     if (maxlvt >= 5) {
         v = apic_read(APIC_LVTTHMR);
         apic_write(APIC_LVTTHMR, v | APIC_LVT_MASKED);
     }
-#endif
-
     if (maxlvt >= 6) {
         v = apic_read(APIC_CMCI);
         apic_write(APIC_CMCI, v | APIC_LVT_MASKED);
     }
+
     /*
      * Clean APIC state for other OSs:
      */
@@ -212,11 +208,8 @@ void clear_local_APIC(void)
         apic_write(APIC_LVTERR, APIC_LVT_MASKED);
     if (maxlvt >= 4)
         apic_write(APIC_LVTPC, APIC_LVT_MASKED);
-
-#ifdef CONFIG_X86_MCE_THERMAL
     if (maxlvt >= 5)
         apic_write(APIC_LVTTHMR, APIC_LVT_MASKED);
-#endif
     if (maxlvt >= 6)
         apic_write(APIC_CMCI, APIC_LVT_MASKED);
 
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -51,7 +51,6 @@ bool __read_mostly lmce_support;
 #define INTEL_SRAR_DATA_LOAD	0x134
 #define INTEL_SRAR_INSTR_FETCH	0x150
 
-#ifdef CONFIG_X86_MCE_THERMAL
 #define MCE_RING                0x1
 static DEFINE_PER_CPU(int, last_state);
 
@@ -174,9 +173,7 @@ static void intel_init_thermal(struct cp
     if ( opt_cpu_info )
         printk(KERN_INFO "CPU%u: Thermal monitoring enabled (%s)\n",
                cpu, tm2 ? "TM2" : "TM1");
-    return;
 }
-#endif /* CONFIG_X86_MCE_THERMAL */
 
 /* Intel MCE handler */
 static inline void intel_get_extended_msr(struct mcinfo_extended *ext, u32 msr)
@@ -941,9 +938,8 @@ enum mcheck_type intel_mcheck_init(struc
     intel_init_mce();
 
     intel_init_cmci(c);
-#ifdef CONFIG_X86_MCE_THERMAL
+
     intel_init_thermal(c);
-#endif
 
     return mcheck_intel;
 }
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -21,7 +21,6 @@
 
 #define CONFIG_X86_PM_TIMER 1
 #define CONFIG_HPET_TIMER 1
-#define CONFIG_X86_MCE_THERMAL 1
 #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
 #define CONFIG_DISCONTIGMEM 1
 #define CONFIG_NUMA_EMU 1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/3] x86: drop CONFIG_X86_MCE_THERMAL
Posted by Andrew Cooper 4 years, 7 months ago
On 06/09/2019 15:00, Jan Beulich wrote:
> There's no point having this if it's not exposed through Kconfig.
>
> Take the liberty and also drop an unnecessary "return" in context.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/3] x86/apic: include the LDR when clearing out APIC registers
Posted by Jan Beulich 4 years, 7 months ago
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.

Signed-off-by: Bandan Das <bsd@redhat.com>
[Linux commit 558682b5291937a70748d36fd9ba757fb25b99ae]
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -212,6 +212,10 @@ void clear_local_APIC(void)
         apic_write(APIC_LVTTHMR, APIC_LVT_MASKED);
     if (maxlvt >= 6)
         apic_write(APIC_CMCI, APIC_LVT_MASKED);
+    if (!x2apic_enabled) {
+        v = apic_read(APIC_LDR) & ~APIC_LDR_MASK;
+        apic_write(APIC_LDR, v);
+    }
 
     if (maxlvt > 3)        /* Due to Pentium errata 3AP and 11AP. */
         apic_write(APIC_ESR, 0);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/3] x86/apic: include the LDR when clearing out APIC registers
Posted by Andrew Cooper 4 years, 7 months ago
On 06/09/2019 15:01, Jan Beulich wrote:
> 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.
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> [Linux commit 558682b5291937a70748d36fd9ba757fb25b99ae]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/3] x86/apic: include the LDR when clearing out APIC registers
Posted by Jan Beulich 4 years, 7 months ago
On 06.09.2019 16:01, Jan Beulich wrote:
> 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.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> [Linux commit 558682b5291937a70748d36fd9ba757fb25b99ae]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

FTR, commit 950b07c14e8c59444e2359f15fd70ed5112e11a0 reverts
this in Linux, but only for breaking offlining and then re-
onlining the BSP. Since we don't support this (yet), I don't
see a reason for us to revert as well.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 3/3] x86/apic: do not initialize LDR and DFR for bigsmp
Posted by Jan Beulich 4 years, 7 months ago
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.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Bandan Das <bsd@redhat.com>
[Linux commit bae3a8d3308ee69a7dbdf145911b18dfda8ade0d]

Drop init_apic_ldr_x2apic_phys() at the same time.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/genapic/delivery.c
+++ b/xen/arch/x86/genapic/delivery.c
@@ -40,11 +40,7 @@ unsigned int cpu_mask_to_apicid_flat(con
 
 void init_apic_ldr_phys(void)
 {
-	unsigned long val;
-	apic_write(APIC_DFR, APIC_DFR_FLAT);
-	/* A dummy logical ID should be fine. We only deliver in phys mode. */
-	val = apic_read(APIC_LDR) & ~APIC_LDR_MASK;
-	apic_write(APIC_LDR, val);
+	/* We only deliver in phys mode - no setup needed. */
 }
 
 void __init clustered_apic_check_phys(void)
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -38,10 +38,6 @@ static inline u32 x2apic_cluster(unsigne
     return per_cpu(cpu_2_logical_apicid, cpu) >> 16;
 }
 
-static void init_apic_ldr_x2apic_phys(void)
-{
-}
-
 static void init_apic_ldr_x2apic_cluster(void)
 {
     unsigned int cpu, this_cpu = smp_processor_id();
@@ -167,7 +163,7 @@ static const struct genapic __initconstr
     APIC_INIT("x2apic_phys", NULL),
     .int_delivery_mode = dest_Fixed,
     .int_dest_mode = 0 /* physical delivery */,
-    .init_apic_ldr = init_apic_ldr_x2apic_phys,
+    .init_apic_ldr = init_apic_ldr_phys,
     .clustered_apic_check = clustered_apic_check_x2apic,
     .vector_allocation_cpumask = vector_allocation_cpumask_phys,
     .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/3] x86/apic: do not initialize LDR and DFR for bigsmp
Posted by Andrew Cooper 4 years, 7 months ago
On 06/09/2019 15:01, Jan Beulich wrote:
> 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.
>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> [Linux commit bae3a8d3308ee69a7dbdf145911b18dfda8ade0d]
>
> Drop init_apic_ldr_x2apic_phys() at the same time.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel