Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1691098 > unrolled thread

[v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle

Started by"Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
First post2017-07-19 10:20 +0200
Last post2017-07-19 14:50 +0200
Articles 4 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> - 2017-07-19 10:20 +0200
    Re: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for  stop4 cpuidle Nicholas Piggin <npiggin@gmail.com> - 2017-07-19 11:10 +0200
      Re: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle Michael Ellerman <mpe@ellerman.id.au> - 2017-07-19 14:10 +0200
        Re: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for  stop4 cpuidle Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2017-07-19 14:50 +0200

#1691098 — [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle

From"Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Date2017-07-19 10:20 +0200
Subject[v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle
Message-ID<u4Sn8-5Rq-5@gated-at.bofh.it>
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

The stop4 idle state on POWER9 is a deep idle state which loses
hypervisor resources, but whose latency is low enough that it can be
exposed via cpuidle.

Until now, the deep idle states which lose hypervisor resources (eg:
winkle) were only exposed via CPU-Hotplug.  Hence currently on wakeup
from such states, barring a few SPRs which need to be restored to
their older value, rest of the SPRS are reinitialized to their values
corresponding to that at boot time.

When stop4 is used in the context of cpuidle, we want these additional
SPRs to be restored to their older value, to ensure that the context
on the CPU coming back from idle is same as it was before going idle.

In this patch, we define a SPR save area in PACA (since we have used
up the volatile register space in the stack) and on POWER9, we restore
SPRN_PID, SPRN_LDBAR, SPRN_FSCR, SPRN_HFSCR, SPRN_MMCRA, SPRN_MMCR1,
SPRN_MMCR2 to the values they had before entering stop.

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/paca.h   |  7 ++++++
 arch/powerpc/kernel/asm-offsets.c | 12 ++++++++++
 arch/powerpc/kernel/idle_book3s.S | 46 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index dc88a31..a6b9ea6 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -48,6 +48,7 @@
 #define get_lppaca()	(get_paca()->lppaca_ptr)
 #define get_slb_shadow()	(get_paca()->slb_shadow_ptr)
 
+#define MAX_STOP_SPRS     7
 struct task_struct;
 
 /*
@@ -183,6 +184,12 @@ struct paca_struct {
 	struct paca_struct **thread_sibling_pacas;
 	/* The PSSCR value that the kernel requested before going to stop */
 	u64 requested_psscr;
+
+	/*
+	 * Save area for additional SPRs that need to be
+	 * saved/restored during cpuidle stop.
+	 */
+	u64 stop_spr_save_area[MAX_STOP_SPRS];
 #endif
 
 #ifdef CONFIG_PPC_STD_MMU_64
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index a7b5af3..0262283 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -743,6 +743,18 @@ int main(void)
 	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
 	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
 	OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr);
+
+	OFFSET(PACA_PID, paca_struct, stop_spr_save_area[0]);
+	OFFSET(PACA_LDBAR, paca_struct, stop_spr_save_area[1]);
+	OFFSET(PACA_FSCR, paca_struct, stop_spr_save_area[2]);
+	OFFSET(PACA_HFSCR, paca_struct, stop_spr_save_area[3]);
+
+	/* On POWER9, we are already saving MMCR0 for ESL=EC=1 */
+	OFFSET(PACA_MMCRA, paca_struct, stop_spr_save_area[4]);
+	OFFSET(PACA_MMCR1, paca_struct, stop_spr_save_area[5]);
+	OFFSET(PACA_MMCR2, paca_struct, stop_spr_save_area[6]);
+
+
 #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 5adb390e..7083f63 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -84,7 +84,50 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_ARCH_300)
 	std	r3,_WORT(r1)
 	mfspr	r3,SPRN_WORC
 	std	r3,_WORC(r1)
+BEGIN_FTR_SECTION
+	blr
+END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
+	/* Save additional SPRs for POWER9 */
+power9_save_additional_sprs:
+	mfspr	r3, SPRN_PID
+	mfspr	r4, SPRN_LDBAR
+	std	r3, PACA_PID(r13)
+	std	r4, PACA_LDBAR(r13)
+
+	mfspr	r3, SPRN_FSCR
+	mfspr	r4, SPRN_HFSCR
+	std	r3, PACA_FSCR(r13)
+	std	r4, PACA_HFSCR(r13)
+
+	mfspr	r3, SPRN_MMCRA
+	mfspr	r4, SPRN_MMCR1
+	std	r3, PACA_MMCRA(r13)
+	std	r4, PACA_MMCR1(r13)
+
+	mfspr	r3, SPRN_MMCR2
+	std	r3, PACA_MMCR2(r13)
+	blr
 
+power9_restore_additional_sprs:
+	ld	r3,_LPCR(r1)
+	ld	r4, PACA_PID(r13)
+	mtspr	SPRN_LPCR,r3
+	mtspr	SPRN_PID, r4
+
+	ld	r3, PACA_LDBAR(r13)
+	ld	r4, PACA_FSCR(r13)
+	mtspr	SPRN_LDBAR, r3
+	mtspr	SPRN_FSCR, r4
+
+	ld	r3, PACA_HFSCR(r13)
+	ld	r4, PACA_MMCRA(r13)
+	mtspr	SPRN_HFSCR, r3
+	mtspr	SPRN_MMCRA, r4
+	/* We have already restored PACA_MMCR0 */
+	ld	r3, PACA_MMCR1(r13)
+	ld	r4, PACA_MMCR2(r13)
+	mtspr	SPRN_MMCR1, r3
+	mtspr	SPRN_MMCR2, r4
 	blr
 
 /*
@@ -791,8 +834,7 @@ no_segments:
 	bctrl
 
 BEGIN_FTR_SECTION
-	ld	r4,_LPCR(r1)
-	mtspr	SPRN_LPCR,r4
+	bl 	power9_restore_additional_sprs
 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
 hypervisor_state_restored:
 
-- 
1.9.4

[toc] | [next] | [standalone]

#1691146 — Re: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle

FromNicholas Piggin <npiggin@gmail.com>
Date2017-07-19 11:10 +0200
SubjectRe: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle
Message-ID<u4T9y-6rf-45@gated-at.bofh.it>
In reply to#1691098
On Wed, 19 Jul 2017 13:48:49 +0530
"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> The stop4 idle state on POWER9 is a deep idle state which loses
> hypervisor resources, but whose latency is low enough that it can be
> exposed via cpuidle.
> 
> Until now, the deep idle states which lose hypervisor resources (eg:
> winkle) were only exposed via CPU-Hotplug.  Hence currently on wakeup
> from such states, barring a few SPRs which need to be restored to
> their older value, rest of the SPRS are reinitialized to their values
> corresponding to that at boot time.
> 
> When stop4 is used in the context of cpuidle, we want these additional
> SPRs to be restored to their older value, to ensure that the context
> on the CPU coming back from idle is same as it was before going idle.
> 
> In this patch, we define a SPR save area in PACA (since we have used
> up the volatile register space in the stack) and on POWER9, we restore
> SPRN_PID, SPRN_LDBAR, SPRN_FSCR, SPRN_HFSCR, SPRN_MMCRA, SPRN_MMCR1,
> SPRN_MMCR2 to the values they had before entering stop.
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/paca.h   |  7 ++++++
>  arch/powerpc/kernel/asm-offsets.c | 12 ++++++++++
>  arch/powerpc/kernel/idle_book3s.S | 46 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index dc88a31..a6b9ea6 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -48,6 +48,7 @@
>  #define get_lppaca()	(get_paca()->lppaca_ptr)
>  #define get_slb_shadow()	(get_paca()->slb_shadow_ptr)
>  
> +#define MAX_STOP_SPRS     7
>  struct task_struct;
>  
>  /*
> @@ -183,6 +184,12 @@ struct paca_struct {
>  	struct paca_struct **thread_sibling_pacas;
>  	/* The PSSCR value that the kernel requested before going to stop */
>  	u64 requested_psscr;
> +
> +	/*
> +	 * Save area for additional SPRs that need to be
> +	 * saved/restored during cpuidle stop.
> +	 */
> +	u64 stop_spr_save_area[MAX_STOP_SPRS];
>  #endif
>  
>  #ifdef CONFIG_PPC_STD_MMU_64
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index a7b5af3..0262283 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -743,6 +743,18 @@ int main(void)
>  	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
>  	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
>  	OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr);
> +
> +	OFFSET(PACA_PID, paca_struct, stop_spr_save_area[0]);
> +	OFFSET(PACA_LDBAR, paca_struct, stop_spr_save_area[1]);
> +	OFFSET(PACA_FSCR, paca_struct, stop_spr_save_area[2]);
> +	OFFSET(PACA_HFSCR, paca_struct, stop_spr_save_area[3]);
> +
> +	/* On POWER9, we are already saving MMCR0 for ESL=EC=1 */
> +	OFFSET(PACA_MMCRA, paca_struct, stop_spr_save_area[4]);
> +	OFFSET(PACA_MMCR1, paca_struct, stop_spr_save_area[5]);
> +	OFFSET(PACA_MMCR2, paca_struct, stop_spr_save_area[6]);

Don't these offset names go against convention?

Look at e.g., how PACA_EXGEN is used. I would prefer using that
convention. You could make the name slightly shorter too, e.g.,
just stop_sprs or so.


> @@ -791,8 +834,7 @@ no_segments:
>  	bctrl
>  
>  BEGIN_FTR_SECTION
> -	ld	r4,_LPCR(r1)
> -	mtspr	SPRN_LPCR,r4
> +	bl 	power9_restore_additional_sprs
>  END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>  hypervisor_state_restored:
>  

Also perhaps just a little comment in the code explaining why
power8 does not need this because it only uses it for hotplug
as you do in the changelog would help when reading the code.

Sorry I didn't pick this up on your v1 patches. If you change
those minor things, please add

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick

[toc] | [prev] | [next] | [standalone]

#1691623

FromMichael Ellerman <mpe@ellerman.id.au>
Date2017-07-19 14:10 +0200
Message-ID<u4VXI-8ol-17@gated-at.bofh.it>
In reply to#1691146
Nicholas Piggin <npiggin@gmail.com> writes:

> On Wed, 19 Jul 2017 13:48:49 +0530
> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> wrote:
>
>> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>> 
>> The stop4 idle state on POWER9 is a deep idle state which loses
>> hypervisor resources, but whose latency is low enough that it can be
>> exposed via cpuidle.
>> 
>> Until now, the deep idle states which lose hypervisor resources (eg:
>> winkle) were only exposed via CPU-Hotplug.  Hence currently on wakeup
>> from such states, barring a few SPRs which need to be restored to
>> their older value, rest of the SPRS are reinitialized to their values
>> corresponding to that at boot time.
>> 
>> When stop4 is used in the context of cpuidle, we want these additional
>> SPRs to be restored to their older value, to ensure that the context
>> on the CPU coming back from idle is same as it was before going idle.
>> 
>> In this patch, we define a SPR save area in PACA (since we have used
>> up the volatile register space in the stack) and on POWER9, we restore
>> SPRN_PID, SPRN_LDBAR, SPRN_FSCR, SPRN_HFSCR, SPRN_MMCRA, SPRN_MMCR1,
>> SPRN_MMCR2 to the values they had before entering stop.
>> 
>> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/include/asm/paca.h   |  7 ++++++
>>  arch/powerpc/kernel/asm-offsets.c | 12 ++++++++++
>>  arch/powerpc/kernel/idle_book3s.S | 46 +++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 63 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
>> index dc88a31..a6b9ea6 100644
>> --- a/arch/powerpc/include/asm/paca.h
>> +++ b/arch/powerpc/include/asm/paca.h
>> @@ -48,6 +48,7 @@
>>  #define get_lppaca()	(get_paca()->lppaca_ptr)
>>  #define get_slb_shadow()	(get_paca()->slb_shadow_ptr)
>>  
>> +#define MAX_STOP_SPRS     7
>>  struct task_struct;
>>  
>>  /*
>> @@ -183,6 +184,12 @@ struct paca_struct {
>>  	struct paca_struct **thread_sibling_pacas;
>>  	/* The PSSCR value that the kernel requested before going to stop */
>>  	u64 requested_psscr;
>> +
>> +	/*
>> +	 * Save area for additional SPRs that need to be
>> +	 * saved/restored during cpuidle stop.
>> +	 */
>> +	u64 stop_spr_save_area[MAX_STOP_SPRS];
>>  #endif
>>  
>>  #ifdef CONFIG_PPC_STD_MMU_64
>> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
>> index a7b5af3..0262283 100644
>> --- a/arch/powerpc/kernel/asm-offsets.c
>> +++ b/arch/powerpc/kernel/asm-offsets.c
>> @@ -743,6 +743,18 @@ int main(void)
>>  	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
>>  	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
>>  	OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr);
>> +
>> +	OFFSET(PACA_PID, paca_struct, stop_spr_save_area[0]);
>> +	OFFSET(PACA_LDBAR, paca_struct, stop_spr_save_area[1]);
>> +	OFFSET(PACA_FSCR, paca_struct, stop_spr_save_area[2]);
>> +	OFFSET(PACA_HFSCR, paca_struct, stop_spr_save_area[3]);
>> +
>> +	/* On POWER9, we are already saving MMCR0 for ESL=EC=1 */
>> +	OFFSET(PACA_MMCRA, paca_struct, stop_spr_save_area[4]);
>> +	OFFSET(PACA_MMCR1, paca_struct, stop_spr_save_area[5]);
>> +	OFFSET(PACA_MMCR2, paca_struct, stop_spr_save_area[6]);
>
> Don't these offset names go against convention?
>
> Look at e.g., how PACA_EXGEN is used. I would prefer using that
> convention. You could make the name slightly shorter too, e.g.,
> just stop_sprs or so.

Yes please.

If I see PACA_MMCRA I'm expecting that's paca->mmcra.

Also if the same values always go in the same place then please use a
proper struct, rather than an array. ie.

struct stop_sprs
{
	u64 pid;
        u64 ldbar;
        ...
}

cheers

[toc] | [prev] | [next] | [standalone]

#1691649 — Re: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle

FromGautham R Shenoy <ego@linux.vnet.ibm.com>
Date2017-07-19 14:50 +0200
SubjectRe: [v2 PATCH 1/2] powernv/powerpc:Save/Restore additional SPRs for stop4 cpuidle
Message-ID<u4WAq-bK-9@gated-at.bofh.it>
In reply to#1691623
Hi Nicholas, Michael,

On Wed, Jul 19, 2017 at 10:07:05PM +1000, Michael Ellerman wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> >> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> >> index a7b5af3..0262283 100644
> >> --- a/arch/powerpc/kernel/asm-offsets.c
> >> +++ b/arch/powerpc/kernel/asm-offsets.c
> >> @@ -743,6 +743,18 @@ int main(void)
> >>  	OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
> >>  	OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
> >>  	OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr);
> >> +
> >> +	OFFSET(PACA_PID, paca_struct, stop_spr_save_area[0]);
> >> +	OFFSET(PACA_LDBAR, paca_struct, stop_spr_save_area[1]);
> >> +	OFFSET(PACA_FSCR, paca_struct, stop_spr_save_area[2]);
> >> +	OFFSET(PACA_HFSCR, paca_struct, stop_spr_save_area[3]);
> >> +
> >> +	/* On POWER9, we are already saving MMCR0 for ESL=EC=1 */
> >> +	OFFSET(PACA_MMCRA, paca_struct, stop_spr_save_area[4]);
> >> +	OFFSET(PACA_MMCR1, paca_struct, stop_spr_save_area[5]);
> >> +	OFFSET(PACA_MMCR2, paca_struct, stop_spr_save_area[6]);
> >
> > Don't these offset names go against convention?
> >
> > Look at e.g., how PACA_EXGEN is used. I would prefer using that
> > convention. You could make the name slightly shorter too, e.g.,
> > just stop_sprs or so.
> 
> Yes please.
> 
> If I see PACA_MMCRA I'm expecting that's paca->mmcra.

Ah, ok. I will fix this. 

> 
> Also if the same values always go in the same place then please use a
> proper struct, rather than an array. ie.
>

Ok, I will add the struct instead of a array.

> struct stop_sprs
> {
> 	u64 pid;
>         u64 ldbar;
>         ...
> }
> 
> cheers
> 
--
Thanks and Regards
gautham.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web