[PATCH] smp: force all cpu to boot once under maxcpus option

[PATCH] smp: force all cpu to boot once under maxcpus option

[Pingfan Liu]

On x86 it's required to boot all logical CPUs at least once so that the
init code can get a chance to set CR4.MCE on each CPU. Otherwise, a
broadacasted MCE observing CR4.MCE=0b on any core will shutdown the
machine.

The option 'nosmt' has already complied with the above rule. In the case of
maxcpus, the initialization of capped out cpus may be deferred indefinitely
until a user brings them up. This exposes the machine under the risk of
sudden shutdown indefinitely.

Minimize the risk window by initializing all cpus at boot time.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Mukesh Ojha <mojha@codeaurora.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: linux-kernel@vger.kernel.org
---
 include/linux/smp.h |  1 +
 kernel/cpu.c        | 20 ++++++++++++++++++--
 kernel/smp.c        |  4 ++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index a56f08f..9d2c692 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -130,6 +130,7 @@ extern void __init setup_nr_cpu_ids(void);
 extern void __init smp_init(void);
 
 extern int __boot_cpu_id;
+extern bool smp_boot_done;
 
 static inline int get_boot_cpu_id(void)
 {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index ef1c565..ab19dc8 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -439,6 +439,21 @@ static inline bool cpu_smt_allowed(unsigned int cpu)
 static inline bool cpu_smt_allowed(unsigned int cpu) { return true; }
 #endif
 
+static inline bool maxcpus_allowed(unsigned int cpu)
+{
+	/* maxcpus only takes effect during system bootup */
+	if (smp_boot_done)
+		return true;
+	if (num_online_cpus() < setup_max_cpus)
+		return true;
+	/*
+	 * maxcpus should allow cpu to set CR4.MCE asap, otherwise the set may
+	 * be deferred indefinitely.
+	 */
+	if (!per_cpu(cpuhp_state, cpu).booted_once)
+		return true;
+}
+
 static inline enum cpuhp_state
 cpuhp_set_state(struct cpuhp_cpu_state *st, enum cpuhp_state target)
 {
@@ -525,8 +540,9 @@ static int bringup_wait_for_ap(unsigned int cpu)
 	 * CPU marked itself as booted_once in cpu_notify_starting() so the
 	 * cpu_smt_allowed() check will now return false if this is not the
 	 * primary sibling.
+	 * In case of maxcpus, the capped out cpus comply with the same rule.
 	 */
-	if (!cpu_smt_allowed(cpu))
+	if (!cpu_smt_allowed(cpu) || !maxcpus_allowed(cpu))
 		return -ECANCELED;
 
 	if (st->target <= CPUHP_AP_ONLINE_IDLE)
@@ -1177,7 +1193,7 @@ static int do_cpu_up(unsigned int cpu, enum cpuhp_state target)
 		err = -EBUSY;
 		goto out;
 	}
-	if (!cpu_smt_allowed(cpu)) {
+	if (!cpu_smt_allowed(cpu) || !maxcpus_allowed(cpu)) {
 		err = -EPERM;
 		goto out;
 	}
diff --git a/kernel/smp.c b/kernel/smp.c
index d155374..a5f82d53 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -560,6 +560,9 @@ void __init setup_nr_cpu_ids(void)
 	nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
 }
 
+bool smp_boot_done __read_mostly;
+EXPORT_SYMBOL(smp_boot_done);
+
 /* Called by boot processor to activate the rest. */
 void __init smp_init(void)
 {
@@ -587,6 +590,7 @@ void __init smp_init(void)
 
 	/* Any cleanup work */
 	smp_cpus_done(setup_max_cpus);
+	smp_boot_done = true;
 }
 
 /*
-- 
2.7.5

Re: [PATCH] smp: force all cpu to boot once under maxcpus option

[Thomas Gleixner]

On Wed, 10 Jul 2019, Pingfan Liu wrote:
>  
> +static inline bool maxcpus_allowed(unsigned int cpu)
> +{
> +	/* maxcpus only takes effect during system bootup */
> +	if (smp_boot_done)
> +		return true;
> +	if (num_online_cpus() < setup_max_cpus)
> +		return true;
> +	/*
> +	 * maxcpus should allow cpu to set CR4.MCE asap, otherwise the set may
> +	 * be deferred indefinitely.
> +	 */
> +	if (!per_cpu(cpuhp_state, cpu).booted_once)
> +		return true;

As this is a x86 only issue, you cannot inflict this magic on every
architecture.

Aside of that this does not solve the problem at all because smp_init()
still does:

        for_each_present_cpu(cpu) {
                if (num_online_cpus() >= setup_max_cpus)
                        break;
                if (!cpu_online(cpu))
                        cpu_up(cpu);
        }

So the remaining CPUs are not onlined at all.

Thanks,

	tglx

Re: [PATCH] smp: force all cpu to boot once under maxcpus option

[Pingfan Liu]

On Mon, Jul 22, 2019 at 11:41:29AM +0200, Thomas Gleixner wrote:
> On Wed, 10 Jul 2019, Pingfan Liu wrote:
> >  
> > +static inline bool maxcpus_allowed(unsigned int cpu)
> > +{
> > +	/* maxcpus only takes effect during system bootup */
> > +	if (smp_boot_done)
> > +		return true;
> > +	if (num_online_cpus() < setup_max_cpus)
> > +		return true;
> > +	/*
> > +	 * maxcpus should allow cpu to set CR4.MCE asap, otherwise the set may
> > +	 * be deferred indefinitely.
> > +	 */
> > +	if (!per_cpu(cpuhp_state, cpu).booted_once)
> > +		return true;
> 
> As this is a x86 only issue, you cannot inflict this magic on every
> architecture.
OK.
In my developing patch which fixes nr_cpus issue, I takes the following way:
(I am diverted to other things, have not finished test yet, hope to
turn back soon.)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 362dd89..c009169 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -956,6 +956,87 @@ int common_cpu_up(unsigned int cpu, struct task_struct *idle)
        return 0;
 }

+void __init bring_capped_cpu_steady(void)
+{
[...]
+}
+
 /*
  * NOTE - on most systems this is a PHYSICAL apic ID, but on multiquad
  * (ie clustered apic addressing mode), this is a LOGICAL apic ID.
diff --git a/kernel/smp.c b/kernel/smp.c
index d155374..b04961c 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -560,6 +560,10 @@ void __init setup_nr_cpu_ids(void)
        nr_cpu_ids = find_last_bit(cpumask_bits(cpu_possible_mask),NR_CPUS) + 1;
 }

+void __weak bring_capped_cpu_steady(void)
+{
+}
+
 /* Called by boot processor to activate the rest. */
 void __init smp_init(void)
 {
@@ -579,6 +583,8 @@ void __init smp_init(void)
                        cpu_up(cpu);
        }

+       /* force cpus capped by nr_cpus option into steady state */
+       bring_capped_cpu_steady();
        num_nodes = num_online_nodes();


The initial motivation is to step around percpu area required by cpu hotplug
framework. But it also provide the abstraction of archs.

What do you think about resolving maxcpus by the similar abstraction?

> 
> Aside of that this does not solve the problem at all because smp_init()
> still does:
> 
>         for_each_present_cpu(cpu) {
>                 if (num_online_cpus() >= setup_max_cpus)
>                         break;
Yes, this logic should be removed, then maxcpus_allowed() can take effect. But
now, it may take a quite different way to resolve it.

Thanks for your kindly review.

Regards,
  Pingfan
>                 if (!cpu_online(cpu))
>                         cpu_up(cpu);
>         }
> 
> So the remaining CPUs are not onlined at all.
> 
> Thanks,
> 
> 	tglx