Hi Paul,
On Thu, Sep 14, 2017 at 02:57:07PM +1000, Paul Mackerras wrote:
> Commit f3b3f28493d9 ("powerpc/powernv/idle: Don't override
> default/deepest directly in kernel", 2017-03-22) made the following
> change in pnv_cpu_offline() in arch/powerpc/platforms/powernv/idle.c:
>
> - if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> + if (cpu_has_feature(CPU_FTR_ARCH_300) && deepest_stop_found) {
> srr1 = power9_idle_stop(pnv_deepest_stop_psscr_val,
> pnv_deepest_stop_psscr_mask);
> } else if (idle_states & OPAL_PM_WINKLE_ENABLED) {
> srr1 = power7_winkle();
>
This patch follows an earlier patch with commit 900612315788
("powerpc/powernv/smp: Add busy-wait loop as fall back for
CPU-Hotplug") to ensure that when platform idle states aren't
available, the offlined CPUs spin in a busy while loop.
> Which seems to be saying that previously, on POWER9 we would always
> call power9_idle_stop(), but now we might possibly call
> power7_idle_insn() to do a nap or sleep or rvwinkle instruction.
>
On POWER9, none of the platform idle states (stop states) exposed by
the skiboot have OPAL_PM_[NAP/SLEEP/WINKLE]_ENABLED set in their
respective flags. Hence we will never enter those "else if"
conditions.
> Is this really what was meant? Nap, sleep and rvwinkle are illegal
> instructions on POWER9. It looks to me as if that statement needs to
> be restructured to do what the commit description says, which is to
> call power9_idle_stop if we have found a valid stop state, and
> otherwise to poll.
> At present we are relying on idle_states not not having any of the
> nap/sleep/winkle bits set in it. Is that guaranteed on POWER9? If so
> it is at least deserving of a comment.
How about the following patch
---------------------------x8---------x8---------------------------------
>From a17e4f71bfc9d208c45335acb47fc2b3a9f61923 Mon Sep 17 00:00:00 2001
From: "Gautham R. Shenoy" <e...@linux.vnet.ibm.com>
Date: Fri, 15 Sep 2017 13:32:04 +0530
Subject: [PATCH] powerpc/powernv/idle: Restructure pnv_cpu_offline for
readability
The current code for handling CPU offline will call a function to put
a CPU in a stop state if this is a POWER9 CPU and if a suitable stop
state is found. If one of these conditions fails, it checks for the
OPAL flags for the idle state to see if winkle, fastsleep, nap are
available (which shouldn't be available on POWER9) and finally put the
CPU into a busy while loop.
This code gives an impression that we are exploring the possibility of
putting the CPUs into fastsleep/nap/winkle on POWER9 even though that
is not possible and is illegal.
Hence rewrite the code to seperately handle the offline for POWER9 and
pre-POWER9 CPUs.
This patch introduces no functional change. Just restructures the code
for improved readability.
Signed-off-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/idle.c | 70 +++++++++++++++++++++++------------
1 file changed, 47 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/idle.c
b/arch/powerpc/platforms/powernv/idle.c
index a553aee..67b5475 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -380,6 +380,29 @@ void power9_idle_type(unsigned long stop_psscr_val,
}
/*
+ * offline_idle_loop: Offlined CPUs will busy-idle on platforms that
+ * don't have any platform idle state enabled.
+ *
+ * @cpu: The cpu that is offlined.
+ *
+ * Returns 0 since the srr1 wasn't lost as we didn't execute any
+ * platform idle.
+ *
+ */
+unsigned long offline_idle_loop(unsigned int cpu)
+{
+ /* This is the fallback method. We emulate snooze */
+ while (!generic_check_cpu_restart(cpu)) {
+ HMT_low();
+ HMT_very_low();
+ }
+
+ HMT_medium();
+
+ return 0;
+}
+
+/*
* Used for ppc_md.power_save which needs a function with no parameters
*/
void power9_idle(void)
@@ -391,6 +414,9 @@ void power9_idle(void)
/*
* pnv_cpu_offline: A function that puts the CPU into the deepest
* available platform idle state on a CPU-Offline.
+ * If no platform idle state is available, the CPU busy waits.
+ * (see offline_idle_loop())
+ *
* interrupts hard disabled and no lazy irq pending.
*/
unsigned long pnv_cpu_offline(unsigned int cpu)
@@ -400,32 +426,30 @@ unsigned long pnv_cpu_offline(unsigned int cpu)
__ppc64_runlatch_off();
- if (cpu_has_feature(CPU_FTR_ARCH_300) && deepest_stop_found) {
- unsigned long psscr;
-
- psscr = mfspr(SPRN_PSSCR);
- psscr = (psscr & ~pnv_deepest_stop_psscr_mask) |
- pnv_deepest_stop_psscr_val;
- srr1 = power9_idle_stop(psscr);
-
- } else if ((idle_states & OPAL_PM_WINKLE_ENABLED) &&
- (idle_states & OPAL_PM_LOSE_FULL_CONTEXT)) {
- srr1 = power7_idle_insn(PNV_THREAD_WINKLE);
- } else if ((idle_states & OPAL_PM_SLEEP_ENABLED) ||
- (idle_states & OPAL_PM_SLEEP_ENABLED_ER1)) {
- srr1 = power7_idle_insn(PNV_THREAD_SLEEP);
- } else if (idle_states & OPAL_PM_NAP_ENABLED) {
- srr1 = power7_idle_insn(PNV_THREAD_NAP);
+ if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+ if (deepest_stop_found) {
+ unsigned long psscr;
+
+ psscr = mfspr(SPRN_PSSCR);
+ psscr = (psscr & ~pnv_deepest_stop_psscr_mask) |
+ pnv_deepest_stop_psscr_val;
+ srr1 = power9_idle_stop(psscr);
+ } else {
+ srr1 = offline_idle_loop(cpu);
+ }
} else {
- /* This is the fallback method. We emulate snooze */
- while (!generic_check_cpu_restart(cpu)) {
- HMT_low();
- HMT_very_low();
+ if ((idle_states & OPAL_PM_WINKLE_ENABLED) &&
+ (idle_states & OPAL_PM_LOSE_FULL_CONTEXT)) {
+ srr1 = power7_idle_insn(PNV_THREAD_WINKLE);
+ } else if ((idle_states & OPAL_PM_SLEEP_ENABLED) ||
+ (idle_states & OPAL_PM_SLEEP_ENABLED_ER1)) {
+ srr1 = power7_idle_insn(PNV_THREAD_SLEEP);
+ } else if (idle_states & OPAL_PM_NAP_ENABLED) {
+ srr1 = power7_idle_insn(PNV_THREAD_NAP);
+ } else {
+ srr1 = offline_idle_loop(cpu);
}
- srr1 = 0;
- HMT_medium();
}
-
__ppc64_runlatch_on();
return srr1;
--
1.9.4
>
> Paul.
>
