Messages in this thread | | | From | Atish Patra <> | Date | Wed, 12 Feb 2020 10:57:37 -0800 | Subject | Re: [PATCH v8 07/11] RISC-V: Add cpu_ops and modify default booting method |
| |
On Tue, Feb 11, 2020 at 8:28 PM Anup Patel <anup@brainfault.org> wrote: > > On Wed, Feb 12, 2020 at 7:21 AM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com> > > --- > > 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 <anup@brainfault.org> > > Regards, > Anup >
-- Regards, Atish
| |