2018-08-16 02:49:51

by Atish Patra

Subject: [RFC PATCH 0/5] RISC-V: Improve smp functionality & support cpu hotplug

This patch series implements following smp related features.
Some of the work has been inspired from ARM64.

1. Decouple linux logical cpu ids from hardware cpu id
2. Introduce cpu_operations structure for better flexibility &
extesnability of future smp enablement methods. It also makes it
easier to implement different booting algorithms later.
3. Support cpu hotplug.

Tested on QEMU & HighFive Unleashed board with/without SMP enabled.

Atish Patra (5):
RISC-V: Add logical CPU indexing for RISC-V
RISC-V: Use Linux logical cpu number instead of hartid
RISC-V: Add cpu_operatios structure
RISC-V: Move interrupt cause declarations to irq.h
RISC-V: Support cpu hotplug.

arch/riscv/Kconfig | 12 ++-
arch/riscv/include/asm/irq.h | 7 ++
arch/riscv/include/asm/smp.h | 42 +++++++++-
arch/riscv/include/asm/tlbflush.h | 17 +++-
arch/riscv/kernel/cpu.c | 4 +-
arch/riscv/kernel/head.S | 13 +++
arch/riscv/kernel/irq.c | 7 --
arch/riscv/kernel/process.c | 7 ++
arch/riscv/kernel/setup.c | 27 +++++++
arch/riscv/kernel/smp.c | 51 +++++++++---
arch/riscv/kernel/smpboot.c | 161 +++++++++++++++++++++++++++++++++-----
arch/riscv/kernel/traps.c | 6 +-
drivers/clocksource/riscv_timer.c | 12 ++-
drivers/irqchip/irq-sifive-plic.c | 11 ++-
14 files changed, 325 insertions(+), 52 deletions(-)

--
2.7.4

2018-08-16 02:44:48

by Atish Patra

Subject: [RFC PATCH 1/5] RISC-V: Add logical CPU indexing for RISC-V

Currently, both linux cpu id and hardware cpu id are same.
This is not recommended as it will lead to discontinuous cpu
indexing in Linux. Moreover, kdump kernel will run from CPU0
which would be absent if we follow existing scheme.

Implement a logical mapping between Linux cpu id and hardware
cpuid to decouple these two. Always mark the boot processor as
cpu0 and all other cpus get the logical cpu id based on their
booting order.

Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/smp.h | 17 ++++++++++++++++-
arch/riscv/kernel/setup.c | 2 ++
arch/riscv/kernel/smp.c | 19 +++++++++++++++++++
3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 36016845..0763337b 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -22,6 +22,12 @@
#include <linux/cpumask.h>
#include <linux/irqreturn.h>

+/*
+ * Mapping between linux logical cpu index and hartid.
+ */
+extern u64 __cpu_logical_map[NR_CPUS];
+#define cpu_logical_map(cpu) __cpu_logical_map[cpu]
+
#ifdef CONFIG_SMP

/* SMP initialization hook for setup_arch */
@@ -33,6 +39,8 @@ void arch_send_call_function_ipi_mask(struct cpumask *mask);
/* Hook for the generic smp_call_function_single() routine. */
void arch_send_call_function_single_ipi(int cpu);

+int riscv_hartid_to_cpuid(int hartid);
+void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out);
/*
* This is particularly ugly: it appears we can't actually get the definition
* of task_struct here, but we need access to the CPU this task is running on.
@@ -41,6 +49,13 @@ void arch_send_call_function_single_ipi(int cpu);
*/
#define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_CPU)))

-#endif /* CONFIG_SMP */
+#else
+
+static inline int riscv_hartid_to_cpuid(int hartid) { return 0 ; }
+static inline void cpuid_to_hartid_mask(const struct cpumask *in,
+ struct cpumask *out) {
+ cpumask_set_cpu(cpu_logical_map(0), out);
+}

+#endif /* CONFIG_SMP */
#endif /* _ASM_RISCV_SMP_H */
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index db20dc63..e21ed481 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -82,6 +82,8 @@ EXPORT_SYMBOL(empty_zero_page);
/* The lucky hart to first increment this variable will boot the other cores */
atomic_t hart_lottery;

+u64 __cpu_logical_map[NR_CPUS];
+
#ifdef CONFIG_BLK_DEV_INITRD
static void __init setup_initrd(void)
{
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 906fe21e..d55379ee 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -38,7 +38,26 @@ enum ipi_message_type {
IPI_MAX
};

+int riscv_hartid_to_cpuid(int hartid)
+{
+ int i = -1;
+
+ for (i = 0; i < NR_CPUS; i++)
+ if (cpu_logical_map(i) == hartid)
+ return i;
+
+ pr_err("Couldn't find cpu id for hartid [%d]\n", hartid);
+ BUG();
+ return i;
+}

+void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out)
+{
+ int cpu;
+
+ for_each_cpu(cpu, in)
+ cpumask_set_cpu(cpu_logical_map(cpu), out);
+}
/* Unsupported */
int setup_profiling_timer(unsigned int multiplier)
{
--
2.7.4

2018-08-16 02:44:48

by Atish Patra

Subject: [RFC PATCH 5/5] RISC-V: Support cpu hotplug.

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.

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/smp.h | 17 ++++++-
arch/riscv/kernel/head.S | 13 ++++++
arch/riscv/kernel/process.c | 7 +++
arch/riscv/kernel/setup.c | 15 +++++++
arch/riscv/kernel/smpboot.c | 105 +++++++++++++++++++++++++++++++++++++++++--
arch/riscv/kernel/traps.c | 6 +--
7 files changed, 166 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 4764fdeb..51c6ac8d 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/smp.h b/arch/riscv/include/asm/smp.h
index 2bb6e6c2..3a1923ef 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -33,6 +33,10 @@ struct cpu_operations {
int (*cpu_init)(unsigned int);
int (*cpu_prepare)(unsigned int);
int (*cpu_boot)(unsigned int, struct task_struct *);
+#ifdef CONFIG_HOTPLUG_CPU
+ int (*cpu_disable)(unsigned int cpu);
+ void (*cpu_die)(unsigned int cpu);
+#endif
};
extern struct cpu_operations cpu_ops;
void smp_set_cpu_ops(const struct cpu_operations *cpu_ops);
@@ -58,6 +62,18 @@ void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out);
*/
#define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_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);
+int can_hotplug_cpu(void);
+#else
+
+static inline int can_hotplug_cpu(void) { return 0; }
+
+#endif /* CONFIG_HOTPLUG_CPU */
+
#else

static inline int riscv_hartid_to_cpuid(int hartid) { return 0 ; }
@@ -66,6 +82,5 @@ static inline void cpuid_to_hartid_mask(const struct cpumask *in,
cpumask_set_cpu(cpu_logical_map(0), out);
}

-
#endif /* CONFIG_SMP */
#endif /* _ASM_RISCV_SMP_H */
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index d1beecf1..81258c42 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -150,6 +150,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/process.c b/arch/riscv/kernel/process.c
index d7c6ca7c..cb209139 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -42,6 +42,13 @@ void arch_cpu_idle(void)
local_irq_enable();
}

+#ifdef CONFIG_HOTPLUG_CPU
+void arch_cpu_idle_dead(void)
+{
+ cpu_play_dead();
+}
+#endif
+
void show_regs(struct pt_regs *regs)
{
show_regs_print_info(KERN_DEFAULT);
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 97b586f8..fc2cfa8e 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -81,6 +81,7 @@ EXPORT_SYMBOL(empty_zero_page);

/* The lucky hart to first increment this variable will boot the other cores */
atomic_t hart_lottery;
+static DEFINE_PER_CPU(struct cpu, cpu_devices);

u64 __cpu_logical_map[NR_CPUS];

@@ -259,3 +260,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/smpboot.c b/arch/riscv/kernel/smpboot.c
index 045a1a45..a470ae29 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -30,7 +30,7 @@
#include <linux/irq.h>
#include <linux/of.h>
#include <linux/sched/task_stack.h>
-#include <linux/smp.h>
+#include <linux/sched/hotplug.h>
#include <asm/irq.h>
#include <asm/mmu_context.h>
#include <asm/tlbflush.h>
@@ -102,11 +102,17 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
if (cpu_ops.cpu_boot)
err = cpu_ops.cpu_boot(hartid, tidle);
if (!err) {
+
+#ifdef CONFIG_HOTPLUG_CPU
+ arch_send_call_function_single_ipi(cpu);
+#endif
while (!cpu_online(cpu))
cpu_relax();
+ pr_notice("CPU%u: online\n", cpu);
} else {
pr_err("CPU %d [hartid %d]failed to boot\n", cpu, hartid);
}
+
return 0;
}

@@ -114,10 +120,98 @@ void __init smp_cpus_done(unsigned int max_cpus)
{
}

+#ifdef CONFIG_HOTPLUG_CPU
+int can_hotplug_cpu(void)
+{
+ if (cpu_ops.cpu_die)
+ return 1;
+ else
+ return 0;
+}
+
+/*
+ * __cpu_disable runs on the processor to be shutdown.
+ */
+int __cpu_disable(void)
+{
+ int ret = 0;
+ unsigned int cpu = smp_processor_id();
+
+ if (cpu_ops.cpu_disable)
+ ret = cpu_ops.cpu_disable(cpu);
+ if (ret)
+ return ret;
+
+ 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 */
+}
+
+int default_cpu_disable(unsigned int cpu)
+{
+ if (!cpu_ops.cpu_die)
+ return -EOPNOTSUPP;
+ return 0;
+}
+
+/*
+ * Called from the idle thread for the CPU which has been shutdown.
+ *
+ */
+void cpu_play_dead(void)
+{
+ int cpu = smp_processor_id();
+
+ idle_task_exit();
+
+ (void)cpu_report_death();
+
+ /* Do not disable software interrupt to restart cpu after WFI */
+ csr_clear(sie, SIE_STIE | SIE_SEIE);
+ if (cpu_ops.cpu_die)
+ cpu_ops.cpu_die(cpu);
+}
+
+void default_cpu_die(unsigned int cpu)
+{
+ int 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);
+ /* only break if wfi returns for an enabled interrupt */
+ } while ((sipval & sieval) == 0 &&
+ scauseval != INTERRUPT_CAUSE_SOFTWARE);
+
+ boot_sec_cpu();
+}
+
+#endif /* CONFIG_HOTPLUG_CPU */
/*
* C entry point for a secondary processor.
*/
-asmlinkage void __init smp_callin(void)
+asmlinkage void smp_callin(void)
{
struct mm_struct *mm = &init_mm;

@@ -127,15 +221,18 @@ 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);
local_flush_tlb_all();
local_irq_enable();
preempt_disable();
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}

-
struct cpu_operations default_ops = {
.name = "default",
.cpu_boot = default_cpu_boot,
+#ifdef CONFIG_HOTPLUG_CPU
+ .cpu_disable = default_cpu_disable,
+ .cpu_die = default_cpu_die,
+#endif
};
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

2018-08-16 02:57:27

by Atish Patra

Subject: [RFC PATCH 4/5] RISC-V: Move interrupt cause declarations to irq.h

CPU hotplug code in smpboot requires interrupt cause
to identify the cause of cpu wakeup.

Move the IRQ cause declarations to appropriate header
file.

Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/irq.h | 7 +++++++
arch/riscv/kernel/irq.c | 7 -------
2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index 996b6fbe..abf2e4e3 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -20,6 +20,13 @@
void riscv_timer_interrupt(void);
void riscv_software_interrupt(void);

+/*
+ * Possible interrupt causes:
+ */
+#define INTERRUPT_CAUSE_SOFTWARE 1
+#define INTERRUPT_CAUSE_TIMER 5
+#define INTERRUPT_CAUSE_EXTERNAL 9
+
#include <asm-generic/irq.h>

#endif /* _ASM_RISCV_IRQ_H */
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 0cfac48a..0dd2df01 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -10,13 +10,6 @@
#include <linux/irqdomain.h>

/*
- * Possible interrupt causes:
- */
-#define INTERRUPT_CAUSE_SOFTWARE 1
-#define INTERRUPT_CAUSE_TIMER 5
-#define INTERRUPT_CAUSE_EXTERNAL 9
-
-/*
* The high order bit of the trap cause register is always set for
* interrupts, which allows us to differentiate them from exceptions
* quickly. The INTERRUPT_CAUSE_* macros don't contain that bit, so we
--
2.7.4

2018-08-16 02:57:27

by Atish Patra

Subject: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

Defining cpu_operations now helps adding cpu hotplug
support in proper manner. Moreover, it provides flexibility
in supporting other cpu enable/boot methods can be
supported in future. This patch has been largely inspired from
ARM64. However, a default boot method is defined for RISC-V unlike
ARM64.

Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/smp.h | 10 ++++++++++
arch/riscv/kernel/smp.c | 8 ++++++++
arch/riscv/kernel/smpboot.c | 34 ++++++++++++++++++++++++++++------
3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 0763337b..2bb6e6c2 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -28,6 +28,15 @@
extern u64 __cpu_logical_map[NR_CPUS];
#define cpu_logical_map(cpu) __cpu_logical_map[cpu]

+struct cpu_operations {
+ const char *name;
+ int (*cpu_init)(unsigned int);
+ int (*cpu_prepare)(unsigned int);
+ int (*cpu_boot)(unsigned int, struct task_struct *);
+};
+extern struct cpu_operations cpu_ops;
+void smp_set_cpu_ops(const struct cpu_operations *cpu_ops);
+
#ifdef CONFIG_SMP

/* SMP initialization hook for setup_arch */
@@ -57,5 +66,6 @@ static inline void cpuid_to_hartid_mask(const struct cpumask *in,
cpumask_set_cpu(cpu_logical_map(0), out);
}

+
#endif /* CONFIG_SMP */
#endif /* _ASM_RISCV_SMP_H */
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 4ab70480..5de58ced 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -58,6 +58,14 @@ void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out)
for_each_cpu(cpu, in)
cpumask_set_cpu(cpu_logical_map(cpu), out);
}
+struct cpu_operations cpu_ops __ro_after_init;
+
+void smp_set_cpu_ops(const struct cpu_operations *ops)
+{
+ if (ops)
+ cpu_ops = *ops;
+}
+
/* Unsupported */
int setup_profiling_timer(unsigned int multiplier)
{
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 6ab2bb1b..045a1a45 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -30,6 +30,7 @@
#include <linux/irq.h>
#include <linux/of.h>
#include <linux/sched/task_stack.h>
+#include <linux/smp.h>
#include <asm/irq.h>
#include <asm/mmu_context.h>
#include <asm/tlbflush.h>
@@ -38,6 +39,7 @@

void *__cpu_up_stack_pointer[NR_CPUS];
void *__cpu_up_task_pointer[NR_CPUS];
+struct cpu_operations default_ops;

void __init smp_prepare_boot_cpu(void)
{
@@ -53,6 +55,7 @@ void __init setup_smp(void)
int hart, found_boot_cpu = 0;
int cpuid = 1;

+ smp_set_cpu_ops(&default_ops);
while ((dn = of_find_node_by_type(dn, "cpu"))) {
hart = riscv_of_processor_hart(dn);

@@ -73,10 +76,8 @@ void __init setup_smp(void)
BUG_ON(!found_boot_cpu);
}

-int __cpu_up(unsigned int cpu, struct task_struct *tidle)
+int default_cpu_boot(unsigned int hartid, struct task_struct *tidle)
{
- int hartid = cpu_logical_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
@@ -84,13 +85,28 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
* setup by that main hart. Writing __cpu_up_stack_pointer signals to
* the spinning harts that they can continue the boot process.
*/
- smp_mb();
+
__cpu_up_stack_pointer[hartid] = task_stack_page(tidle) + THREAD_SIZE;
__cpu_up_task_pointer[hartid] = tidle;
+ return 0;
+}
+
+int __cpu_up(unsigned int cpu, struct task_struct *tidle)
+{
+ int err = -1;
+ int hartid = cpu_logical_map(cpu);

- while (!cpu_online(cpu))
- cpu_relax();
+ tidle->thread_info.cpu = cpu;
+ smp_mb();

+ if (cpu_ops.cpu_boot)
+ err = cpu_ops.cpu_boot(hartid, tidle);
+ if (!err) {
+ while (!cpu_online(cpu))
+ cpu_relax();
+ } else {
+ pr_err("CPU %d [hartid %d]failed to boot\n", cpu, hartid);
+ }
return 0;
}

@@ -117,3 +133,9 @@ asmlinkage void __init smp_callin(void)
preempt_disable();
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
}
+
+
+struct cpu_operations default_ops = {
+ .name = "default",
+ .cpu_boot = default_cpu_boot,
+};
--
2.7.4

2018-08-16 03:07:57

by Atish Patra

Subject: [RFC PATCH 2/5] RISC-V: Use Linux logical cpu number instead of hartid

Setup the cpu_logical_map during boot. Moreover, every SBI call
and PLIC context are based on the physical hartid. Use the logical
cpu to hartid mapping to pass correct hartid to respective functions.

Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/tlbflush.h | 17 +++++++++++++----
arch/riscv/kernel/cpu.c | 4 +++-
arch/riscv/kernel/setup.c | 10 ++++++++++
arch/riscv/kernel/smp.c | 24 +++++++++++++++---------
arch/riscv/kernel/smpboot.c | 30 ++++++++++++++++++------------
drivers/clocksource/riscv_timer.c | 12 ++++++++----
drivers/irqchip/irq-sifive-plic.c | 11 +++++++----
7 files changed, 74 insertions(+), 34 deletions(-)

diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
index 85c2d8ba..ecfd9b0e 100644
--- a/arch/riscv/include/asm/tlbflush.h
+++ b/arch/riscv/include/asm/tlbflush.h
@@ -16,6 +16,7 @@
#define _ASM_RISCV_TLBFLUSH_H

#include <linux/mm_types.h>
+#include <asm/smp.h>

/*
* Flush entire local TLB. 'sfence.vma' implicitly fences with the instruction
@@ -49,13 +50,21 @@ static inline void flush_tlb_range(struct vm_area_struct *vma,

#include <asm/sbi.h>

-#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
+static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
+ unsigned long size)
+{
+ struct cpumask hmask;
+
+ cpuid_to_hartid_mask(cmask, &hmask);
+ sbi_remote_sfence_vma(hmask.bits, start, size);
+}
+
+#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
#define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
#define flush_tlb_range(vma, start, end) \
- sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
- start, (end) - (start))
+ remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
#define flush_tlb_mm(mm) \
- sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)
+ remote_sfence_vma(mm_cpumask(mm), 0, -1)

#endif /* CONFIG_SMP */

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index ca6c81e5..f8a18ace 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -14,6 +14,7 @@
#include <linux/init.h>
#include <linux/seq_file.h>
#include <linux/of.h>
+#include <asm/smp.h>

/* Return -1 if not a valid hart */
int riscv_of_processor_hart(struct device_node *node)
@@ -79,7 +80,8 @@ static void c_stop(struct seq_file *m, void *v)
static int c_show(struct seq_file *m, void *v)
{
unsigned long hart_id = (unsigned long)v - 1;
- struct device_node *node = of_get_cpu_node(hart_id, NULL);
+ struct device_node *node = of_get_cpu_node(cpu_logical_map(hart_id),
+ NULL);
const char *compat, *isa, *mmu;

seq_printf(m, "hart\t: %lu\n", hart_id);
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index e21ed481..97b586f8 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -84,6 +84,16 @@ atomic_t hart_lottery;

u64 __cpu_logical_map[NR_CPUS];

+void __init smp_setup_processor_id(void)
+{
+ int cpu = smp_processor_id();
+
+ cpu_logical_map(0) = cpu;
+
+ /* Change the boot cpu ID in thread_info */
+ current->thread_info.cpu = 0;
+}
+
#ifdef CONFIG_BLK_DEV_INITRD
static void __init setup_initrd(void)
{
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index d55379ee..4ab70480 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -97,14 +97,18 @@ void riscv_software_interrupt(void)
static void
send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
{
- int i;
+ int cpuid, hartid;
+ struct cpumask hartid_mask;

+ cpumask_clear(&hartid_mask);
mb();
- for_each_cpu(i, to_whom)
- set_bit(operation, &ipi_data[i].bits);
-
+ for_each_cpu(cpuid, to_whom) {
+ set_bit(operation, &ipi_data[cpuid].bits);
+ hartid = cpu_logical_map(cpuid);
+ cpumask_set_cpu(hartid, &hartid_mask);
+ }
mb();
- sbi_send_ipi(cpumask_bits(to_whom));
+ sbi_send_ipi(cpumask_bits(&hartid_mask));
}

void arch_send_call_function_ipi_mask(struct cpumask *mask)
@@ -146,7 +150,7 @@ void smp_send_reschedule(int cpu)
void flush_icache_mm(struct mm_struct *mm, bool local)
{
unsigned int cpu;
- cpumask_t others, *mask;
+ cpumask_t others, hmask, *mask;

preempt_disable();

@@ -164,9 +168,11 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
*/
cpumask_andnot(&others, mm_cpumask(mm), cpumask_of(cpu));
local |= cpumask_empty(&others);
- if (mm != current->active_mm || !local)
- sbi_remote_fence_i(others.bits);
- else {
+ if (mm != current->active_mm || !local) {
+ cpumask_clear(&hmask);
+ cpuid_to_hartid_mask(&others, &hmask);
+ sbi_remote_fence_i(hmask.bits);
+ } else {
/*
* It's assumed that at least one strongly ordered operation is
* performed on this hart between setting a hart's cpumask bit
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 56abab6a..6ab2bb1b 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -50,27 +50,33 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
void __init setup_smp(void)
{
struct device_node *dn = NULL;
- int hart, im_okay_therefore_i_am = 0;
+ int hart, found_boot_cpu = 0;
+ int cpuid = 1;

while ((dn = of_find_node_by_type(dn, "cpu"))) {
hart = riscv_of_processor_hart(dn);
- if (hart >= 0) {
- set_cpu_possible(hart, true);
- set_cpu_present(hart, true);
- if (hart == smp_processor_id()) {
- BUG_ON(im_okay_therefore_i_am);
- im_okay_therefore_i_am = 1;
- }
+
+ if (hart < 0)
+ continue;
+ if (hart == cpu_logical_map(0)) {
+ BUG_ON(found_boot_cpu);
+ found_boot_cpu = 1;
+ continue;
}
+
+ cpu_logical_map(cpuid) = hart;
+ set_cpu_possible(cpuid, true);
+ set_cpu_present(cpuid, true);
+ cpuid++;
}

- BUG_ON(!im_okay_therefore_i_am);
+ BUG_ON(!found_boot_cpu);
}

int __cpu_up(unsigned int cpu, struct task_struct *tidle)
{
+ int hartid = cpu_logical_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
@@ -79,8 +85,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
* the spinning harts that they can continue the boot process.
*/
smp_mb();
- __cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
- __cpu_up_task_pointer[cpu] = tidle;
+ __cpu_up_stack_pointer[hartid] = task_stack_page(tidle) + THREAD_SIZE;
+ __cpu_up_task_pointer[hartid] = tidle;

while (!cpu_online(cpu))
cpu_relax();
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 4e8b347e..f1f205e5 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -8,6 +8,7 @@
#include <linux/cpu.h>
#include <linux/delay.h>
#include <linux/irq.h>
+#include <asm/smp.h>
#include <asm/sbi.h>

/*
@@ -84,13 +85,16 @@ void riscv_timer_interrupt(void)

static int __init riscv_timer_init_dt(struct device_node *n)
{
- int cpu_id = riscv_of_processor_hart(n), error;
+ int cpuid, hartid, error;
struct clocksource *cs;

- if (cpu_id != smp_processor_id())
+ hartid = riscv_of_processor_hart(n);
+ cpuid = riscv_hartid_to_cpuid(hartid);
+
+ if (cpuid != smp_processor_id())
return 0;

- cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
+ cs = per_cpu_ptr(&riscv_clocksource, cpuid);
clocksource_register_hz(cs, riscv_timebase);

error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
@@ -98,7 +102,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
riscv_timer_starting_cpu, riscv_timer_dying_cpu);
if (error)
pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
- error, cpu_id);
+ error, cpuid);
return error;
}

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 298685e5..2eb2e78c 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -15,6 +15,7 @@
#include <linux/of_irq.h>
#include <linux/platform_device.h>
#include <linux/spinlock.h>
+#include <asm/smp.h>

/*
* This driver implements a version of the RISC-V PLIC with the actual layout
@@ -93,10 +94,11 @@ static inline void plic_toggle(int ctxid, int hwirq, int enable)
static inline void plic_irq_toggle(struct irq_data *d, int enable)
{
int cpu;
+ struct plic_handler *handler;

writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
- struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
+ handler = per_cpu_ptr(&plic_handlers, cpu);

if (handler->present)
plic_toggle(handler->ctxid, d->hwirq, enable);
@@ -217,7 +219,7 @@ static int __init plic_init(struct device_node *node,
struct of_phandle_args parent;
struct plic_handler *handler;
irq_hw_number_t hwirq;
- int cpu;
+ int cpu, hartid;

if (of_irq_parse_one(node, i, &parent)) {
pr_err("failed to parse parent for context %d.\n", i);
@@ -228,12 +230,13 @@ static int __init plic_init(struct device_node *node,
if (parent.args[0] == -1)
continue;

- cpu = plic_find_hart_id(parent.np);
- if (cpu < 0) {
+ hartid = plic_find_hart_id(parent.np);
+ if (hartid < 0) {
pr_warn("failed to parse hart ID for context %d.\n", i);
continue;
}

+ cpu = riscv_hartid_to_cpuid(hartid);
handler = per_cpu_ptr(&plic_handlers, cpu);
handler->present = true;
handler->ctxid = i;
--
2.7.4

2018-08-16 07:54:56

by Anup Patel

Subject: Re: [RFC PATCH 1/5] RISC-V: Add logical CPU indexing for RISC-V

On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
> Currently, both linux cpu id and hardware cpu id are same.
> This is not recommended as it will lead to discontinuous cpu
> indexing in Linux. Moreover, kdump kernel will run from CPU0
> which would be absent if we follow existing scheme.
>
> Implement a logical mapping between Linux cpu id and hardware
> cpuid to decouple these two. Always mark the boot processor as
> cpu0 and all other cpus get the logical cpu id based on their
> booting order.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/smp.h | 17 ++++++++++++++++-
> arch/riscv/kernel/setup.c | 2 ++
> arch/riscv/kernel/smp.c | 19 +++++++++++++++++++
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index 36016845..0763337b 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -22,6 +22,12 @@
> #include <linux/cpumask.h>
> #include <linux/irqreturn.h>
>
> +/*
> + * Mapping between linux logical cpu index and hartid.
> + */
> +extern u64 __cpu_logical_map[NR_CPUS];
> +#define cpu_logical_map(cpu) __cpu_logical_map[cpu]
> +
> #ifdef CONFIG_SMP
>
> /* SMP initialization hook for setup_arch */
> @@ -33,6 +39,8 @@ void arch_send_call_function_ipi_mask(struct cpumask *mask);
> /* Hook for the generic smp_call_function_single() routine. */
> void arch_send_call_function_single_ipi(int cpu);
>
> +int riscv_hartid_to_cpuid(int hartid);
> +void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out);
> /*
> * This is particularly ugly: it appears we can't actually get the definition
> * of task_struct here, but we need access to the CPU this task is running on.
> @@ -41,6 +49,13 @@ void arch_send_call_function_single_ipi(int cpu);
> */
> #define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_CPU)))
>
> -#endif /* CONFIG_SMP */
> +#else
> +
> +static inline int riscv_hartid_to_cpuid(int hartid) { return 0 ; }
> +static inline void cpuid_to_hartid_mask(const struct cpumask *in,
> + struct cpumask *out) {
> + cpumask_set_cpu(cpu_logical_map(0), out);
> +}
>
> +#endif /* CONFIG_SMP */
> #endif /* _ASM_RISCV_SMP_H */
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index db20dc63..e21ed481 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -82,6 +82,8 @@ EXPORT_SYMBOL(empty_zero_page);
> /* The lucky hart to first increment this variable will boot the other cores */
> atomic_t hart_lottery;
>
> +u64 __cpu_logical_map[NR_CPUS];

If hardware IDs are always machine word size then its better to use
"unsigned long" in-place of u64.

The __cpu_logical_map[] should be zero initially because zero is a
valid hardware ID. Better set all entries to -1 by assigning { -1 } to the
array.

Also, I feel __cpu_logical_map[] should be part of smp.c instead of
setup.c. Any particular reason for having it in setup.c?

> +
> #ifdef CONFIG_BLK_DEV_INITRD
> static void __init setup_initrd(void)
> {
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index 906fe21e..d55379ee 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -38,7 +38,26 @@ enum ipi_message_type {
> IPI_MAX
> };
>
> +int riscv_hartid_to_cpuid(int hartid)
> +{
> + int i = -1;
> +
> + for (i = 0; i < NR_CPUS; i++)
> + if (cpu_logical_map(i) == hartid)
> + return i;
> +
> + pr_err("Couldn't find cpu id for hartid [%d]\n", hartid);
> + BUG();
> + return i;
> +}
>
> +void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out)

May be rename cpuid_to_hartid_mask() to riscv_cpuid_to_hartid_mask()
for consistency.

> +{
> + int cpu;
> +
> + for_each_cpu(cpu, in)
> + cpumask_set_cpu(cpu_logical_map(cpu), out);
> +}
> /* Unsupported */
> int setup_profiling_timer(unsigned int multiplier)
> {
> --
> 2.7.4
>

2018-08-16 08:19:50

by Anup Patel

Subject: Re: [RFC PATCH 2/5] RISC-V: Use Linux logical cpu number instead of hartid

On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
> Setup the cpu_logical_map during boot. Moreover, every SBI call
> and PLIC context are based on the physical hartid. Use the logical
> cpu to hartid mapping to pass correct hartid to respective functions.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/tlbflush.h | 17 +++++++++++++----
> arch/riscv/kernel/cpu.c | 4 +++-
> arch/riscv/kernel/setup.c | 10 ++++++++++
> arch/riscv/kernel/smp.c | 24 +++++++++++++++---------
> arch/riscv/kernel/smpboot.c | 30 ++++++++++++++++++------------
> drivers/clocksource/riscv_timer.c | 12 ++++++++----
> drivers/irqchip/irq-sifive-plic.c | 11 +++++++----
> 7 files changed, 74 insertions(+), 34 deletions(-)
>
> diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
> index 85c2d8ba..ecfd9b0e 100644
> --- a/arch/riscv/include/asm/tlbflush.h
> +++ b/arch/riscv/include/asm/tlbflush.h
> @@ -16,6 +16,7 @@
> #define _ASM_RISCV_TLBFLUSH_H
>
> #include <linux/mm_types.h>
> +#include <asm/smp.h>
>
> /*
> * Flush entire local TLB. 'sfence.vma' implicitly fences with the instruction
> @@ -49,13 +50,21 @@ static inline void flush_tlb_range(struct vm_area_struct *vma,
>
> #include <asm/sbi.h>
>
> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
> + unsigned long size)
> +{
> + struct cpumask hmask;
> +
> + cpuid_to_hartid_mask(cmask, &hmask);
> + sbi_remote_sfence_vma(hmask.bits, start, size);
> +}
> +
> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
> #define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
> #define flush_tlb_range(vma, start, end) \
> - sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
> - start, (end) - (start))
> + remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
> #define flush_tlb_mm(mm) \
> - sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)
> + remote_sfence_vma(mm_cpumask(mm), 0, -1)
>
> #endif /* CONFIG_SMP */
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index ca6c81e5..f8a18ace 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -14,6 +14,7 @@
> #include <linux/init.h>
> #include <linux/seq_file.h>
> #include <linux/of.h>
> +#include <asm/smp.h>
>
> /* Return -1 if not a valid hart */
> int riscv_of_processor_hart(struct device_node *node)
> @@ -79,7 +80,8 @@ static void c_stop(struct seq_file *m, void *v)
> static int c_show(struct seq_file *m, void *v)
> {
> unsigned long hart_id = (unsigned long)v - 1;
> - struct device_node *node = of_get_cpu_node(hart_id, NULL);
> + struct device_node *node = of_get_cpu_node(cpu_logical_map(hart_id),
> + NULL);

The hart_id is misleading name here. It should be cpu_id. Please replace
all instances of hart_id with cpu_id and where hard ID is to be displayed
use cpu_logical_map(cpu_id).

> const char *compat, *isa, *mmu;
>
> seq_printf(m, "hart\t: %lu\n", hart_id);
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index e21ed481..97b586f8 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -84,6 +84,16 @@ atomic_t hart_lottery;
>
> u64 __cpu_logical_map[NR_CPUS];
>
> +void __init smp_setup_processor_id(void)
> +{
> + int cpu = smp_processor_id();
> +
> + cpu_logical_map(0) = cpu;

I think this should be:
cpu_logical_map(cpu) = hart_id;

Here hart_id for boot CPU will be value of a0 register passed at boot-time.

> +
> + /* Change the boot cpu ID in thread_info */
> + current->thread_info.cpu = 0;
> +}
> +
> #ifdef CONFIG_BLK_DEV_INITRD
> static void __init setup_initrd(void)
> {
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index d55379ee..4ab70480 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -97,14 +97,18 @@ void riscv_software_interrupt(void)
> static void
> send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
> {
> - int i;
> + int cpuid, hartid;
> + struct cpumask hartid_mask;
>
> + cpumask_clear(&hartid_mask);
> mb();
> - for_each_cpu(i, to_whom)
> - set_bit(operation, &ipi_data[i].bits);
> -
> + for_each_cpu(cpuid, to_whom) {
> + set_bit(operation, &ipi_data[cpuid].bits);
> + hartid = cpu_logical_map(cpuid);
> + cpumask_set_cpu(hartid, &hartid_mask);
> + }
> mb();
> - sbi_send_ipi(cpumask_bits(to_whom));
> + sbi_send_ipi(cpumask_bits(&hartid_mask));
> }
>
> void arch_send_call_function_ipi_mask(struct cpumask *mask)
> @@ -146,7 +150,7 @@ void smp_send_reschedule(int cpu)
> void flush_icache_mm(struct mm_struct *mm, bool local)
> {
> unsigned int cpu;
> - cpumask_t others, *mask;
> + cpumask_t others, hmask, *mask;
>
> preempt_disable();
>
> @@ -164,9 +168,11 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
> */
> cpumask_andnot(&others, mm_cpumask(mm), cpumask_of(cpu));
> local |= cpumask_empty(&others);
> - if (mm != current->active_mm || !local)
> - sbi_remote_fence_i(others.bits);
> - else {
> + if (mm != current->active_mm || !local) {
> + cpumask_clear(&hmask);
> + cpuid_to_hartid_mask(&others, &hmask);
> + sbi_remote_fence_i(hmask.bits);
> + } else {
> /*
> * It's assumed that at least one strongly ordered operation is
> * performed on this hart between setting a hart's cpumask bit
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 56abab6a..6ab2bb1b 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -50,27 +50,33 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> void __init setup_smp(void)
> {
> struct device_node *dn = NULL;
> - int hart, im_okay_therefore_i_am = 0;
> + int hart, found_boot_cpu = 0;
> + int cpuid = 1;
>
> while ((dn = of_find_node_by_type(dn, "cpu"))) {
> hart = riscv_of_processor_hart(dn);
> - if (hart >= 0) {
> - set_cpu_possible(hart, true);
> - set_cpu_present(hart, true);
> - if (hart == smp_processor_id()) {
> - BUG_ON(im_okay_therefore_i_am);
> - im_okay_therefore_i_am = 1;
> - }
> +
> + if (hart < 0)
> + continue;
> + if (hart == cpu_logical_map(0)) {
> + BUG_ON(found_boot_cpu);
> + found_boot_cpu = 1;
> + continue;
> }
> +
> + cpu_logical_map(cpuid) = hart;
> + set_cpu_possible(cpuid, true);
> + set_cpu_present(cpuid, true);
> + cpuid++;
> }
>
> - BUG_ON(!im_okay_therefore_i_am);
> + BUG_ON(!found_boot_cpu);
> }
>
> int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> {
> + int hartid = cpu_logical_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
> @@ -79,8 +85,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> * the spinning harts that they can continue the boot process.
> */
> smp_mb();
> - __cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
> - __cpu_up_task_pointer[cpu] = tidle;
> + __cpu_up_stack_pointer[hartid] = task_stack_page(tidle) + THREAD_SIZE;
> + __cpu_up_task_pointer[hartid] = tidle;
>
> while (!cpu_online(cpu))
> cpu_relax();
> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
> index 4e8b347e..f1f205e5 100644
> --- a/drivers/clocksource/riscv_timer.c
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -8,6 +8,7 @@
> #include <linux/cpu.h>
> #include <linux/delay.h>
> #include <linux/irq.h>
> +#include <asm/smp.h>
> #include <asm/sbi.h>
>
> /*
> @@ -84,13 +85,16 @@ void riscv_timer_interrupt(void)
>
> static int __init riscv_timer_init_dt(struct device_node *n)
> {
> - int cpu_id = riscv_of_processor_hart(n), error;
> + int cpuid, hartid, error;
> struct clocksource *cs;
>
> - if (cpu_id != smp_processor_id())
> + hartid = riscv_of_processor_hart(n);
> + cpuid = riscv_hartid_to_cpuid(hartid);
> +
> + if (cpuid != smp_processor_id())
> return 0;
>
> - cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
> + cs = per_cpu_ptr(&riscv_clocksource, cpuid);
> clocksource_register_hz(cs, riscv_timebase);
>
> error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
> @@ -98,7 +102,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
> riscv_timer_starting_cpu, riscv_timer_dying_cpu);
> if (error)
> pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
> - error, cpu_id);
> + error, cpuid);
> return error;
> }
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 298685e5..2eb2e78c 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -15,6 +15,7 @@
> #include <linux/of_irq.h>
> #include <linux/platform_device.h>
> #include <linux/spinlock.h>
> +#include <asm/smp.h>
>
> /*
> * This driver implements a version of the RISC-V PLIC with the actual layout
> @@ -93,10 +94,11 @@ static inline void plic_toggle(int ctxid, int hwirq, int enable)
> static inline void plic_irq_toggle(struct irq_data *d, int enable)
> {
> int cpu;
> + struct plic_handler *handler;
>
> writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
> for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
> - struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> + handler = per_cpu_ptr(&plic_handlers, cpu);
>
> if (handler->present)
> plic_toggle(handler->ctxid, d->hwirq, enable);
> @@ -217,7 +219,7 @@ static int __init plic_init(struct device_node *node,
> struct of_phandle_args parent;
> struct plic_handler *handler;
> irq_hw_number_t hwirq;
> - int cpu;
> + int cpu, hartid;
>
> if (of_irq_parse_one(node, i, &parent)) {
> pr_err("failed to parse parent for context %d.\n", i);
> @@ -228,12 +230,13 @@ static int __init plic_init(struct device_node *node,
> if (parent.args[0] == -1)
> continue;
>
> - cpu = plic_find_hart_id(parent.np);
> - if (cpu < 0) {
> + hartid = plic_find_hart_id(parent.np);
> + if (hartid < 0) {
> pr_warn("failed to parse hart ID for context %d.\n", i);
> continue;
> }
>
> + cpu = riscv_hartid_to_cpuid(hartid);
> handler = per_cpu_ptr(&plic_handlers, cpu);
> handler->present = true;
> handler->ctxid = i;
> --
> 2.7.4
>

Regards,
Anup

2018-08-16 08:26:24

by Anup Patel

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
> Defining cpu_operations now helps adding cpu hotplug
> support in proper manner. Moreover, it provides flexibility
> in supporting other cpu enable/boot methods can be
> supported in future. This patch has been largely inspired from
> ARM64. However, a default boot method is defined for RISC-V unlike
> ARM64.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/smp.h | 10 ++++++++++
> arch/riscv/kernel/smp.c | 8 ++++++++
> arch/riscv/kernel/smpboot.c | 34 ++++++++++++++++++++++++++++------
> 3 files changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index 0763337b..2bb6e6c2 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -28,6 +28,15 @@
> extern u64 __cpu_logical_map[NR_CPUS];
> #define cpu_logical_map(cpu) __cpu_logical_map[cpu]
>
> +struct cpu_operations {
> + const char *name;
> + int (*cpu_init)(unsigned int);
> + int (*cpu_prepare)(unsigned int);
> + int (*cpu_boot)(unsigned int, struct task_struct *);
> +};
> +extern struct cpu_operations cpu_ops;
> +void smp_set_cpu_ops(const struct cpu_operations *cpu_ops);
> +
> #ifdef CONFIG_SMP
>
> /* SMP initialization hook for setup_arch */
> @@ -57,5 +66,6 @@ static inline void cpuid_to_hartid_mask(const struct cpumask *in,
> cpumask_set_cpu(cpu_logical_map(0), out);
> }
>
> +
> #endif /* CONFIG_SMP */
> #endif /* _ASM_RISCV_SMP_H */
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index 4ab70480..5de58ced 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -58,6 +58,14 @@ void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out)
> for_each_cpu(cpu, in)
> cpumask_set_cpu(cpu_logical_map(cpu), out);
> }
> +struct cpu_operations cpu_ops __ro_after_init;
> +
> +void smp_set_cpu_ops(const struct cpu_operations *ops)
> +{
> + if (ops)
> + cpu_ops = *ops;
> +}
> +

Move both cpu_ops and smp_set_cpu_ops() to smpboot.c. This way
you will not require to make cpu_ops as global.

Further, I think cpu_ops should be a pointer and initial value should
be &default_ops.

Something like this:
struct cpu_operations *cpu_ops __ro_after_init = &default_ops;

> /* Unsupported */
> int setup_profiling_timer(unsigned int multiplier)
> {
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 6ab2bb1b..045a1a45 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -30,6 +30,7 @@
> #include <linux/irq.h>
> #include <linux/of.h>
> #include <linux/sched/task_stack.h>
> +#include <linux/smp.h>
> #include <asm/irq.h>
> #include <asm/mmu_context.h>
> #include <asm/tlbflush.h>
> @@ -38,6 +39,7 @@
>
> void *__cpu_up_stack_pointer[NR_CPUS];
> void *__cpu_up_task_pointer[NR_CPUS];
> +struct cpu_operations default_ops;
>
> void __init smp_prepare_boot_cpu(void)
> {
> @@ -53,6 +55,7 @@ void __init setup_smp(void)
> int hart, found_boot_cpu = 0;
> int cpuid = 1;
>
> + smp_set_cpu_ops(&default_ops);
> while ((dn = of_find_node_by_type(dn, "cpu"))) {
> hart = riscv_of_processor_hart(dn);
>
> @@ -73,10 +76,8 @@ void __init setup_smp(void)
> BUG_ON(!found_boot_cpu);
> }
>
> -int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> +int default_cpu_boot(unsigned int hartid, struct task_struct *tidle)
> {
> - int hartid = cpu_logical_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
> @@ -84,13 +85,28 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> * setup by that main hart. Writing __cpu_up_stack_pointer signals to
> * the spinning harts that they can continue the boot process.
> */
> - smp_mb();
> +
> __cpu_up_stack_pointer[hartid] = task_stack_page(tidle) + THREAD_SIZE;
> __cpu_up_task_pointer[hartid] = tidle;
> + return 0;
> +}
> +
> +int __cpu_up(unsigned int cpu, struct task_struct *tidle)
> +{
> + int err = -1;
> + int hartid = cpu_logical_map(cpu);
>
> - while (!cpu_online(cpu))
> - cpu_relax();
> + tidle->thread_info.cpu = cpu;
> + smp_mb();
>
> + if (cpu_ops.cpu_boot)
> + err = cpu_ops.cpu_boot(hartid, tidle);
> + if (!err) {
> + while (!cpu_online(cpu))
> + cpu_relax();
> + } else {
> + pr_err("CPU %d [hartid %d]failed to boot\n", cpu, hartid);
> + }
> return 0;
> }
>
> @@ -117,3 +133,9 @@ asmlinkage void __init smp_callin(void)
> preempt_disable();
> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
> }
> +
> +
> +struct cpu_operations default_ops = {
> + .name = "default",
> + .cpu_boot = default_cpu_boot,
> +};

I think having dedicated source file for default_ops makes more sense
so that we have a clear/simple reference implementation of cpu_operations.

Eventually, we might have one source file for each type of SMP enable
method.

Try to keep smpboot.c generic and independent of any cpu_operations.
What say?

Regards,
Anup

2018-08-16 08:46:47

by Atish Patra

Subject: Re: [RFC PATCH 1/5] RISC-V: Add logical CPU indexing for RISC-V

On 8/15/18 9:06 PM, Anup Patel wrote:
> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>> Currently, both linux cpu id and hardware cpu id are same.
>> This is not recommended as it will lead to discontinuous cpu
>> indexing in Linux. Moreover, kdump kernel will run from CPU0
>> which would be absent if we follow existing scheme.
>>
>> Implement a logical mapping between Linux cpu id and hardware
>> cpuid to decouple these two. Always mark the boot processor as
>> cpu0 and all other cpus get the logical cpu id based on their
>> booting order.
>>
>> Signed-off-by: Atish Patra <[email protected]>
>> ---
>> arch/riscv/include/asm/smp.h | 17 ++++++++++++++++-
>> arch/riscv/kernel/setup.c | 2 ++
>> arch/riscv/kernel/smp.c | 19 +++++++++++++++++++
>> 3 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
>> index 36016845..0763337b 100644
>> --- a/arch/riscv/include/asm/smp.h
>> +++ b/arch/riscv/include/asm/smp.h
>> @@ -22,6 +22,12 @@
>> #include <linux/cpumask.h>
>> #include <linux/irqreturn.h>
>>
>> +/*
>> + * Mapping between linux logical cpu index and hartid.
>> + */
>> +extern u64 __cpu_logical_map[NR_CPUS];
>> +#define cpu_logical_map(cpu) __cpu_logical_map[cpu]
>> +
>> #ifdef CONFIG_SMP
>>
>> /* SMP initialization hook for setup_arch */
>> @@ -33,6 +39,8 @@ void arch_send_call_function_ipi_mask(struct cpumask *mask);
>> /* Hook for the generic smp_call_function_single() routine. */
>> void arch_send_call_function_single_ipi(int cpu);
>>
>> +int riscv_hartid_to_cpuid(int hartid);
>> +void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out);
>> /*
>> * This is particularly ugly: it appears we can't actually get the definition
>> * of task_struct here, but we need access to the CPU this task is running on.
>> @@ -41,6 +49,13 @@ void arch_send_call_function_single_ipi(int cpu);
>> */
>> #define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_CPU)))
>>
>> -#endif /* CONFIG_SMP */
>> +#else
>> +
>> +static inline int riscv_hartid_to_cpuid(int hartid) { return 0 ; }
>> +static inline void cpuid_to_hartid_mask(const struct cpumask *in,
>> + struct cpumask *out) {
>> + cpumask_set_cpu(cpu_logical_map(0), out);
>> +}
>>
>> +#endif /* CONFIG_SMP */
>> #endif /* _ASM_RISCV_SMP_H */
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index db20dc63..e21ed481 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -82,6 +82,8 @@ EXPORT_SYMBOL(empty_zero_page);
>> /* The lucky hart to first increment this variable will boot the other cores */
>> atomic_t hart_lottery;
>>
>> +u64 __cpu_logical_map[NR_CPUS];
>
> If hardware IDs are always machine word size then its better to use
> "unsigned long" in-place of u64.
>

Good point. Thanks.

> The __cpu_logical_map[] should be zero initially because zero is a
> valid hardware ID. Better set all entries to -1 by assigning { -1 } to the
> array.
>

ok. I will introduce INVALID_HART_ID (=-1) and initialize with that.

> Also, I feel __cpu_logical_map[] should be part of smp.c instead of
> setup.c. Any particular reason for having it in setup.c?
>

currently smp.c is only compiled in if CONFIG_SMP. That's why I kept it
in setup.c


Regards,
Atish
>> +
>> #ifdef CONFIG_BLK_DEV_INITRD
>> static void __init setup_initrd(void)
>> {
>> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
>> index 906fe21e..d55379ee 100644
>> --- a/arch/riscv/kernel/smp.c
>> +++ b/arch/riscv/kernel/smp.c
>> @@ -38,7 +38,26 @@ enum ipi_message_type {
>> IPI_MAX
>> };
>>
>> +int riscv_hartid_to_cpuid(int hartid)
>> +{
>> + int i = -1;
>> +
>> + for (i = 0; i < NR_CPUS; i++)
>> + if (cpu_logical_map(i) == hartid)
>> + return i;
>> +
>> + pr_err("Couldn't find cpu id for hartid [%d]\n", hartid);
>> + BUG();
>> + return i;
>> +}
>>
>> +void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out)
>
> May be rename cpuid_to_hartid_mask() to riscv_cpuid_to_hartid_mask()
> for consistency.
>
>> +{
>> + int cpu;
>> +
>> + for_each_cpu(cpu, in)
>> + cpumask_set_cpu(cpu_logical_map(cpu), out);
>> +}
>> /* Unsupported */
>> int setup_profiling_timer(unsigned int multiplier)
>> {
>> --
>> 2.7.4
>>
>

2018-08-16 08:58:22

by Atish Patra

Subject: Re: [RFC PATCH 2/5] RISC-V: Use Linux logical cpu number instead of hartid

On 8/15/18 9:24 PM, Anup Patel wrote:
> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>> Setup the cpu_logical_map during boot. Moreover, every SBI call
>> and PLIC context are based on the physical hartid. Use the logical
>> cpu to hartid mapping to pass correct hartid to respective functions.
>>
>> Signed-off-by: Atish Patra <[email protected]>
>> ---
>> arch/riscv/include/asm/tlbflush.h | 17 +++++++++++++----
>> arch/riscv/kernel/cpu.c | 4 +++-
>> arch/riscv/kernel/setup.c | 10 ++++++++++
>> arch/riscv/kernel/smp.c | 24 +++++++++++++++---------
>> arch/riscv/kernel/smpboot.c | 30 ++++++++++++++++++------------
>> drivers/clocksource/riscv_timer.c | 12 ++++++++----
>> drivers/irqchip/irq-sifive-plic.c | 11 +++++++----
>> 7 files changed, 74 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/tlbflush.h b/arch/riscv/include/asm/tlbflush.h
>> index 85c2d8ba..ecfd9b0e 100644
>> --- a/arch/riscv/include/asm/tlbflush.h
>> +++ b/arch/riscv/include/asm/tlbflush.h
>> @@ -16,6 +16,7 @@
>> #define _ASM_RISCV_TLBFLUSH_H
>>
>> #include <linux/mm_types.h>
>> +#include <asm/smp.h>
>>
>> /*
>> * Flush entire local TLB. 'sfence.vma' implicitly fences with the instruction
>> @@ -49,13 +50,21 @@ static inline void flush_tlb_range(struct vm_area_struct *vma,
>>
>> #include <asm/sbi.h>
>>
>> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
>> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
>> + unsigned long size)
>> +{
>> + struct cpumask hmask;
>> +
>> + cpuid_to_hartid_mask(cmask, &hmask);
>> + sbi_remote_sfence_vma(hmask.bits, start, size);
>> +}
>> +
>> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
>> #define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
>> #define flush_tlb_range(vma, start, end) \
>> - sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
>> - start, (end) - (start))
>> + remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) - (start))
>> #define flush_tlb_mm(mm) \
>> - sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)
>> + remote_sfence_vma(mm_cpumask(mm), 0, -1)
>>
>> #endif /* CONFIG_SMP */
>>
>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>> index ca6c81e5..f8a18ace 100644
>> --- a/arch/riscv/kernel/cpu.c
>> +++ b/arch/riscv/kernel/cpu.c
>> @@ -14,6 +14,7 @@
>> #include <linux/init.h>
>> #include <linux/seq_file.h>
>> #include <linux/of.h>
>> +#include <asm/smp.h>
>>
>> /* Return -1 if not a valid hart */
>> int riscv_of_processor_hart(struct device_node *node)
>> @@ -79,7 +80,8 @@ static void c_stop(struct seq_file *m, void *v)
>> static int c_show(struct seq_file *m, void *v)
>> {
>> unsigned long hart_id = (unsigned long)v - 1;
>> - struct device_node *node = of_get_cpu_node(hart_id, NULL);
>> + struct device_node *node = of_get_cpu_node(cpu_logical_map(hart_id),
>> + NULL);
>
> The hart_id is misleading name here. It should be cpu_id. Please replace
> all instances of hart_id with cpu_id and where hard ID is to be displayed
> use cpu_logical_map(cpu_id).
>
Correct. Thanks for catching it. I will fix it in v2.


>> const char *compat, *isa, *mmu;
>>
>> seq_printf(m, "hart\t: %lu\n", hart_id);
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index e21ed481..97b586f8 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -84,6 +84,16 @@ atomic_t hart_lottery;
>>
>> u64 __cpu_logical_map[NR_CPUS];
>>
>> +void __init smp_setup_processor_id(void)
>> +{
>> + int cpu = smp_processor_id();
>> +
>> + cpu_logical_map(0) = cpu;
>
> I think this should be:
> cpu_logical_map(cpu) = hart_id;
>
> Here hart_id for boot CPU will be value of a0 register passed at boot-time.
>
I will change the variable name to hart_id. The assembly code in head.S
have already stored hart id in thread info structure. So
smp_processor_id() and hart id would be the same.


Regards,
Atish
>> +
>> + /* Change the boot cpu ID in thread_info */
>> + current->thread_info.cpu = 0;
>> +}
>> +
>> #ifdef CONFIG_BLK_DEV_INITRD
>> static void __init setup_initrd(void)
>> {
>> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
>> index d55379ee..4ab70480 100644
>> --- a/arch/riscv/kernel/smp.c
>> +++ b/arch/riscv/kernel/smp.c
>> @@ -97,14 +97,18 @@ void riscv_software_interrupt(void)
>> static void
>> send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
>> {
>> - int i;
>> + int cpuid, hartid;
>> + struct cpumask hartid_mask;
>>
>> + cpumask_clear(&hartid_mask);
>> mb();
>> - for_each_cpu(i, to_whom)
>> - set_bit(operation, &ipi_data[i].bits);
>> -
>> + for_each_cpu(cpuid, to_whom) {
>> + set_bit(operation, &ipi_data[cpuid].bits);
>> + hartid = cpu_logical_map(cpuid);
>> + cpumask_set_cpu(hartid, &hartid_mask);
>> + }
>> mb();
>> - sbi_send_ipi(cpumask_bits(to_whom));
>> + sbi_send_ipi(cpumask_bits(&hartid_mask));
>> }
>>
>> void arch_send_call_function_ipi_mask(struct cpumask *mask)
>> @@ -146,7 +150,7 @@ void smp_send_reschedule(int cpu)
>> void flush_icache_mm(struct mm_struct *mm, bool local)
>> {
>> unsigned int cpu;
>> - cpumask_t others, *mask;
>> + cpumask_t others, hmask, *mask;
>>
>> preempt_disable();
>>
>> @@ -164,9 +168,11 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
>> */
>> cpumask_andnot(&others, mm_cpumask(mm), cpumask_of(cpu));
>> local |= cpumask_empty(&others);
>> - if (mm != current->active_mm || !local)
>> - sbi_remote_fence_i(others.bits);
>> - else {
>> + if (mm != current->active_mm || !local) {
>> + cpumask_clear(&hmask);
>> + cpuid_to_hartid_mask(&others, &hmask);
>> + sbi_remote_fence_i(hmask.bits);
>> + } else {
>> /*
>> * It's assumed that at least one strongly ordered operation is
>> * performed on this hart between setting a hart's cpumask bit
>> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>> index 56abab6a..6ab2bb1b 100644
>> --- a/arch/riscv/kernel/smpboot.c
>> +++ b/arch/riscv/kernel/smpboot.c
>> @@ -50,27 +50,33 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>> void __init setup_smp(void)
>> {
>> struct device_node *dn = NULL;
>> - int hart, im_okay_therefore_i_am = 0;
>> + int hart, found_boot_cpu = 0;
>> + int cpuid = 1;
>>
>> while ((dn = of_find_node_by_type(dn, "cpu"))) {
>> hart = riscv_of_processor_hart(dn);
>> - if (hart >= 0) {
>> - set_cpu_possible(hart, true);
>> - set_cpu_present(hart, true);
>> - if (hart == smp_processor_id()) {
>> - BUG_ON(im_okay_therefore_i_am);
>> - im_okay_therefore_i_am = 1;
>> - }
>> +
>> + if (hart < 0)
>> + continue;
>> + if (hart == cpu_logical_map(0)) {
>> + BUG_ON(found_boot_cpu);
>> + found_boot_cpu = 1;
>> + continue;
>> }
>> +
>> + cpu_logical_map(cpuid) = hart;
>> + set_cpu_possible(cpuid, true);
>> + set_cpu_present(cpuid, true);
>> + cpuid++;
>> }
>>
>> - BUG_ON(!im_okay_therefore_i_am);
>> + BUG_ON(!found_boot_cpu);
>> }
>>
>> int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> {
>> + int hartid = cpu_logical_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
>> @@ -79,8 +85,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> * the spinning harts that they can continue the boot process.
>> */
>> smp_mb();
>> - __cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
>> - __cpu_up_task_pointer[cpu] = tidle;
>> + __cpu_up_stack_pointer[hartid] = task_stack_page(tidle) + THREAD_SIZE;
>> + __cpu_up_task_pointer[hartid] = tidle;
>>
>> while (!cpu_online(cpu))
>> cpu_relax();
>> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
>> index 4e8b347e..f1f205e5 100644
>> --- a/drivers/clocksource/riscv_timer.c
>> +++ b/drivers/clocksource/riscv_timer.c
>> @@ -8,6 +8,7 @@
>> #include <linux/cpu.h>
>> #include <linux/delay.h>
>> #include <linux/irq.h>
>> +#include <asm/smp.h>
>> #include <asm/sbi.h>
>>
>> /*
>> @@ -84,13 +85,16 @@ void riscv_timer_interrupt(void)
>>
>> static int __init riscv_timer_init_dt(struct device_node *n)
>> {
>> - int cpu_id = riscv_of_processor_hart(n), error;
>> + int cpuid, hartid, error;
>> struct clocksource *cs;
>>
>> - if (cpu_id != smp_processor_id())
>> + hartid = riscv_of_processor_hart(n);
>> + cpuid = riscv_hartid_to_cpuid(hartid);
>> +
>> + if (cpuid != smp_processor_id())
>> return 0;
>>
>> - cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
>> + cs = per_cpu_ptr(&riscv_clocksource, cpuid);
>> clocksource_register_hz(cs, riscv_timebase);
>>
>> error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
>> @@ -98,7 +102,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>> riscv_timer_starting_cpu, riscv_timer_dying_cpu);
>> if (error)
>> pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
>> - error, cpu_id);
>> + error, cpuid);
>> return error;
>> }
>>
>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>> index 298685e5..2eb2e78c 100644
>> --- a/drivers/irqchip/irq-sifive-plic.c
>> +++ b/drivers/irqchip/irq-sifive-plic.c
>> @@ -15,6 +15,7 @@
>> #include <linux/of_irq.h>
>> #include <linux/platform_device.h>
>> #include <linux/spinlock.h>
>> +#include <asm/smp.h>
>>
>> /*
>> * This driver implements a version of the RISC-V PLIC with the actual layout
>> @@ -93,10 +94,11 @@ static inline void plic_toggle(int ctxid, int hwirq, int enable)
>> static inline void plic_irq_toggle(struct irq_data *d, int enable)
>> {
>> int cpu;
>> + struct plic_handler *handler;
>>
>> writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
>> for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
>> - struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
>> + handler = per_cpu_ptr(&plic_handlers, cpu);
>>
>> if (handler->present)
>> plic_toggle(handler->ctxid, d->hwirq, enable);
>> @@ -217,7 +219,7 @@ static int __init plic_init(struct device_node *node,
>> struct of_phandle_args parent;
>> struct plic_handler *handler;
>> irq_hw_number_t hwirq;
>> - int cpu;
>> + int cpu, hartid;
>>
>> if (of_irq_parse_one(node, i, &parent)) {
>> pr_err("failed to parse parent for context %d.\n", i);
>> @@ -228,12 +230,13 @@ static int __init plic_init(struct device_node *node,
>> if (parent.args[0] == -1)
>> continue;
>>
>> - cpu = plic_find_hart_id(parent.np);
>> - if (cpu < 0) {
>> + hartid = plic_find_hart_id(parent.np);
>> + if (hartid < 0) {
>> pr_warn("failed to parse hart ID for context %d.\n", i);
>> continue;
>> }
>>
>> + cpu = riscv_hartid_to_cpuid(hartid);
>> handler = per_cpu_ptr(&plic_handlers, cpu);
>> handler->present = true;
>> handler->ctxid = i;
>> --
>> 2.7.4
>>
>
> Regards,
> Anup
>

2018-08-16 09:24:16

by Anup Patel

Subject: Re: [RFC PATCH 1/5] RISC-V: Add logical CPU indexing for RISC-V

On Thu, Aug 16, 2018 at 10:47 AM, Atish Patra <[email protected]> wrote:
> On 8/15/18 9:06 PM, Anup Patel wrote:
>>
>> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>>>
>>> Currently, both linux cpu id and hardware cpu id are same.
>>> This is not recommended as it will lead to discontinuous cpu
>>> indexing in Linux. Moreover, kdump kernel will run from CPU0
>>> which would be absent if we follow existing scheme.
>>>
>>> Implement a logical mapping between Linux cpu id and hardware
>>> cpuid to decouple these two. Always mark the boot processor as
>>> cpu0 and all other cpus get the logical cpu id based on their
>>> booting order.
>>>
>>> Signed-off-by: Atish Patra <[email protected]>
>>> ---
>>> arch/riscv/include/asm/smp.h | 17 ++++++++++++++++-
>>> arch/riscv/kernel/setup.c | 2 ++
>>> arch/riscv/kernel/smp.c | 19 +++++++++++++++++++
>>> 3 files changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
>>> index 36016845..0763337b 100644
>>> --- a/arch/riscv/include/asm/smp.h
>>> +++ b/arch/riscv/include/asm/smp.h
>>> @@ -22,6 +22,12 @@
>>> #include <linux/cpumask.h>
>>> #include <linux/irqreturn.h>
>>>
>>> +/*
>>> + * Mapping between linux logical cpu index and hartid.
>>> + */
>>> +extern u64 __cpu_logical_map[NR_CPUS];
>>> +#define cpu_logical_map(cpu) __cpu_logical_map[cpu]
>>> +
>>> #ifdef CONFIG_SMP
>>>
>>> /* SMP initialization hook for setup_arch */
>>> @@ -33,6 +39,8 @@ void arch_send_call_function_ipi_mask(struct cpumask
>>> *mask);
>>> /* Hook for the generic smp_call_function_single() routine. */
>>> void arch_send_call_function_single_ipi(int cpu);
>>>
>>> +int riscv_hartid_to_cpuid(int hartid);
>>> +void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask
>>> *out);
>>> /*
>>> * This is particularly ugly: it appears we can't actually get the
>>> definition
>>> * of task_struct here, but we need access to the CPU this task is
>>> running on.
>>> @@ -41,6 +49,13 @@ void arch_send_call_function_single_ipi(int cpu);
>>> */
>>> #define raw_smp_processor_id() (*((int*)((char*)get_current() +
>>> TASK_TI_CPU)))
>>>
>>> -#endif /* CONFIG_SMP */
>>> +#else
>>> +
>>> +static inline int riscv_hartid_to_cpuid(int hartid) { return 0 ; }
>>> +static inline void cpuid_to_hartid_mask(const struct cpumask *in,
>>> + struct cpumask *out) {
>>> + cpumask_set_cpu(cpu_logical_map(0), out);
>>> +}
>>>
>>> +#endif /* CONFIG_SMP */
>>> #endif /* _ASM_RISCV_SMP_H */
>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>> index db20dc63..e21ed481 100644
>>> --- a/arch/riscv/kernel/setup.c
>>> +++ b/arch/riscv/kernel/setup.c
>>> @@ -82,6 +82,8 @@ EXPORT_SYMBOL(empty_zero_page);
>>> /* The lucky hart to first increment this variable will boot the other
>>> cores */
>>> atomic_t hart_lottery;
>>>
>>> +u64 __cpu_logical_map[NR_CPUS];
>>
>>
>> If hardware IDs are always machine word size then its better to use
>> "unsigned long" in-place of u64.
>>
>
> Good point. Thanks.
>
>> The __cpu_logical_map[] should be zero initially because zero is a
>> valid hardware ID. Better set all entries to -1 by assigning { -1 } to the
>> array.
>>
>
> ok. I will introduce INVALID_HART_ID (=-1) and initialize with that.
>
>> Also, I feel __cpu_logical_map[] should be part of smp.c instead of
>> setup.c. Any particular reason for having it in setup.c?
>>
>
> currently smp.c is only compiled in if CONFIG_SMP. That's why I kept it in
> setup.c

Ahh, got it. For now let it be here till we figure better place for it.

Regards,
Anup

2018-08-16 09:24:17

by Atish Patra

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On 8/15/18 10:02 PM, Anup Patel wrote:
> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>> Defining cpu_operations now helps adding cpu hotplug
>> support in proper manner. Moreover, it provides flexibility
>> in supporting other cpu enable/boot methods can be
>> supported in future. This patch has been largely inspired from
>> ARM64. However, a default boot method is defined for RISC-V unlike
>> ARM64.
>>
>> Signed-off-by: Atish Patra <[email protected]>
>> ---
>> arch/riscv/include/asm/smp.h | 10 ++++++++++
>> arch/riscv/kernel/smp.c | 8 ++++++++
>> arch/riscv/kernel/smpboot.c | 34 ++++++++++++++++++++++++++++------
>> 3 files changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
>> index 0763337b..2bb6e6c2 100644
>> --- a/arch/riscv/include/asm/smp.h
>> +++ b/arch/riscv/include/asm/smp.h
>> @@ -28,6 +28,15 @@
>> extern u64 __cpu_logical_map[NR_CPUS];
>> #define cpu_logical_map(cpu) __cpu_logical_map[cpu]
>>
>> +struct cpu_operations {
>> + const char *name;
>> + int (*cpu_init)(unsigned int);
>> + int (*cpu_prepare)(unsigned int);
>> + int (*cpu_boot)(unsigned int, struct task_struct *);
>> +};
>> +extern struct cpu_operations cpu_ops;
>> +void smp_set_cpu_ops(const struct cpu_operations *cpu_ops);
>> +
>> #ifdef CONFIG_SMP
>>
>> /* SMP initialization hook for setup_arch */
>> @@ -57,5 +66,6 @@ static inline void cpuid_to_hartid_mask(const struct cpumask *in,
>> cpumask_set_cpu(cpu_logical_map(0), out);
>> }
>>
>> +
>> #endif /* CONFIG_SMP */
>> #endif /* _ASM_RISCV_SMP_H */
>> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
>> index 4ab70480..5de58ced 100644
>> --- a/arch/riscv/kernel/smp.c
>> +++ b/arch/riscv/kernel/smp.c
>> @@ -58,6 +58,14 @@ void cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out)
>> for_each_cpu(cpu, in)
>> cpumask_set_cpu(cpu_logical_map(cpu), out);
>> }
>> +struct cpu_operations cpu_ops __ro_after_init;
>> +
>> +void smp_set_cpu_ops(const struct cpu_operations *ops)
>> +{
>> + if (ops)
>> + cpu_ops = *ops;
>> +}
>> +
>
> Move both cpu_ops and smp_set_cpu_ops() to smpboot.c. This way
> you will not require to make cpu_ops as global.
>
ok.

> Further, I think cpu_ops should be a pointer and initial value should
> be &default_ops.
>
> Something like this:
> struct cpu_operations *cpu_ops __ro_after_init = &default_ops;
>

That will work too. However, setting it through smp_set_cpu_ops provides
a sample implementation for any future SMP enable methods. That's why I
used the API. I can change it if you think otherwise.


>> /* Unsupported */
>> int setup_profiling_timer(unsigned int multiplier)
>> {
>> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>> index 6ab2bb1b..045a1a45 100644
>> --- a/arch/riscv/kernel/smpboot.c
>> +++ b/arch/riscv/kernel/smpboot.c
>> @@ -30,6 +30,7 @@
>> #include <linux/irq.h>
>> #include <linux/of.h>
>> #include <linux/sched/task_stack.h>
>> +#include <linux/smp.h>
>> #include <asm/irq.h>
>> #include <asm/mmu_context.h>
>> #include <asm/tlbflush.h>
>> @@ -38,6 +39,7 @@
>>
>> void *__cpu_up_stack_pointer[NR_CPUS];
>> void *__cpu_up_task_pointer[NR_CPUS];
>> +struct cpu_operations default_ops;
>>
>> void __init smp_prepare_boot_cpu(void)
>> {
>> @@ -53,6 +55,7 @@ void __init setup_smp(void)
>> int hart, found_boot_cpu = 0;
>> int cpuid = 1;
>>
>> + smp_set_cpu_ops(&default_ops);
>> while ((dn = of_find_node_by_type(dn, "cpu"))) {
>> hart = riscv_of_processor_hart(dn);
>>
>> @@ -73,10 +76,8 @@ void __init setup_smp(void)
>> BUG_ON(!found_boot_cpu);
>> }
>>
>> -int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> +int default_cpu_boot(unsigned int hartid, struct task_struct *tidle)
>> {
>> - int hartid = cpu_logical_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
>> @@ -84,13 +85,28 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> * setup by that main hart. Writing __cpu_up_stack_pointer signals to
>> * the spinning harts that they can continue the boot process.
>> */
>> - smp_mb();
>> +
>> __cpu_up_stack_pointer[hartid] = task_stack_page(tidle) + THREAD_SIZE;
>> __cpu_up_task_pointer[hartid] = tidle;
>> + return 0;
>> +}
>> +
>> +int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>> +{
>> + int err = -1;
>> + int hartid = cpu_logical_map(cpu);
>>
>> - while (!cpu_online(cpu))
>> - cpu_relax();
>> + tidle->thread_info.cpu = cpu;
>> + smp_mb();
>>
>> + if (cpu_ops.cpu_boot)
>> + err = cpu_ops.cpu_boot(hartid, tidle);
>> + if (!err) {
>> + while (!cpu_online(cpu))
>> + cpu_relax();
>> + } else {
>> + pr_err("CPU %d [hartid %d]failed to boot\n", cpu, hartid);
>> + }
>> return 0;
>> }
>>
>> @@ -117,3 +133,9 @@ asmlinkage void __init smp_callin(void)
>> preempt_disable();
>> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
>> }
>> +
>> +
>> +struct cpu_operations default_ops = {
>> + .name = "default",
>> + .cpu_boot = default_cpu_boot,
>> +};
>
> I think having dedicated source file for default_ops makes more sense
> so that we have a clear/simple reference implementation of cpu_operations.
>
> Eventually, we might have one source file for each type of SMP enable
> method.
>
> Try to keep smpboot.c generic and independent of any cpu_operations.
> What say?
>

Any other SMP enable method should definitely get its own file. I am not
sure about the default one though. As default_ops is truly the default
booting method which will always be present in absence of anything else,
I thought keeping it smpboot.c emphasizes that point. Moreover, it's not
that big even. But I am open to moving to it's own source file if you
strongly think we should do that.


Regards,
Atish
> Regards,
> Anup
>

2018-08-16 09:24:52

by Anup Patel

Subject: Re: [RFC PATCH 2/5] RISC-V: Use Linux logical cpu number instead of hartid

On Thu, Aug 16, 2018 at 10:53 AM, Atish Patra <[email protected]> wrote:
> On 8/15/18 9:24 PM, Anup Patel wrote:
>>
>> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>>>
>>> Setup the cpu_logical_map during boot. Moreover, every SBI call
>>> and PLIC context are based on the physical hartid. Use the logical
>>> cpu to hartid mapping to pass correct hartid to respective functions.
>>>
>>> Signed-off-by: Atish Patra <[email protected]>
>>> ---
>>> arch/riscv/include/asm/tlbflush.h | 17 +++++++++++++----
>>> arch/riscv/kernel/cpu.c | 4 +++-
>>> arch/riscv/kernel/setup.c | 10 ++++++++++
>>> arch/riscv/kernel/smp.c | 24 +++++++++++++++---------
>>> arch/riscv/kernel/smpboot.c | 30 ++++++++++++++++++------------
>>> drivers/clocksource/riscv_timer.c | 12 ++++++++----
>>> drivers/irqchip/irq-sifive-plic.c | 11 +++++++----
>>> 7 files changed, 74 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/tlbflush.h
>>> b/arch/riscv/include/asm/tlbflush.h
>>> index 85c2d8ba..ecfd9b0e 100644
>>> --- a/arch/riscv/include/asm/tlbflush.h
>>> +++ b/arch/riscv/include/asm/tlbflush.h
>>> @@ -16,6 +16,7 @@
>>> #define _ASM_RISCV_TLBFLUSH_H
>>>
>>> #include <linux/mm_types.h>
>>> +#include <asm/smp.h>
>>>
>>> /*
>>> * Flush entire local TLB. 'sfence.vma' implicitly fences with the
>>> instruction
>>> @@ -49,13 +50,21 @@ static inline void flush_tlb_range(struct
>>> vm_area_struct *vma,
>>>
>>> #include <asm/sbi.h>
>>>
>>> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
>>> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned
>>> long start,
>>> + unsigned long size)
>>> +{
>>> + struct cpumask hmask;
>>> +
>>> + cpuid_to_hartid_mask(cmask, &hmask);
>>> + sbi_remote_sfence_vma(hmask.bits, start, size);
>>> +}
>>> +
>>> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
>>> #define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
>>> #define flush_tlb_range(vma, start, end) \
>>> - sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
>>> - start, (end) - (start))
>>> + remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) -
>>> (start))
>>> #define flush_tlb_mm(mm) \
>>> - sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)
>>> + remote_sfence_vma(mm_cpumask(mm), 0, -1)
>>>
>>> #endif /* CONFIG_SMP */
>>>
>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>> index ca6c81e5..f8a18ace 100644
>>> --- a/arch/riscv/kernel/cpu.c
>>> +++ b/arch/riscv/kernel/cpu.c
>>> @@ -14,6 +14,7 @@
>>> #include <linux/init.h>
>>> #include <linux/seq_file.h>
>>> #include <linux/of.h>
>>> +#include <asm/smp.h>
>>>
>>> /* Return -1 if not a valid hart */
>>> int riscv_of_processor_hart(struct device_node *node)
>>> @@ -79,7 +80,8 @@ static void c_stop(struct seq_file *m, void *v)
>>> static int c_show(struct seq_file *m, void *v)
>>> {
>>> unsigned long hart_id = (unsigned long)v - 1;
>>> - struct device_node *node = of_get_cpu_node(hart_id, NULL);
>>> + struct device_node *node =
>>> of_get_cpu_node(cpu_logical_map(hart_id),
>>> + NULL);
>>
>>
>> The hart_id is misleading name here. It should be cpu_id. Please replace
>> all instances of hart_id with cpu_id and where hard ID is to be displayed
>> use cpu_logical_map(cpu_id).
>>
> Correct. Thanks for catching it. I will fix it in v2.
>
>
>>> const char *compat, *isa, *mmu;
>>>
>>> seq_printf(m, "hart\t: %lu\n", hart_id);
>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>> index e21ed481..97b586f8 100644
>>> --- a/arch/riscv/kernel/setup.c
>>> +++ b/arch/riscv/kernel/setup.c
>>> @@ -84,6 +84,16 @@ atomic_t hart_lottery;
>>>
>>> u64 __cpu_logical_map[NR_CPUS];
>>>
>>> +void __init smp_setup_processor_id(void)
>>> +{
>>> + int cpu = smp_processor_id();
>>> +
>>> + cpu_logical_map(0) = cpu;
>>
>>
>> I think this should be:
>> cpu_logical_map(cpu) = hart_id;
>>
>> Here hart_id for boot CPU will be value of a0 register passed at
>> boot-time.
>>
> I will change the variable name to hart_id. The assembly code in head.S have
> already stored hart id in thread info structure. So smp_processor_id() and
> hart id would be the same.
>
>

I guess this means that for boot CPU, cpuid == hartid

This is very confusing because other places I see CPU0 is the boot CPU.

I think assembly code in head.S should store 0 in thread info for boot CPU.

Regards,
Anup

2018-08-16 09:33:43

by Atish Patra

Subject: Re: [RFC PATCH 2/5] RISC-V: Use Linux logical cpu number instead of hartid

On 8/15/18 10:45 PM, Anup Patel wrote:
> On Thu, Aug 16, 2018 at 10:53 AM, Atish Patra <[email protected]> wrote:
>> On 8/15/18 9:24 PM, Anup Patel wrote:
>>>
>>> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>>>>
>>>> Setup the cpu_logical_map during boot. Moreover, every SBI call
>>>> and PLIC context are based on the physical hartid. Use the logical
>>>> cpu to hartid mapping to pass correct hartid to respective functions.
>>>>
>>>> Signed-off-by: Atish Patra <[email protected]>
>>>> ---
>>>> arch/riscv/include/asm/tlbflush.h | 17 +++++++++++++----
>>>> arch/riscv/kernel/cpu.c | 4 +++-
>>>> arch/riscv/kernel/setup.c | 10 ++++++++++
>>>> arch/riscv/kernel/smp.c | 24 +++++++++++++++---------
>>>> arch/riscv/kernel/smpboot.c | 30 ++++++++++++++++++------------
>>>> drivers/clocksource/riscv_timer.c | 12 ++++++++----
>>>> drivers/irqchip/irq-sifive-plic.c | 11 +++++++----
>>>> 7 files changed, 74 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/include/asm/tlbflush.h
>>>> b/arch/riscv/include/asm/tlbflush.h
>>>> index 85c2d8ba..ecfd9b0e 100644
>>>> --- a/arch/riscv/include/asm/tlbflush.h
>>>> +++ b/arch/riscv/include/asm/tlbflush.h
>>>> @@ -16,6 +16,7 @@
>>>> #define _ASM_RISCV_TLBFLUSH_H
>>>>
>>>> #include <linux/mm_types.h>
>>>> +#include <asm/smp.h>
>>>>
>>>> /*
>>>> * Flush entire local TLB. 'sfence.vma' implicitly fences with the
>>>> instruction
>>>> @@ -49,13 +50,21 @@ static inline void flush_tlb_range(struct
>>>> vm_area_struct *vma,
>>>>
>>>> #include <asm/sbi.h>
>>>>
>>>> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
>>>> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned
>>>> long start,
>>>> + unsigned long size)
>>>> +{
>>>> + struct cpumask hmask;
>>>> +
>>>> + cpuid_to_hartid_mask(cmask, &hmask);
>>>> + sbi_remote_sfence_vma(hmask.bits, start, size);
>>>> +}
>>>> +
>>>> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
>>>> #define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
>>>> #define flush_tlb_range(vma, start, end) \
>>>> - sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
>>>> - start, (end) - (start))
>>>> + remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) -
>>>> (start))
>>>> #define flush_tlb_mm(mm) \
>>>> - sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)
>>>> + remote_sfence_vma(mm_cpumask(mm), 0, -1)
>>>>
>>>> #endif /* CONFIG_SMP */
>>>>
>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>>> index ca6c81e5..f8a18ace 100644
>>>> --- a/arch/riscv/kernel/cpu.c
>>>> +++ b/arch/riscv/kernel/cpu.c
>>>> @@ -14,6 +14,7 @@
>>>> #include <linux/init.h>
>>>> #include <linux/seq_file.h>
>>>> #include <linux/of.h>
>>>> +#include <asm/smp.h>
>>>>
>>>> /* Return -1 if not a valid hart */
>>>> int riscv_of_processor_hart(struct device_node *node)
>>>> @@ -79,7 +80,8 @@ static void c_stop(struct seq_file *m, void *v)
>>>> static int c_show(struct seq_file *m, void *v)
>>>> {
>>>> unsigned long hart_id = (unsigned long)v - 1;
>>>> - struct device_node *node = of_get_cpu_node(hart_id, NULL);
>>>> + struct device_node *node =
>>>> of_get_cpu_node(cpu_logical_map(hart_id),
>>>> + NULL);
>>>
>>>
>>> The hart_id is misleading name here. It should be cpu_id. Please replace
>>> all instances of hart_id with cpu_id and where hard ID is to be displayed
>>> use cpu_logical_map(cpu_id).
>>>
>> Correct. Thanks for catching it. I will fix it in v2.
>>
>>
>>>> const char *compat, *isa, *mmu;
>>>>
>>>> seq_printf(m, "hart\t: %lu\n", hart_id);
>>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>>> index e21ed481..97b586f8 100644
>>>> --- a/arch/riscv/kernel/setup.c
>>>> +++ b/arch/riscv/kernel/setup.c
>>>> @@ -84,6 +84,16 @@ atomic_t hart_lottery;
>>>>
>>>> u64 __cpu_logical_map[NR_CPUS];
>>>>
>>>> +void __init smp_setup_processor_id(void)
>>>> +{
>>>> + int cpu = smp_processor_id();
>>>> +
>>>> + cpu_logical_map(0) = cpu;
>>>
>>>
>>> I think this should be:
>>> cpu_logical_map(cpu) = hart_id;
>>>
>>> Here hart_id for boot CPU will be value of a0 register passed at
>>> boot-time.
>>>
>> I will change the variable name to hart_id. The assembly code in head.S have
>> already stored hart id in thread info structure. So smp_processor_id() and
>> hart id would be the same.
>>
>>
>
> I guess this means that for boot CPU, cpuid == hartid
>

No. I set the cpuid 0 for boot cpu by doing this.

+ /* Change the boot cpu ID in thread_info */
+ current->thread_info.cpu = 0;

> This is very confusing because other places I see CPU0 is the boot CPU.
>

CPU0 is the boot cpu.
> I think assembly code in head.S should store 0 in thread info for boot CPU.
>
If we do that, we loose track of boot cpu hartid. We have to store it
somewhere else to update the cpu_logical_map(0). Isn't it ?

Regards,
Atish
> Regards,
> Anup
>

2018-08-16 09:45:21

by Anup Patel

Subject: Re: [RFC PATCH 2/5] RISC-V: Use Linux logical cpu number instead of hartid

On Thu, Aug 16, 2018 at 11:22 AM, Atish Patra <[email protected]> wrote:
> On 8/15/18 10:45 PM, Anup Patel wrote:
>>
>> On Thu, Aug 16, 2018 at 10:53 AM, Atish Patra <[email protected]> wrote:
>>>
>>> On 8/15/18 9:24 PM, Anup Patel wrote:
>>>>
>>>>
>>>> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]>
>>>> wrote:
>>>>>
>>>>>
>>>>> Setup the cpu_logical_map during boot. Moreover, every SBI call
>>>>> and PLIC context are based on the physical hartid. Use the logical
>>>>> cpu to hartid mapping to pass correct hartid to respective functions.
>>>>>
>>>>> Signed-off-by: Atish Patra <[email protected]>
>>>>> ---
>>>>> arch/riscv/include/asm/tlbflush.h | 17 +++++++++++++----
>>>>> arch/riscv/kernel/cpu.c | 4 +++-
>>>>> arch/riscv/kernel/setup.c | 10 ++++++++++
>>>>> arch/riscv/kernel/smp.c | 24 +++++++++++++++---------
>>>>> arch/riscv/kernel/smpboot.c | 30
>>>>> ++++++++++++++++++------------
>>>>> drivers/clocksource/riscv_timer.c | 12 ++++++++----
>>>>> drivers/irqchip/irq-sifive-plic.c | 11 +++++++----
>>>>> 7 files changed, 74 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/include/asm/tlbflush.h
>>>>> b/arch/riscv/include/asm/tlbflush.h
>>>>> index 85c2d8ba..ecfd9b0e 100644
>>>>> --- a/arch/riscv/include/asm/tlbflush.h
>>>>> +++ b/arch/riscv/include/asm/tlbflush.h
>>>>> @@ -16,6 +16,7 @@
>>>>> #define _ASM_RISCV_TLBFLUSH_H
>>>>>
>>>>> #include <linux/mm_types.h>
>>>>> +#include <asm/smp.h>
>>>>>
>>>>> /*
>>>>> * Flush entire local TLB. 'sfence.vma' implicitly fences with the
>>>>> instruction
>>>>> @@ -49,13 +50,21 @@ static inline void flush_tlb_range(struct
>>>>> vm_area_struct *vma,
>>>>>
>>>>> #include <asm/sbi.h>
>>>>>
>>>>> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
>>>>> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned
>>>>> long start,
>>>>> + unsigned long size)
>>>>> +{
>>>>> + struct cpumask hmask;
>>>>> +
>>>>> + cpuid_to_hartid_mask(cmask, &hmask);
>>>>> + sbi_remote_sfence_vma(hmask.bits, start, size);
>>>>> +}
>>>>> +
>>>>> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
>>>>> #define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
>>>>> #define flush_tlb_range(vma, start, end) \
>>>>> - sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
>>>>> - start, (end) - (start))
>>>>> + remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) -
>>>>> (start))
>>>>> #define flush_tlb_mm(mm) \
>>>>> - sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)
>>>>> + remote_sfence_vma(mm_cpumask(mm), 0, -1)
>>>>>
>>>>> #endif /* CONFIG_SMP */
>>>>>
>>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>>>> index ca6c81e5..f8a18ace 100644
>>>>> --- a/arch/riscv/kernel/cpu.c
>>>>> +++ b/arch/riscv/kernel/cpu.c
>>>>> @@ -14,6 +14,7 @@
>>>>> #include <linux/init.h>
>>>>> #include <linux/seq_file.h>
>>>>> #include <linux/of.h>
>>>>> +#include <asm/smp.h>
>>>>>
>>>>> /* Return -1 if not a valid hart */
>>>>> int riscv_of_processor_hart(struct device_node *node)
>>>>> @@ -79,7 +80,8 @@ static void c_stop(struct seq_file *m, void *v)
>>>>> static int c_show(struct seq_file *m, void *v)
>>>>> {
>>>>> unsigned long hart_id = (unsigned long)v - 1;
>>>>> - struct device_node *node = of_get_cpu_node(hart_id, NULL);
>>>>> + struct device_node *node =
>>>>> of_get_cpu_node(cpu_logical_map(hart_id),
>>>>> + NULL);
>>>>
>>>>
>>>>
>>>> The hart_id is misleading name here. It should be cpu_id. Please replace
>>>> all instances of hart_id with cpu_id and where hard ID is to be
>>>> displayed
>>>> use cpu_logical_map(cpu_id).
>>>>
>>> Correct. Thanks for catching it. I will fix it in v2.
>>>
>>>
>>>>> const char *compat, *isa, *mmu;
>>>>>
>>>>> seq_printf(m, "hart\t: %lu\n", hart_id);
>>>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>>>> index e21ed481..97b586f8 100644
>>>>> --- a/arch/riscv/kernel/setup.c
>>>>> +++ b/arch/riscv/kernel/setup.c
>>>>> @@ -84,6 +84,16 @@ atomic_t hart_lottery;
>>>>>
>>>>> u64 __cpu_logical_map[NR_CPUS];
>>>>>
>>>>> +void __init smp_setup_processor_id(void)
>>>>> +{
>>>>> + int cpu = smp_processor_id();
>>>>> +
>>>>> + cpu_logical_map(0) = cpu;
>>>>
>>>>
>>>>
>>>> I think this should be:
>>>> cpu_logical_map(cpu) = hart_id;
>>>>
>>>> Here hart_id for boot CPU will be value of a0 register passed at
>>>> boot-time.
>>>>
>>> I will change the variable name to hart_id. The assembly code in head.S
>>> have
>>> already stored hart id in thread info structure. So smp_processor_id()
>>> and
>>> hart id would be the same.
>>>
>>>
>>
>> I guess this means that for boot CPU, cpuid == hartid
>>
>
> No. I set the cpuid 0 for boot cpu by doing this.
>
> + /* Change the boot cpu ID in thread_info */
> + current->thread_info.cpu = 0;
>
>> This is very confusing because other places I see CPU0 is the boot CPU.
>>
>
> CPU0 is the boot cpu.
>>
>> I think assembly code in head.S should store 0 in thread info for boot
>> CPU.
>>
> If we do that, we loose track of boot cpu hartid. We have to store it
> somewhere else to update the cpu_logical_map(0). Isn't it ?

We can have variable of machine word size declared in assembly
named "boot_cpu_hart_id" which will hold the hart ID of boot CPU.

No need to update thread_info.cpu on boot CPU. In fact, set
thread_info.cpu to 0 in head.S itself.

Regards,
Anup

2018-08-16 10:30:08

by Anup Patel

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Thu, Aug 16, 2018 at 11:10 AM, Atish Patra <[email protected]> wrote:
> On 8/15/18 10:02 PM, Anup Patel wrote:
>>
>> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>>>
>>> Defining cpu_operations now helps adding cpu hotplug
>>> support in proper manner. Moreover, it provides flexibility
>>> in supporting other cpu enable/boot methods can be
>>> supported in future. This patch has been largely inspired from
>>> ARM64. However, a default boot method is defined for RISC-V unlike
>>> ARM64.
>>>
>>> Signed-off-by: Atish Patra <[email protected]>
>>> ---
>>> arch/riscv/include/asm/smp.h | 10 ++++++++++
>>> arch/riscv/kernel/smp.c | 8 ++++++++
>>> arch/riscv/kernel/smpboot.c | 34 ++++++++++++++++++++++++++++------
>>> 3 files changed, 46 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
>>> index 0763337b..2bb6e6c2 100644
>>> --- a/arch/riscv/include/asm/smp.h
>>> +++ b/arch/riscv/include/asm/smp.h
>>> @@ -28,6 +28,15 @@
>>> extern u64 __cpu_logical_map[NR_CPUS];
>>> #define cpu_logical_map(cpu) __cpu_logical_map[cpu]
>>>
>>> +struct cpu_operations {
>>> + const char *name;
>>> + int (*cpu_init)(unsigned int);
>>> + int (*cpu_prepare)(unsigned int);
>>> + int (*cpu_boot)(unsigned int, struct task_struct *);
>>> +};
>>> +extern struct cpu_operations cpu_ops;
>>> +void smp_set_cpu_ops(const struct cpu_operations *cpu_ops);
>>> +
>>> #ifdef CONFIG_SMP
>>>
>>> /* SMP initialization hook for setup_arch */
>>> @@ -57,5 +66,6 @@ static inline void cpuid_to_hartid_mask(const struct
>>> cpumask *in,
>>> cpumask_set_cpu(cpu_logical_map(0), out);
>>> }
>>>
>>> +
>>> #endif /* CONFIG_SMP */
>>> #endif /* _ASM_RISCV_SMP_H */
>>> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
>>> index 4ab70480..5de58ced 100644
>>> --- a/arch/riscv/kernel/smp.c
>>> +++ b/arch/riscv/kernel/smp.c
>>> @@ -58,6 +58,14 @@ void cpuid_to_hartid_mask(const struct cpumask *in,
>>> struct cpumask *out)
>>> for_each_cpu(cpu, in)
>>> cpumask_set_cpu(cpu_logical_map(cpu), out);
>>> }
>>> +struct cpu_operations cpu_ops __ro_after_init;
>>> +
>>> +void smp_set_cpu_ops(const struct cpu_operations *ops)
>>> +{
>>> + if (ops)
>>> + cpu_ops = *ops;
>>> +}
>>> +
>>
>>
>> Move both cpu_ops and smp_set_cpu_ops() to smpboot.c. This way
>> you will not require to make cpu_ops as global.
>>
> ok.
>
>> Further, I think cpu_ops should be a pointer and initial value should
>> be &default_ops.
>>
>> Something like this:
>> struct cpu_operations *cpu_ops __ro_after_init = &default_ops;
>>
>
> That will work too. However, setting it through smp_set_cpu_ops provides a
> sample implementation for any future SMP enable methods. That's why I used
> the API. I can change it if you think otherwise.

Having thought about this more, I think cpu_ops should be an pointer array
of NR_CPUS size. This means its not necessary to have have same ops for
all CPUs. The ARM64 implementation of CPU operations also allows separate
CPU operations for each CPU.

For example, let's us assume that we have an SOC where we 2 cores
per-cluster and N clusters. All CPUs of cluster0 comes up at the same time
whereas cluster1 onwards we have to bring-up CPUs using special HW
mechanism.

>
>
>
>>> /* Unsupported */
>>> int setup_profiling_timer(unsigned int multiplier)
>>> {
>>> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>>> index 6ab2bb1b..045a1a45 100644
>>> --- a/arch/riscv/kernel/smpboot.c
>>> +++ b/arch/riscv/kernel/smpboot.c
>>> @@ -30,6 +30,7 @@
>>> #include <linux/irq.h>
>>> #include <linux/of.h>
>>> #include <linux/sched/task_stack.h>
>>> +#include <linux/smp.h>
>>> #include <asm/irq.h>
>>> #include <asm/mmu_context.h>
>>> #include <asm/tlbflush.h>
>>> @@ -38,6 +39,7 @@
>>>
>>> void *__cpu_up_stack_pointer[NR_CPUS];
>>> void *__cpu_up_task_pointer[NR_CPUS];
>>> +struct cpu_operations default_ops;
>>>
>>> void __init smp_prepare_boot_cpu(void)
>>> {
>>> @@ -53,6 +55,7 @@ void __init setup_smp(void)
>>> int hart, found_boot_cpu = 0;
>>> int cpuid = 1;
>>>
>>> + smp_set_cpu_ops(&default_ops);
>>> while ((dn = of_find_node_by_type(dn, "cpu"))) {
>>> hart = riscv_of_processor_hart(dn);
>>>
>>> @@ -73,10 +76,8 @@ void __init setup_smp(void)
>>> BUG_ON(!found_boot_cpu);
>>> }
>>>
>>> -int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>>> +int default_cpu_boot(unsigned int hartid, struct task_struct *tidle)
>>> {
>>> - int hartid = cpu_logical_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
>>> @@ -84,13 +85,28 @@ int __cpu_up(unsigned int cpu, struct task_struct
>>> *tidle)
>>> * setup by that main hart. Writing __cpu_up_stack_pointer
>>> signals to
>>> * the spinning harts that they can continue the boot process.
>>> */
>>> - smp_mb();
>>> +
>>> __cpu_up_stack_pointer[hartid] = task_stack_page(tidle) +
>>> THREAD_SIZE;
>>> __cpu_up_task_pointer[hartid] = tidle;
>>> + return 0;
>>> +}
>>> +
>>> +int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>>> +{
>>> + int err = -1;
>>> + int hartid = cpu_logical_map(cpu);
>>>
>>> - while (!cpu_online(cpu))
>>> - cpu_relax();
>>> + tidle->thread_info.cpu = cpu;
>>> + smp_mb();
>>>
>>> + if (cpu_ops.cpu_boot)
>>> + err = cpu_ops.cpu_boot(hartid, tidle);
>>> + if (!err) {
>>> + while (!cpu_online(cpu))
>>> + cpu_relax();
>>> + } else {
>>> + pr_err("CPU %d [hartid %d]failed to boot\n", cpu,
>>> hartid);
>>> + }
>>> return 0;
>>> }
>>>
>>> @@ -117,3 +133,9 @@ asmlinkage void __init smp_callin(void)
>>> preempt_disable();
>>> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
>>> }
>>> +
>>> +
>>> +struct cpu_operations default_ops = {
>>> + .name = "default",
>>> + .cpu_boot = default_cpu_boot,
>>> +};
>>
>>
>> I think having dedicated source file for default_ops makes more sense
>> so that we have a clear/simple reference implementation of cpu_operations.
>>
>> Eventually, we might have one source file for each type of SMP enable
>> method.
>>
>> Try to keep smpboot.c generic and independent of any cpu_operations.
>> What say?
>>
>
> Any other SMP enable method should definitely get its own file. I am not
> sure about the default one though. As default_ops is truly the default
> booting method which will always be present in absence of anything else, I
> thought keeping it smpboot.c emphasizes that point. Moreover, it's not that
> big even. But I am open to moving to it's own source file if you strongly
> think we should do that.
>

I am more inclined towards keeping default_ops in separate source but it's
not a big deal. Let's wait for more comments.

Regards,
Anup

2018-08-16 18:49:03

by Atish Patra

Subject: Re: [RFC PATCH 2/5] RISC-V: Use Linux logical cpu number instead of hartid

On 8/15/18 11:03 PM, Anup Patel wrote:
> On Thu, Aug 16, 2018 at 11:22 AM, Atish Patra <[email protected]> wrote:
>> On 8/15/18 10:45 PM, Anup Patel wrote:
>>>
>>> On Thu, Aug 16, 2018 at 10:53 AM, Atish Patra <[email protected]> wrote:
>>>>
>>>> On 8/15/18 9:24 PM, Anup Patel wrote:
>>>>>
>>>>>
>>>>> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Setup the cpu_logical_map during boot. Moreover, every SBI call
>>>>>> and PLIC context are based on the physical hartid. Use the logical
>>>>>> cpu to hartid mapping to pass correct hartid to respective functions.
>>>>>>
>>>>>> Signed-off-by: Atish Patra <[email protected]>
>>>>>> ---
>>>>>> arch/riscv/include/asm/tlbflush.h | 17 +++++++++++++----
>>>>>> arch/riscv/kernel/cpu.c | 4 +++-
>>>>>> arch/riscv/kernel/setup.c | 10 ++++++++++
>>>>>> arch/riscv/kernel/smp.c | 24 +++++++++++++++---------
>>>>>> arch/riscv/kernel/smpboot.c | 30
>>>>>> ++++++++++++++++++------------
>>>>>> drivers/clocksource/riscv_timer.c | 12 ++++++++----
>>>>>> drivers/irqchip/irq-sifive-plic.c | 11 +++++++----
>>>>>> 7 files changed, 74 insertions(+), 34 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/include/asm/tlbflush.h
>>>>>> b/arch/riscv/include/asm/tlbflush.h
>>>>>> index 85c2d8ba..ecfd9b0e 100644
>>>>>> --- a/arch/riscv/include/asm/tlbflush.h
>>>>>> +++ b/arch/riscv/include/asm/tlbflush.h
>>>>>> @@ -16,6 +16,7 @@
>>>>>> #define _ASM_RISCV_TLBFLUSH_H
>>>>>>
>>>>>> #include <linux/mm_types.h>
>>>>>> +#include <asm/smp.h>
>>>>>>
>>>>>> /*
>>>>>> * Flush entire local TLB. 'sfence.vma' implicitly fences with the
>>>>>> instruction
>>>>>> @@ -49,13 +50,21 @@ static inline void flush_tlb_range(struct
>>>>>> vm_area_struct *vma,
>>>>>>
>>>>>> #include <asm/sbi.h>
>>>>>>
>>>>>> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
>>>>>> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned
>>>>>> long start,
>>>>>> + unsigned long size)
>>>>>> +{
>>>>>> + struct cpumask hmask;
>>>>>> +
>>>>>> + cpuid_to_hartid_mask(cmask, &hmask);
>>>>>> + sbi_remote_sfence_vma(hmask.bits, start, size);
>>>>>> +}
>>>>>> +
>>>>>> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
>>>>>> #define flush_tlb_page(vma, addr) flush_tlb_range(vma, addr, 0)
>>>>>> #define flush_tlb_range(vma, start, end) \
>>>>>> - sbi_remote_sfence_vma(mm_cpumask((vma)->vm_mm)->bits, \
>>>>>> - start, (end) - (start))
>>>>>> + remote_sfence_vma(mm_cpumask((vma)->vm_mm), start, (end) -
>>>>>> (start))
>>>>>> #define flush_tlb_mm(mm) \
>>>>>> - sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)
>>>>>> + remote_sfence_vma(mm_cpumask(mm), 0, -1)
>>>>>>
>>>>>> #endif /* CONFIG_SMP */
>>>>>>
>>>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>>>>> index ca6c81e5..f8a18ace 100644
>>>>>> --- a/arch/riscv/kernel/cpu.c
>>>>>> +++ b/arch/riscv/kernel/cpu.c
>>>>>> @@ -14,6 +14,7 @@
>>>>>> #include <linux/init.h>
>>>>>> #include <linux/seq_file.h>
>>>>>> #include <linux/of.h>
>>>>>> +#include <asm/smp.h>
>>>>>>
>>>>>> /* Return -1 if not a valid hart */
>>>>>> int riscv_of_processor_hart(struct device_node *node)
>>>>>> @@ -79,7 +80,8 @@ static void c_stop(struct seq_file *m, void *v)
>>>>>> static int c_show(struct seq_file *m, void *v)
>>>>>> {
>>>>>> unsigned long hart_id = (unsigned long)v - 1;
>>>>>> - struct device_node *node = of_get_cpu_node(hart_id, NULL);
>>>>>> + struct device_node *node =
>>>>>> of_get_cpu_node(cpu_logical_map(hart_id),
>>>>>> + NULL);
>>>>>
>>>>>
>>>>>
>>>>> The hart_id is misleading name here. It should be cpu_id. Please replace
>>>>> all instances of hart_id with cpu_id and where hard ID is to be
>>>>> displayed
>>>>> use cpu_logical_map(cpu_id).
>>>>>
>>>> Correct. Thanks for catching it. I will fix it in v2.
>>>>
>>>>
>>>>>> const char *compat, *isa, *mmu;
>>>>>>
>>>>>> seq_printf(m, "hart\t: %lu\n", hart_id);
>>>>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>>>>> index e21ed481..97b586f8 100644
>>>>>> --- a/arch/riscv/kernel/setup.c
>>>>>> +++ b/arch/riscv/kernel/setup.c
>>>>>> @@ -84,6 +84,16 @@ atomic_t hart_lottery;
>>>>>>
>>>>>> u64 __cpu_logical_map[NR_CPUS];
>>>>>>
>>>>>> +void __init smp_setup_processor_id(void)
>>>>>> +{
>>>>>> + int cpu = smp_processor_id();
>>>>>> +
>>>>>> + cpu_logical_map(0) = cpu;
>>>>>
>>>>>
>>>>>
>>>>> I think this should be:
>>>>> cpu_logical_map(cpu) = hart_id;
>>>>>
>>>>> Here hart_id for boot CPU will be value of a0 register passed at
>>>>> boot-time.
>>>>>
>>>> I will change the variable name to hart_id. The assembly code in head.S
>>>> have
>>>> already stored hart id in thread info structure. So smp_processor_id()
>>>> and
>>>> hart id would be the same.
>>>>
>>>>
>>>
>>> I guess this means that for boot CPU, cpuid == hartid
>>>
>>
>> No. I set the cpuid 0 for boot cpu by doing this.
>>
>> + /* Change the boot cpu ID in thread_info */
>> + current->thread_info.cpu = 0;
>>
>>> This is very confusing because other places I see CPU0 is the boot CPU.
>>>
>>
>> CPU0 is the boot cpu.
>>>
>>> I think assembly code in head.S should store 0 in thread info for boot
>>> CPU.
>>>
>> If we do that, we loose track of boot cpu hartid. We have to store it
>> somewhere else to update the cpu_logical_map(0). Isn't it ?
>
> We can have variable of machine word size declared in assembly
> named "boot_cpu_hart_id" which will hold the hart ID of boot CPU.
>
> No need to update thread_info.cpu on boot CPU. In fact, set
> thread_info.cpu to 0 in head.S itself.
>

Hmm. That will work too. Thanks.

Atish
> Regards,
> Anup
>

2018-08-18 01:26:30

by Atish Patra

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On 8/15/18 11:21 PM, Anup Patel wrote:
> On Thu, Aug 16, 2018 at 11:10 AM, Atish Patra <[email protected]> wrote:
>> On 8/15/18 10:02 PM, Anup Patel wrote:
>>>
>>> On Thu, Aug 16, 2018 at 5:26 AM, Atish Patra <[email protected]> wrote:
>>>>
>>>> Defining cpu_operations now helps adding cpu hotplug
>>>> support in proper manner. Moreover, it provides flexibility
>>>> in supporting other cpu enable/boot methods can be
>>>> supported in future. This patch has been largely inspired from
>>>> ARM64. However, a default boot method is defined for RISC-V unlike
>>>> ARM64.
>>>>
>>>> Signed-off-by: Atish Patra <[email protected]>
>>>> ---
>>>> arch/riscv/include/asm/smp.h | 10 ++++++++++
>>>> arch/riscv/kernel/smp.c | 8 ++++++++
>>>> arch/riscv/kernel/smpboot.c | 34 ++++++++++++++++++++++++++++------
>>>> 3 files changed, 46 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
>>>> index 0763337b..2bb6e6c2 100644
>>>> --- a/arch/riscv/include/asm/smp.h
>>>> +++ b/arch/riscv/include/asm/smp.h
>>>> @@ -28,6 +28,15 @@
>>>> extern u64 __cpu_logical_map[NR_CPUS];
>>>> #define cpu_logical_map(cpu) __cpu_logical_map[cpu]
>>>>
>>>> +struct cpu_operations {
>>>> + const char *name;
>>>> + int (*cpu_init)(unsigned int);
>>>> + int (*cpu_prepare)(unsigned int);
>>>> + int (*cpu_boot)(unsigned int, struct task_struct *);
>>>> +};
>>>> +extern struct cpu_operations cpu_ops;
>>>> +void smp_set_cpu_ops(const struct cpu_operations *cpu_ops);
>>>> +
>>>> #ifdef CONFIG_SMP
>>>>
>>>> /* SMP initialization hook for setup_arch */
>>>> @@ -57,5 +66,6 @@ static inline void cpuid_to_hartid_mask(const struct
>>>> cpumask *in,
>>>> cpumask_set_cpu(cpu_logical_map(0), out);
>>>> }
>>>>
>>>> +
>>>> #endif /* CONFIG_SMP */
>>>> #endif /* _ASM_RISCV_SMP_H */
>>>> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
>>>> index 4ab70480..5de58ced 100644
>>>> --- a/arch/riscv/kernel/smp.c
>>>> +++ b/arch/riscv/kernel/smp.c
>>>> @@ -58,6 +58,14 @@ void cpuid_to_hartid_mask(const struct cpumask *in,
>>>> struct cpumask *out)
>>>> for_each_cpu(cpu, in)
>>>> cpumask_set_cpu(cpu_logical_map(cpu), out);
>>>> }
>>>> +struct cpu_operations cpu_ops __ro_after_init;
>>>> +
>>>> +void smp_set_cpu_ops(const struct cpu_operations *ops)
>>>> +{
>>>> + if (ops)
>>>> + cpu_ops = *ops;
>>>> +}
>>>> +
>>>
>>>
>>> Move both cpu_ops and smp_set_cpu_ops() to smpboot.c. This way
>>> you will not require to make cpu_ops as global.
>>>
>> ok.
>>
>>> Further, I think cpu_ops should be a pointer and initial value should
>>> be &default_ops.
>>>
>>> Something like this:
>>> struct cpu_operations *cpu_ops __ro_after_init = &default_ops;
>>>
>>
>> That will work too. However, setting it through smp_set_cpu_ops provides a
>> sample implementation for any future SMP enable methods. That's why I used
>> the API. I can change it if you think otherwise.
>
> Having thought about this more, I think cpu_ops should be an pointer array
> of NR_CPUS size. This means its not necessary to have have same ops for
> all CPUs. The ARM64 implementation of CPU operations also allows separate
> CPU operations for each CPU.
>

I initially had NR_CPUs based pointer array implementation similar to
arm64. However, I couldn't find a use case for it. So I removed it.


> For example, let's us assume that we have an SOC where we 2 cores
> per-cluster and N clusters. All CPUs of cluster0 comes up at the same time
> whereas cluster1 onwards we have to bring-up CPUs using special HW
> mechanism.
>

I was not aware of such a use case. If that's a valid possible future
use case, we should make it pointer array implementation. I will add it
in v2

Regards,
Atish
>>
>>
>>
>>>> /* Unsupported */
>>>> int setup_profiling_timer(unsigned int multiplier)
>>>> {
>>>> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
>>>> index 6ab2bb1b..045a1a45 100644
>>>> --- a/arch/riscv/kernel/smpboot.c
>>>> +++ b/arch/riscv/kernel/smpboot.c
>>>> @@ -30,6 +30,7 @@
>>>> #include <linux/irq.h>
>>>> #include <linux/of.h>
>>>> #include <linux/sched/task_stack.h>
>>>> +#include <linux/smp.h>
>>>> #include <asm/irq.h>
>>>> #include <asm/mmu_context.h>
>>>> #include <asm/tlbflush.h>
>>>> @@ -38,6 +39,7 @@
>>>>
>>>> void *__cpu_up_stack_pointer[NR_CPUS];
>>>> void *__cpu_up_task_pointer[NR_CPUS];
>>>> +struct cpu_operations default_ops;
>>>>
>>>> void __init smp_prepare_boot_cpu(void)
>>>> {
>>>> @@ -53,6 +55,7 @@ void __init setup_smp(void)
>>>> int hart, found_boot_cpu = 0;
>>>> int cpuid = 1;
>>>>
>>>> + smp_set_cpu_ops(&default_ops);
>>>> while ((dn = of_find_node_by_type(dn, "cpu"))) {
>>>> hart = riscv_of_processor_hart(dn);
>>>>
>>>> @@ -73,10 +76,8 @@ void __init setup_smp(void)
>>>> BUG_ON(!found_boot_cpu);
>>>> }
>>>>
>>>> -int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>>>> +int default_cpu_boot(unsigned int hartid, struct task_struct *tidle)
>>>> {
>>>> - int hartid = cpu_logical_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
>>>> @@ -84,13 +85,28 @@ int __cpu_up(unsigned int cpu, struct task_struct
>>>> *tidle)
>>>> * setup by that main hart. Writing __cpu_up_stack_pointer
>>>> signals to
>>>> * the spinning harts that they can continue the boot process.
>>>> */
>>>> - smp_mb();
>>>> +
>>>> __cpu_up_stack_pointer[hartid] = task_stack_page(tidle) +
>>>> THREAD_SIZE;
>>>> __cpu_up_task_pointer[hartid] = tidle;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>>>> +{
>>>> + int err = -1;
>>>> + int hartid = cpu_logical_map(cpu);
>>>>
>>>> - while (!cpu_online(cpu))
>>>> - cpu_relax();
>>>> + tidle->thread_info.cpu = cpu;
>>>> + smp_mb();
>>>>
>>>> + if (cpu_ops.cpu_boot)
>>>> + err = cpu_ops.cpu_boot(hartid, tidle);
>>>> + if (!err) {
>>>> + while (!cpu_online(cpu))
>>>> + cpu_relax();
>>>> + } else {
>>>> + pr_err("CPU %d [hartid %d]failed to boot\n", cpu,
>>>> hartid);
>>>> + }
>>>> return 0;
>>>> }
>>>>
>>>> @@ -117,3 +133,9 @@ asmlinkage void __init smp_callin(void)
>>>> preempt_disable();
>>>> cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
>>>> }
>>>> +
>>>> +
>>>> +struct cpu_operations default_ops = {
>>>> + .name = "default",
>>>> + .cpu_boot = default_cpu_boot,
>>>> +};
>>>
>>>
>>> I think having dedicated source file for default_ops makes more sense
>>> so that we have a clear/simple reference implementation of cpu_operations.
>>>
>>> Eventually, we might have one source file for each type of SMP enable
>>> method.
>>>
>>> Try to keep smpboot.c generic and independent of any cpu_operations.
>>> What say?
>>>
>>
>> Any other SMP enable method should definitely get its own file. I am not
>> sure about the default one though. As default_ops is truly the default
>> booting method which will always be present in absence of anything else, I
>> thought keeping it smpboot.c emphasizes that point. Moreover, it's not that
>> big even. But I am open to moving to it's own source file if you strongly
>> think we should do that.
>>
>
> I am more inclined towards keeping default_ops in separate source but it's
> not a big deal. Let's wait for more comments.
>
> Regards,
> Anup
>

2018-08-21 07:49:58

by Christoph Hellwig

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Thu, Aug 16, 2018 at 11:51:03AM +0530, Anup Patel wrote:
> Having thought about this more, I think cpu_ops should be an pointer array
> of NR_CPUS size. This means its not necessary to have have same ops for
> all CPUs. The ARM64 implementation of CPU operations also allows separate
> CPU operations for each CPU.
>
> For example, let's us assume that we have an SOC where we 2 cores
> per-cluster and N clusters. All CPUs of cluster0 comes up at the same time
> whereas cluster1 onwards we have to bring-up CPUs using special HW
> mechanism.

All this (including the patch itself) seems a little hypothetical.
I'd rather only add all this infrastructure once it actually is needed.

2018-08-21 07:51:00

by Christoph Hellwig

Subject: Re: [RFC PATCH 4/5] RISC-V: Move interrupt cause declarations to irq.h

On Wed, Aug 15, 2018 at 04:56:16PM -0700, Atish Patra wrote:
> CPU hotplug code in smpboot requires interrupt cause
> to identify the cause of cpu wakeup.
>
> Move the IRQ cause declarations to appropriate header
> file.

asm/irq.h is a header indirectly included by just about every
driver. This is a very bad place for such internal definitions.

2018-08-21 07:56:17

by Christoph Hellwig

Subject: Re: [RFC PATCH 5/5] RISC-V: Support cpu hotplug.

> if (!err) {
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> + arch_send_call_function_single_ipi(cpu);
> +#endif

Please just provide a stub version of arch_send_call_function_single_ipi
for the !CONFIG_HOTPLUG_CPU case instead of the ifdef here.

> +#ifdef CONFIG_HOTPLUG_CPU
> +int can_hotplug_cpu(void)

This should be a bool.

> +{
> + if (cpu_ops.cpu_die)
> + return 1;
> + else
> + return 0;
> +}

return cpu_ops.cpu_die != NULL;

> +void default_cpu_die(unsigned int cpu)
> +{
> + int 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);
> + /* only break if wfi returns for an enabled interrupt */
> + } while ((sipval & sieval) == 0 &&
> + scauseval != INTERRUPT_CAUSE_SOFTWARE);
> +
> + boot_sec_cpu();
> +}

I suspect all of this except for the boot_sec_cpu() should go into
a helper in irq.c. Also as-is this probably doesn't work as scauseval
will have INTERRUPT_CAUSE_FLAG set, making the comparism never true.

2018-08-21 17:09:52

by Anup Patel

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Tue, Aug 21, 2018 at 1:18 PM, Christoph Hellwig <[email protected]> wrote:
> On Thu, Aug 16, 2018 at 11:51:03AM +0530, Anup Patel wrote:
>> Having thought about this more, I think cpu_ops should be an pointer array
>> of NR_CPUS size. This means its not necessary to have have same ops for
>> all CPUs. The ARM64 implementation of CPU operations also allows separate
>> CPU operations for each CPU.
>>
>> For example, let's us assume that we have an SOC where we 2 cores
>> per-cluster and N clusters. All CPUs of cluster0 comes up at the same time
>> whereas cluster1 onwards we have to bring-up CPUs using special HW
>> mechanism.
>
> All this (including the patch itself) seems a little hypothetical.
> I'd rather only add all this infrastructure once it actually is needed.

The cpu_operations is certainly required because SOC vendors will add
vendor-specific mechanism to selectively bringing-up CPUs/HARTs instead
of all CPUs entering Linux kernel simultaneously. In fact, we might also end-up
having CPU ON/OFF operations in SBI.

Having separate cpu_operations for each CPU is good for flexibility because
CPU clusters might have different way of bringing up CPUs (for example, take
any Hetrogeneous Multiprocessor Systems (HMP)).

IMHO, RISCV Linux port is new and this the right time to implement critical
infrastructure things (such as cpu_operations). Also, its not something radical
because we are taking inspiration from existing Linux ports (such as ARM64).

Regards,
Anup

2018-08-21 20:47:13

by Atish Patra

Subject: Re: [RFC PATCH 5/5] RISC-V: Support cpu hotplug.

On 8/21/18 12:55 AM, Christoph Hellwig wrote:
>> if (!err) {
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> + arch_send_call_function_single_ipi(cpu);
>> +#endif
>
> Please just provide a stub version of arch_send_call_function_single_ipi
> for the !CONFIG_HOTPLUG_CPU case instead of the ifdef here.
>

Done.

>> +#ifdef CONFIG_HOTPLUG_CPU
>> +int can_hotplug_cpu(void)
>
> This should be a bool.
>
>> +{
>> + if (cpu_ops.cpu_die)
>> + return 1;
>> + else
>> + return 0;
>> +}
>
> return cpu_ops.cpu_die != NULL;
>
>> +void default_cpu_die(unsigned int cpu)
>> +{
>> + int 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);
>> + /* only break if wfi returns for an enabled interrupt */
>> + } while ((sipval & sieval) == 0 &&
>> + scauseval != INTERRUPT_CAUSE_SOFTWARE);
>> +
>> + boot_sec_cpu();
>> +}
>
> I suspect all of this except for the boot_sec_cpu() should go into
> a helper in irq.c.

ok. I will try that. That will also solve interrupt cause declarations
issue. So I can drop patch 4.

Also as-is this probably doesn't work as scauseval
> will have INTERRUPT_CAUSE_FLAG set, making the comparism never true.
>

Oops. Thanks for catching that. scauseval is declared as int which made
this code to work. Fixed it.


Regards,
Atish

2018-08-22 06:06:10

by Christoph Hellwig

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Tue, Aug 21, 2018 at 10:34:38PM +0530, Anup Patel wrote:
> The cpu_operations is certainly required because SOC vendors will add
> vendor-specific mechanism to selectively bringing-up CPUs/HARTs instead
> of all CPUs entering Linux kernel simultaneously. In fact, we might also end-up
> having CPU ON/OFF operations in SBI.

Your forgot an essential part in your analysis: Right now we only have
one single way to deal with cpu on/offlining, and that is the dummy WFI
kind. Once other ways show up we can build proper infrastructure, but
until then this is just a white elephant as we have no idea how these
abstractions will look like.

And my hope is that we'll just see new SBI calls, in which case we'll
just need SBI and dummy version and can avoid all the indirect calls.

2018-08-22 15:44:49

by Anup Patel

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Wed, Aug 22, 2018 at 11:33 AM, Christoph Hellwig <[email protected]> wrote:
> On Tue, Aug 21, 2018 at 10:34:38PM +0530, Anup Patel wrote:
>> The cpu_operations is certainly required because SOC vendors will add
>> vendor-specific mechanism to selectively bringing-up CPUs/HARTs instead
>> of all CPUs entering Linux kernel simultaneously. In fact, we might also end-up
>> having CPU ON/OFF operations in SBI.
>
> Your forgot an essential part in your analysis: Right now we only have
> one single way to deal with cpu on/offlining, and that is the dummy WFI
> kind. Once other ways show up we can build proper infrastructure, but
> until then this is just a white elephant as we have no idea how these
> abstractions will look like.
>
> And my hope is that we'll just see new SBI calls, in which case we'll
> just need SBI and dummy version and can avoid all the indirect calls.

IMHO, rather than waiting for new CPU ON/OFF methods to come-up we
can keep the cpu_operations ready. Also, we are not re-inventing anything
here which we might have to discard later because cpu_operations are
already tried and hardened for Linux ARM64.

I agree with you that in long-term SBI-based CPU ON/OFF will be widely
used. Most likely we will have at-least two CPU ON/OFF methods:
1. Existing lottery based spinning
2. New SBI calls

Regards,
Anup

2018-08-22 17:19:00

by Palmer Dabbelt

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Tue, 21 Aug 2018 23:03:53 PDT (-0700), Christoph Hellwig wrote:
> On Tue, Aug 21, 2018 at 10:34:38PM +0530, Anup Patel wrote:
>> The cpu_operations is certainly required because SOC vendors will add
>> vendor-specific mechanism to selectively bringing-up CPUs/HARTs instead
>> of all CPUs entering Linux kernel simultaneously. In fact, we might also end-up
>> having CPU ON/OFF operations in SBI.
>
> Your forgot an essential part in your analysis: Right now we only have
> one single way to deal with cpu on/offlining, and that is the dummy WFI
> kind. Once other ways show up we can build proper infrastructure, but
> until then this is just a white elephant as we have no idea how these
> abstractions will look like.
>
> And my hope is that we'll just see new SBI calls, in which case we'll
> just need SBI and dummy version and can avoid all the indirect calls.

Yes, the goal here is to define one good interface via the SBI. We've got a
session at Plumbers this year and one of the blocks is to sit down and talk
about this API, hopefully we can make some headway while there.

2018-08-23 04:27:15

by Atish Patra

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On 8/22/18 8:54 PM, Anup Patel wrote:
> On Wed, Aug 22, 2018 at 11:33 AM, Christoph Hellwig <[email protected]> wrote:
>> On Tue, Aug 21, 2018 at 10:34:38PM +0530, Anup Patel wrote:
>>> The cpu_operations is certainly required because SOC vendors will add
>>> vendor-specific mechanism to selectively bringing-up CPUs/HARTs instead
>>> of all CPUs entering Linux kernel simultaneously. In fact, we might also end-up
>>> having CPU ON/OFF operations in SBI.
>>
>> Your forgot an essential part in your analysis: Right now we only have
>> one single way to deal with cpu on/offlining, and that is the dummy WFI
>> kind. Once other ways show up we can build proper infrastructure, but
>> until then this is just a white elephant as we have no idea how these
>> abstractions will look like.
>>
>> And my hope is that we'll just see new SBI calls, in which case we'll
>> just need SBI and dummy version and can avoid all the indirect calls.
>
> IMHO, rather than waiting for new CPU ON/OFF methods to come-up we
> can keep the cpu_operations ready. Also, we are not re-inventing anything
> here which we might have to discard later because cpu_operations are
> already tried and hardened for Linux ARM64.
>
> I agree with you that in long-term SBI-based CPU ON/OFF will be widely
> used. Most likely we will have at-least two CPU ON/OFF methods:
> 1. Existing lottery based spinning
> 2. New SBI calls
>
> Regards,
> Anup
>

I am fine with either keeping the cpu_ops infrastructure for now or
reintroducing again along with better smp enablement methods.

Anyways, there were concerns about all existing booting method (all cpu
thrown to Linux at the same time). I was thinking to adopt spin table
boot method for RISC-V as well. I can drop this patch now and
reintroduce with spin table boot method.

Any thoughts ?

Regards,
Atish

2018-08-23 16:25:39

by Christoph Hellwig

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Wed, Aug 22, 2018 at 08:54:51PM +0530, Anup Patel wrote:
> IMHO, rather than waiting for new CPU ON/OFF methods to come-up we
> can keep the cpu_operations ready. Also, we are not re-inventing anything
> here which we might have to discard later because cpu_operations are
> already tried and hardened for Linux ARM64.

Which is a different cpu architecture, and has shown to actually need
it. IFF we end up needing it on riscv we can still copy and paste
it from AMD64.


> I agree with you that in long-term SBI-based CPU ON/OFF will be widely
> used. Most likely we will have at-least two CPU ON/OFF methods:
> 1. Existing lottery based spinning
> 2. New SBI calls

And in this most likely case there is no need for an ops vector,
a simple if/else will be much simpler and cleaner.

2018-08-23 16:28:59

by Anup Patel

Subject: Re: [RFC PATCH 3/5] RISC-V: Add cpu_operatios structure

On Thu, Aug 23, 2018 at 9:37 AM, Christoph Hellwig <[email protected]> wrote:
> On Wed, Aug 22, 2018 at 08:54:51PM +0530, Anup Patel wrote:
>> IMHO, rather than waiting for new CPU ON/OFF methods to come-up we
>> can keep the cpu_operations ready. Also, we are not re-inventing anything
>> here which we might have to discard later because cpu_operations are
>> already tried and hardened for Linux ARM64.
>
> Which is a different cpu architecture, and has shown to actually need
> it. IFF we end up needing it on riscv we can still copy and paste
> it from AMD64.
>
>
>> I agree with you that in long-term SBI-based CPU ON/OFF will be widely
>> used. Most likely we will have at-least two CPU ON/OFF methods:
>> 1. Existing lottery based spinning
>> 2. New SBI calls
>
> And in this most likely case there is no need for an ops vector,
> a simple if/else will be much simpler and cleaner.

Like Atish mentioned, there is a possibility of existing HW going for
spin-table method instead of lottery based spinning.

This means in future we will have three or more CPU ON/OFF methods:
1. Existing lottery based spinning
2. spin-table
3. New SBI calls

I am fine dropping cpu_operations now but I am sure we will end-up
adding it back eventually.

Regards,
Anup