by Atish Patra

[permalink] [raw]

Subject: Re: [PATCH v3 12/12] RISC-V: Support cpu hotplug.

On 9/6/18 11:25 AM, Atish Patra wrote:
> On 9/6/18 3:21 AM, Mark Rutland wrote:
>> Hi,
>>
>> On Thu, Sep 06, 2018 at 01:05:35AM -0700, Atish Patra wrote:
>>> This patch enable support for cpu hotplug in RISC-V.
>>>
>>> In absence of generic cpu stop functions, WFI is used
>>> to put the cpu in low power state during offline. An IPI
>>> is sent to bring it out of WFI during online operation.
>>
>> AFAICT, this doesn't actually return the CPU to firwmare, and the WFI and
>> return code are in-kernel. From experience on arm and arm64 I would *very*
>> strongly advise against this kind of pseudo-hotplug, as it causes many more
>> long-term problems than it solves.
>>
> I completely agree with you. The idea was here to have a working
> cpu-hotplug solution until we have a concrete implementation of cpu
> stop/start via firmware. Once we have that, we just need to replace the
> wakeup and wait_for_software_interrupt calls. The current implementation
> via WFI was never meant to be a long term solution.
>
>

Any thoughts on this ? Is this good enough argument to keep the current
version of hotplug feature and make necessary changes after we have a
proper cpu start/stop via firmware (which may take some significant time)?

Regards,
Atish
>> For instance, this causes significant pain with kexec, since the prior kernel's
>> text must be kept around forever for the offline CPUs. Likewise with hibernate
>> suspend/resume.
>>
>> Depending on how your architecture works, there can also be a number of
>> problems with cache/TLB maintenance for secondary CPUs which are unexpectedly
>> online.
>>
>> I would suggest that someone should look at writing a FW standard for CPU
>> hotplug, akin to PSCI for arm/arm64. At minimum, you will want:
>>
>
> I have already started working on it in parallel. As of now, I am
> planning to strengthen SBI spec by making it more flexible/extensible
> with some of the below features you mentioned. But we can come up with a
> new spec altogether if that makes more sense.
>
>> - A mechanism to determine the version of FW and/or features supported by the FW.
>>
>> - a mechanism to hotplug-in a specific CPU to a runtime-specified address,
>> ideally with some unique token (e.g. so you can pass a pointer to its stack
>> or whatever).
>>
>> - a mechanism to hotplug-out the calling CPU.
>>
>> - a mechanism to determine when a specific CPU has been hotplugged out. This is
>> necessary for kexec and other things to be robust.
>>
>
> Thanks for the pointers. I had not looked into 2&4 earlier. I will go
> over PSCI docs again.
>
> Regards,
> Atish
>
>> Thanks,
>> Mark.
>>
>>>
>>> Tested both on QEMU and HighFive Unleashed board with
>>> 4 cpus. Test result follows.
>>>
>>> $ echo 0 > /sys/devices/system/cpu/cpu2/online
>>> [ 31.828562] CPU2: shutdown
>>> $ cat /proc/cpuinfo
>>> hart : 0
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> hart : 1
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> hart : 3
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> $ echo 0 > /sys/devices/system/cpu/cpu3/online
>>> [ 52.968495] CPU3: shutdown
>>> $ cat /proc/cpuinfo
>>> hart : 0
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> hart : 2
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> $ echo 1 > /sys/devices/system/cpu/cpu3/online
>>> [ 64.298250] CPU3: online
>>> $ cat /proc/cpuinfo
>>> hart : 0
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> hart : 1
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> hart : 3
>>> isa : rv64imafdc
>>> mmu : sv39
>>> uarch : sifive,rocket0
>>>
>>> Signed-off-by: Atish Patra <[email protected]>
>>> ---
>>> arch/riscv/Kconfig | 12 ++++++-
>>> arch/riscv/include/asm/irq.h | 1 +
>>> arch/riscv/include/asm/smp.h | 28 ++++++++++++++++
>>> arch/riscv/kernel/Makefile | 1 +
>>> arch/riscv/kernel/cpu-hotplug.c | 72 +++++++++++++++++++++++++++++++++++++++++
>>> arch/riscv/kernel/head.S | 13 ++++++++
>>> arch/riscv/kernel/irq.c | 24 ++++++++++++++
>>> arch/riscv/kernel/setup.c | 17 +++++++++-
>>> arch/riscv/kernel/smp.c | 8 +----
>>> arch/riscv/kernel/smpboot.c | 7 ++--
>>> arch/riscv/kernel/traps.c | 6 ++--
>>> 11 files changed, 175 insertions(+), 14 deletions(-)
>>> create mode 100644 arch/riscv/kernel/cpu-hotplug.c
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index a3449802..ea2e617e 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -21,7 +21,6 @@ config RISCV
>>> select COMMON_CLK
>>> select DMA_DIRECT_OPS
>>> select GENERIC_CLOCKEVENTS
>>> - select GENERIC_CPU_DEVICES
>>> select GENERIC_IRQ_SHOW
>>> select GENERIC_PCI_IOMAP
>>> select GENERIC_STRNCPY_FROM_USER
>>> @@ -167,6 +166,17 @@ config SMP
>>>
>>> If you don't know what to do here, say N.
>>>
>>> +config HOTPLUG_CPU
>>> + bool "Support for hot-pluggable CPUs"
>>> + depends on SMP
>>> + select GENERIC_IRQ_MIGRATION
>>> + help
>>> +
>>> + Say Y here to experiment with turning CPUs off and on. CPUs
>>> + can be controlled through /sys/devices/system/cpu.
>>> +
>>> + Say N if you want to disable CPU hotplug.
>>> +
>>> config NR_CPUS
>>> int "Maximum number of CPUs (2-32)"
>>> range 2 32
>>> diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
>>> index 996b6fbe..a873a72d 100644
>>> --- a/arch/riscv/include/asm/irq.h
>>> +++ b/arch/riscv/include/asm/irq.h
>>> @@ -19,6 +19,7 @@
>>>
>>> void riscv_timer_interrupt(void);
>>> void riscv_software_interrupt(void);
>>> +void wait_for_software_interrupt(void);
>>>
>>> #include <asm-generic/irq.h>
>>>
>>> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
>>> index fce312ce..8145b865 100644
>>> --- a/arch/riscv/include/asm/smp.h
>>> +++ b/arch/riscv/include/asm/smp.h
>>> @@ -25,8 +25,29 @@
>>> extern unsigned long __cpuid_to_hardid_map[NR_CPUS];
>>> #define cpuid_to_hardid_map(cpu) __cpuid_to_hardid_map[cpu]
>>>
>>> +#if defined CONFIG_SMP && defined CONFIG_HOTPLUG_CPU
>>> +void arch_send_call_wakeup_ipi(int cpu);
>>> +bool can_hotplug_cpu(void);
>>> +#else
>>> +static inline bool can_hotplug_cpu(void)
>>> +{
>>> + return 0;
>>> +}
>>> +static inline void arch_send_call_wakeup_ipi(int cpu) { }
>>> +#endif
>>> +
>>> #ifdef CONFIG_SMP
>>>
>>> +enum ipi_message_type {
>>> + IPI_RESCHEDULE,
>>> + IPI_CALL_FUNC,
>>> + IPI_CALL_WAKEUP,
>>> + IPI_MAX
>>> +};
>>> +
>>> +void send_ipi_message(const struct cpumask *to_whom,
>>> + enum ipi_message_type operation);
>>> +
>>> /* SMP initialization hook for setup_arch */
>>> void __init setup_smp(void);
>>>
>>> @@ -45,6 +66,13 @@ void riscv_cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out);
>>> */
>>> #define raw_smp_processor_id() (current_thread_info()->cpu)
>>>
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +int __cpu_disable(void);
>>> +void __cpu_die(unsigned int cpu);
>>> +void cpu_play_dead(void);
>>> +void boot_sec_cpu(void);
>>> +#endif /* CONFIG_HOTPLUG_CPU */
>>> +
>>> #else
>>>
>>> static inline int riscv_hartid_to_cpuid(int hartid)
>>> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
>>> index e1274fc0..62043204 100644
>>> --- a/arch/riscv/kernel/Makefile
>>> +++ b/arch/riscv/kernel/Makefile
>>> @@ -35,6 +35,7 @@ obj-$(CONFIG_SMP) += smpboot.o
>>> obj-$(CONFIG_SMP) += smp.o
>>> obj-$(CONFIG_MODULES) += module.o
>>> obj-$(CONFIG_MODULE_SECTIONS) += module-sections.o
>>> +obj-$(CONFIG_HOTPLUG_CPU) += cpu-hotplug.o
>>>
>>> obj-$(CONFIG_FUNCTION_TRACER) += mcount.o ftrace.o
>>> obj-$(CONFIG_DYNAMIC_FTRACE) += mcount-dyn.o
>>> diff --git a/arch/riscv/kernel/cpu-hotplug.c b/arch/riscv/kernel/cpu-hotplug.c
>>> new file mode 100644
>>> index 00000000..7a152972
>>> --- /dev/null
>>> +++ b/arch/riscv/kernel/cpu-hotplug.c
>>> @@ -0,0 +1,72 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2018 Western Digital Corporation or its affiliates.
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/mm.h>
>>> +#include <linux/sched.h>
>>> +#include <linux/err.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/cpu.h>
>>> +#include <linux/sched/hotplug.h>
>>> +#include <asm/irq.h>
>>> +#include <asm/sbi.h>
>>> +
>>> +bool can_hotplug_cpu(void)
>>> +{
>>> + return true;
>>> +}
>>> +
>>> +void arch_cpu_idle_dead(void)
>>> +{
>>> + cpu_play_dead();
>>> +}
>>> +
>>> +/*
>>> + * __cpu_disable runs on the processor to be shutdown.
>>> + */
>>> +int __cpu_disable(void)
>>> +{
>>> + int ret = 0;
>>> + unsigned int cpu = smp_processor_id();
>>> +
>>> + set_cpu_online(cpu, false);
>>> + irq_migrate_all_off_this_cpu();
>>> +
>>> + return ret;
>>> +}
>>> +/*
>>> + * called on the thread which is asking for a CPU to be shutdown -
>>> + * waits until shutdown has completed, or it is timed out.
>>> + */
>>> +void __cpu_die(unsigned int cpu)
>>> +{
>>> + if (!cpu_wait_death(cpu, 5)) {
>>> + pr_err("CPU %u: didn't die\n", cpu);
>>> + return;
>>> + }
>>> + pr_notice("CPU%u: shutdown\n", cpu);
>>> + /*TODO: Do we need to verify is cpu is really dead */
>>> +}
>>> +
>>> +/*
>>> + * Called from the idle thread for the CPU which has been shutdown.
>>> + *
>>> + */
>>> +void cpu_play_dead(void)
>>> +{
>>> + idle_task_exit();
>>> +
>>> + (void)cpu_report_death();
>>> +
>>> + /* Do not disable software interrupt to restart cpu after WFI */
>>> + csr_clear(sie, SIE_STIE | SIE_SEIE);
>>> + wait_for_software_interrupt();
>>> + boot_sec_cpu();
>>> +}
>>> +
>>> +void arch_send_call_wakeup_ipi(int cpu)
>>> +{
>>> + send_ipi_message(cpumask_of(cpu), IPI_CALL_WAKEUP);
>>> +}
>>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>>> index 711190d4..07d9fb38 100644
>>> --- a/arch/riscv/kernel/head.S
>>> +++ b/arch/riscv/kernel/head.S
>>> @@ -153,6 +153,19 @@ relocate:
>>> j .Lsecondary_park
>>> END(_start)
>>>
>>> +#ifdef CONFIG_SMP
>>> +.section .text
>>> +.global boot_sec_cpu
>>> +
>>> +boot_sec_cpu:
>>> + /* clear all pending flags */
>>> + csrw sip, zero
>>> + /* Mask all interrupts */
>>> + csrw sie, zero
>>> + fence
>>> +
>>> + tail smp_callin
>>> +#endif
>>> __PAGE_ALIGNED_BSS
>>> /* Empty zero page */
>>> .balign PAGE_SIZE
>>> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
>>> index 0cfac48a..7e14b0d9 100644
>>> --- a/arch/riscv/kernel/irq.c
>>> +++ b/arch/riscv/kernel/irq.c
>>> @@ -53,6 +53,30 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
>>> set_irq_regs(old_regs);
>>> }
>>>
>>> +/*
>>> + * This function doesn't return until a software interrupt is sent via IPI.
>>> + * Obviously, all the interrupts except software interrupt should be disabled
>>> + * before this function is called.
>>> + */
>>> +void wait_for_software_interrupt(void)
>>> +{
>>> + unsigned long sipval, sieval, scauseval;
>>> +
>>> + /* clear all pending flags */
>>> + csr_write(sip, 0);
>>> + /* clear any previous scause data */
>>> + csr_write(scause, 0);
>>> +
>>> + do {
>>> + wait_for_interrupt();
>>> + sipval = csr_read(sip);
>>> + sieval = csr_read(sie);
>>> + scauseval = csr_read(scause) & ~INTERRUPT_CAUSE_FLAG;
>>> + /* only break if wfi returns for an enabled interrupt */
>>> + } while ((sipval & sieval) == 0 &&
>>> + scauseval != INTERRUPT_CAUSE_SOFTWARE);
>>> +}
>>> +
>>> void __init init_IRQ(void)
>>> {
>>> irqchip_init();
>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>> index a5fac1b7..612f0c21 100644
>>> --- a/arch/riscv/kernel/setup.c
>>> +++ b/arch/riscv/kernel/setup.c
>>> @@ -30,11 +30,11 @@
>>> #include <linux/of_platform.h>
>>> #include <linux/sched/task.h>
>>> #include <linux/swiotlb.h>
>>> +#include <linux/smp.h>
>>>
>>> #include <asm/setup.h>
>>> #include <asm/sections.h>
>>> #include <asm/pgtable.h>
>>> -#include <asm/smp.h>
>>> #include <asm/sbi.h>
>>> #include <asm/tlbflush.h>
>>> #include <asm/thread_info.h>
>>> @@ -82,6 +82,7 @@ EXPORT_SYMBOL(empty_zero_page);
>>> /* The lucky hart to first increment this variable will boot the other cores */
>>> atomic_t hart_lottery;
>>> unsigned long boot_cpu_hartid;
>>> +static DEFINE_PER_CPU(struct cpu, cpu_devices);
>>>
>>> unsigned long __cpuid_to_hardid_map[NR_CPUS] = {
>>> [0 ... NR_CPUS-1] = INVALID_HARTID
>>> @@ -257,3 +258,17 @@ void __init setup_arch(char **cmdline_p)
>>> riscv_fill_hwcap();
>>> }
>>>
>>> +static int __init topology_init(void)
>>> +{
>>> + int i;
>>> +
>>> + for_each_possible_cpu(i) {
>>> + struct cpu *cpu = &per_cpu(cpu_devices, i);
>>> +
>>> + cpu->hotpluggable = can_hotplug_cpu();
>>> + register_cpu(cpu, i);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +subsys_initcall(topology_init);
>>> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
>>> index 89d95866..629456bb 100644
>>> --- a/arch/riscv/kernel/smp.c
>>> +++ b/arch/riscv/kernel/smp.c
>>> @@ -32,12 +32,6 @@ static struct {
>>> unsigned long bits ____cacheline_aligned;
>>> } ipi_data[NR_CPUS] __cacheline_aligned;
>>>
>>> -enum ipi_message_type {
>>> - IPI_RESCHEDULE,
>>> - IPI_CALL_FUNC,
>>> - IPI_MAX
>>> -};
>>> -
>>> int riscv_hartid_to_cpuid(int hartid)
>>> {
>>> int i = -1;
>>> @@ -94,7 +88,7 @@ void riscv_software_interrupt(void)
>>> }
>>> }
>>>
>>> -static void
>>> +void
>>> send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
>>> {
>>> int cpuid, hartid;
>>> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>>> index f44ae780..a9138431 100644
>>> --- a/arch/riscv/kernel/smpboot.c
>>> +++ b/arch/riscv/kernel/smpboot.c
>>> @@ -92,9 +92,12 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>>> task_stack_page(tidle) + THREAD_SIZE);
>>> WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
>>>
>>> + arch_send_call_wakeup_ipi(cpu);
>>> while (!cpu_online(cpu))
>>> cpu_relax();
>>>
>>> + pr_notice("CPU%u: online\n", cpu);
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -105,7 +108,7 @@ void __init smp_cpus_done(unsigned int max_cpus)
>>> /*
>>> * C entry point for a secondary processor.
>>> */
>>> -asmlinkage void __init smp_callin(void)
>>> +asmlinkage void smp_callin(void)
>>> {
>>> struct mm_struct *mm = &init_mm;
>>>
>>> @@ -115,7 +118,7 @@ asmlinkage void __init smp_callin(void)
>>>
>>> trap_init();
>>> notify_cpu_starting(smp_processor_id());
>>> - set_cpu_online(smp_processor_id(), 1);
>>> + set_cpu_online(smp_processor_id(), true);
>>> /*
>>> * Remote TLB flushes are ignored while the CPU is offline, so emit
>>> * a local TLB flush right now just in case.
>>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>>> index 24a9333d..8b331619 100644
>>> --- a/arch/riscv/kernel/traps.c
>>> +++ b/arch/riscv/kernel/traps.c
>>> @@ -153,7 +153,7 @@ int is_valid_bugaddr(unsigned long pc)
>>> }
>>> #endif /* CONFIG_GENERIC_BUG */
>>>
>>> -void __init trap_init(void)
>>> +void trap_init(void)
>>> {
>>> /*
>>> * Set sup0 scratch register to 0, indicating to exception vector
>>> @@ -162,6 +162,6 @@ void __init trap_init(void)
>>> csr_write(sscratch, 0);
>>> /* Set the exception vector address */
>>> csr_write(stvec, &handle_exception);
>>> - /* Enable all interrupts */
>>> - csr_write(sie, -1);
>>> + /* Enable all interrupts but timer interrupt*/
>>> + csr_set(sie, SIE_SSIE | SIE_SEIE);
>>> }
>>> --
>>> 2.7.4
>>>
>>
>
>