From: "Gautham R. Shenoy" <[email protected]>
Hi,
This patchset contains fixes to make CPU-Hotplug working on correctly
on POWER9 DD1 systems.
There are three patches in the series.
- The first patch adds a fallback mechanism for CPU-Hotplug when no
platform idle state is available.
- The second patch ensures that the kernel doesn't use any stop state
that is not exposed by the firmware.
- The third patch adds a recovery framework for correctly recovering
paca pointer of the thread waking up from a stop.
These patches are based on powerpc/linux.git "fixes" with the top
commit a7d2475af7aed ("powerpc: Sort the selects under CONFIG_PPC").
The patches have been tested with stop1 (ESL=EC=1) as the
deepest-state entered into during CPU-Hotplug.
Gautham R. Shenoy (3):
powernv:smp: Add busy-wait loop as fall back for CPU-Hotplug
powernv:idle: Don't override default/deepest directly in kernel
powernv:Recover correct PACA on wakeup from a stop on P9 DD1
arch/powerpc/include/asm/paca.h | 5 ++++
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/idle_book3s.S | 43 +++++++++++++++++++++++++++-
arch/powerpc/platforms/powernv/idle.c | 49 ++++++++++++++++++++++++++------
arch/powerpc/platforms/powernv/powernv.h | 1 +
arch/powerpc/platforms/powernv/smp.c | 18 ++++++++++--
6 files changed, 105 insertions(+), 12 deletions(-)
--
1.9.4
From: "Gautham R. Shenoy" <[email protected]>
Currently during idle-init on power9, if we don't find suitable stop
states in the device tree that can be used as the
default_stop/deepest_stop, we set stop0 (ESL=1,EC=1) as the default
stop state psscr to be used by power9_idle and deepest stop state
which is used by CPU-Hotplug.
However, if the platform firmware has not configured or enabled a stop
state, the kernel should not make any assumptions and fallback to a
default choice.
If the kernel uses a stop state that is not configured by the platform
firmware, it may lead to further failures which should be avoided.
In this patch, we modify the init code to ensure that the kernel uses
only the stop states exposed by the firmware through the device
tree. When a suitable default stop state isn't found, we disable
ppc_md.power_save for power9. Similarly, when a suitable
deepest_stop_state is not found in the device tree exported by the
firmware, fall back to the default busy-wait loop in the CPU-Hotplug
code.
[Changelog written with inputs from [email protected]]
Signed-off-by: Gautham R. Shenoy <[email protected]>
---
arch/powerpc/platforms/powernv/idle.c | 27 ++++++++++++++++++---------
arch/powerpc/platforms/powernv/powernv.h | 1 +
arch/powerpc/platforms/powernv/smp.c | 8 +++++++-
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 4ee837e..9fde6e4 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -147,7 +147,6 @@ u32 pnv_get_supported_cpuidle_states(void)
}
EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
-
static void pnv_fastsleep_workaround_apply(void *info)
{
@@ -243,6 +242,7 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
*/
u64 pnv_default_stop_val;
u64 pnv_default_stop_mask;
+bool default_stop_found;
/*
* Used for ppc_md.power_save which needs a function with no parameters
@@ -264,6 +264,7 @@ static void power9_idle(void)
*/
u64 pnv_deepest_stop_psscr_val;
u64 pnv_deepest_stop_psscr_mask;
+bool deepest_stop_found;
/*
* Power ISA 3.0 idle initialization.
@@ -352,7 +353,6 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
u32 *residency_ns = NULL;
u64 max_residency_ns = 0;
int rc = 0, i;
- bool default_stop_found = false, deepest_stop_found = false;
psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), GFP_KERNEL);
psscr_mask = kcalloc(dt_idle_states, sizeof(*psscr_mask), GFP_KERNEL);
@@ -433,20 +433,22 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
}
if (!default_stop_found) {
- pnv_default_stop_val = PSSCR_HV_DEFAULT_VAL;
- pnv_default_stop_mask = PSSCR_HV_DEFAULT_MASK;
- pr_warn("Setting default stop psscr val=0x%016llx,mask=0x%016llx\n",
+ pr_warn("powernv:idle: Default stop not found. Disabling ppcmd.powersave\n");
+ } else {
+ pr_info("powernv:idle: Default stop: psscr = 0x%016llx,mask=0x%016llx\n",
pnv_default_stop_val, pnv_default_stop_mask);
}
if (!deepest_stop_found) {
- pnv_deepest_stop_psscr_val = PSSCR_HV_DEFAULT_VAL;
- pnv_deepest_stop_psscr_mask = PSSCR_HV_DEFAULT_MASK;
- pr_warn("Setting default stop psscr val=0x%016llx,mask=0x%016llx\n",
+ pr_warn("powernv:idle: Deepest stop not found. CPU-Hotplug is affected\n");
+ } else {
+ pr_info("powernv:idle: Deepest stop: psscr = 0x%016llx,mask=0x%016llx\n",
pnv_deepest_stop_psscr_val,
pnv_deepest_stop_psscr_mask);
}
+ pr_info("powernv:idle: RL value of first deep stop = 0x%llx\n",
+ pnv_first_deep_stop_state);
out:
kfree(psscr_val);
kfree(psscr_mask);
@@ -454,6 +456,12 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
return rc;
}
+bool pnv_check_deepest_stop(void)
+{
+ return deepest_stop_found;
+}
+EXPORT_SYMBOL_GPL(pnv_check_deepest_stop);
+
/*
* Probe device tree for supported idle states
*/
@@ -526,7 +534,8 @@ static int __init pnv_init_idle_states(void)
if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED)
ppc_md.power_save = power7_idle;
- else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
+ else if ((supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) &&
+ default_stop_found)
ppc_md.power_save = power9_idle;
out:
diff --git a/arch/powerpc/platforms/powernv/powernv.h b/arch/powerpc/platforms/powernv/powernv.h
index 6130522..9acd5eb 100644
--- a/arch/powerpc/platforms/powernv/powernv.h
+++ b/arch/powerpc/platforms/powernv/powernv.h
@@ -18,6 +18,7 @@ static inline void pnv_pci_shutdown(void) { }
#endif
extern u32 pnv_get_supported_cpuidle_states(void);
+bool pnv_check_deepest_stop(void);
extern u64 pnv_deepest_stop_psscr_val;
extern u64 pnv_deepest_stop_psscr_mask;
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 8d5b99e..3f66e6f 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -140,6 +140,7 @@ static void pnv_smp_cpu_kill_self(void)
unsigned int cpu;
unsigned long srr1, wmask;
u32 idle_states;
+ bool deepest_stop_found;
/* Standard hot unplug procedure */
local_irq_disable();
@@ -155,6 +156,7 @@ static void pnv_smp_cpu_kill_self(void)
wmask = SRR1_WAKEMASK_P8;
idle_states = pnv_get_supported_cpuidle_states();
+ deepest_stop_found = pnv_check_deepest_stop();
/* We don't want to take decrementer interrupts while we are offline,
* so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
@@ -184,7 +186,11 @@ static void pnv_smp_cpu_kill_self(void)
ppc64_runlatch_off();
- if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+ if (cpu_has_feature(CPU_FTR_ARCH_300) && deepest_stop_found) {
+ pr_info("CPU %u offlining with psscr = 0x%016llx\n",
+ cpu, pnv_deepest_stop_psscr_val);
+ pr_info("CPU%d down paca pir %016x pir %lx\n",
+ cpu, hard_smp_processor_id(), mfspr(SPRN_PIR));
srr1 = power9_idle_stop(pnv_deepest_stop_psscr_val,
pnv_deepest_stop_psscr_mask);
} else if (idle_states & OPAL_PM_WINKLE_ENABLED) {
--
1.9.4
From: "Gautham R. Shenoy" <[email protected]>
Currently, the powernv cpu-offline function assumes that platform idle
states such as stop on POWER9, winkle/sleep/nap on POWER8 are always
available. On POWER8, it picks nap as the default state if other deep
idle states like sleep/winkle are not available and enabled in the
platform.
On POWER9, nap is not available and all idle states are managed by
STOP instruction. The parameters to the idle state are passed through
processor stop status control register (PSSCR). Hence as such
executing STOP would take parameters from current PSSCR. We do not
want to make any assumptions in kernel on what STOP states and PSSCR
features are configured by the platform.
Ideally platform will configure a good set of stop states that can be
used in the kernel. We would like to start with a clean slate, if the
platform choose to not configure any state or there is an error in
platform firmware that lead to no stop states being configured or
allowed to be requested.
This patch adds a fallback method for CPU-Hotplug that is similar to
snooze loop at idle where the threads are left to spin at low priority
and hence reduce the cycles consumed.
This is a safe fallback mechanism in the case when no stop state would
be requested if the platform firmware did not configure them most
likely due to an error condition.
Requesting a stop state when the platform has not configured them or
enabled them would lead to further error conditions which could be
difficult to debug.
[Changelog written with inputs from [email protected]]
Signed-off-by: Gautham R. Shenoy <[email protected]>
---
arch/powerpc/platforms/powernv/smp.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index e39e6c4..8d5b99e 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -192,8 +192,16 @@ static void pnv_smp_cpu_kill_self(void)
} else if ((idle_states & OPAL_PM_SLEEP_ENABLED) ||
(idle_states & OPAL_PM_SLEEP_ENABLED_ER1)) {
srr1 = power7_sleep();
- } else {
+ } else if (idle_states & OPAL_PM_NAP_ENABLED) {
srr1 = power7_nap(1);
+ } else {
+ /* This is the fallback method. We emulate snooze */
+ while (!generic_check_cpu_restart(cpu)) {
+ HMT_low();
+ HMT_very_low();
+ }
+ srr1 = 0;
+ HMT_medium();
}
ppc64_runlatch_on();
--
1.9.4
From: "Gautham R. Shenoy" <[email protected]>
POWER9 platform can be configured to rebalance per-thread resources
within a core in order to improve SMT performance. Certain STOP
states can be configure to relinquish resources include some
hypervisor SPRs in order to enable SMT thread folding.
Due to relinquishing of per-thread resources under certain platform
configuration, certain SPR context could be lost within a core that
needs to be recovered. This state lose is due to reconfiguration of
SMT threads and not due to actual electrical power lose.
This patch implements a context recovery framework within threads of a
core, by provisioning space in paca_struct for saving every sibling
threads's paca pointers. Basically, we should be able to arrive at the
right paca pointer from any of the thread's existing paca pointer.
At bootup, during powernv idle-init, we save the paca address of every
CPU in each one its siblings paca_struct in the slot corresponding to
this CPU's index in the core.
On wakeup from a stop, the thread will determine its index in the core
from the lower 2 bits of the PIR register and recover its PACA pointer
by indexing into the correct slot in the provisioned space in the
current PACA.
[Changelog written with inputs from [email protected]]
Signed-off-by: Gautham R. Shenoy <[email protected]>
---
arch/powerpc/include/asm/paca.h | 5 ++++
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/idle_book3s.S | 43 ++++++++++++++++++++++++++++++++++-
arch/powerpc/platforms/powernv/idle.c | 22 ++++++++++++++++++
4 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 708c3e5..4405630 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -172,6 +172,11 @@ struct paca_struct {
u8 thread_mask;
/* Mask to denote subcore sibling threads */
u8 subcore_sibling_mask;
+ /*
+ * Pointer to an array which contains pointer
+ * to the sibling threads' paca.
+ */
+ struct paca_struct *thread_sibling_pacas[8];
#endif
#ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 4367e7d..6ec5016 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -727,6 +727,7 @@ int main(void)
OFFSET(PACA_THREAD_IDLE_STATE, paca_struct, thread_idle_state);
OFFSET(PACA_THREAD_MASK, paca_struct, thread_mask);
OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
+ OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
#endif
DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 9957287..5a90f2c 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -385,7 +385,48 @@ _GLOBAL(power9_idle_stop)
*/
_GLOBAL(pnv_restore_hyp_resource)
BEGIN_FTR_SECTION
- ld r2,PACATOC(r13);
+BEGIN_FTR_SECTION_NESTED(70)
+/* Save our LR in r17 */
+ mflr r17
+/*
+ * On entering certain stop states, the thread might relinquish its
+ * per-thread resources due to some reconfiguration for improved SMT
+ * performance. This would result in certain SPR context such as
+ * HSPRG0 (which contains the paca pointer) to be lost within the core.
+ *
+ * Fortunately, the PIR is invariant to thread reconfiguration. Since
+ * this thread's paca pointer is recorded in all its sibling's
+ * paca, we can correctly recover this thread's paca pointer if we
+ * know the index of this thread in the core.
+ * This index can be obtained from the lower two bits of the PIR.
+ *
+ * i.e, thread's position in the core = PIR[62:63].
+ * If this value is i, then this thread's paca is
+ * paca->thread_sibling_pacas[i].
+ */
+ mfspr r4, SPRN_PIR
+ andi. r4, r4, 0x3
+/*
+ * Since each entry in thread_sibling_pacas is 8 bytes
+ * we need to left-shift by 3 bits. Thus r4 = i * 8
+ */
+ sldi r4, r4, 3
+/* Get &paca->thread_sibling_pacas[0] in r5 */
+ addi r5, r13, PACA_SIBLING_PACA_PTRS
+/* Load paca->thread_sibling_pacas[i] into r3 */
+ ldx r3, r4, r5
+/* Move our paca pointer to r13 */
+ mr r13, r3
+/* Correctly set up our PACA */
+ ld r2, PACATOC(r13)
+ ld r1, PACAR1(r13)
+ bl setup_paca
+/* Now we are all set! Set our LR and TOC */
+ mtlr r17
+ ld r2, PACATOC(r13)
+FTR_SECTION_ELSE_NESTED(70)
+ ld r2, PACATOC(r13)
+ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_POWER9_DD1, 70)
/*
* POWER ISA 3. Use PSSCR to determine if we
* are waking up from deep idle state
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 9fde6e4..87311c2 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -532,6 +532,28 @@ static int __init pnv_init_idle_states(void)
pnv_alloc_idle_core_states();
+ /*
+ * For each CPU, record its PACA address in each of it's
+ * sibling thread's PACA at the slot corresponding to this
+ * CPU's index in the core.
+ */
+ if (cpu_has_feature(CPU_FTR_POWER9_DD1)) {
+ int cpu;
+
+ pr_info("powernv: idle: Saving PACA pointers of all CPUs in their thread sibling PACA\n");
+ for_each_possible_cpu(cpu) {
+ int base_cpu = cpu_first_thread_sibling(cpu);
+ int idx = cpu_thread_in_core(cpu);
+ int i;
+
+ for (i = 0; i < threads_per_core; i++) {
+ int j = base_cpu + i;
+
+ paca[j].thread_sibling_pacas[idx] = &paca[cpu];
+ }
+ }
+ }
+
if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED)
ppc_md.power_save = power7_idle;
else if ((supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) &&
--
1.9.4
On Mon, 13 Mar 2017 11:31:26 +0530
"Gautham R. Shenoy" <[email protected]> wrote:
> [Changelog written with inputs from [email protected]]
> Signed-off-by: Gautham R. Shenoy <[email protected]>
Reviewed-by: Nicholas Piggin <[email protected]>
> ---
> arch/powerpc/platforms/powernv/smp.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> index e39e6c4..8d5b99e 100644
> --- a/arch/powerpc/platforms/powernv/smp.c
> +++ b/arch/powerpc/platforms/powernv/smp.c
> @@ -192,8 +192,16 @@ static void pnv_smp_cpu_kill_self(void)
> } else if ((idle_states & OPAL_PM_SLEEP_ENABLED) ||
> (idle_states & OPAL_PM_SLEEP_ENABLED_ER1)) {
> srr1 = power7_sleep();
> - } else {
> + } else if (idle_states & OPAL_PM_NAP_ENABLED) {
> srr1 = power7_nap(1);
> + } else {
> + /* This is the fallback method. We emulate snooze */
> + while (!generic_check_cpu_restart(cpu)) {
> + HMT_low();
> + HMT_very_low();
> + }
> + srr1 = 0;
> + HMT_medium();
> }
>
> ppc64_runlatch_on();
On Mon, 13 Mar 2017 11:31:27 +0530
"Gautham R. Shenoy" <[email protected]> wrote:
> From: "Gautham R. Shenoy" <[email protected]>
>
> Currently during idle-init on power9, if we don't find suitable stop
> states in the device tree that can be used as the
> default_stop/deepest_stop, we set stop0 (ESL=1,EC=1) as the default
> stop state psscr to be used by power9_idle and deepest stop state
> which is used by CPU-Hotplug.
>
> However, if the platform firmware has not configured or enabled a stop
> state, the kernel should not make any assumptions and fallback to a
> default choice.
>
> If the kernel uses a stop state that is not configured by the platform
> firmware, it may lead to further failures which should be avoided.
>
> In this patch, we modify the init code to ensure that the kernel uses
> only the stop states exposed by the firmware through the device
> tree. When a suitable default stop state isn't found, we disable
> ppc_md.power_save for power9. Similarly, when a suitable
> deepest_stop_state is not found in the device tree exported by the
> firmware, fall back to the default busy-wait loop in the CPU-Hotplug
> code.
Seems reasonable. I have a few comments that you may consider. Nothing
too major.
Btw., it would be nice to move this hotplug idling selection code to
idle.c. Have the hotplug just ask to enter the best available idle mode
and that's it. I'm not asking you to do that for this series, but perhaps
consider it for the future.
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 4ee837e..9fde6e4 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -147,7 +147,6 @@ u32 pnv_get_supported_cpuidle_states(void)
> }
> EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
>
> -
> static void pnv_fastsleep_workaround_apply(void *info)
>
> {
> @@ -243,6 +242,7 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
> */
> u64 pnv_default_stop_val;
> u64 pnv_default_stop_mask;
> +bool default_stop_found;
>
> /*
> * Used for ppc_md.power_save which needs a function with no parameters
> @@ -264,6 +264,7 @@ static void power9_idle(void)
> */
> u64 pnv_deepest_stop_psscr_val;
> u64 pnv_deepest_stop_psscr_mask;
> +bool deepest_stop_found;
>
> /*
> * Power ISA 3.0 idle initialization.
If the hotplug idle code was in idle.c, then all this deepest/default stop
logic and register settings would be static to idle.c, which would be nice.
If you have a function to check if deepest stop is found, then you don't need
a non-static variable here (or for default_stop_found AFAIKS).
> @@ -352,7 +353,6 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
> u32 *residency_ns = NULL;
> u64 max_residency_ns = 0;
> int rc = 0, i;
> - bool default_stop_found = false, deepest_stop_found = false;
>
> psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), GFP_KERNEL);
> psscr_mask = kcalloc(dt_idle_states, sizeof(*psscr_mask), GFP_KERNEL);
> @@ -433,20 +433,22 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
> }
>
> if (!default_stop_found) {
> - pnv_default_stop_val = PSSCR_HV_DEFAULT_VAL;
> - pnv_default_stop_mask = PSSCR_HV_DEFAULT_MASK;
> - pr_warn("Setting default stop psscr val=0x%016llx,mask=0x%016llx\n",
> + pr_warn("powernv:idle: Default stop not found. Disabling ppcmd.powersave\n");
> + } else {
> + pr_info("powernv:idle: Default stop: psscr = 0x%016llx,mask=0x%016llx\n",
> pnv_default_stop_val, pnv_default_stop_mask);
> }
>
> if (!deepest_stop_found) {
> - pnv_deepest_stop_psscr_val = PSSCR_HV_DEFAULT_VAL;
> - pnv_deepest_stop_psscr_mask = PSSCR_HV_DEFAULT_MASK;
> - pr_warn("Setting default stop psscr val=0x%016llx,mask=0x%016llx\n",
> + pr_warn("powernv:idle: Deepest stop not found. CPU-Hotplug is affected\n");
I guess these warnings are meant for developers, but it might be nice
to make them slightly more meaningful? Prefix of choice seems to be
"cpuidle-powernv:"
Then you could have "no suitable stop state provided by firmware. Disabling
idle power saving" and "no suitable stop state provided by firmware. Offlined
CPUs will busy-wait", perhaps?
Just a suggestion.
> + } else {
> + pr_info("powernv:idle: Deepest stop: psscr = 0x%016llx,mask=0x%016llx\n",
> pnv_deepest_stop_psscr_val,
> pnv_deepest_stop_psscr_mask);
> }
>
> + pr_info("powernv:idle: RL value of first deep stop = 0x%llx\n",
> + pnv_first_deep_stop_state);
cpuidle-powernv: prefix for these too?
> out:
> kfree(psscr_val);
> kfree(psscr_mask);
> @@ -454,6 +456,12 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
> return rc;
> }
>
> +bool pnv_check_deepest_stop(void)
> +{
> + return deepest_stop_found;
> +}
> +EXPORT_SYMBOL_GPL(pnv_check_deepest_stop);
Does this need to be exported? AFAIKS it's not used in a module.
> +
> /*
> * Probe device tree for supported idle states
> */
> @@ -526,7 +534,8 @@ static int __init pnv_init_idle_states(void)
>
> if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED)
> ppc_md.power_save = power7_idle;
> - else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
> + else if ((supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) &&
> + default_stop_found)
> ppc_md.power_save = power9_idle;
>
> out:
Is this the only use of default_stop_found? & OPAL_PM_STOP_INST_FAST
should always be the same as default_stop_found value, no? In that case
can you just remove the OPAL_PM_STOP_INST_FAST test here? (I like the
boolean and prefer the default idle state selection logic to stay in the
idle init above where you have it commented).
> diff --git a/arch/powerpc/platforms/powernv/powernv.h b/arch/powerpc/platforms/powernv/powernv.h
> index 6130522..9acd5eb 100644
> --- a/arch/powerpc/platforms/powernv/powernv.h
> +++ b/arch/powerpc/platforms/powernv/powernv.h
> @@ -18,6 +18,7 @@ static inline void pnv_pci_shutdown(void) { }
> #endif
>
> extern u32 pnv_get_supported_cpuidle_states(void);
> +bool pnv_check_deepest_stop(void);
> extern u64 pnv_deepest_stop_psscr_val;
> extern u64 pnv_deepest_stop_psscr_mask;
>
> diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> index 8d5b99e..3f66e6f 100644
> --- a/arch/powerpc/platforms/powernv/smp.c
> +++ b/arch/powerpc/platforms/powernv/smp.c
> @@ -140,6 +140,7 @@ static void pnv_smp_cpu_kill_self(void)
> unsigned int cpu;
> unsigned long srr1, wmask;
> u32 idle_states;
> + bool deepest_stop_found;
>
> /* Standard hot unplug procedure */
> local_irq_disable();
> @@ -155,6 +156,7 @@ static void pnv_smp_cpu_kill_self(void)
> wmask = SRR1_WAKEMASK_P8;
>
> idle_states = pnv_get_supported_cpuidle_states();
> + deepest_stop_found = pnv_check_deepest_stop();
>
> /* We don't want to take decrementer interrupts while we are offline,
> * so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
> @@ -184,7 +186,11 @@ static void pnv_smp_cpu_kill_self(void)
>
> ppc64_runlatch_off();
>
> - if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> + if (cpu_has_feature(CPU_FTR_ARCH_300) && deepest_stop_found) {
> + pr_info("CPU %u offlining with psscr = 0x%016llx\n",
> + cpu, pnv_deepest_stop_psscr_val);
> + pr_info("CPU%d down paca pir %016x pir %lx\n",
> + cpu, hard_smp_processor_id(), mfspr(SPRN_PIR));
How much log info is appropriate here?
> srr1 = power9_idle_stop(pnv_deepest_stop_psscr_val,
> pnv_deepest_stop_psscr_mask);
> } else if (idle_states & OPAL_PM_WINKLE_ENABLED) {
On Mon, 13 Mar 2017 11:31:28 +0530
"Gautham R. Shenoy" <[email protected]> wrote:
> From: "Gautham R. Shenoy" <[email protected]>
>
> POWER9 platform can be configured to rebalance per-thread resources
> within a core in order to improve SMT performance. Certain STOP
> states can be configure to relinquish resources include some
> hypervisor SPRs in order to enable SMT thread folding.
>
> Due to relinquishing of per-thread resources under certain platform
> configuration, certain SPR context could be lost within a core that
> needs to be recovered. This state lose is due to reconfiguration of
> SMT threads and not due to actual electrical power lose.
Interesting. This will clash slightly with my patches, but that's
no big deal.
> This patch implements a context recovery framework within threads of a
> core, by provisioning space in paca_struct for saving every sibling
> threads's paca pointers. Basically, we should be able to arrive at the
> right paca pointer from any of the thread's existing paca pointer.
>
> At bootup, during powernv idle-init, we save the paca address of every
> CPU in each one its siblings paca_struct in the slot corresponding to
> this CPU's index in the core.
>
> On wakeup from a stop, the thread will determine its index in the core
> from the lower 2 bits of the PIR register and recover its PACA pointer
> by indexing into the correct slot in the provisioned space in the
> current PACA.
>
> [Changelog written with inputs from [email protected]]
>
> Signed-off-by: Gautham R. Shenoy <[email protected]>
> ---
> arch/powerpc/include/asm/paca.h | 5 ++++
> arch/powerpc/kernel/asm-offsets.c | 1 +
> arch/powerpc/kernel/idle_book3s.S | 43 ++++++++++++++++++++++++++++++++++-
> arch/powerpc/platforms/powernv/idle.c | 22 ++++++++++++++++++
> 4 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index 708c3e5..4405630 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -172,6 +172,11 @@ struct paca_struct {
> u8 thread_mask;
> /* Mask to denote subcore sibling threads */
> u8 subcore_sibling_mask;
> + /*
> + * Pointer to an array which contains pointer
> + * to the sibling threads' paca.
> + */
> + struct paca_struct *thread_sibling_pacas[8];
> #endif
>
> #ifdef CONFIG_PPC_BOOK3S_64
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 4367e7d..6ec5016 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -727,6 +727,7 @@ int main(void)
> OFFSET(PACA_THREAD_IDLE_STATE, paca_struct, thread_idle_state);
> OFFSET(PACA_THREAD_MASK, paca_struct, thread_mask);
> OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
> + OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
> #endif
>
> DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index 9957287..5a90f2c 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -385,7 +385,48 @@ _GLOBAL(power9_idle_stop)
> */
> _GLOBAL(pnv_restore_hyp_resource)
> BEGIN_FTR_SECTION
> - ld r2,PACATOC(r13);
> +BEGIN_FTR_SECTION_NESTED(70)
> +/* Save our LR in r17 */
> + mflr r17
> +/*
> + * On entering certain stop states, the thread might relinquish its
> + * per-thread resources due to some reconfiguration for improved SMT
> + * performance. This would result in certain SPR context such as
> + * HSPRG0 (which contains the paca pointer) to be lost within the core.
> + *
> + * Fortunately, the PIR is invariant to thread reconfiguration. Since
> + * this thread's paca pointer is recorded in all its sibling's
> + * paca, we can correctly recover this thread's paca pointer if we
> + * know the index of this thread in the core.
> + * This index can be obtained from the lower two bits of the PIR.
> + *
> + * i.e, thread's position in the core = PIR[62:63].
> + * If this value is i, then this thread's paca is
> + * paca->thread_sibling_pacas[i].
> + */
> + mfspr r4, SPRN_PIR
> + andi. r4, r4, 0x3
> +/*
> + * Since each entry in thread_sibling_pacas is 8 bytes
> + * we need to left-shift by 3 bits. Thus r4 = i * 8
> + */
> + sldi r4, r4, 3
> +/* Get &paca->thread_sibling_pacas[0] in r5 */
> + addi r5, r13, PACA_SIBLING_PACA_PTRS
I don't actually understand how this works. We have in r13 a paca
pointer already, and it must be a pointer to one of the threads in
this core, but it may not be a pointer to this thread's paca?
Can you explain how that works a bit more? Where does that r13 come
from?
The exception code before we reach here already saves registers into
the paca save area (PACA_EXGEN), so this is going to go bad if we
are saving into another thread's paca. I have a patch that makes the
0x100 vector use a a new save area, which would make it mostly work
(ignoring rare system reset interrupts).
And if we have any valid paca there, we should be able to load kbase
from it, so that should work with relocatable kernel.
But that all hinges on what exactly this r13 is...
Thanks,
Nick
Hi Nick,
Thanks for reviewing the patch.
On Wed, Mar 15, 2017 at 12:05:43AM +1000, Nicholas Piggin wrote:
> On Mon, 13 Mar 2017 11:31:27 +0530
> "Gautham R. Shenoy" <[email protected]> wrote:
>
> > From: "Gautham R. Shenoy" <[email protected]>
> >
> > Currently during idle-init on power9, if we don't find suitable stop
> > states in the device tree that can be used as the
> > default_stop/deepest_stop, we set stop0 (ESL=1,EC=1) as the default
> > stop state psscr to be used by power9_idle and deepest stop state
> > which is used by CPU-Hotplug.
> >
> > However, if the platform firmware has not configured or enabled a stop
> > state, the kernel should not make any assumptions and fallback to a
> > default choice.
> >
> > If the kernel uses a stop state that is not configured by the platform
> > firmware, it may lead to further failures which should be avoided.
> >
> > In this patch, we modify the init code to ensure that the kernel uses
> > only the stop states exposed by the firmware through the device
> > tree. When a suitable default stop state isn't found, we disable
> > ppc_md.power_save for power9. Similarly, when a suitable
> > deepest_stop_state is not found in the device tree exported by the
> > firmware, fall back to the default busy-wait loop in the CPU-Hotplug
> > code.
>
> Seems reasonable. I have a few comments that you may consider. Nothing
> too major.
>
> Btw., it would be nice to move this hotplug idling selection code to
> idle.c. Have the hotplug just ask to enter the best available idle mode
> and that's it. I'm not asking you to do that for this series, but perhaps
> consider it for the future.
That's not a bad idea. I will do it in the respin of the patchset.
>
>
> > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> > index 4ee837e..9fde6e4 100644
> > --- a/arch/powerpc/platforms/powernv/idle.c
> > +++ b/arch/powerpc/platforms/powernv/idle.c
> > @@ -147,7 +147,6 @@ u32 pnv_get_supported_cpuidle_states(void)
> > }
> > EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
> >
> > -
> > static void pnv_fastsleep_workaround_apply(void *info)
> >
> > {
> > @@ -243,6 +242,7 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
> > */
> > u64 pnv_default_stop_val;
> > u64 pnv_default_stop_mask;
> > +bool default_stop_found;
> >
> > /*
> > * Used for ppc_md.power_save which needs a function with no parameters
> > @@ -264,6 +264,7 @@ static void power9_idle(void)
> > */
> > u64 pnv_deepest_stop_psscr_val;
> > u64 pnv_deepest_stop_psscr_mask;
> > +bool deepest_stop_found;
> >
> > /*
> > * Power ISA 3.0 idle initialization.
>
> If the hotplug idle code was in idle.c, then all this deepest/default stop
> logic and register settings would be static to idle.c, which would be nice.
>
> If you have a function to check if deepest stop is found, then you don't need
> a non-static variable here (or for default_stop_found AFAIKS).
Sure!
>
>
> > @@ -352,7 +353,6 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
> > u32 *residency_ns = NULL;
> > u64 max_residency_ns = 0;
> > int rc = 0, i;
> > - bool default_stop_found = false, deepest_stop_found = false;
> >
> > psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), GFP_KERNEL);
> > psscr_mask = kcalloc(dt_idle_states, sizeof(*psscr_mask), GFP_KERNEL);
> > @@ -433,20 +433,22 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
> > }
> >
> > if (!default_stop_found) {
> > - pnv_default_stop_val = PSSCR_HV_DEFAULT_VAL;
> > - pnv_default_stop_mask = PSSCR_HV_DEFAULT_MASK;
> > - pr_warn("Setting default stop psscr val=0x%016llx,mask=0x%016llx\n",
> > + pr_warn("powernv:idle: Default stop not found. Disabling ppcmd.powersave\n");
> > + } else {
> > + pr_info("powernv:idle: Default stop: psscr = 0x%016llx,mask=0x%016llx\n",
> > pnv_default_stop_val, pnv_default_stop_mask);
> > }
> >
> > if (!deepest_stop_found) {
> > - pnv_deepest_stop_psscr_val = PSSCR_HV_DEFAULT_VAL;
> > - pnv_deepest_stop_psscr_mask = PSSCR_HV_DEFAULT_MASK;
> > - pr_warn("Setting default stop psscr val=0x%016llx,mask=0x%016llx\n",
> > + pr_warn("powernv:idle: Deepest stop not found. CPU-Hotplug is affected\n");
>
> I guess these warnings are meant for developers, but it might be nice
> to make them slightly more meaningful? Prefix of choice seems to be
> "cpuidle-powernv:"
>
> Then you could have "no suitable stop state provided by firmware. Disabling
> idle power saving" and "no suitable stop state provided by firmware. Offlined
> CPUs will busy-wait", perhaps?
How about
pr_warn("cpuidle-powernv: No suitable stop for CPU-Hotplug. Offlined CPUs will busy wait\n");
>
> Just a suggestion.
>
> > + } else {
> > + pr_info("powernv:idle: Deepest stop: psscr = 0x%016llx,mask=0x%016llx\n",
> > pnv_deepest_stop_psscr_val,
> > pnv_deepest_stop_psscr_mask);
> > }
> >
> > + pr_info("powernv:idle: RL value of first deep stop = 0x%llx\n",
> > + pnv_first_deep_stop_state);
>
> cpuidle-powernv: prefix for these too?
Will fix.
>
> > out:
> > kfree(psscr_val);
> > kfree(psscr_mask);
> > @@ -454,6 +456,12 @@ static int __init pnv_power9_idle_init(struct device_node *np, u32 *flags,
> > return rc;
> > }
> >
> > +bool pnv_check_deepest_stop(void)
> > +{
> > + return deepest_stop_found;
> > +}
> > +EXPORT_SYMBOL_GPL(pnv_check_deepest_stop);
>
> Does this need to be exported? AFAIKS it's not used in a module.
No, it is not used in a module. Will get rid of it.
>
> > +
> > /*
> > * Probe device tree for supported idle states
> > */
> > @@ -526,7 +534,8 @@ static int __init pnv_init_idle_states(void)
> >
> > if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED)
> > ppc_md.power_save = power7_idle;
> > - else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
> > + else if ((supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) &&
> > + default_stop_found)
> > ppc_md.power_save = power9_idle;
> >
> > out:
>
> Is this the only use of default_stop_found? & OPAL_PM_STOP_INST_FAST
> should always be the same as default_stop_found value, no? In that case
> can you just remove the OPAL_PM_STOP_INST_FAST test here? (I like the
> boolean and prefer the default idle state selection logic to stay in the
> idle init above where you have it commented).
Yup. You are right, the check is redundant. We only consider
STOP_INST_FAST states for default in power9_idle_init(). Will fix this
and move initialization of ppc_md.power_save to the init function.
>
>
> > diff --git a/arch/powerpc/platforms/powernv/powernv.h b/arch/powerpc/platforms/powernv/powernv.h
> > index 6130522..9acd5eb 100644
> > --- a/arch/powerpc/platforms/powernv/powernv.h
> > +++ b/arch/powerpc/platforms/powernv/powernv.h
> > @@ -18,6 +18,7 @@ static inline void pnv_pci_shutdown(void) { }
> > #endif
> >
> > extern u32 pnv_get_supported_cpuidle_states(void);
> > +bool pnv_check_deepest_stop(void);
> > extern u64 pnv_deepest_stop_psscr_val;
> > extern u64 pnv_deepest_stop_psscr_mask;
> >
> > diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> > index 8d5b99e..3f66e6f 100644
> > --- a/arch/powerpc/platforms/powernv/smp.c
> > +++ b/arch/powerpc/platforms/powernv/smp.c
> > @@ -140,6 +140,7 @@ static void pnv_smp_cpu_kill_self(void)
> > unsigned int cpu;
> > unsigned long srr1, wmask;
> > u32 idle_states;
> > + bool deepest_stop_found;
> >
> > /* Standard hot unplug procedure */
> > local_irq_disable();
> > @@ -155,6 +156,7 @@ static void pnv_smp_cpu_kill_self(void)
> > wmask = SRR1_WAKEMASK_P8;
> >
> > idle_states = pnv_get_supported_cpuidle_states();
> > + deepest_stop_found = pnv_check_deepest_stop();
> >
> > /* We don't want to take decrementer interrupts while we are offline,
> > * so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
> > @@ -184,7 +186,11 @@ static void pnv_smp_cpu_kill_self(void)
> >
> > ppc64_runlatch_off();
> >
> > - if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> > + if (cpu_has_feature(CPU_FTR_ARCH_300) && deepest_stop_found) {
> > + pr_info("CPU %u offlining with psscr = 0x%016llx\n",
> > + cpu, pnv_deepest_stop_psscr_val);
> > + pr_info("CPU%d down paca pir %016x pir %lx\n",
> > + cpu, hard_smp_processor_id(), mfspr(SPRN_PIR));
>
> How much log info is appropriate here?
This should have been pr_debug. I will clean up this part.
>
> > srr1 = power9_idle_stop(pnv_deepest_stop_psscr_val,
> > pnv_deepest_stop_psscr_mask);
> > } else if (idle_states & OPAL_PM_WINKLE_ENABLED) {
>
--
Thanks and Regards
gautham.