cpu/hotplug: broken sibling thread hotplug

cpu/hotplug: broken sibling thread hotplug

[Igor Mammedov]

In case guest is booted with one CPU present and then later
a sibling CPU is hotplugged [1], it stays offline since SMT
is disabled.

Bisects to
 73d5e2b47264 ("cpu/hotplug: detect SMT disabled by BIOS")
which used __max_smt_threads to decide disabling SMT and in
case [1] only primary CPU thread is present hence SMT
is disabled.

Later bc2d8d262cba (cpu/hotplug: Fix SMT supported evaluation),
rewrites code path but evaluation criteria still depends on
sibling thread being present at boot time, so problem persist.

1) QEMU -smp 1,sockets=2,cores=1,threads=2 -monitor stdio ...
   # hotplug sibling thread
   (qemu) device_add qemu64-x86_64-cpu,socket-id=0,core-id=0,thread-id=1

I've failed to find reasoning behind statement:
  "
  cpu/hotplug: detect SMT disabled by BIOS

    If SMT is disabled in BIOS, the CPU code doesn't properly detect it.
  "

Question is 
 1: why cpu_smt_check_topology_early() at check_bugs()
wasn't sufficient to detect SMT disabled in BIOS and
 2: why side-effect of present at boot siblings were used
    to keep SMT enabled?

Following quick hack fixes the sibling issue but that's
effectively means reverting both above mentioned so we are
back to the original issue "If SMT is disabled in BIOS, ..."
which roots I weren't able to locate.

---
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 91d5c38..44df8cd 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -415,7 +415,7 @@ void __init cpu_smt_check_topology_early(void)
  */
 void __init cpu_smt_check_topology(void)
 {
-       if (!cpu_smt_available)
+       if (!topology_smt_supported())
                cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
 }
---

Re: cpu/hotplug: broken sibling thread hotplug

[Josh Poimboeuf]

On Thu, Jan 24, 2019 at 04:57:13PM +0100, Igor Mammedov wrote:
> In case guest is booted with one CPU present and then later
> a sibling CPU is hotplugged [1], it stays offline since SMT
> is disabled.
> 
> Bisects to
>  73d5e2b47264 ("cpu/hotplug: detect SMT disabled by BIOS")
> which used __max_smt_threads to decide disabling SMT and in
> case [1] only primary CPU thread is present hence SMT
> is disabled.
> 
> Later bc2d8d262cba (cpu/hotplug: Fix SMT supported evaluation),
> rewrites code path but evaluation criteria still depends on
> sibling thread being present at boot time, so problem persist.
> 
> 1) QEMU -smp 1,sockets=2,cores=1,threads=2 -monitor stdio ...
>    # hotplug sibling thread
>    (qemu) device_add qemu64-x86_64-cpu,socket-id=0,core-id=0,thread-id=1
> 
> I've failed to find reasoning behind statement:
>   "
>   cpu/hotplug: detect SMT disabled by BIOS
> 
>     If SMT is disabled in BIOS, the CPU code doesn't properly detect it.
>   "
> 
> Question is 
>  1: why cpu_smt_check_topology_early() at check_bugs()
> wasn't sufficient to detect SMT disabled in BIOS and
>  2: why side-effect of present at boot siblings were used
>     to keep SMT enabled?

Hi Igor,

Thanks for reporting this.

The original problems with SMT disabled in BIOS were:

1) /sys/devices/system/cpu/smt showed "on"; and

2) vmx_vm_init() was incorrectly showing the L1TF_MSG_SMT warning.

So 73d5e2b47264 ("cpu/hotplug: detect SMT disabled by BIOS") was
intended to fix that, by reporting SMT as permanently disabled, when the
BIOS has done so.

The problem is, I don't think there's a way to detect a difference
between the HW "SMT disabled by BIOS" case and the virt "Sibling not yet
plugged in" case.

I'd propose that we consider #1 above to *not* be a problem.  Because in
the virt case, it's possible that a sibling thread can be brought online
later.  So it makes sense to default with smt control "on" to allow for
that possibility.

The real problem is #2.  Which is a simple fix: change vmx_vm_init() to
query the current SMT state with sched_smt_active(), instead of looking
at the "control" sysfs value.

How about this patch?  It's just a revert of 73d5e2b47264 and
bc2d8d262cba, plus the 1-line vmx_vm_init() change.  If it looks ok to
you, I can clean it up and submit an official version.

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 453e66291e38..2a4db86c2780 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -70,7 +70,7 @@ void __init check_bugs(void)
 	 * identify_boot_cpu() initialized SMT support information, let the
 	 * core code know.
 	 */
-	cpu_smt_check_topology_early();
+	cpu_smt_check_topology();
 
 	if (!IS_ENABLED(CONFIG_SMP)) {
 		pr_info("CPU: ");
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 886ad6db0926..494f32ae6e6f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11102,7 +11102,7 @@ static int vmx_vm_init(struct kvm *kvm)
 			 * Warn upon starting the first VM in a potentially
 			 * insecure environment.
 			 */
-			if (cpu_smt_control == CPU_SMT_ENABLED)
+			if (sched_smt_active())
 				pr_warn_once(L1TF_MSG_SMT);
 			if (l1tf_vmx_mitigation == VMENTER_L1D_FLUSH_NEVER)
 				pr_warn_once(L1TF_MSG_L1D);
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 218df7f4d3e1..5041357d0297 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -180,12 +180,10 @@ enum cpuhp_smt_control {
 #if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
 extern enum cpuhp_smt_control cpu_smt_control;
 extern void cpu_smt_disable(bool force);
-extern void cpu_smt_check_topology_early(void);
 extern void cpu_smt_check_topology(void);
 #else
 # define cpu_smt_control		(CPU_SMT_ENABLED)
 static inline void cpu_smt_disable(bool force) { }
-static inline void cpu_smt_check_topology_early(void) { }
 static inline void cpu_smt_check_topology(void) { }
 #endif
 
diff --git a/kernel/cpu.c b/kernel/cpu.c
index d7c2e48ed10d..240fb4ab3f38 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -360,8 +360,6 @@ void __weak arch_smt_update(void) { }
 enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
 EXPORT_SYMBOL_GPL(cpu_smt_control);
 
-static bool cpu_smt_available __read_mostly;
-
 void __init cpu_smt_disable(bool force)
 {
 	if (cpu_smt_control == CPU_SMT_FORCE_DISABLED ||
@@ -378,25 +376,11 @@ void __init cpu_smt_disable(bool force)
 
 /*
  * The decision whether SMT is supported can only be done after the full
- * CPU identification. Called from architecture code before non boot CPUs
- * are brought up.
- */
-void __init cpu_smt_check_topology_early(void)
-{
-	if (!topology_smt_supported())
-		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
-}
-
-/*
- * If SMT was disabled by BIOS, detect it here, after the CPUs have been
- * brought online. This ensures the smt/l1tf sysfs entries are consistent
- * with reality. cpu_smt_available is set to true during the bringup of non
- * boot CPUs when a SMT sibling is detected. Note, this may overwrite
- * cpu_smt_control's previous setting.
+ * CPU identification. Called from architecture code.
  */
 void __init cpu_smt_check_topology(void)
 {
-	if (!cpu_smt_available)
+	if (!topology_smt_supported())
 		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
 }
 
@@ -409,18 +393,10 @@ early_param("nosmt", smt_cmdline_disable);
 
 static inline bool cpu_smt_allowed(unsigned int cpu)
 {
-	if (topology_is_primary_thread(cpu))
+	if (cpu_smt_control == CPU_SMT_ENABLED)
 		return true;
 
-	/*
-	 * If the CPU is not a 'primary' thread and the booted_once bit is
-	 * set then the processor has SMT support. Store this information
-	 * for the late check of SMT support in cpu_smt_check_topology().
-	 */
-	if (per_cpu(cpuhp_state, cpu).booted_once)
-		cpu_smt_available = true;
-
-	if (cpu_smt_control == CPU_SMT_ENABLED)
+	if (topology_is_primary_thread(cpu))
 		return true;
 
 	/*
diff --git a/kernel/smp.c b/kernel/smp.c
index d86eec5f51c1..084c8b3a2681 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -584,8 +584,6 @@ void __init smp_init(void)
 		num_nodes, (num_nodes > 1 ? "s" : ""),
 		num_cpus,  (num_cpus  > 1 ? "s" : ""));
 
-	/* Final decision about SMT support */
-	cpu_smt_check_topology();
 	/* Any cleanup work */
 	smp_cpus_done(setup_max_cpus);
 }

Re: cpu/hotplug: broken sibling thread hotplug

[Josh Poimboeuf]

On Fri, Jan 25, 2019 at 10:36:57AM -0600, Josh Poimboeuf wrote:
> How about this patch?  It's just a revert of 73d5e2b47264 and
> bc2d8d262cba, plus the 1-line vmx_vm_init() change.  If it looks ok to
> you, I can clean it up and submit an official version.

This one actually compiles...


diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 1de0f4170178..01874d54f4fd 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -71,7 +71,7 @@ void __init check_bugs(void)
 	 * identify_boot_cpu() initialized SMT support information, let the
 	 * core code know.
 	 */
-	cpu_smt_check_topology_early();
+	cpu_smt_check_topology();
 
 	if (!IS_ENABLED(CONFIG_SMP)) {
 		pr_info("CPU: ");
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index f6915f10e584..e2d6fa3c60d5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -26,6 +26,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/mm.h>
 #include <linux/sched.h>
+#include <linux/sched/smt.h>
 #include <linux/slab.h>
 #include <linux/tboot.h>
 #include <linux/trace_events.h>
@@ -6816,7 +6817,7 @@ static int vmx_vm_init(struct kvm *kvm)
 			 * Warn upon starting the first VM in a potentially
 			 * insecure environment.
 			 */
-			if (cpu_smt_control == CPU_SMT_ENABLED)
+			if (sched_smt_active())
 				pr_warn_once(L1TF_MSG_SMT);
 			if (l1tf_vmx_mitigation == VMENTER_L1D_FLUSH_NEVER)
 				pr_warn_once(L1TF_MSG_L1D);
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 218df7f4d3e1..5041357d0297 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -180,12 +180,10 @@ enum cpuhp_smt_control {
 #if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
 extern enum cpuhp_smt_control cpu_smt_control;
 extern void cpu_smt_disable(bool force);
-extern void cpu_smt_check_topology_early(void);
 extern void cpu_smt_check_topology(void);
 #else
 # define cpu_smt_control		(CPU_SMT_ENABLED)
 static inline void cpu_smt_disable(bool force) { }
-static inline void cpu_smt_check_topology_early(void) { }
 static inline void cpu_smt_check_topology(void) { }
 #endif
 
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 34e40ef2f6e0..05d359e4173a 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -387,8 +387,6 @@ void __weak arch_smt_update(void) { }
 enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
 EXPORT_SYMBOL_GPL(cpu_smt_control);
 
-static bool cpu_smt_available __read_mostly;
-
 void __init cpu_smt_disable(bool force)
 {
 	if (cpu_smt_control == CPU_SMT_FORCE_DISABLED ||
@@ -406,25 +404,11 @@ void __init cpu_smt_disable(bool force)
 
 /*
  * The decision whether SMT is supported can only be done after the full
- * CPU identification. Called from architecture code before non boot CPUs
- * are brought up.
- */
-void __init cpu_smt_check_topology_early(void)
-{
-	if (!topology_smt_supported())
-		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
-}
-
-/*
- * If SMT was disabled by BIOS, detect it here, after the CPUs have been
- * brought online. This ensures the smt/l1tf sysfs entries are consistent
- * with reality. cpu_smt_available is set to true during the bringup of non
- * boot CPUs when a SMT sibling is detected. Note, this may overwrite
- * cpu_smt_control's previous setting.
+ * CPU identification. Called from architecture code.
  */
 void __init cpu_smt_check_topology(void)
 {
-	if (!cpu_smt_available)
+	if (!topology_smt_supported())
 		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
 }
 
@@ -437,18 +421,10 @@ early_param("nosmt", smt_cmdline_disable);
 
 static inline bool cpu_smt_allowed(unsigned int cpu)
 {
-	if (topology_is_primary_thread(cpu))
+	if (cpu_smt_control == CPU_SMT_ENABLED)
 		return true;
 
-	/*
-	 * If the CPU is not a 'primary' thread and the booted_once bit is
-	 * set then the processor has SMT support. Store this information
-	 * for the late check of SMT support in cpu_smt_check_topology().
-	 */
-	if (per_cpu(cpuhp_state, cpu).booted_once)
-		cpu_smt_available = true;
-
-	if (cpu_smt_control == CPU_SMT_ENABLED)
+	if (topology_is_primary_thread(cpu))
 		return true;
 
 	/*
diff --git a/kernel/smp.c b/kernel/smp.c
index 163c451af42e..f4cf1b0bb3b8 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -584,8 +584,6 @@ void __init smp_init(void)
 		num_nodes, (num_nodes > 1 ? "s" : ""),
 		num_cpus,  (num_cpus  > 1 ? "s" : ""));
 
-	/* Final decision about SMT support */
-	cpu_smt_check_topology();
 	/* Any cleanup work */
 	smp_cpus_done(setup_max_cpus);
 }

Re: cpu/hotplug: broken sibling thread hotplug

[Igor Mammedov]

On Fri, 25 Jan 2019 11:02:03 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Fri, Jan 25, 2019 at 10:36:57AM -0600, Josh Poimboeuf wrote:
> > How about this patch?  It's just a revert of 73d5e2b47264 and
> > bc2d8d262cba, plus the 1-line vmx_vm_init() change.  If it looks ok to
> > you, I can clean it up and submit an official version.  
> 
> This one actually compiles...

Looks good to me,
(one small question below)

 
[...]
>  static inline bool cpu_smt_allowed(unsigned int cpu)
>  {
> -	if (topology_is_primary_thread(cpu))
> +	if (cpu_smt_control == CPU_SMT_ENABLED)
>  		return true;
>  
> -	/*
> -	 * If the CPU is not a 'primary' thread and the booted_once bit is
> -	 * set then the processor has SMT support. Store this information
> -	 * for the late check of SMT support in cpu_smt_check_topology().
> -	 */
> -	if (per_cpu(cpuhp_state, cpu).booted_once)
> -		cpu_smt_available = true;
> -
> -	if (cpu_smt_control == CPU_SMT_ENABLED)
> +	if (topology_is_primary_thread(cpu))
>  		return true;
why did you swap cpu_smt_control and topology_is_primary_thread checks?

>  
>  	/*

Re: cpu/hotplug: broken sibling thread hotplug

[Josh Poimboeuf]

On Mon, Jan 28, 2019 at 11:13:04AM +0100, Igor Mammedov wrote:
> On Fri, 25 Jan 2019 11:02:03 -0600
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Fri, Jan 25, 2019 at 10:36:57AM -0600, Josh Poimboeuf wrote:
> > > How about this patch?  It's just a revert of 73d5e2b47264 and
> > > bc2d8d262cba, plus the 1-line vmx_vm_init() change.  If it looks ok to
> > > you, I can clean it up and submit an official version.  
> > 
> > This one actually compiles...
> 
> Looks good to me,
> (one small question below)
> 
>  
> [...]
> >  static inline bool cpu_smt_allowed(unsigned int cpu)
> >  {
> > -	if (topology_is_primary_thread(cpu))
> > +	if (cpu_smt_control == CPU_SMT_ENABLED)
> >  		return true;
> >  
> > -	/*
> > -	 * If the CPU is not a 'primary' thread and the booted_once bit is
> > -	 * set then the processor has SMT support. Store this information
> > -	 * for the late check of SMT support in cpu_smt_check_topology().
> > -	 */
> > -	if (per_cpu(cpuhp_state, cpu).booted_once)
> > -		cpu_smt_available = true;
> > -
> > -	if (cpu_smt_control == CPU_SMT_ENABLED)
> > +	if (topology_is_primary_thread(cpu))
> >  		return true;
> why did you swap cpu_smt_control and topology_is_primary_thread checks?

That's just the revert of bc2d8d262cba5.

-- 
Josh

Re: cpu/hotplug: broken sibling thread hotplug

[Igor Mammedov]

On Mon, 28 Jan 2019 06:52:52 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Mon, Jan 28, 2019 at 11:13:04AM +0100, Igor Mammedov wrote:
> > On Fri, 25 Jan 2019 11:02:03 -0600
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >   
> > > On Fri, Jan 25, 2019 at 10:36:57AM -0600, Josh Poimboeuf wrote:  
> > > > How about this patch?  It's just a revert of 73d5e2b47264 and
> > > > bc2d8d262cba, plus the 1-line vmx_vm_init() change.  If it looks ok to
> > > > you, I can clean it up and submit an official version.    
> > > 
> > > This one actually compiles...  
> > 
> > Looks good to me,
> > (one small question below)
> > 
> >  
> > [...]  
> > >  static inline bool cpu_smt_allowed(unsigned int cpu)
> > >  {
> > > -	if (topology_is_primary_thread(cpu))
> > > +	if (cpu_smt_control == CPU_SMT_ENABLED)
> > >  		return true;
> > >  
> > > -	/*
> > > -	 * If the CPU is not a 'primary' thread and the booted_once bit is
> > > -	 * set then the processor has SMT support. Store this information
> > > -	 * for the late check of SMT support in cpu_smt_check_topology().
> > > -	 */
> > > -	if (per_cpu(cpuhp_state, cpu).booted_once)
> > > -		cpu_smt_available = true;
> > > -
> > > -	if (cpu_smt_control == CPU_SMT_ENABLED)
> > > +	if (topology_is_primary_thread(cpu))
> > >  		return true;  
> > why did you swap cpu_smt_control and topology_is_primary_thread checks?  
> 
> That's just the revert of bc2d8d262cba5.
ok. waiting for formal patch
pls, CC kvm@vger.kernel.org so Paolo could have a chance to review
(pick it up if Thomas is ok with approach)