Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
On 24/07/20 6:55 am, Michael Neuling wrote: On Fri, 2020-07-10 at 10:52 +0530, Pratik Rajesh Sampat wrote: Additional registers DAWR0, DAWRX0 may be lost on Power 10 for stop levels < 4. Therefore save the values of these SPRs before entering a "stop" state and restore their values on wakeup. Signed-off-by: Pratik Rajesh Sampat --- arch/powerpc/platforms/powernv/idle.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c index 19d94d021357..f2e2a6a4c274 100644 --- a/arch/powerpc/platforms/powernv/idle.c +++ b/arch/powerpc/platforms/powernv/idle.c @@ -600,6 +600,8 @@ struct p9_sprs { u64 iamr; u64 amor; u64 uamor; + u64 dawr0; + u64 dawrx0; }; static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) sprs.iamr = mfspr(SPRN_IAMR); sprs.amor = mfspr(SPRN_AMOR); sprs.uamor = mfspr(SPRN_UAMOR); + if (cpu_has_feature(CPU_FTR_ARCH_31)) { You are actually viewing an old version of the patches The main point of change were based on comments from Nick Piggin, I have changed the top level function check from ARCH_300 to a P9 PVR check instead. A similar thing needs to be done for P10, however as the P10 PVR isn't exposed yet, I've shelved this particular patch. Nick's comment to check based on PVR:https://lkml.org/lkml/2020/7/13/1018 v4 of the series:https://lkml.org/lkml/2020/7/21/784 Thanks for your review, Pratik Can you add a comment here saying even though DAWR0 is ARCH_30, it's only required to be saved on 31. Otherwise this looks pretty odd. + sprs.dawr0 = mfspr(SPRN_DAWR0); + sprs.dawrx0 = mfspr(SPRN_DAWRX0); + } srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) mtspr(SPRN_IAMR,sprs.iamr); mtspr(SPRN_AMOR,sprs.amor); mtspr(SPRN_UAMOR, sprs.uamor); + if (cpu_has_feature(CPU_FTR_ARCH_31)) { + mtspr(SPRN_DAWR0, sprs.dawr0); + mtspr(SPRN_DAWRX0, sprs.dawrx0); + } /* * Workaround for POWER9 DD2.0, if we lost resources, the ERAT
Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
On Fri, 2020-07-10 at 10:52 +0530, Pratik Rajesh Sampat wrote: > Additional registers DAWR0, DAWRX0 may be lost on Power 10 for > stop levels < 4. > Therefore save the values of these SPRs before entering a "stop" > state and restore their values on wakeup. > > Signed-off-by: Pratik Rajesh Sampat > --- > arch/powerpc/platforms/powernv/idle.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/powerpc/platforms/powernv/idle.c > b/arch/powerpc/platforms/powernv/idle.c > index 19d94d021357..f2e2a6a4c274 100644 > --- a/arch/powerpc/platforms/powernv/idle.c > +++ b/arch/powerpc/platforms/powernv/idle.c > @@ -600,6 +600,8 @@ struct p9_sprs { > u64 iamr; > u64 amor; > u64 uamor; > + u64 dawr0; > + u64 dawrx0; > }; > > static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) > @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long > psscr, bool mmu_on) > sprs.iamr = mfspr(SPRN_IAMR); > sprs.amor = mfspr(SPRN_AMOR); > sprs.uamor = mfspr(SPRN_UAMOR); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { Can you add a comment here saying even though DAWR0 is ARCH_30, it's only required to be saved on 31. Otherwise this looks pretty odd. > + sprs.dawr0 = mfspr(SPRN_DAWR0); > + sprs.dawrx0 = mfspr(SPRN_DAWRX0); > + } > > srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ > > @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long > psscr, bool mmu_on) > mtspr(SPRN_IAMR,sprs.iamr); > mtspr(SPRN_AMOR,sprs.amor); > mtspr(SPRN_UAMOR, sprs.uamor); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { > + mtspr(SPRN_DAWR0, sprs.dawr0); > + mtspr(SPRN_DAWRX0, sprs.dawrx0); > + } > > /* >* Workaround for POWER9 DD2.0, if we lost resources, the ERAT
Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0, DAWRX0 for P10
Hi Nick, On 7/13/20 11:22 AM, Nicholas Piggin wrote: Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm: Additional registers DAWR0, DAWRX0 may be lost on Power 10 for stop levels < 4. Therefore save the values of these SPRs before entering a "stop" state and restore their values on wakeup. Hmm, where do you get this from? Documentation I see says DAWR is lost on POWER9 but not P10. Does idle thread even need to save DAWR, or does it get switched when going to a thread that has a watchpoint set? I don't know how idle states works internally but IIUC, we need to save/restore DAWRs. This is needed when user creates per-cpu watchpoint event. Ravi
Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0,DAWRX0 for P10
Hi Pratik, On 7/10/20 10:52 AM, Pratik Rajesh Sampat wrote: Additional registers DAWR0, DAWRX0 may be lost on Power 10 for stop levels < 4. p10 has one more pair DAWR1/DAWRX1. Please include that as well. Ravi
Re: [PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0, DAWRX0 for P10
Excerpts from Pratik Rajesh Sampat's message of July 10, 2020 3:22 pm: > Additional registers DAWR0, DAWRX0 may be lost on Power 10 for > stop levels < 4. > Therefore save the values of these SPRs before entering a "stop" > state and restore their values on wakeup. Hmm, where do you get this from? Documentation I see says DAWR is lost on POWER9 but not P10. Does idle thread even need to save DAWR, or does it get switched when going to a thread that has a watchpoint set? Thanks, Nick > > Signed-off-by: Pratik Rajesh Sampat > --- > arch/powerpc/platforms/powernv/idle.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/powerpc/platforms/powernv/idle.c > b/arch/powerpc/platforms/powernv/idle.c > index 19d94d021357..f2e2a6a4c274 100644 > --- a/arch/powerpc/platforms/powernv/idle.c > +++ b/arch/powerpc/platforms/powernv/idle.c > @@ -600,6 +600,8 @@ struct p9_sprs { > u64 iamr; > u64 amor; > u64 uamor; > + u64 dawr0; > + u64 dawrx0; > }; > > static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) > @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long > psscr, bool mmu_on) > sprs.iamr = mfspr(SPRN_IAMR); > sprs.amor = mfspr(SPRN_AMOR); > sprs.uamor = mfspr(SPRN_UAMOR); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { > + sprs.dawr0 = mfspr(SPRN_DAWR0); > + sprs.dawrx0 = mfspr(SPRN_DAWRX0); > + } > > srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ > > @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long > psscr, bool mmu_on) > mtspr(SPRN_IAMR,sprs.iamr); > mtspr(SPRN_AMOR,sprs.amor); > mtspr(SPRN_UAMOR, sprs.uamor); > + if (cpu_has_feature(CPU_FTR_ARCH_31)) { > + mtspr(SPRN_DAWR0, sprs.dawr0); > + mtspr(SPRN_DAWRX0, sprs.dawrx0); > + } > > /* >* Workaround for POWER9 DD2.0, if we lost resources, the ERAT > -- > 2.25.4 > >
[PATCH v2 2/3] powerpc/powernv/idle: save-restore DAWR0, DAWRX0 for P10
Additional registers DAWR0, DAWRX0 may be lost on Power 10 for stop levels < 4. Therefore save the values of these SPRs before entering a "stop" state and restore their values on wakeup. Signed-off-by: Pratik Rajesh Sampat --- arch/powerpc/platforms/powernv/idle.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c index 19d94d021357..f2e2a6a4c274 100644 --- a/arch/powerpc/platforms/powernv/idle.c +++ b/arch/powerpc/platforms/powernv/idle.c @@ -600,6 +600,8 @@ struct p9_sprs { u64 iamr; u64 amor; u64 uamor; + u64 dawr0; + u64 dawrx0; }; static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) @@ -687,6 +689,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) sprs.iamr = mfspr(SPRN_IAMR); sprs.amor = mfspr(SPRN_AMOR); sprs.uamor = mfspr(SPRN_UAMOR); + if (cpu_has_feature(CPU_FTR_ARCH_31)) { + sprs.dawr0 = mfspr(SPRN_DAWR0); + sprs.dawrx0 = mfspr(SPRN_DAWRX0); + } srr1 = isa300_idle_stop_mayloss(psscr); /* go idle */ @@ -710,6 +716,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on) mtspr(SPRN_IAMR,sprs.iamr); mtspr(SPRN_AMOR,sprs.amor); mtspr(SPRN_UAMOR, sprs.uamor); + if (cpu_has_feature(CPU_FTR_ARCH_31)) { + mtspr(SPRN_DAWR0, sprs.dawr0); + mtspr(SPRN_DAWRX0, sprs.dawrx0); + } /* * Workaround for POWER9 DD2.0, if we lost resources, the ERAT -- 2.25.4