Re: [PATCH v3 4/4] powernv: powerpc: Add winkle support for offline cpus

From: Shreyas B Prabhu
Date: Mon Dec 08 2014 - 16:54:57 EST

Next message: Andy Lutomirski: "[PATCH v2] x86_64, switch_to: Load TLS descriptors before switching DS and ES"
Previous message: Yinghai Lu: "[PATCH] PCI: Clear bridge MEM_64 flag if one child does not support it"
In reply to: Paul Mackerras: "Re: [PATCH v3 4/4] powernv: powerpc: Add winkle support for offline cpus"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Monday 08 December 2014 11:22 AM, Paul Mackerras wrote:
> On Thu, Dec 04, 2014 at 12:58:23PM +0530, Shreyas B. Prabhu wrote:
>> Winkle is a deep idle state supported in power8 chips. A core enters
>> winkle when all the threads of the core enter winkle. In this state
>> power supply to the entire chiplet i.e core, private L2 and private L3
>> is turned off. As a result it gives higher powersavings compared to
>> sleep.
>>
>> But entering winkle results in a total hypervisor state loss. Hence the
>> hypervisor context has to be preserved before entering winkle and
>> restored upon wake up.
>>
>> Power-on Reset Engine (PORE) is a dedicated engine which is responsible
>> for powering on the chiplet during wake up. It can be programmed to
>> restore the register contests of a few specific registers. This patch
>> uses PORE to restore register state wherever possible and uses stack to
>> save and restore rest of the necessary registers.
>>
>> With hypervisor state restore things fall under three categories-
>> per-core state, per-subcore state and per-thread state. To manage this,
>> extend the infrastructure introduced for sleep. Mainly we add a paca
>> variable subcore_sibling_mask. Using this and the core_idle_state we can
>> distingush first thread in core and subcore.
>
> Comments below...
>
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 7637889..2b9b5fb 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -102,9 +102,7 @@ system_reset_pSeries:
>> #ifdef CONFIG_PPC_P7_NAP
>> BEGIN_FTR_SECTION
>> /* Running native on arch 2.06 or later, check if we are
>> - * waking up from nap. We only handle no state loss and
>> - * supervisor state loss. We do -not- handle hypervisor
>> - * state loss at this time.
>> + * waking up from nap/sleep/winkle.
>> */
>> mfspr r13,SPRN_SRR1
>> rlwinm. r13,r13,47-31,30,31
>> @@ -112,7 +110,17 @@ BEGIN_FTR_SECTION
>>
>> cmpwi cr3,r13,2
>>
>> - GET_PACA(r13)
>> + /* Check if last bit of HSPGR0 is set. This indicates whether we are
>> + * waking up from winkle */
>> + li r3,1
>> + mfspr r4,SPRN_HSPRG0
>> + and r5,r4,r3
>> + cmpwi cr4,r5,1 /* Store result in cr4 for later use */
>> +
>> + andc r4,r4,r3
>> + mtspr SPRN_HSPRG0,r4
>> +
>> + mr r13,r4
>
> This seems unnecessarily convoluted. How about:
>
> GET_PACA(r13)
> clrldi r5,r13,63
> clrrdi r13,r13,1
> cmpwi cr4,r5,1
> mtspr SPRN_HSPRG0,r13
>
Yes, makes more sense. I'll use this.

>> diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
>> index 8c3a1f4..8102075 100644
>> --- a/arch/powerpc/kernel/idle_power7.S
>> +++ b/arch/powerpc/kernel/idle_power7.S
>> @@ -19,8 +19,24 @@
>> #include <asm/kvm_book3s_asm.h>
>> #include <asm/opal.h>
>> #include <asm/cpuidle.h>
>> +#include <asm/mmu-hash64.h>
>>
>> #undef DEBUG
>> +/*
>> + * Use unused space in the interrupt stack to save and restore
>> + * registers for winkle support.
>> + */
>> +#define _SDR1 GPR3
>> +#define _RPR GPR4
>> +#define _SPURR GPR5
>> +#define _PURR GPR6
>> +#define _TSCR GPR7
>> +#define _DSCR GPR8
>> +#define _AMOR GPR9
>> +#define _PMC5 GPR10
>> +#define _PMC6 GPR11
>
> Why only PMC5 and PMC6 out of all the PMU registers? What about
> PMC1-PMC4 and the MMCR registers? I assume they're lost during winkle
> state also, aren't they? If we're not saving them, what's the point
> of saving and restoring PMC5 and PMC6?
>
Yes all PMC and MMCR contents are lost. Using __restore_cpu_power8, the
MMCR registers are initialized to 0. The reasoning behind specifically
restoring PMC5 and PMC6 was the fact that they are not programmable and
count cycles/instructions by default. We suspected that there might be a
userspace program which relied on PMC5/PMC6 always increasing.
But now on closer look, since these counters are 32 bit and cycles/
instruction counts are bound to exceed it, I doubt such userspace programs
exist. I'll drop PMC5 and PMC6 in the next version.

>> +#define _WORT GPR12
>> +#define _WORC GPR13
>>
>> /* Idle state entry routines */
>>
>> @@ -124,8 +140,8 @@ power7_enter_nap_mode:
>> stb r4,HSTATE_HWTHREAD_STATE(r13)
>> #endif
>> stb r3,PACA_THREAD_IDLE_STATE(r13)
>> - cmpwi cr1,r3,PNV_THREAD_SLEEP
>> - bge cr1,2f
>> + cmpwi cr3,r3,PNV_THREAD_SLEEP
>> + bge cr3,2f
>> IDLE_STATE_ENTER_SEQ(PPC_NAP)
>> /* No return */
>> 2:
>> @@ -154,7 +170,8 @@ pnv_fastsleep_workaround_at_entry:
>> isync
>> bne- lwarx_loop1
>>
>> -common_enter: /* common code for all the threads entering sleep */
>> +common_enter: /* common code for all the threads entering sleep or winkle */
>> + bgt cr3,enter_winkle
>> IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
>>
>> fastsleep_workaround_at_entry:
>> @@ -175,6 +192,34 @@ fastsleep_workaround_at_entry:
>> stw r0,0(r14)
>> b common_enter
>>
>> +enter_winkle:
>> + /*
>> + * Note all register i.e per-core, per-subcore or per-thread is saved
>> + * here since any thread in the core might wake up first
>> + */
>> + mfspr r3,SPRN_SDR1
>> + std r3,_SDR1(r1)
>> + mfspr r3,SPRN_RPR
>> + std r3,_RPR(r1)
>> + mfspr r3,SPRN_SPURR
>> + std r3,_SPURR(r1)
>> + mfspr r3,SPRN_PURR
>> + std r3,_PURR(r1)
>> + mfspr r3,SPRN_TSCR
>> + std r3,_TSCR(r1)
>> + mfspr r3,SPRN_DSCR
>> + std r3,_DSCR(r1)
>> + mfspr r3,SPRN_AMOR
>> + std r3,_AMOR(r1)
>> + mfspr r3,SPRN_PMC5
>> + std r3,_PMC5(r1)
>> + mfspr r3,SPRN_PMC6
>> + std r3,_PMC6(r1)
>> + mfspr r3,SPRN_WORT
>> + std r3,_WORT(r1)
>> + mfspr r3,SPRN_WORC
>> + std r3,_WORC(r1)
>> + IDLE_STATE_ENTER_SEQ(PPC_WINKLE)
>>
>> _GLOBAL(power7_idle)
>> /* Now check if user or arch enabled NAP mode */
>> @@ -197,6 +242,12 @@ _GLOBAL(power7_sleep)
>> b power7_powersave_common
>> /* No return */
>>
>> +_GLOBAL(power7_winkle)
>> + li r3,3
>> + li r4,1
>> + b power7_powersave_common
>> + /* No return */
>> +
>> #define CHECK_HMI_INTERRUPT \
>> mfspr r0,SPRN_SRR1; \
>> BEGIN_FTR_SECTION_NESTED(66); \
>> @@ -238,11 +289,23 @@ lwarx_loop2:
>> bne core_idle_lock_held
>>
>> cmpwi cr2,r15,0
>> + lbz r4,PACA_SUBCORE_SIBLING_MASK(r13)
>> + and r4,r4,r15
>> + cmpwi cr1,r4,0 /* Check if first in subcore */
>> +
>> + /*
>> + * At this stage
>> + * cr1 - 10 if first thread to wakeup in subcore
>> + * cr2 - 10 if first thread to wakeup in core
>> + * cr3- 01 if waking up from sleep or winkle
>> + * cr4 - 10 if waking up from winkle
>> + */
>
> What do "10" and "01" mean in this comment? (If they were CR field
> values in binary they would need to be 3 or 4 bits, not 2.)
>
I'll fix this.

Thanks,
Shreyas

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Andy Lutomirski: "[PATCH v2] x86_64, switch_to: Load TLS descriptors before switching DS and ES"
Previous message: Yinghai Lu: "[PATCH] PCI: Clear bridge MEM_64 flag if one child does not support it"
In reply to: Paul Mackerras: "Re: [PATCH v3 4/4] powernv: powerpc: Add winkle support for offline cpus"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]