Currently, all non-booting harts start booting after the booting hart
updates the per-hart stack pointer. This is done in a way that, it's
difficult to implement any other booting method without breaking the
backward compatibility.
Define a cpu_ops method that allows to introduce other booting methods
in future. Modify the current booting method to be compatible with
cpu_ops.
Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/cpu_ops.h | 34 ++++++++++++++++++
arch/riscv/kernel/Makefile | 2 ++
arch/riscv/kernel/cpu_ops.c | 40 +++++++++++++++++++++
arch/riscv/kernel/cpu_ops_spinwait.c | 42 ++++++++++++++++++++++
arch/riscv/kernel/smpboot.c | 54 +++++++++++++++++-----------
5 files changed, 151 insertions(+), 21 deletions(-)
create mode 100644 arch/riscv/include/asm/cpu_ops.h
create mode 100644 arch/riscv/kernel/cpu_ops.c
create mode 100644 arch/riscv/kernel/cpu_ops_spinwait.c
diff --git a/arch/riscv/include/asm/cpu_ops.h b/arch/riscv/include/asm/cpu_ops.h
new file mode 100644
index 000000000000..7db276284009
--- /dev/null
+++ b/arch/riscv/include/asm/cpu_ops.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2019 Western Digital Corporation or its affiliates.
+ * Based on arch/arm64/include/asm/cpu_ops.h
+ */
+#ifndef __ASM_CPU_OPS_H
+#define __ASM_CPU_OPS_H
+
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/threads.h>
+
+/**
+ * struct cpu_operations - Callback operations for hotplugging CPUs.
+ *
+ * @name: Name of the boot protocol.
+ * @cpu_prepare: Early one-time preparation step for a cpu. If there
+ * is a mechanism for doing so, tests whether it is
+ * possible to boot the given HART.
+ * @cpu_start: Boots a cpu into the kernel.
+ */
+struct cpu_operations {
+ const char *name;
+ int (*cpu_prepare)(unsigned int cpu);
+ int (*cpu_start)(unsigned int cpu,
+ struct task_struct *tidle);
+};
+
+extern const struct cpu_operations *cpu_ops[NR_CPUS];
+int __init cpu_set_ops(int cpu);
+void cpu_update_secondary_bootdata(unsigned int cpuid,
+ struct task_struct *tidle);
+
+#endif /* ifndef __ASM_CPU_OPS_H */
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index f40205cb9a22..f81a6ff88005 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -32,6 +32,8 @@ obj-$(CONFIG_RISCV_M_MODE) += clint.o
obj-$(CONFIG_FPU) += fpu.o
obj-$(CONFIG_SMP) += smpboot.o
obj-$(CONFIG_SMP) += smp.o
+obj-$(CONFIG_SMP) += cpu_ops.o
+obj-$(CONFIG_SMP) += cpu_ops_spinwait.o
obj-$(CONFIG_MODULES) += module.o
obj-$(CONFIG_MODULE_SECTIONS) += module-sections.o
diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
new file mode 100644
index 000000000000..1085def3735a
--- /dev/null
+++ b/arch/riscv/kernel/cpu_ops.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020 Western Digital Corporation or its affiliates.
+ */
+
+#include <linux/errno.h>
+#include <linux/mm.h>
+#include <linux/of.h>
+#include <linux/string.h>
+#include <linux/sched.h>
+#include <linux/sched/task_stack.h>
+#include <asm/cpu_ops.h>
+#include <asm/sbi.h>
+#include <asm/smp.h>
+
+const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
+
+void *__cpu_up_stack_pointer[NR_CPUS];
+void *__cpu_up_task_pointer[NR_CPUS];
+
+extern const struct cpu_operations cpu_ops_spinwait;
+
+void cpu_update_secondary_bootdata(unsigned int cpuid,
+ struct task_struct *tidle)
+{
+ int hartid = cpuid_to_hartid_map(cpuid);
+
+ /* Make sure tidle is updated */
+ smp_mb();
+ WRITE_ONCE(__cpu_up_stack_pointer[hartid],
+ task_stack_page(tidle) + THREAD_SIZE);
+ WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
+}
+
+int __init cpu_set_ops(int cpuid)
+{
+ cpu_ops[cpuid] = &cpu_ops_spinwait;
+
+ return 0;
+}
diff --git a/arch/riscv/kernel/cpu_ops_spinwait.c b/arch/riscv/kernel/cpu_ops_spinwait.c
new file mode 100644
index 000000000000..f828e660294e
--- /dev/null
+++ b/arch/riscv/kernel/cpu_ops_spinwait.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020 Western Digital Corporation or its affiliates.
+ */
+
+#include <linux/errno.h>
+#include <linux/of.h>
+#include <linux/string.h>
+#include <asm/cpu_ops.h>
+#include <asm/sbi.h>
+#include <asm/smp.h>
+
+const struct cpu_operations cpu_ops_spinwait;
+
+static int spinwait_cpu_prepare(unsigned int cpuid)
+{
+ if (!cpu_ops_spinwait.cpu_start) {
+ pr_err("cpu start method not defined for CPU [%d]\n", cpuid);
+ return -ENODEV;
+ }
+ return 0;
+}
+
+static int spinwait_cpu_start(unsigned int cpuid, struct task_struct *tidle)
+{
+ /*
+ * In this protocol, all cpus boot on their own accord. _start
+ * selects the first cpu to boot the kernel and causes the remainder
+ * of the cpus to spin in a loop waiting for their stack pointer to be
+ * setup by that main cpu. Writing to bootdata (i.e __cpu_up_stack_pointer) signals to
+ * the spinning cpus that they can continue the boot process.
+ */
+ cpu_update_secondary_bootdata(cpuid, tidle);
+
+ return 0;
+}
+
+const struct cpu_operations cpu_ops_spinwait = {
+ .name = "spinwait",
+ .cpu_prepare = spinwait_cpu_prepare,
+ .cpu_start = spinwait_cpu_start,
+};
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 8bc01f0ca73b..2ee41c779a16 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -25,6 +25,7 @@
#include <linux/sched/task_stack.h>
#include <linux/sched/mm.h>
#include <asm/clint.h>
+#include <asm/cpu_ops.h>
#include <asm/irq.h>
#include <asm/mmu_context.h>
#include <asm/tlbflush.h>
@@ -34,8 +35,6 @@
#include "head.h"
-void *__cpu_up_stack_pointer[NR_CPUS];
-void *__cpu_up_task_pointer[NR_CPUS];
static DECLARE_COMPLETION(cpu_running);
void __init smp_prepare_boot_cpu(void)
@@ -46,6 +45,7 @@ void __init smp_prepare_boot_cpu(void)
void __init smp_prepare_cpus(unsigned int max_cpus)
{
int cpuid;
+ int ret;
/* This covers non-smp usecase mandated by "nosmp" option */
if (max_cpus == 0)
@@ -54,6 +54,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
for_each_possible_cpu(cpuid) {
if (cpuid == smp_processor_id())
continue;
+ if (cpu_ops[cpuid]->cpu_prepare) {
+ ret = cpu_ops[cpuid]->cpu_prepare(cpuid);
+ if (ret)
+ continue;
+ }
set_cpu_present(cpuid, true);
}
}
@@ -65,6 +70,8 @@ void __init setup_smp(void)
bool found_boot_cpu = false;
int cpuid = 1;
+ cpu_set_ops(0);
+
for_each_of_cpu_node(dn) {
hart = riscv_of_processor_hartid(dn);
if (hart < 0)
@@ -92,36 +99,41 @@ void __init setup_smp(void)
cpuid, nr_cpu_ids);
for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
- if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID)
+ if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
+ if (cpu_set_ops(cpuid)) {
+ cpuid_to_hartid_map(cpuid) = INVALID_HARTID;
+ continue;
+ }
set_cpu_possible(cpuid, true);
+ }
}
}
+int start_secondary_cpu(int cpu, struct task_struct *tidle)
+{
+ if (cpu_ops[cpu]->cpu_start)
+ return cpu_ops[cpu]->cpu_start(cpu, tidle);
+
+ return -EOPNOTSUPP;
+}
+
int __cpu_up(unsigned int cpu, struct task_struct *tidle)
{
int ret = 0;
- int hartid = cpuid_to_hartid_map(cpu);
tidle->thread_info.cpu = cpu;
- /*
- * On RISC-V systems, all harts boot on their own accord. Our _start
- * selects the first hart to boot the kernel and causes the remainder
- * of the harts to spin in a loop waiting for their stack pointer to be
- * setup by that main hart. Writing __cpu_up_stack_pointer signals to
- * the spinning harts that they can continue the boot process.
- */
- smp_mb();
- WRITE_ONCE(__cpu_up_stack_pointer[hartid],
- task_stack_page(tidle) + THREAD_SIZE);
- WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
-
- lockdep_assert_held(&cpu_running);
- wait_for_completion_timeout(&cpu_running,
+ ret = start_secondary_cpu(cpu, tidle);
+ if (!ret) {
+ lockdep_assert_held(&cpu_running);
+ wait_for_completion_timeout(&cpu_running,
msecs_to_jiffies(1000));
- if (!cpu_online(cpu)) {
- pr_crit("CPU%u: failed to come online\n", cpu);
- ret = -EIO;
+ if (!cpu_online(cpu)) {
+ pr_crit("CPU%u: failed to come online\n", cpu);
+ ret = -EIO;
+ }
+ } else {
+ pr_crit("CPU%u: failed to start\n", cpu);
}
return ret;
--
2.24.0
On Wed, Feb 12, 2020 at 7:21 AM Atish Patra <[email protected]> wrote:
>
> Currently, all non-booting harts start booting after the booting hart
> updates the per-hart stack pointer. This is done in a way that, it's
> difficult to implement any other booting method without breaking the
> backward compatibility.
>
> Define a cpu_ops method that allows to introduce other booting methods
> in future. Modify the current booting method to be compatible with
> cpu_ops.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/cpu_ops.h | 34 ++++++++++++++++++
> arch/riscv/kernel/Makefile | 2 ++
> arch/riscv/kernel/cpu_ops.c | 40 +++++++++++++++++++++
> arch/riscv/kernel/cpu_ops_spinwait.c | 42 ++++++++++++++++++++++
> arch/riscv/kernel/smpboot.c | 54 +++++++++++++++++-----------
> 5 files changed, 151 insertions(+), 21 deletions(-)
> create mode 100644 arch/riscv/include/asm/cpu_ops.h
> create mode 100644 arch/riscv/kernel/cpu_ops.c
> create mode 100644 arch/riscv/kernel/cpu_ops_spinwait.c
>
> diff --git a/arch/riscv/include/asm/cpu_ops.h b/arch/riscv/include/asm/cpu_ops.h
> new file mode 100644
> index 000000000000..7db276284009
> --- /dev/null
> +++ b/arch/riscv/include/asm/cpu_ops.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + * Based on arch/arm64/include/asm/cpu_ops.h
> + */
> +#ifndef __ASM_CPU_OPS_H
> +#define __ASM_CPU_OPS_H
> +
> +#include <linux/init.h>
> +#include <linux/sched.h>
> +#include <linux/threads.h>
> +
> +/**
> + * struct cpu_operations - Callback operations for hotplugging CPUs.
> + *
> + * @name: Name of the boot protocol.
> + * @cpu_prepare: Early one-time preparation step for a cpu. If there
> + * is a mechanism for doing so, tests whether it is
> + * possible to boot the given HART.
> + * @cpu_start: Boots a cpu into the kernel.
> + */
> +struct cpu_operations {
> + const char *name;
> + int (*cpu_prepare)(unsigned int cpu);
> + int (*cpu_start)(unsigned int cpu,
> + struct task_struct *tidle);
> +};
> +
> +extern const struct cpu_operations *cpu_ops[NR_CPUS];
> +int __init cpu_set_ops(int cpu);
This function is more like probing for appropriate cpu_ops. Also,
I think we don't need to return anything from cpu_set_ops().
Maybe rename it to "void cpu_probe_ops(int cpu)" ?
> +void cpu_update_secondary_bootdata(unsigned int cpuid,
> + struct task_struct *tidle);
> +
> +#endif /* ifndef __ASM_CPU_OPS_H */
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index f40205cb9a22..f81a6ff88005 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -32,6 +32,8 @@ obj-$(CONFIG_RISCV_M_MODE) += clint.o
> obj-$(CONFIG_FPU) += fpu.o
> obj-$(CONFIG_SMP) += smpboot.o
> obj-$(CONFIG_SMP) += smp.o
> +obj-$(CONFIG_SMP) += cpu_ops.o
> +obj-$(CONFIG_SMP) += cpu_ops_spinwait.o
> obj-$(CONFIG_MODULES) += module.o
> obj-$(CONFIG_MODULE_SECTIONS) += module-sections.o
>
> diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
> new file mode 100644
> index 000000000000..1085def3735a
> --- /dev/null
> +++ b/arch/riscv/kernel/cpu_ops.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020 Western Digital Corporation or its affiliates.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/mm.h>
> +#include <linux/of.h>
> +#include <linux/string.h>
> +#include <linux/sched.h>
> +#include <linux/sched/task_stack.h>
> +#include <asm/cpu_ops.h>
> +#include <asm/sbi.h>
> +#include <asm/smp.h>
> +
> +const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
> +
> +void *__cpu_up_stack_pointer[NR_CPUS];
> +void *__cpu_up_task_pointer[NR_CPUS];
> +
> +extern const struct cpu_operations cpu_ops_spinwait;
> +
> +void cpu_update_secondary_bootdata(unsigned int cpuid,
> + struct task_struct *tidle)
> +{
> + int hartid = cpuid_to_hartid_map(cpuid);
> +
> + /* Make sure tidle is updated */
> + smp_mb();
> + WRITE_ONCE(__cpu_up_stack_pointer[hartid],
> + task_stack_page(tidle) + THREAD_SIZE);
> + WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
> +}
> +
> +int __init cpu_set_ops(int cpuid)
Maybe rename it to "void cpu_probe_ops(int cpu)" ?
> +{
> + cpu_ops[cpuid] = &cpu_ops_spinwait;
> +
> + return 0;
> +}
> diff --git a/arch/riscv/kernel/cpu_ops_spinwait.c b/arch/riscv/kernel/cpu_ops_spinwait.c
> new file mode 100644
> index 000000000000..f828e660294e
> --- /dev/null
> +++ b/arch/riscv/kernel/cpu_ops_spinwait.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020 Western Digital Corporation or its affiliates.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/of.h>
> +#include <linux/string.h>
> +#include <asm/cpu_ops.h>
> +#include <asm/sbi.h>
> +#include <asm/smp.h>
> +
> +const struct cpu_operations cpu_ops_spinwait;
> +
> +static int spinwait_cpu_prepare(unsigned int cpuid)
> +{
> + if (!cpu_ops_spinwait.cpu_start) {
> + pr_err("cpu start method not defined for CPU [%d]\n", cpuid);
> + return -ENODEV;
> + }
> + return 0;
> +}
> +
> +static int spinwait_cpu_start(unsigned int cpuid, struct task_struct *tidle)
> +{
> + /*
> + * In this protocol, all cpus boot on their own accord. _start
> + * selects the first cpu to boot the kernel and causes the remainder
> + * of the cpus to spin in a loop waiting for their stack pointer to be
> + * setup by that main cpu. Writing to bootdata (i.e __cpu_up_stack_pointer) signals to
> + * the spinning cpus that they can continue the boot process.
> + */
> + cpu_update_secondary_bootdata(cpuid, tidle);
> +
> + return 0;
> +}
> +
> +const struct cpu_operations cpu_ops_spinwait = {
> + .name = "spinwait",
> + .cpu_prepare = spinwait_cpu_prepare,
> + .cpu_start = spinwait_cpu_start,
> +};
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 8bc01f0ca73b..2ee41c779a16 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -25,6 +25,7 @@
> #include <linux/sched/task_stack.h>
> #include <linux/sched/mm.h>
> #include <asm/clint.h>
> +#include <asm/cpu_ops.h>
> #include <asm/irq.h>
> #include <asm/mmu_context.h>
> #include <asm/tlbflush.h>
> @@ -34,8 +35,6 @@
>
> #include "head.h"
>
> -void *__cpu_up_stack_pointer[NR_CPUS];
> -void *__cpu_up_task_pointer[NR_CPUS];
> static DECLARE_COMPLETION(cpu_running);
>
> void __init smp_prepare_boot_cpu(void)
> @@ -46,6 +45,7 @@ void __init smp_prepare_boot_cpu(void)
> void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> int cpuid;
> + int ret;
>
> /* This covers non-smp usecase mandated by "nosmp" option */
> if (max_cpus == 0)
> @@ -54,6 +54,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> for_each_possible_cpu(cpuid) {
> if (cpuid == smp_processor_id())
> continue;
> + if (cpu_ops[cpuid]->cpu_prepare) {
> + ret = cpu_ops[cpuid]->cpu_prepare(cpuid);
> + if (ret)
> + continue;
> + }
> set_cpu_present(cpuid, true);
> }
> }
> @@ -65,6 +70,8 @@ void __init setup_smp(void)
> bool found_boot_cpu = false;
> int cpuid = 1;
>
> + cpu_set_ops(0);
> +
> for_each_of_cpu_node(dn) {
> hart = riscv_of_processor_hartid(dn);
> if (hart < 0)
> @@ -92,36 +99,41 @@ void __init setup_smp(void)
> cpuid, nr_cpu_ids);
>
> for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
> - if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID)
> + if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
> + if (cpu_set_ops(cpuid)) {
> + cpuid_to_hartid_map(cpuid) = INVALID_HARTID;
> + continue;
> + }
> set_cpu_possible(cpuid, true);
> + }
> }
> }
>
> +int start_secondary_cpu(int cpu, struct task_struct *tidle)
Make this function static.
> +{
> + if (cpu_ops[cpu]->cpu_start)
> + return cpu_ops[cpu]->cpu_start(cpu, tidle);
> +
> + return -EOPNOTSUPP;
> +}
> +
> int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> {
> int ret = 0;
> - int hartid = cpuid_to_hartid_map(cpu);
> tidle->thread_info.cpu = cpu;
>
> - /*
> - * On RISC-V systems, all harts boot on their own accord. Our _start
> - * selects the first hart to boot the kernel and causes the remainder
> - * of the harts to spin in a loop waiting for their stack pointer to be
> - * setup by that main hart. Writing __cpu_up_stack_pointer signals to
> - * the spinning harts that they can continue the boot process.
> - */
> - smp_mb();
> - WRITE_ONCE(__cpu_up_stack_pointer[hartid],
> - task_stack_page(tidle) + THREAD_SIZE);
> - WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
> -
> - lockdep_assert_held(&cpu_running);
> - wait_for_completion_timeout(&cpu_running,
> + ret = start_secondary_cpu(cpu, tidle);
> + if (!ret) {
> + lockdep_assert_held(&cpu_running);
> + wait_for_completion_timeout(&cpu_running,
> msecs_to_jiffies(1000));
>
> - if (!cpu_online(cpu)) {
> - pr_crit("CPU%u: failed to come online\n", cpu);
> - ret = -EIO;
> + if (!cpu_online(cpu)) {
> + pr_crit("CPU%u: failed to come online\n", cpu);
> + ret = -EIO;
> + }
> + } else {
> + pr_crit("CPU%u: failed to start\n", cpu);
> }
>
> return ret;
> --
> 2.24.0
>
Apart from minor comments above, looks good to me.
Reviewed-by: Anup Patel <[email protected]>
Regards,
Anup
On Tue, Feb 11, 2020 at 8:28 PM Anup Patel <[email protected]> wrote:
>
> On Wed, Feb 12, 2020 at 7:21 AM Atish Patra <[email protected]> wrote:
> >
> > Currently, all non-booting harts start booting after the booting hart
> > updates the per-hart stack pointer. This is done in a way that, it's
> > difficult to implement any other booting method without breaking the
> > backward compatibility.
> >
> > Define a cpu_ops method that allows to introduce other booting methods
> > in future. Modify the current booting method to be compatible with
> > cpu_ops.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/cpu_ops.h | 34 ++++++++++++++++++
> > arch/riscv/kernel/Makefile | 2 ++
> > arch/riscv/kernel/cpu_ops.c | 40 +++++++++++++++++++++
> > arch/riscv/kernel/cpu_ops_spinwait.c | 42 ++++++++++++++++++++++
> > arch/riscv/kernel/smpboot.c | 54 +++++++++++++++++-----------
> > 5 files changed, 151 insertions(+), 21 deletions(-)
> > create mode 100644 arch/riscv/include/asm/cpu_ops.h
> > create mode 100644 arch/riscv/kernel/cpu_ops.c
> > create mode 100644 arch/riscv/kernel/cpu_ops_spinwait.c
> >
> > diff --git a/arch/riscv/include/asm/cpu_ops.h b/arch/riscv/include/asm/cpu_ops.h
> > new file mode 100644
> > index 000000000000..7db276284009
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/cpu_ops.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > + * Based on arch/arm64/include/asm/cpu_ops.h
> > + */
> > +#ifndef __ASM_CPU_OPS_H
> > +#define __ASM_CPU_OPS_H
> > +
> > +#include <linux/init.h>
> > +#include <linux/sched.h>
> > +#include <linux/threads.h>
> > +
> > +/**
> > + * struct cpu_operations - Callback operations for hotplugging CPUs.
> > + *
> > + * @name: Name of the boot protocol.
> > + * @cpu_prepare: Early one-time preparation step for a cpu. If there
> > + * is a mechanism for doing so, tests whether it is
> > + * possible to boot the given HART.
> > + * @cpu_start: Boots a cpu into the kernel.
> > + */
> > +struct cpu_operations {
> > + const char *name;
> > + int (*cpu_prepare)(unsigned int cpu);
> > + int (*cpu_start)(unsigned int cpu,
> > + struct task_struct *tidle);
> > +};
> > +
> > +extern const struct cpu_operations *cpu_ops[NR_CPUS];
> > +int __init cpu_set_ops(int cpu);
>
> This function is more like probing for appropriate cpu_ops. Also,
> I think we don't need to return anything from cpu_set_ops().
>
Correct. I will change it to void but I think set_ops is a better name
as we are actually
setting the right type of ops to cpu_ops.
> Maybe rename it to "void cpu_probe_ops(int cpu)" ?
>
> > +void cpu_update_secondary_bootdata(unsigned int cpuid,
> > + struct task_struct *tidle);
> > +
> > +#endif /* ifndef __ASM_CPU_OPS_H */
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index f40205cb9a22..f81a6ff88005 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -32,6 +32,8 @@ obj-$(CONFIG_RISCV_M_MODE) += clint.o
> > obj-$(CONFIG_FPU) += fpu.o
> > obj-$(CONFIG_SMP) += smpboot.o
> > obj-$(CONFIG_SMP) += smp.o
> > +obj-$(CONFIG_SMP) += cpu_ops.o
> > +obj-$(CONFIG_SMP) += cpu_ops_spinwait.o
> > obj-$(CONFIG_MODULES) += module.o
> > obj-$(CONFIG_MODULE_SECTIONS) += module-sections.o
> >
> > diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
> > new file mode 100644
> > index 000000000000..1085def3735a
> > --- /dev/null
> > +++ b/arch/riscv/kernel/cpu_ops.c
> > @@ -0,0 +1,40 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020 Western Digital Corporation or its affiliates.
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/mm.h>
> > +#include <linux/of.h>
> > +#include <linux/string.h>
> > +#include <linux/sched.h>
> > +#include <linux/sched/task_stack.h>
> > +#include <asm/cpu_ops.h>
> > +#include <asm/sbi.h>
> > +#include <asm/smp.h>
> > +
> > +const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
> > +
> > +void *__cpu_up_stack_pointer[NR_CPUS];
> > +void *__cpu_up_task_pointer[NR_CPUS];
> > +
> > +extern const struct cpu_operations cpu_ops_spinwait;
> > +
> > +void cpu_update_secondary_bootdata(unsigned int cpuid,
> > + struct task_struct *tidle)
> > +{
> > + int hartid = cpuid_to_hartid_map(cpuid);
> > +
> > + /* Make sure tidle is updated */
> > + smp_mb();
> > + WRITE_ONCE(__cpu_up_stack_pointer[hartid],
> > + task_stack_page(tidle) + THREAD_SIZE);
> > + WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
> > +}
> > +
> > +int __init cpu_set_ops(int cpuid)
>
> Maybe rename it to "void cpu_probe_ops(int cpu)" ?
>
> > +{
> > + cpu_ops[cpuid] = &cpu_ops_spinwait;
> > +
> > + return 0;
> > +}
> > diff --git a/arch/riscv/kernel/cpu_ops_spinwait.c b/arch/riscv/kernel/cpu_ops_spinwait.c
> > new file mode 100644
> > index 000000000000..f828e660294e
> > --- /dev/null
> > +++ b/arch/riscv/kernel/cpu_ops_spinwait.c
> > @@ -0,0 +1,42 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020 Western Digital Corporation or its affiliates.
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/of.h>
> > +#include <linux/string.h>
> > +#include <asm/cpu_ops.h>
> > +#include <asm/sbi.h>
> > +#include <asm/smp.h>
> > +
> > +const struct cpu_operations cpu_ops_spinwait;
> > +
> > +static int spinwait_cpu_prepare(unsigned int cpuid)
> > +{
> > + if (!cpu_ops_spinwait.cpu_start) {
> > + pr_err("cpu start method not defined for CPU [%d]\n", cpuid);
> > + return -ENODEV;
> > + }
> > + return 0;
> > +}
> > +
> > +static int spinwait_cpu_start(unsigned int cpuid, struct task_struct *tidle)
> > +{
> > + /*
> > + * In this protocol, all cpus boot on their own accord. _start
> > + * selects the first cpu to boot the kernel and causes the remainder
> > + * of the cpus to spin in a loop waiting for their stack pointer to be
> > + * setup by that main cpu. Writing to bootdata (i.e __cpu_up_stack_pointer) signals to
> > + * the spinning cpus that they can continue the boot process.
> > + */
> > + cpu_update_secondary_bootdata(cpuid, tidle);
> > +
> > + return 0;
> > +}
> > +
> > +const struct cpu_operations cpu_ops_spinwait = {
> > + .name = "spinwait",
> > + .cpu_prepare = spinwait_cpu_prepare,
> > + .cpu_start = spinwait_cpu_start,
> > +};
> > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> > index 8bc01f0ca73b..2ee41c779a16 100644
> > --- a/arch/riscv/kernel/smpboot.c
> > +++ b/arch/riscv/kernel/smpboot.c
> > @@ -25,6 +25,7 @@
> > #include <linux/sched/task_stack.h>
> > #include <linux/sched/mm.h>
> > #include <asm/clint.h>
> > +#include <asm/cpu_ops.h>
> > #include <asm/irq.h>
> > #include <asm/mmu_context.h>
> > #include <asm/tlbflush.h>
> > @@ -34,8 +35,6 @@
> >
> > #include "head.h"
> >
> > -void *__cpu_up_stack_pointer[NR_CPUS];
> > -void *__cpu_up_task_pointer[NR_CPUS];
> > static DECLARE_COMPLETION(cpu_running);
> >
> > void __init smp_prepare_boot_cpu(void)
> > @@ -46,6 +45,7 @@ void __init smp_prepare_boot_cpu(void)
> > void __init smp_prepare_cpus(unsigned int max_cpus)
> > {
> > int cpuid;
> > + int ret;
> >
> > /* This covers non-smp usecase mandated by "nosmp" option */
> > if (max_cpus == 0)
> > @@ -54,6 +54,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> > for_each_possible_cpu(cpuid) {
> > if (cpuid == smp_processor_id())
> > continue;
> > + if (cpu_ops[cpuid]->cpu_prepare) {
> > + ret = cpu_ops[cpuid]->cpu_prepare(cpuid);
> > + if (ret)
> > + continue;
> > + }
> > set_cpu_present(cpuid, true);
> > }
> > }
> > @@ -65,6 +70,8 @@ void __init setup_smp(void)
> > bool found_boot_cpu = false;
> > int cpuid = 1;
> >
> > + cpu_set_ops(0);
> > +
> > for_each_of_cpu_node(dn) {
> > hart = riscv_of_processor_hartid(dn);
> > if (hart < 0)
> > @@ -92,36 +99,41 @@ void __init setup_smp(void)
> > cpuid, nr_cpu_ids);
> >
> > for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
> > - if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID)
> > + if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
> > + if (cpu_set_ops(cpuid)) {
> > + cpuid_to_hartid_map(cpuid) = INVALID_HARTID;
> > + continue;
> > + }
> > set_cpu_possible(cpuid, true);
> > + }
> > }
> > }
> >
> > +int start_secondary_cpu(int cpu, struct task_struct *tidle)
>
> Make this function static.
>
sure.
> > +{
> > + if (cpu_ops[cpu]->cpu_start)
> > + return cpu_ops[cpu]->cpu_start(cpu, tidle);
> > +
> > + return -EOPNOTSUPP;
> > +}
> > +
> > int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> > {
> > int ret = 0;
> > - int hartid = cpuid_to_hartid_map(cpu);
> > tidle->thread_info.cpu = cpu;
> >
> > - /*
> > - * On RISC-V systems, all harts boot on their own accord. Our _start
> > - * selects the first hart to boot the kernel and causes the remainder
> > - * of the harts to spin in a loop waiting for their stack pointer to be
> > - * setup by that main hart. Writing __cpu_up_stack_pointer signals to
> > - * the spinning harts that they can continue the boot process.
> > - */
> > - smp_mb();
> > - WRITE_ONCE(__cpu_up_stack_pointer[hartid],
> > - task_stack_page(tidle) + THREAD_SIZE);
> > - WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
> > -
> > - lockdep_assert_held(&cpu_running);
> > - wait_for_completion_timeout(&cpu_running,
> > + ret = start_secondary_cpu(cpu, tidle);
> > + if (!ret) {
> > + lockdep_assert_held(&cpu_running);
> > + wait_for_completion_timeout(&cpu_running,
> > msecs_to_jiffies(1000));
> >
> > - if (!cpu_online(cpu)) {
> > - pr_crit("CPU%u: failed to come online\n", cpu);
> > - ret = -EIO;
> > + if (!cpu_online(cpu)) {
> > + pr_crit("CPU%u: failed to come online\n", cpu);
> > + ret = -EIO;
> > + }
> > + } else {
> > + pr_crit("CPU%u: failed to start\n", cpu);
> > }
> >
> > return ret;
> > --
> > 2.24.0
> >
>
> Apart from minor comments above, looks good to me.
>
> Reviewed-by: Anup Patel <[email protected]>
>
> Regards,
> Anup
>
--
Regards,
Atish