[PATCH v4 0/5] arm64: Dereference CPU operations indirectly

[PATCH v4 0/5] arm64: Dereference CPU operations indirectly

[Gavin Shan]

The subject perhaps can't precisely indicate what this series does, but
keep it same as before for consistency.

In current implementation, an array (@cpu_ops[NR_CPUS]) is maintained
to dereference the CPU operations. 2KB memory are consumed when NR_CPUS
is configured to 256. It's too much than what I expected. This series
reworks the implementation to dereference the CPU operations by using
a CPU operations pointer with assumption - all CPUs should have unified
CPU operations. With this, 8-bytes memory will be used for same purpose.

PATCH[1/5] isn't too much relevant, to declare ACPI parking protocol only
when CONFIG_ARM64_ACPI_PARKING_PROTOCOL has been enabled. PATCH[2/5]
renames cpu_read_ops() to init_cpu_ops(), which is obviously more precise
because it's initializing the CPU operations. PATCH[3/5] introduces
get_cpu_ops(), preparing for droping the array of CPU operations.
PATCH[4/5] removes the CPU operations deferencing array and replaces it
with a pointer with the assumption: all CPUs should have same enablement
method. PATCH[5/5] removes the cpu argument of get_cpu_ops() as it's not
used any more.

Changelog
=========
v4:
   * Rebase to 5.6.rc3 and retest                        (Gavin Shan)
   * Improved commit log for PATCH[4/5] with link tag    (Lorenzo Pieralisi)
   * Using pointer instead of index to dereference the
     unified CPU operations                              (Lorenzo Pieralisi)
   * Merge logic of cpu_get_ops() to get_cpu_method()    (Gavin Shan)
v3:
   * Assume all CPUs have same enablement method. With this, the used
     memory is further squeezed from 64 bytes to 4 bytes (Lorenzo Pieralisi)
   * Add PATCH[5/5] to remove argument of get_cpu_ops()  (Gavin Shan)
v2:
   * Pack 4 CPUs' indexes into one byte. 64 bytes are consumed in order
     to get the CPU operations                            (Robin Murphy)
   * Use ARRAY_SIZE() to iterate @cpu_ops[]               (Robin Murphy)
   * Make index-0 valid                                   (Robin Murphy)

Gavin Shan (5):
  arm64: Declare ACPI parking protocol CPU operation if needed
  arm64: Rename cpu_read_ops() to init_cpu_ops()
  arm64: Introduce get_cpu_ops() helper function
  arm64: Remove CPU operations dereferencing array
  arm64: Remove argument @cpu of get_cpu_ops()

 arch/arm64/include/asm/cpu_ops.h |  8 ++--
 arch/arm64/kernel/cpu_ops.c      | 77 ++++++++++++++++----------------
 arch/arm64/kernel/cpuidle.c      | 10 ++---
 arch/arm64/kernel/setup.c        |  8 ++--
 arch/arm64/kernel/smp.c          | 60 ++++++++++++++++---------
 5 files changed, 93 insertions(+), 70 deletions(-)

-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/5] arm64: Declare ACPI parking protocol CPU operation if needed

[Gavin Shan]

It's obvious we needn't declare the corresponding CPU operation when
CONFIG_ARM64_ACPI_PARKING_PROTOCOL is disabled.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/kernel/cpu_ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index 7e07072757af..2082cfb1be86 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -15,7 +15,9 @@
 #include <asm/smp_plat.h>
 
 extern const struct cpu_operations smp_spin_table_ops;
+#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
 extern const struct cpu_operations acpi_parking_protocol_ops;
+#endif
 extern const struct cpu_operations cpu_psci_ops;
 
 const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/5] arm64: Rename cpu_read_ops() to init_cpu_ops()

[Gavin Shan]

This renames cpu_read_ops() to init_cpu_ops() as the function it's only
called in initialization phase. Also, we will introduce get_cpu_ops() in
the subsequent patches, to retireve the CPU operation by the given CPU
index. The usage of cpu_read_ops() and get_cpu_ops() are difficult to be
distinguish from their names.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/include/asm/cpu_ops.h | 6 +++---
 arch/arm64/kernel/cpu_ops.c      | 2 +-
 arch/arm64/kernel/setup.c        | 2 +-
 arch/arm64/kernel/smp.c          | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index 86aabf1e0199..baa13b5db2ca 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -56,11 +56,11 @@ struct cpu_operations {
 };
 
 extern const struct cpu_operations *cpu_ops[NR_CPUS];
-int __init cpu_read_ops(int cpu);
+int __init init_cpu_ops(int cpu);
 
-static inline void __init cpu_read_bootcpu_ops(void)
+static inline void __init init_bootcpu_ops(void)
 {
-	cpu_read_ops(0);
+	init_cpu_ops(0);
 }
 
 #endif /* ifndef __ASM_CPU_OPS_H */
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index 2082cfb1be86..a6c3c816b618 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -96,7 +96,7 @@ static const char *__init cpu_read_enable_method(int cpu)
 /*
  * Read a cpu's enable method and record it in cpu_ops.
  */
-int __init cpu_read_ops(int cpu)
+int __init init_cpu_ops(int cpu)
 {
 	const char *enable_method = cpu_read_enable_method(cpu);
 
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index a34890bf309f..f66bd260cce8 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -344,7 +344,7 @@ void __init setup_arch(char **cmdline_p)
 	else
 		psci_acpi_init();
 
-	cpu_read_bootcpu_ops();
+	init_bootcpu_ops();
 	smp_init_cpus();
 	smp_build_mpidr_hash();
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index d4ed9a19d8fe..6f8477d7f3be 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -488,7 +488,7 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
  */
 static int __init smp_cpu_setup(int cpu)
 {
-	if (cpu_read_ops(cpu))
+	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
 	if (cpu_ops[cpu]->cpu_init(cpu))
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/5] arm64: Introduce get_cpu_ops() helper function

[Gavin Shan]

This introduces get_cpu_ops() to return the CPU operations according to
the given CPU index. For now, it simply returns the @cpu_ops[cpu] as
before. So it shouldn't introduce any functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/include/asm/cpu_ops.h |  2 +-
 arch/arm64/kernel/cpu_ops.c      |  7 +++-
 arch/arm64/kernel/cpuidle.c      |  9 ++---
 arch/arm64/kernel/setup.c        |  6 ++--
 arch/arm64/kernel/smp.c          | 57 ++++++++++++++++++++++----------
 5 files changed, 55 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index baa13b5db2ca..d28e8f37d3b4 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -55,8 +55,8 @@ struct cpu_operations {
 #endif
 };
 
-extern const struct cpu_operations *cpu_ops[NR_CPUS];
 int __init init_cpu_ops(int cpu);
+extern const struct cpu_operations *get_cpu_ops(int cpu);
 
 static inline void __init init_bootcpu_ops(void)
 {
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index a6c3c816b618..e133011f64b5 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -20,7 +20,7 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
 #endif
 extern const struct cpu_operations cpu_psci_ops;
 
-const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
+static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
 
 static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
 	&smp_spin_table_ops,
@@ -111,3 +111,8 @@ int __init init_cpu_ops(int cpu)
 
 	return 0;
 }
+
+const struct cpu_operations *get_cpu_ops(int cpu)
+{
+	return cpu_ops[cpu];
+}
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index e4d6af2fdec7..b512b5503f6e 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -18,11 +18,11 @@
 
 int arm_cpuidle_init(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 	int ret = -EOPNOTSUPP;
 
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_suspend &&
-			cpu_ops[cpu]->cpu_init_idle)
-		ret = cpu_ops[cpu]->cpu_init_idle(cpu);
+	if (ops && ops->cpu_suspend && ops->cpu_init_idle)
+		ret = ops->cpu_init_idle(cpu);
 
 	return ret;
 }
@@ -37,8 +37,9 @@ int arm_cpuidle_init(unsigned int cpu)
 int arm_cpuidle_suspend(int index)
 {
 	int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
-	return cpu_ops[cpu]->cpu_suspend(index);
+	return ops->cpu_suspend(index);
 }
 
 #ifdef CONFIG_ACPI
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index f66bd260cce8..3fd2c11c09fc 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -371,8 +371,10 @@ void __init setup_arch(char **cmdline_p)
 static inline bool cpu_can_disable(unsigned int cpu)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_can_disable)
-		return cpu_ops[cpu]->cpu_can_disable(cpu);
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
+	if (ops && ops->cpu_can_disable)
+		return ops->cpu_can_disable(cpu);
 #endif
 	return false;
 }
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 6f8477d7f3be..5e1af1a3c521 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -93,8 +93,10 @@ static inline int op_cpu_kill(unsigned int cpu)
  */
 static int boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	if (cpu_ops[cpu]->cpu_boot)
-		return cpu_ops[cpu]->cpu_boot(cpu);
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
+	if (ops->cpu_boot)
+		return ops->cpu_boot(cpu);
 
 	return -EOPNOTSUPP;
 }
@@ -196,6 +198,7 @@ asmlinkage notrace void secondary_start_kernel(void)
 {
 	u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
 	struct mm_struct *mm = &init_mm;
+	const struct cpu_operations *ops;
 	unsigned int cpu;
 
 	cpu = task_cpu(current);
@@ -227,8 +230,9 @@ asmlinkage notrace void secondary_start_kernel(void)
 	 */
 	check_local_cpu_capabilities();
 
-	if (cpu_ops[cpu]->cpu_postboot)
-		cpu_ops[cpu]->cpu_postboot();
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_postboot)
+		ops->cpu_postboot();
 
 	/*
 	 * Log the CPU info before it is marked online and might get read.
@@ -266,19 +270,21 @@ asmlinkage notrace void secondary_start_kernel(void)
 #ifdef CONFIG_HOTPLUG_CPU
 static int op_cpu_disable(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
 	/*
 	 * If we don't have a cpu_die method, abort before we reach the point
 	 * of no return. CPU0 may not have an cpu_ops, so test for it.
 	 */
-	if (!cpu_ops[cpu] || !cpu_ops[cpu]->cpu_die)
+	if (!ops || !ops->cpu_die)
 		return -EOPNOTSUPP;
 
 	/*
 	 * We may need to abort a hot unplug for some other mechanism-specific
 	 * reason.
 	 */
-	if (cpu_ops[cpu]->cpu_disable)
-		return cpu_ops[cpu]->cpu_disable(cpu);
+	if (ops->cpu_disable)
+		return ops->cpu_disable(cpu);
 
 	return 0;
 }
@@ -314,15 +320,17 @@ int __cpu_disable(void)
 
 static int op_cpu_kill(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
 	/*
 	 * If we have no means of synchronising with the dying CPU, then assume
 	 * that it is really dead. We can only wait for an arbitrary length of
 	 * time and hope that it's dead, so let's skip the wait and just hope.
 	 */
-	if (!cpu_ops[cpu]->cpu_kill)
+	if (!ops->cpu_kill)
 		return 0;
 
-	return cpu_ops[cpu]->cpu_kill(cpu);
+	return ops->cpu_kill(cpu);
 }
 
 /*
@@ -357,6 +365,7 @@ void __cpu_die(unsigned int cpu)
 void cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
 	idle_task_exit();
 
@@ -370,7 +379,7 @@ void cpu_die(void)
 	 * mechanism must perform all required cache maintenance to ensure that
 	 * no dirty lines are lost in the process of shutting down the CPU.
 	 */
-	cpu_ops[cpu]->cpu_die(cpu);
+	ops->cpu_die(cpu);
 
 	BUG();
 }
@@ -383,6 +392,7 @@ void cpu_die(void)
 void cpu_die_early(void)
 {
 	int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
 	pr_crit("CPU%d: will not boot\n", cpu);
 
@@ -392,8 +402,8 @@ void cpu_die_early(void)
 #ifdef CONFIG_HOTPLUG_CPU
 	update_cpu_boot_status(CPU_KILL_ME);
 	/* Check if we can park ourselves */
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
+	if (ops && ops->cpu_die)
+		ops->cpu_die(cpu);
 #endif
 	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
 
@@ -488,10 +498,13 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
  */
 static int __init smp_cpu_setup(int cpu)
 {
+	const struct cpu_operations *ops;
+
 	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
-	if (cpu_ops[cpu]->cpu_init(cpu))
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_init(cpu))
 		return -ENODEV;
 
 	set_cpu_possible(cpu, true);
@@ -714,6 +727,7 @@ void __init smp_init_cpus(void)
 
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
+	const struct cpu_operations *ops;
 	int err;
 	unsigned int cpu;
 	unsigned int this_cpu;
@@ -744,10 +758,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 		if (cpu == smp_processor_id())
 			continue;
 
-		if (!cpu_ops[cpu])
+		ops = get_cpu_ops(cpu);
+		if (!ops)
 			continue;
 
-		err = cpu_ops[cpu]->cpu_prepare(cpu);
+		err = ops->cpu_prepare(cpu);
 		if (err)
 			continue;
 
@@ -855,6 +870,10 @@ static atomic_t waiting_for_crash_ipi = ATOMIC_INIT(0);
 
 static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 {
+#ifdef CONFIG_HOTPLUG_CPU
+	const struct cpu_operations *ops;
+#endif
+
 #ifdef CONFIG_KEXEC_CORE
 	crash_save_cpu(regs, cpu);
 
@@ -864,8 +883,9 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 	sdei_mask_local_cpu();
 
 #ifdef CONFIG_HOTPLUG_CPU
-	if (cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_die)
+		ops->cpu_die(cpu);
 #endif
 
 	/* just in case */
@@ -1044,8 +1064,9 @@ static bool have_cpu_die(void)
 {
 #ifdef CONFIG_HOTPLUG_CPU
 	int any_cpu = raw_smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(any_cpu);
 
-	if (cpu_ops[any_cpu] && cpu_ops[any_cpu]->cpu_die)
+	if (ops && ops->cpu_die)
 		return true;
 #endif
 	return false;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/5] arm64: Remove CPU operations dereferencing array

[Gavin Shan]

One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
much memory has been used for this. Also, all CPUs must use same CPU
operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi
pointed out.

This introduces variable (@cpu_ops) to store the unfied CPU operations.
Those CPUs using different operations, which is figured out from device
tree or ACPI table, won't be brought up. With this, the CPU operations
dereferencing array is removed and 2KB memory is saved. Note the logic
of cpu_get_ops() is merged to get_cpu_method() since the logic is simple
enough and no need to have a function for that.

Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/kernel/cpu_ops.c | 70 +++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 38 deletions(-)

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index e133011f64b5..af73ca502b95 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -20,41 +20,19 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
 #endif
 extern const struct cpu_operations cpu_psci_ops;
 
-static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
-
-static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
+static const struct cpu_operations *const available_cpu_ops[] __initconst = {
 	&smp_spin_table_ops,
-	&cpu_psci_ops,
-	NULL,
-};
-
-static const struct cpu_operations *const acpi_supported_cpu_ops[] __initconst = {
 #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
 	&acpi_parking_protocol_ops,
 #endif
 	&cpu_psci_ops,
-	NULL,
 };
+static const struct cpu_operations *cpu_ops __ro_after_init;
 
-static const struct cpu_operations * __init cpu_get_ops(const char *name)
-{
-	const struct cpu_operations *const *ops;
-
-	ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;
-
-	while (*ops) {
-		if (!strcmp(name, (*ops)->name))
-			return *ops;
-
-		ops++;
-	}
-
-	return NULL;
-}
-
-static const char *__init cpu_read_enable_method(int cpu)
+static const struct cpu_operations * __init get_cpu_method(int cpu)
 {
 	const char *enable_method;
+	int i;
 
 	if (acpi_disabled) {
 		struct device_node *dn = of_get_cpu_node(cpu, NULL);
@@ -91,22 +69,38 @@ static const char *__init cpu_read_enable_method(int cpu)
 		}
 	}
 
-	return enable_method;
+	if (!enable_method) {
+		pr_warn("No enable-method found on CPU %d\n", cpu);
+		return NULL;
+	}
+
+	/* Search in the array with method */
+	for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
+		if (!strcmp(available_cpu_ops[i]->name, enable_method))
+			return available_cpu_ops[i];
+	}
+
+	return NULL;
 }
-/*
- * Read a cpu's enable method and record it in cpu_ops.
- */
+
 int __init init_cpu_ops(int cpu)
 {
-	const char *enable_method = cpu_read_enable_method(cpu);
-
-	if (!enable_method)
-		return -ENODEV;
+	const struct cpu_operations *ops = get_cpu_method(cpu);
 
-	cpu_ops[cpu] = cpu_get_ops(enable_method);
-	if (!cpu_ops[cpu]) {
-		pr_warn("Unsupported enable-method: %s\n", enable_method);
+	if (!ops)
 		return -EOPNOTSUPP;
+
+	/* Update global CPU operations if it's not initialized yet */
+	if (!cpu_ops) {
+		cpu_ops = ops;
+		return 0;
+	}
+
+	/* We should have unified CPU operations */
+	if (ops != cpu_ops) {
+		pr_warn("Invalid CPU operations %s (%s) on CPU %d\n",
+			ops->name, cpu_ops->name, cpu);
+		return -EINVAL;
 	}
 
 	return 0;
@@ -114,5 +108,5 @@ int __init init_cpu_ops(int cpu)
 
 const struct cpu_operations *get_cpu_ops(int cpu)
 {
-	return cpu_ops[cpu];
+	return cpu_ops;
 }
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 5/5] arm64: Remove argument @cpu of get_cpu_ops()

[Gavin Shan]

This removes the argument (@cpu) of get_cpu_ops() as it's not used.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/include/asm/cpu_ops.h |  2 +-
 arch/arm64/kernel/cpu_ops.c      |  2 +-
 arch/arm64/kernel/cpuidle.c      |  5 ++---
 arch/arm64/kernel/setup.c        |  2 +-
 arch/arm64/kernel/smp.c          | 21 ++++++++++-----------
 5 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index d28e8f37d3b4..1d5c514ca053 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -56,7 +56,7 @@ struct cpu_operations {
 };
 
 int __init init_cpu_ops(int cpu);
-extern const struct cpu_operations *get_cpu_ops(int cpu);
+extern const struct cpu_operations *get_cpu_ops(void);
 
 static inline void __init init_bootcpu_ops(void)
 {
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index af73ca502b95..a32a2e07e7e6 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -106,7 +106,7 @@ int __init init_cpu_ops(int cpu)
 	return 0;
 }
 
-const struct cpu_operations *get_cpu_ops(int cpu)
+const struct cpu_operations *get_cpu_ops(void)
 {
 	return cpu_ops;
 }
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index b512b5503f6e..da2db14d2d45 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -18,7 +18,7 @@
 
 int arm_cpuidle_init(unsigned int cpu)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 	int ret = -EOPNOTSUPP;
 
 	if (ops && ops->cpu_suspend && ops->cpu_init_idle)
@@ -36,8 +36,7 @@ int arm_cpuidle_init(unsigned int cpu)
  */
 int arm_cpuidle_suspend(int index)
 {
-	int cpu = smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	return ops->cpu_suspend(index);
 }
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 3fd2c11c09fc..5c68fa2d9d1b 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -371,7 +371,7 @@ void __init setup_arch(char **cmdline_p)
 static inline bool cpu_can_disable(unsigned int cpu)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	if (ops && ops->cpu_can_disable)
 		return ops->cpu_can_disable(cpu);
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 5e1af1a3c521..0180d4163e17 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -93,7 +93,7 @@ static inline int op_cpu_kill(unsigned int cpu)
  */
 static int boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	if (ops->cpu_boot)
 		return ops->cpu_boot(cpu);
@@ -230,7 +230,7 @@ asmlinkage notrace void secondary_start_kernel(void)
 	 */
 	check_local_cpu_capabilities();
 
-	ops = get_cpu_ops(cpu);
+	ops = get_cpu_ops();
 	if (ops->cpu_postboot)
 		ops->cpu_postboot();
 
@@ -270,7 +270,7 @@ asmlinkage notrace void secondary_start_kernel(void)
 #ifdef CONFIG_HOTPLUG_CPU
 static int op_cpu_disable(unsigned int cpu)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	/*
 	 * If we don't have a cpu_die method, abort before we reach the point
@@ -320,7 +320,7 @@ int __cpu_disable(void)
 
 static int op_cpu_kill(unsigned int cpu)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	/*
 	 * If we have no means of synchronising with the dying CPU, then assume
@@ -365,7 +365,7 @@ void __cpu_die(unsigned int cpu)
 void cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	idle_task_exit();
 
@@ -392,7 +392,7 @@ void cpu_die(void)
 void cpu_die_early(void)
 {
 	int cpu = smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	pr_crit("CPU%d: will not boot\n", cpu);
 
@@ -503,7 +503,7 @@ static int __init smp_cpu_setup(int cpu)
 	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
-	ops = get_cpu_ops(cpu);
+	ops = get_cpu_ops();
 	if (ops->cpu_init(cpu))
 		return -ENODEV;
 
@@ -758,7 +758,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 		if (cpu == smp_processor_id())
 			continue;
 
-		ops = get_cpu_ops(cpu);
+		ops = get_cpu_ops();
 		if (!ops)
 			continue;
 
@@ -883,7 +883,7 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 	sdei_mask_local_cpu();
 
 #ifdef CONFIG_HOTPLUG_CPU
-	ops = get_cpu_ops(cpu);
+	ops = get_cpu_ops();
 	if (ops->cpu_die)
 		ops->cpu_die(cpu);
 #endif
@@ -1063,8 +1063,7 @@ int setup_profiling_timer(unsigned int multiplier)
 static bool have_cpu_die(void)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	int any_cpu = raw_smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(any_cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	if (ops && ops->cpu_die)
 		return true;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/5] arm64: Dereference CPU operations indirectly

[Gavin Shan]

On 2/26/20 11:23 AM, Gavin Shan wrote:
> The subject perhaps can't precisely indicate what this series does, but
> keep it same as before for consistency.
> 
> In current implementation, an array (@cpu_ops[NR_CPUS]) is maintained
> to dereference the CPU operations. 2KB memory are consumed when NR_CPUS
> is configured to 256. It's too much than what I expected. This series
> reworks the implementation to dereference the CPU operations by using
> a CPU operations pointer with assumption - all CPUs should have unified
> CPU operations. With this, 8-bytes memory will be used for same purpose.
> 
> PATCH[1/5] isn't too much relevant, to declare ACPI parking protocol only
> when CONFIG_ARM64_ACPI_PARKING_PROTOCOL has been enabled. PATCH[2/5]
> renames cpu_read_ops() to init_cpu_ops(), which is obviously more precise
> because it's initializing the CPU operations. PATCH[3/5] introduces
> get_cpu_ops(), preparing for droping the array of CPU operations.
> PATCH[4/5] removes the CPU operations deferencing array and replaces it
> with a pointer with the assumption: all CPUs should have same enablement
> method. PATCH[5/5] removes the cpu argument of get_cpu_ops() as it's not
> used any more.
> 

Lorenzo, kindly ping...

> Changelog
> =========
> v4:
>     * Rebase to 5.6.rc3 and retest                        (Gavin Shan)
>     * Improved commit log for PATCH[4/5] with link tag    (Lorenzo Pieralisi)
>     * Using pointer instead of index to dereference the
>       unified CPU operations                              (Lorenzo Pieralisi)
>     * Merge logic of cpu_get_ops() to get_cpu_method()    (Gavin Shan)
> v3:
>     * Assume all CPUs have same enablement method. With this, the used
>       memory is further squeezed from 64 bytes to 4 bytes (Lorenzo Pieralisi)
>     * Add PATCH[5/5] to remove argument of get_cpu_ops()  (Gavin Shan)
> v2:
>     * Pack 4 CPUs' indexes into one byte. 64 bytes are consumed in order
>       to get the CPU operations                            (Robin Murphy)
>     * Use ARRAY_SIZE() to iterate @cpu_ops[]               (Robin Murphy)
>     * Make index-0 valid                                   (Robin Murphy)
> 
> Gavin Shan (5):
>    arm64: Declare ACPI parking protocol CPU operation if needed
>    arm64: Rename cpu_read_ops() to init_cpu_ops()
>    arm64: Introduce get_cpu_ops() helper function
>    arm64: Remove CPU operations dereferencing array
>    arm64: Remove argument @cpu of get_cpu_ops()
> 
>   arch/arm64/include/asm/cpu_ops.h |  8 ++--
>   arch/arm64/kernel/cpu_ops.c      | 77 ++++++++++++++++----------------
>   arch/arm64/kernel/cpuidle.c      | 10 ++---
>   arch/arm64/kernel/setup.c        |  8 ++--
>   arch/arm64/kernel/smp.c          | 60 ++++++++++++++++---------
>   5 files changed, 93 insertions(+), 70 deletions(-)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/5] arm64: Rename cpu_read_ops() to init_cpu_ops()

[Mark Rutland]

On Wed, Feb 26, 2020 at 11:23:53AM +1100, Gavin Shan wrote:
> This renames cpu_read_ops() to init_cpu_ops() as the function it's only
> called in initialization phase. Also, we will introduce get_cpu_ops() in
> the subsequent patches, to retireve the CPU operation by the given CPU
> index. The usage of cpu_read_ops() and get_cpu_ops() are difficult to be
> distinguish from their names.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

That rationale makes sense to me, and the patch itself looks sound, so:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/cpu_ops.h | 6 +++---
>  arch/arm64/kernel/cpu_ops.c      | 2 +-
>  arch/arm64/kernel/setup.c        | 2 +-
>  arch/arm64/kernel/smp.c          | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
> index 86aabf1e0199..baa13b5db2ca 100644
> --- a/arch/arm64/include/asm/cpu_ops.h
> +++ b/arch/arm64/include/asm/cpu_ops.h
> @@ -56,11 +56,11 @@ struct cpu_operations {
>  };
>  
>  extern const struct cpu_operations *cpu_ops[NR_CPUS];
> -int __init cpu_read_ops(int cpu);
> +int __init init_cpu_ops(int cpu);
>  
> -static inline void __init cpu_read_bootcpu_ops(void)
> +static inline void __init init_bootcpu_ops(void)
>  {
> -	cpu_read_ops(0);
> +	init_cpu_ops(0);
>  }
>  
>  #endif /* ifndef __ASM_CPU_OPS_H */
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index 2082cfb1be86..a6c3c816b618 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -96,7 +96,7 @@ static const char *__init cpu_read_enable_method(int cpu)
>  /*
>   * Read a cpu's enable method and record it in cpu_ops.
>   */
> -int __init cpu_read_ops(int cpu)
> +int __init init_cpu_ops(int cpu)
>  {
>  	const char *enable_method = cpu_read_enable_method(cpu);
>  
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index a34890bf309f..f66bd260cce8 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -344,7 +344,7 @@ void __init setup_arch(char **cmdline_p)
>  	else
>  		psci_acpi_init();
>  
> -	cpu_read_bootcpu_ops();
> +	init_bootcpu_ops();
>  	smp_init_cpus();
>  	smp_build_mpidr_hash();
>  
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index d4ed9a19d8fe..6f8477d7f3be 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -488,7 +488,7 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
>   */
>  static int __init smp_cpu_setup(int cpu)
>  {
> -	if (cpu_read_ops(cpu))
> +	if (init_cpu_ops(cpu))
>  		return -ENODEV;
>  
>  	if (cpu_ops[cpu]->cpu_init(cpu))
> -- 
> 2.23.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] arm64: Declare ACPI parking protocol CPU operation if needed

[Mark Rutland]

On Wed, Feb 26, 2020 at 11:23:52AM +1100, Gavin Shan wrote:
> It's obvious we needn't declare the corresponding CPU operation when
> CONFIG_ARM64_ACPI_PARKING_PROTOCOL is disabled.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Is there a problem leaving this as-is, e.g. a compiler warning? If so,
it'd be nice to mention that in the commit message.

We don't always bother placing declartions under ifdefs where the use
would result in a link-time error.

No strong feelings form me either way, so FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/cpu_ops.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index 7e07072757af..2082cfb1be86 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -15,7 +15,9 @@
>  #include <asm/smp_plat.h>
>  
>  extern const struct cpu_operations smp_spin_table_ops;
> +#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
>  extern const struct cpu_operations acpi_parking_protocol_ops;
> +#endif
>  extern const struct cpu_operations cpu_psci_ops;
>  
>  const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
> -- 
> 2.23.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] arm64: Introduce get_cpu_ops() helper function

[Mark Rutland]

On Wed, Feb 26, 2020 at 11:23:54AM +1100, Gavin Shan wrote:
> This introduces get_cpu_ops() to return the CPU operations according to
> the given CPU index. For now, it simply returns the @cpu_ops[cpu] as
> before. So it shouldn't introduce any functional changes.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Generally this looks good to me; I like that it simplifies the cases
where we get the ops repeatedly today.

I have one comment below.

> @@ -383,6 +392,7 @@ void cpu_die(void)
>  void cpu_die_early(void)
>  {
>  	int cpu = smp_processor_id();
> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
>  
>  	pr_crit("CPU%d: will not boot\n", cpu);
>  
> @@ -392,8 +402,8 @@ void cpu_die_early(void)
>  #ifdef CONFIG_HOTPLUG_CPU
>  	update_cpu_boot_status(CPU_KILL_ME);
>  	/* Check if we can park ourselves */
> -	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
> -		cpu_ops[cpu]->cpu_die(cpu);
> +	if (ops && ops->cpu_die)
> +		ops->cpu_die(cpu);
>  #endif

Can we factor this out the die logic into a helper:

| static void __cpu_try_die(int cpu)
| {
| #ifdef CONFIG_HOTPLUG_CPU
| 	const struct cpu_operations *ops = get_cpu_ops(cpu);
| 	if (ops && ops->cpu_die)
| 		ops->cpu_die(cpu);
| #endif
| }

... with cpu_die_early() having:

| if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
| 	update_cpu_boot_status(CPU_KILL_ME);
|	__cpu_try_die(cpu);
| }

... and likewise in ipi_cpu_crash_stop(), without the
update_cpu_boot_status() ...

> @@ -855,6 +870,10 @@ static atomic_t waiting_for_crash_ipi = ATOMIC_INIT(0);
>  
>  static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
>  {
> +#ifdef CONFIG_HOTPLUG_CPU
> +	const struct cpu_operations *ops;
> +#endif

... where this can go ...

> +
>  #ifdef CONFIG_KEXEC_CORE
>  	crash_save_cpu(regs, cpu);
>  
> @@ -864,8 +883,9 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
>  	sdei_mask_local_cpu();
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> -	if (cpu_ops[cpu]->cpu_die)
> -		cpu_ops[cpu]->cpu_die(cpu);
> +	ops = get_cpu_ops(cpu);
> +	if (ops->cpu_die)
> +		ops->cpu_die(cpu);
>  #endif

... and this can be:

| if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
| 	__cpu_try_die(cpu);

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] arm64: Remove CPU operations dereferencing array

[Mark Rutland]

On Wed, Feb 26, 2020 at 11:23:55AM +1100, Gavin Shan wrote:
> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
> much memory has been used for this. Also, all CPUs must use same CPU
> operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi
> pointed out.

I suspect there might be some pre PSCIv0.2 platforms where the boot CPU
doesn't have PSCI, but others do. On those platforms, this could be
because CPU0 cannot be hotplugged out, and we must avoid doing so.

Can you check the in-kernel DTs to see if any of those exist?

Other than that, I agree that mandating uniformity is the best approach
here.

>  int __init init_cpu_ops(int cpu)
>  {
> -	const char *enable_method = cpu_read_enable_method(cpu);
> -
> -	if (!enable_method)
> -		return -ENODEV;
> +	const struct cpu_operations *ops = get_cpu_method(cpu);
>  
> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
> -	if (!cpu_ops[cpu]) {
> -		pr_warn("Unsupported enable-method: %s\n", enable_method);
> +	if (!ops)
>  		return -EOPNOTSUPP;
> +
> +	/* Update global CPU operations if it's not initialized yet */
> +	if (!cpu_ops) {
> +		cpu_ops = ops;
> +		return 0;
> +	}

As above, I don't think this is quite right. If we're going to mandate
uniformity, we should init the ops from the boot CPU, and then verify
that every other CPU matches that. The initialization of the global ops
should not be conditional.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/5] arm64: Declare ACPI parking protocol CPU operation if needed

[Gavin Shan]

On 3/17/20 9:28 PM, Mark Rutland wrote:
> On Wed, Feb 26, 2020 at 11:23:52AM +1100, Gavin Shan wrote:
>> It's obvious we needn't declare the corresponding CPU operation when
>> CONFIG_ARM64_ACPI_PARKING_PROTOCOL is disabled.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Is there a problem leaving this as-is, e.g. a compiler warning? If so,
> it'd be nice to mention that in the commit message.
> 
> We don't always bother placing declartions under ifdefs where the use
> would result in a link-time error.
> 
> No strong feelings form me either way, so FWIW:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> Mark.
> 

It doesn't cause a compiler warning because the corresponding CPU operations
is declared as "extern". I will have commit log in next revision as below:

It's obvious we needn't declare the corresponding CPU operation when
CONFIG_ARM64_ACPI_PARKING_PROTOCOL is disabled, even it doesn't cause
any compiling warnings.

Thanks,
Gavin

>> ---
>>   arch/arm64/kernel/cpu_ops.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
>> index 7e07072757af..2082cfb1be86 100644
>> --- a/arch/arm64/kernel/cpu_ops.c
>> +++ b/arch/arm64/kernel/cpu_ops.c
>> @@ -15,7 +15,9 @@
>>   #include <asm/smp_plat.h>
>>   
>>   extern const struct cpu_operations smp_spin_table_ops;
>> +#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
>>   extern const struct cpu_operations acpi_parking_protocol_ops;
>> +#endif
>>   extern const struct cpu_operations cpu_psci_ops;
>>   
>>   const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
>> -- 
>> 2.23.0
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/5] arm64: Introduce get_cpu_ops() helper function

[Gavin Shan]

On 3/17/20 9:48 PM, Mark Rutland wrote:
> On Wed, Feb 26, 2020 at 11:23:54AM +1100, Gavin Shan wrote:
>> This introduces get_cpu_ops() to return the CPU operations according to
>> the given CPU index. For now, it simply returns the @cpu_ops[cpu] as
>> before. So it shouldn't introduce any functional changes.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Generally this looks good to me; I like that it simplifies the cases
> where we get the ops repeatedly today.
> 
> I have one comment below.
> 
>> @@ -383,6 +392,7 @@ void cpu_die(void)
>>   void cpu_die_early(void)
>>   {
>>   	int cpu = smp_processor_id();
>> +	const struct cpu_operations *ops = get_cpu_ops(cpu);
>>   
>>   	pr_crit("CPU%d: will not boot\n", cpu);
>>   
>> @@ -392,8 +402,8 @@ void cpu_die_early(void)
>>   #ifdef CONFIG_HOTPLUG_CPU
>>   	update_cpu_boot_status(CPU_KILL_ME);
>>   	/* Check if we can park ourselves */
>> -	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
>> -		cpu_ops[cpu]->cpu_die(cpu);
>> +	if (ops && ops->cpu_die)
>> +		ops->cpu_die(cpu);
>>   #endif
> 
> Can we factor this out the die logic into a helper:
> 
> | static void __cpu_try_die(int cpu)
> | {
> | #ifdef CONFIG_HOTPLUG_CPU
> | 	const struct cpu_operations *ops = get_cpu_ops(cpu);
> | 	if (ops && ops->cpu_die)
> | 		ops->cpu_die(cpu);
> | #endif
> | }
> 
> ... with cpu_die_early() having:
> 
> | if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
> | 	update_cpu_boot_status(CPU_KILL_ME);
> |	__cpu_try_die(cpu);
> | }
> 
> ... and likewise in ipi_cpu_crash_stop(), without the
> update_cpu_boot_status() ...
> 
>> @@ -855,6 +870,10 @@ static atomic_t waiting_for_crash_ipi = ATOMIC_INIT(0);
>>   
>>   static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
>>   {
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	const struct cpu_operations *ops;
>> +#endif
> 
> ... where this can go ...
> 
>> +
>>   #ifdef CONFIG_KEXEC_CORE
>>   	crash_save_cpu(regs, cpu);
>>   
>> @@ -864,8 +883,9 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
>>   	sdei_mask_local_cpu();
>>   
>>   #ifdef CONFIG_HOTPLUG_CPU
>> -	if (cpu_ops[cpu]->cpu_die)
>> -		cpu_ops[cpu]->cpu_die(cpu);
>> +	ops = get_cpu_ops(cpu);
>> +	if (ops->cpu_die)
>> +		ops->cpu_die(cpu);
>>   #endif
> 
> ... and this can be:
> 
> | if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> | 	__cpu_try_die(cpu);
> 
> Thanks,
> Mark.
> 

Thanks for the detailed comments. With it, the code looks cleaner. I'll
have this in next revision (v5).

Thanks,
Gavin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] arm64: Remove CPU operations dereferencing array

[Gavin Shan]

On 3/17/20 9:56 PM, Mark Rutland wrote:
> On Wed, Feb 26, 2020 at 11:23:55AM +1100, Gavin Shan wrote:
>> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
>> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
>> much memory has been used for this. Also, all CPUs must use same CPU
>> operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi
>> pointed out.
> 
> I suspect there might be some pre PSCIv0.2 platforms where the boot CPU
> doesn't have PSCI, but others do. On those platforms, this could be
> because CPU0 cannot be hotplugged out, and we must avoid doing so.
> 
> Can you check the in-kernel DTs to see if any of those exist?
> 
> Other than that, I agree that mandating uniformity is the best approach
> here.
> 

Thanks for the comments, Mark. There are platforms where the CPU#0 and other
CPUs have different "enable-method" specified. More specificly, CPU#0 doesn't
have "enable-method" while other CPUs have "psci" specified:

    lg/lg1312.dtsi
    lg/lg1313.dtsi
    mediatek/mt2712e.dtsi

In order to support two enable methods, I think we have two options here:
(1) Revert the code to what we had in v2. Two bits consumed by one CPU,
which is taken as index to a CPU operation array. The code can be found in
link [1] (2) Two CPU operation pointers are maintained. One is used to track
the CPU#0's operations and other one is for other cpus.

[1] https://patchwork.kernel.org/patch/11363745/

Please let me know which one is better.

>>   int __init init_cpu_ops(int cpu)
>>   {
>> -	const char *enable_method = cpu_read_enable_method(cpu);
>> -
>> -	if (!enable_method)
>> -		return -ENODEV;
>> +	const struct cpu_operations *ops = get_cpu_method(cpu);
>>   
>> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
>> -	if (!cpu_ops[cpu]) {
>> -		pr_warn("Unsupported enable-method: %s\n", enable_method);
>> +	if (!ops)
>>   		return -EOPNOTSUPP;
>> +
>> +	/* Update global CPU operations if it's not initialized yet */
>> +	if (!cpu_ops) {
>> +		cpu_ops = ops;
>> +		return 0;
>> +	}
> 
> As above, I don't think this is quite right. If we're going to mandate
> uniformity, we should init the ops from the boot CPU, and then verify
> that every other CPU matches that. The initialization of the global ops
> should not be conditional.> 
I think you're correct because CPU#0's "enable-method" can be unspecified.
In this case, the CPU#0's operations will be set to same one as other CPUs,
which isn't correct. I will see how to handle this comments when the above
comments is resolved. This might become invalid after the above comments get
resolved.

Thanks,
Gavin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/5] arm64: Remove CPU operations dereferencing array

[Gavin Shan]

On 3/18/20 2:53 PM, Gavin Shan wrote:
> On 3/17/20 9:56 PM, Mark Rutland wrote:
>> On Wed, Feb 26, 2020 at 11:23:55AM +1100, Gavin Shan wrote:
>>> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
>>> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
>>> much memory has been used for this. Also, all CPUs must use same CPU
>>> operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi
>>> pointed out.
>>
>> I suspect there might be some pre PSCIv0.2 platforms where the boot CPU
>> doesn't have PSCI, but others do. On those platforms, this could be
>> because CPU0 cannot be hotplugged out, and we must avoid doing so.
>>
>> Can you check the in-kernel DTs to see if any of those exist?
>>
>> Other than that, I agree that mandating uniformity is the best approach
>> here.
>>
> 
> Thanks for the comments, Mark. There are platforms where the CPU#0 and other
> CPUs have different "enable-method" specified. More specificly, CPU#0 doesn't
> have "enable-method" while other CPUs have "psci" specified:
> 
>     lg/lg1312.dtsi
>     lg/lg1313.dtsi
>     mediatek/mt2712e.dtsi
> 
> In order to support two enable methods, I think we have two options here:
> (1) Revert the code to what we had in v2. Two bits consumed by one CPU,
> which is taken as index to a CPU operation array. The code can be found in
> link [1] (2) Two CPU operation pointers are maintained. One is used to track
> the CPU#0's operations and other one is for other cpus.
> 
> [1] https://patchwork.kernel.org/patch/11363745/
> 
> Please let me know which one is better.
> 

Mark, I plan to post a new revision to have option#2 after thinking
about it: we will have two pointers to track the operations for boot cpu
and the secondary CPUs separately. the consumed memory isn't scaled up to
the configured CPU number. So I think it's better than option#1, which
uses two-bits as index to CPU operation array.

Thanks,
Gavin

>>>   int __init init_cpu_ops(int cpu)
>>>   {
>>> -    const char *enable_method = cpu_read_enable_method(cpu);
>>> -
>>> -    if (!enable_method)
>>> -        return -ENODEV;
>>> +    const struct cpu_operations *ops = get_cpu_method(cpu);
>>> -    cpu_ops[cpu] = cpu_get_ops(enable_method);
>>> -    if (!cpu_ops[cpu]) {
>>> -        pr_warn("Unsupported enable-method: %s\n", enable_method);
>>> +    if (!ops)
>>>           return -EOPNOTSUPP;
>>> +
>>> +    /* Update global CPU operations if it's not initialized yet */
>>> +    if (!cpu_ops) {
>>> +        cpu_ops = ops;
>>> +        return 0;
>>> +    }
>>
>> As above, I don't think this is quite right. If we're going to mandate
>> uniformity, we should init the ops from the boot CPU, and then verify
>> that every other CPU matches that. The initialization of the global ops
>> should not be conditional.> 
> I think you're correct because CPU#0's "enable-method" can be unspecified.
> In this case, the CPU#0's operations will be set to same one as other CPUs,
> which isn't correct. I will see how to handle this comments when the above
> comments is resolved. This might become invalid after the above comments get
> resolved.
> 
> Thanks,
> Gavin
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel