| Message ID | 1499272696-28751-6-git-send-email-ego@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, 5 Jul 2017 22:08:16 +0530 "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > Currently, we use the opal call opal_slw_set_reg() to inform the that > the Sleep-Winkle Engine (SLW) to restore the contents of some of the > Hypervisor state on wakeup from deep idle states that lose full > hypervisor context (characterized by the flag > OPAL_PM_LOSE_FULL_CONTEXT). > > However, the current code has a bug in that if opal_slw_set_reg() > fails, we don't disable the use of these deep states (winkle on > POWER8, stop4 onwards on POWER9). > > This patch fixes this bug by ensuring that if the the sleep winkle > engine is unable to restore the hypervisor states in > pnv_save_sprs_for_deep_states(), then we mark as invalid the states > which lose full context. > > As a side-effect, since supported_cpuidle_states in > pnv_probe_idle_states() consists of flags of only the valid states, > this patch will ensure that no other subsystem in the kernel can use > the states which lose full context on stop-api failures. Looks good. Is there something minimal we can do for stable here? Aside question, do we need to restore LPCR at all with the SLW engine? It gets set up again when by the idle wakeup code. And does POWER9 really need MSR and PSSCR restored by SLW? (going a bit off topic here, I'm just curious) Thanks, Nick
On Fri, Jul 07, 2017 at 01:29:16AM +1000, Nicholas Piggin wrote: > On Wed, 5 Jul 2017 22:08:16 +0530 > "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote: > > > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > > Currently, we use the opal call opal_slw_set_reg() to inform the that > > the Sleep-Winkle Engine (SLW) to restore the contents of some of the > > Hypervisor state on wakeup from deep idle states that lose full > > hypervisor context (characterized by the flag > > OPAL_PM_LOSE_FULL_CONTEXT). > > > > However, the current code has a bug in that if opal_slw_set_reg() > > fails, we don't disable the use of these deep states (winkle on > > POWER8, stop4 onwards on POWER9). > > > > This patch fixes this bug by ensuring that if the the sleep winkle > > engine is unable to restore the hypervisor states in > > pnv_save_sprs_for_deep_states(), then we mark as invalid the states > > which lose full context. > > > > As a side-effect, since supported_cpuidle_states in > > pnv_probe_idle_states() consists of flags of only the valid states, > > this patch will ensure that no other subsystem in the kernel can use > > the states which lose full context on stop-api failures. > > Looks good. Is there something minimal we can do for stable here? > > Aside question, do we need to restore LPCR at all with the SLW engine? > It gets set up again when by the idle wakeup code. > > And does POWER9 really need MSR and PSSCR restored by SLW? (going a bit > off topic here, I'm just curious) MSR is needed to be restored so that we wakeup with the right endianness and with the IR,DR disabled. PSSCR is set to a value so that in case of a special wakeup for a deep-stop, the SLW can program the core to go back to the stop level provided by the PSSCR value via the stop-api. > > Thanks, > Nick > -- Thanks and Regards gautham.
On Fri, 7 Jul 2017 23:07:10 +0530 Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote: > On Fri, Jul 07, 2017 at 01:29:16AM +1000, Nicholas Piggin wrote: > > On Wed, 5 Jul 2017 22:08:16 +0530 > > "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote: > > > > > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > > > > Currently, we use the opal call opal_slw_set_reg() to inform the that > > > the Sleep-Winkle Engine (SLW) to restore the contents of some of the > > > Hypervisor state on wakeup from deep idle states that lose full > > > hypervisor context (characterized by the flag > > > OPAL_PM_LOSE_FULL_CONTEXT). > > > > > > However, the current code has a bug in that if opal_slw_set_reg() > > > fails, we don't disable the use of these deep states (winkle on > > > POWER8, stop4 onwards on POWER9). > > > > > > This patch fixes this bug by ensuring that if the the sleep winkle > > > engine is unable to restore the hypervisor states in > > > pnv_save_sprs_for_deep_states(), then we mark as invalid the states > > > which lose full context. > > > > > > As a side-effect, since supported_cpuidle_states in > > > pnv_probe_idle_states() consists of flags of only the valid states, > > > this patch will ensure that no other subsystem in the kernel can use > > > the states which lose full context on stop-api failures. > > > > Looks good. Is there something minimal we can do for stable here? > > > > Aside question, do we need to restore LPCR at all with the SLW engine? > > It gets set up again when by the idle wakeup code. > > > > > > And does POWER9 really need MSR and PSSCR restored by SLW? (going a bit > > off topic here, I'm just curious) > > MSR is needed to be restored so that we wakeup with the right > endianness and with the IR,DR disabled. And POWER8 does not require this? > PSSCR is set to a value so that in case of a special wakeup for a > deep-stop, the SLW can program the core to go back to the stop level > provided by the PSSCR value via the stop-api. It always restores to deepest stop? Is there any way to restore to the achieved stop level? Maybe there is no usefulness for that. Thanks, Nick
On Sat, Jul 08, 2017 at 07:05:26PM +1000, Nicholas Piggin wrote: > On Fri, 7 Jul 2017 23:07:10 +0530 > Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote: > > > On Fri, Jul 07, 2017 at 01:29:16AM +1000, Nicholas Piggin wrote: > > > On Wed, 5 Jul 2017 22:08:16 +0530 > > > "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote: > > > > > > > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > > > > > > Currently, we use the opal call opal_slw_set_reg() to inform the that > > > > the Sleep-Winkle Engine (SLW) to restore the contents of some of the > > > > Hypervisor state on wakeup from deep idle states that lose full > > > > hypervisor context (characterized by the flag > > > > OPAL_PM_LOSE_FULL_CONTEXT). > > > > > > > > However, the current code has a bug in that if opal_slw_set_reg() > > > > fails, we don't disable the use of these deep states (winkle on > > > > POWER8, stop4 onwards on POWER9). > > > > > > > > This patch fixes this bug by ensuring that if the the sleep winkle > > > > engine is unable to restore the hypervisor states in > > > > pnv_save_sprs_for_deep_states(), then we mark as invalid the states > > > > which lose full context. > > > > > > > > As a side-effect, since supported_cpuidle_states in > > > > pnv_probe_idle_states() consists of flags of only the valid states, > > > > this patch will ensure that no other subsystem in the kernel can use > > > > the states which lose full context on stop-api failures. > > > > > > Looks good. Is there something minimal we can do for stable here? > > > > > > Aside question, do we need to restore LPCR at all with the SLW engine? > > > It gets set up again when by the idle wakeup code. > > > > > > > > > > And does POWER9 really need MSR and PSSCR restored by SLW? (going a bit > > > off topic here, I'm just curious) > > > > MSR is needed to be restored so that we wakeup with the right > > endianness and with the IR,DR disabled. > > And POWER8 does not require this? > > > PSSCR is set to a value so that in case of a special wakeup for a > > deep-stop, the SLW can program the core to go back to the stop level > > provided by the PSSCR value via the stop-api. > > It always restores to deepest stop? Is there any way to restore to the > achieved stop level? Maybe there is no usefulness for that. That would have been ideal. But there's no way to achieve that at the moment. The alternative is to have call the stop-api with psscr set to the desired stop level before every stop entry. This is will consume additional cycles which is what we are trying to avoid. So we are currently setting the psscr value to deepest stop via stop api as a compromise, because then on wakeup, we end up restoring more than what would typically be required, but that's still ok since we would be erring on the side of caution. Programming the PSSCR to any other value might have safety concerns. Eg: Suppose a core which was in stop11 got woken up by a special wakeup and if the psscr programmed via stop API was stop4 then the firmware will put the core in stop4. Now, since stop4 doesn't lose timebase and stop11 does, in the aforementioned case TB would have gone out of sync in the duration that the core was in stop11. Thus, when the core wakes up in the kernel in stop4, the kernel won't resync the TB which is a problematic. > > Thanks, > Nick >
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c index 254a0db8..8d07ce6 100644 --- a/arch/powerpc/platforms/powernv/idle.c +++ b/arch/powerpc/platforms/powernv/idle.c @@ -217,9 +217,6 @@ static void pnv_alloc_idle_core_states(void) } update_subcore_sibling_mask(); - - if (supported_cpuidle_states & OPAL_PM_LOSE_FULL_CONTEXT) - pnv_save_sprs_for_deep_states(); } u32 pnv_get_supported_cpuidle_states(void) @@ -518,6 +515,57 @@ static void __init pnv_power9_idle_init(void) u64 max_residency_ns = 0; int i; int dt_idle_states = pnv_idle.nr_states; + bool save_sprs_for_deep_stop = false; + bool disable_lose_full_context = false; + u64 psscr_rl, residency_ns, psscr_val, psscr_mask; + u32 flags; + + /* + * pnv_deepest_stop_{val,mask} should be set to values + * corresponding to the deepest stop state. + */ + for (i = 0; i < dt_idle_states; i++) { + psscr_val = pnv_idle.states[i].ctrl_reg_val; + psscr_mask = pnv_idle.states[i].ctrl_reg_mask; + psscr_rl = psscr_val & PSSCR_RL_MASK; + flags = pnv_idle.states[i].flags; + residency_ns = pnv_idle.states[i].residency_ns; + + if (flags & OPAL_PM_LOSE_FULL_CONTEXT) + save_sprs_for_deep_stop = true; + + if (max_residency_ns < residency_ns) { + max_residency_ns = residency_ns; + pnv_deepest_stop_psscr_val = psscr_val; + pnv_deepest_stop_psscr_mask = psscr_mask; + deepest_stop_found = true; + } + } + + /* + * pnv_save_sprs_for_deep_states() expects + * pnv_deepest_stop_psscr_val to be initialized. + */ + if (save_sprs_for_deep_stop) { + int rc; + + rc = pnv_save_sprs_for_deep_states(); + + /* + * If the Sleep-Winkle Engine is unable to restore the + * critical SPRs on wakeup from some of the deep stop + * states that lose full context, then we mark such + * deep states as invalid and recompute the + * pnv_deepest_stop_psscr_val/mask from among the + * valid states. + */ + if (unlikely(rc)) { + pr_warn("cpuidle-powernv:Disabling full-context loss states.SLW unable to restore SPRs\n"); + disable_lose_full_context = true; + max_residency_ns = 0; + deepest_stop_found = false; + } + } /* * Set pnv_first_deep_stop_state, pnv_deepest_stop_psscr_{val,mask}, @@ -526,16 +574,20 @@ static void __init pnv_power9_idle_init(void) * pnv_first_deep_stop_state should be set to the first stop * level to cause hypervisor state loss. * - * pnv_deepest_stop_{val,mask} should be set to values corresponding to - * the deepest stop state. * * pnv_default_stop_{val,mask} should be set to values corresponding to * the shallowest (OPAL_PM_STOP_INST_FAST) loss-less stop state. */ pnv_first_deep_stop_state = MAX_STOP_STATE; for (i = 0; i < dt_idle_states; i++) { - u64 psscr_rl, residency_ns, psscr_val, psscr_mask; - u32 flags; + flags = pnv_idle.states[i].flags; + + if ((flags & OPAL_PM_LOSE_FULL_CONTEXT) && + disable_lose_full_context) { + pnv_idle.states[i].valid = false; + pr_warn("cpuidle-powernv: Disabling full-context loss state :%s\n", + pnv_idle.states[i].name); + } if (!pnv_idle.states[i].valid) continue; @@ -543,15 +595,14 @@ static void __init pnv_power9_idle_init(void) psscr_val = pnv_idle.states[i].ctrl_reg_val; psscr_mask = pnv_idle.states[i].ctrl_reg_mask; psscr_rl = psscr_val & PSSCR_RL_MASK; - flags = pnv_idle.states[i].flags; + residency_ns = pnv_idle.states[i].residency_ns; if ((flags & OPAL_PM_LOSE_FULL_CONTEXT) && - (pnv_first_deep_stop_state > psscr_rl)) { + (pnv_first_deep_stop_state > psscr_rl)) pnv_first_deep_stop_state = psscr_rl; - } - if (max_residency_ns < residency_ns) { + if (unlikely(max_residency_ns < residency_ns)) { max_residency_ns = residency_ns; pnv_deepest_stop_psscr_val = psscr_val; pnv_deepest_stop_psscr_mask = psscr_mask; @@ -593,6 +644,8 @@ static void __init pnv_power8_idle_init(void) bool has_nap = false; bool has_sleep_er1 = false; int dt_idle_states = pnv_idle.nr_states; + bool disable_full_context_loss = false; + bool sprs_for_deep_state_saved = false; for (i = 0; i < dt_idle_states; i++) { struct pnv_idle_state *state = &pnv_idle.states[i]; @@ -601,6 +654,29 @@ static void __init pnv_power8_idle_init(void) has_nap = true; if (state->flags & OPAL_PM_SLEEP_ENABLED_ER1) has_sleep_er1 = true; + if (state->flags & OPAL_PM_LOSE_FULL_CONTEXT) { + int rc; + + if (sprs_for_deep_state_saved) + continue; + if (disable_full_context_loss) { + state->valid = false; + pr_warn("cpuidle-powernv: Disabling full-context loss state :%s\n", + pnv_idle.states[i].name); + continue; + } + + rc = pnv_save_sprs_for_deep_states(); + if (likely(!rc)) { + sprs_for_deep_state_saved = true; + } else { + pr_warn("cpuidle-powernv:Disabling full-context loss states.SLW unable to restore SPRs.\n"); + disable_full_context_loss = true; + state->valid = false; + pr_warn("cpuidle-powernv:Disabling full-context loss state:%s\n", + pnv_idle.states[i].name); + } + } } if (!has_sleep_er1) {