Message ID | 1531843216-22209-1-git-send-email-ego@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [RESEND] powerpc/powernv : Save/Restore SPRG3 on entry/exit from stop. | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
> DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER); > diff --git a/arch/powerpc/kernel/idle_book3s.S > b/arch/powerpc/kernel/idle_book3s.S > index d85d551..5069d42 100644 > --- a/arch/powerpc/kernel/idle_book3s.S > +++ b/arch/powerpc/kernel/idle_book3s.S > @@ -120,6 +120,9 @@ power9_save_additional_sprs: > mfspr r4, SPRN_MMCR2 > std r3, STOP_MMCR1(r13) > std r4, STOP_MMCR2(r13) > + > + mfspr r3, SPRN_SPRG3 > + std r3, STOP_SPRG3(r13) We don't need to save it. Just restore it from paca->sprg_vdso which should never change. How can we do better at catching these missing SPRGs? We missed this one and looking at c1b25a17d249 we missed the AMOR a couple of months back. I'd rather we had some systematic way of finding the ones we are missing, rather than playing wake-a-mole. Mikey > blr > > power9_restore_additional_sprs: > @@ -144,7 +147,9 @@ power9_restore_additional_sprs: > mtspr SPRN_MMCR1, r4 > > ld r3, STOP_MMCR2(r13) > + ld r4, STOP_SPRG3(r13) > mtspr SPRN_MMCR2, r3 > + mtspr SPRN_SPRG3, r4 > blr > > /*
Hello Mikey, On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote: > > > DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER); > > diff --git a/arch/powerpc/kernel/idle_book3s.S > > b/arch/powerpc/kernel/idle_book3s.S > > index d85d551..5069d42 100644 > > --- a/arch/powerpc/kernel/idle_book3s.S > > +++ b/arch/powerpc/kernel/idle_book3s.S > > @@ -120,6 +120,9 @@ power9_save_additional_sprs: > > mfspr r4, SPRN_MMCR2 > > std r3, STOP_MMCR1(r13) > > std r4, STOP_MMCR2(r13) > > + > > + mfspr r3, SPRN_SPRG3 > > + std r3, STOP_SPRG3(r13) > > We don't need to save it. Just restore it from paca->sprg_vdso which should > never change. Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso. > > How can we do better at catching these missing SPRGs? We can go through the list of SPRs from the POWER9 User Manual and document explicitly why we don't have to save/restore certain SPRs during the execution of the stop instruction. Does this sound ok ? (Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual accessible from https://openpowerfoundation.org/?resource_lib=power9-processor-users-manual) > > We missed this one and looking at c1b25a17d249 we missed the AMOR a couple of > months back. I'd rather we had some systematic way of finding the ones we are > missing, rather than playing wake-a-mole. I agree, we need something more systematic than the try-catch method thing we have now. For deep stop states on POWER9, we looked at the list of SPRs that were being lost and restored during winkle on POWER8. The additional SPRs that we took care of were the ones related to the Radix and IMC. Now since winkle was used only in the context of CPU-Hotplug, the CPU-online code would reinit some of the SPRs such as SPRG3, which is why we didn't see this problem on POWER8. So, that is one obvious place to audit. AMOR was a bad miss. It was being restored in POWER8 as part of the subcore restore code, so like RPR, it should have been restored along with the other per-core SPRs. > > Mikey > > > blr > > > > power9_restore_additional_sprs: > > @@ -144,7 +147,9 @@ power9_restore_additional_sprs: > > mtspr SPRN_MMCR1, r4 > > > > ld r3, STOP_MMCR2(r13) > > + ld r4, STOP_SPRG3(r13) > > mtspr SPRN_MMCR2, r3 > > + mtspr SPRN_SPRG3, r4 > > blr > > > > /*
On Wed, 2018-07-18 at 13:42 +0530, Gautham R Shenoy wrote: > Hello Mikey, > > On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote: > > > > > DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER); > > > diff --git a/arch/powerpc/kernel/idle_book3s.S > > > b/arch/powerpc/kernel/idle_book3s.S > > > index d85d551..5069d42 100644 > > > --- a/arch/powerpc/kernel/idle_book3s.S > > > +++ b/arch/powerpc/kernel/idle_book3s.S > > > @@ -120,6 +120,9 @@ power9_save_additional_sprs: > > > mfspr r4, SPRN_MMCR2 > > > std r3, STOP_MMCR1(r13) > > > std r4, STOP_MMCR2(r13) > > > + > > > + mfspr r3, SPRN_SPRG3 > > > + std r3, STOP_SPRG3(r13) > > > > We don't need to save it. Just restore it from paca->sprg_vdso which should > > never change. > > Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso. > > > > > How can we do better at catching these missing SPRGs? > > We can go through the list of SPRs from the POWER9 User Manual and > document explicitly why we don't have to save/restore certain SPRs > during the execution of the stop instruction. Does this sound ok ? > > (Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual > accessible from > https://openpowerfoundation.org/?resource_lib=power9-processor-users-manual) I was thinking of a boot time test case built into linux. linux has some boot time test cases which you can enable via CONFIG options. Firstly you could see if an SPR exists using the same trick xmon does in dump_one_spr(). Then once you have a list of usable SPRs, you could write all the known ones (I assume you'd have to leave out some, like the PSSCR), then set the appropriate stop level, make sure you got into that stop level, and then see if that register was changed. Then you'd have an automated list of registers you need to make sure you save/restore at each stop level. Could something like that work? Mikey
Michael Neuling <mikey@neuling.org> writes: > On Wed, 2018-07-18 at 13:42 +0530, Gautham R Shenoy wrote: >> On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote: >> > >> > > DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER); >> > > diff --git a/arch/powerpc/kernel/idle_book3s.S >> > > b/arch/powerpc/kernel/idle_book3s.S >> > > index d85d551..5069d42 100644 >> > > --- a/arch/powerpc/kernel/idle_book3s.S >> > > +++ b/arch/powerpc/kernel/idle_book3s.S >> > > @@ -120,6 +120,9 @@ power9_save_additional_sprs: >> > > mfspr r4, SPRN_MMCR2 >> > > std r3, STOP_MMCR1(r13) >> > > std r4, STOP_MMCR2(r13) >> > > + >> > > + mfspr r3, SPRN_SPRG3 >> > > + std r3, STOP_SPRG3(r13) >> > >> > We don't need to save it. Just restore it from paca->sprg_vdso which should >> > never change. >> >> Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso. >> >> > >> > How can we do better at catching these missing SPRGs? >> >> We can go through the list of SPRs from the POWER9 User Manual and >> document explicitly why we don't have to save/restore certain SPRs >> during the execution of the stop instruction. Does this sound ok ? >> >> (Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual >> accessible from >> https://openpowerfoundation.org/?resource_lib=power9-processor-users-manual) > > I was thinking of a boot time test case built into linux. linux has some boot > time test cases which you can enable via CONFIG options. > > Firstly you could see if an SPR exists using the same trick xmon does in > dump_one_spr(). Then once you have a list of usable SPRs, you could write all > the known ones (I assume you'd have to leave out some, like the PSSCR), then set Write what value? Ideally you want to write a random bit pattern to reduce the chance that only some bits are being restored. But you can't do that because writing a value to an SPRs has an effect. Some of them might even need to be zero, in which case you can't really distinguish that from a non-restored zero. > the appropriate stop level, make sure you got into that stop level, and then see > if that register was changed. Then you'd have an automated list of registers you > need to make sure you save/restore at each stop level. > > Could something like that work? Maybe. Ignoring the problem of whether you can write a meaningful value to some of the SPRs, I'm not entirely convinced it's going to work. But maybe I'm wrong. But there's a much simpler solution, we should 1) have a selftest for getcpu() and 2) we should be running the glibc (I think?) test suite that found this in the first place. It's frankly embarrassing that we didn't find this. cheers
On Fri, 2018-07-20 at 12:32 +1000, Michael Ellerman wrote: > Michael Neuling <mikey@neuling.org> writes: > > On Wed, 2018-07-18 at 13:42 +0530, Gautham R Shenoy wrote: > > > On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote: > > > > > > > > > DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER); > > > > > diff --git a/arch/powerpc/kernel/idle_book3s.S > > > > > b/arch/powerpc/kernel/idle_book3s.S > > > > > index d85d551..5069d42 100644 > > > > > --- a/arch/powerpc/kernel/idle_book3s.S > > > > > +++ b/arch/powerpc/kernel/idle_book3s.S > > > > > @@ -120,6 +120,9 @@ power9_save_additional_sprs: > > > > > mfspr r4, SPRN_MMCR2 > > > > > std r3, STOP_MMCR1(r13) > > > > > std r4, STOP_MMCR2(r13) > > > > > + > > > > > + mfspr r3, SPRN_SPRG3 > > > > > + std r3, STOP_SPRG3(r13) > > > > > > > > We don't need to save it. Just restore it from paca->sprg_vdso which > > > > should > > > > never change. > > > > > > Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso. > > > > > > > > > > > How can we do better at catching these missing SPRGs? > > > > > > We can go through the list of SPRs from the POWER9 User Manual and > > > document explicitly why we don't have to save/restore certain SPRs > > > during the execution of the stop instruction. Does this sound ok ? > > > > > > (Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual > > > accessible from > > > https://openpowerfoundation.org/?resource_lib=power9-processor-users-manua > > > l) > > > > I was thinking of a boot time test case built into linux. linux has some > > boot > > time test cases which you can enable via CONFIG options. > > > > Firstly you could see if an SPR exists using the same trick xmon does in > > dump_one_spr(). Then once you have a list of usable SPRs, you could write > > all > > the known ones (I assume you'd have to leave out some, like the PSSCR), then > > set > > Write what value? > > Ideally you want to write a random bit pattern to reduce the chance > that only some bits are being restored. The xmon dump_one_spr() trick tries to work around that by writing one random value and then a different one to see if it really is a nop. > But you can't do that because writing a value to an SPRs has an effect. Sure that's a concern but xmon seems to get away with it. > Some of them might even need to be zero, in which case you can't really > distinguish that from a non-restored zero. It doesn't need to be perfect. It just needs to catch more than we have now. > > the appropriate stop level, make sure you got into that stop level, and then > > see > > if that register was changed. Then you'd have an automated list of registers > > you > > need to make sure you save/restore at each stop level. > > > > Could something like that work? > > Maybe. > > Ignoring the problem of whether you can write a meaningful value to some > of the SPRs, I'm not entirely convinced it's going to work. But maybe > I'm wrong. Yeah, I'm not convinced it'll work either but it would be a nice piece of test infrastructure to have if it does work. We'd still need to marry up the SPR numbers we get from the test to what's actually being restored in Linux. > But there's a much simpler solution, we should 1) have a selftest for > getcpu() and 2) we should be running the glibc (I think?) test suite > that found this in the first place. It's frankly embarrassing that we > didn't find this. Yeah, we should do that also, but how do we catch the next SPR we are missing. I'd like some systematic way of doing that rather than wack-a-mole. Mikey
Michael Neuling <mikey@neuling.org> writes: > On Fri, 2018-07-20 at 12:32 +1000, Michael Ellerman wrote: >> Michael Neuling <mikey@neuling.org> writes: >> > On Wed, 2018-07-18 at 13:42 +0530, Gautham R Shenoy wrote: >> > > On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote: >> > > > >> > > > > DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER); >> > > > > diff --git a/arch/powerpc/kernel/idle_book3s.S >> > > > > b/arch/powerpc/kernel/idle_book3s.S >> > > > > index d85d551..5069d42 100644 >> > > > > --- a/arch/powerpc/kernel/idle_book3s.S >> > > > > +++ b/arch/powerpc/kernel/idle_book3s.S >> > > > > @@ -120,6 +120,9 @@ power9_save_additional_sprs: >> > > > > mfspr r4, SPRN_MMCR2 >> > > > > std r3, STOP_MMCR1(r13) >> > > > > std r4, STOP_MMCR2(r13) >> > > > > + >> > > > > + mfspr r3, SPRN_SPRG3 >> > > > > + std r3, STOP_SPRG3(r13) >> > > > >> > > > We don't need to save it. Just restore it from paca->sprg_vdso which >> > > > should >> > > > never change. >> > > >> > > Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso. >> > > >> > > > >> > > > How can we do better at catching these missing SPRGs? >> > > >> > > We can go through the list of SPRs from the POWER9 User Manual and >> > > document explicitly why we don't have to save/restore certain SPRs >> > > during the execution of the stop instruction. Does this sound ok ? >> > > >> > > (Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual >> > > accessible from >> > > https://openpowerfoundation.org/?resource_lib=power9-processor-users-manua >> > > l) >> > >> > I was thinking of a boot time test case built into linux. linux has some >> > boot >> > time test cases which you can enable via CONFIG options. >> > >> > Firstly you could see if an SPR exists using the same trick xmon does in >> > dump_one_spr(). Then once you have a list of usable SPRs, you could write >> > all >> > the known ones (I assume you'd have to leave out some, like the PSSCR), then >> > set >> >> Write what value? >> >> Ideally you want to write a random bit pattern to reduce the chance >> that only some bits are being restored. > > The xmon dump_one_spr() trick tries to work around that by writing one random > value and then a different one to see if it really is a nop. > >> But you can't do that because writing a value to an SPRs has an effect. > > Sure that's a concern but xmon seems to get away with it. I don't think it writes, but maybe I'm reading the code wrong. Writing a random value to the MSR could be fun :) >> Some of them might even need to be zero, in which case you can't really >> distinguish that from a non-restored zero. > > It doesn't need to be perfect. It just needs to catch more than we have now. Sure. >> > the appropriate stop level, make sure you got into that stop level, and then >> > see >> > if that register was changed. Then you'd have an automated list of registers >> > you >> > need to make sure you save/restore at each stop level. >> > >> > Could something like that work? >> >> Maybe. >> >> Ignoring the problem of whether you can write a meaningful value to some >> of the SPRs, I'm not entirely convinced it's going to work. But maybe >> I'm wrong. > > Yeah, I'm not convinced it'll work either but it would be a nice piece of test > infrastructure to have if it does work. Yeah I guess I'd rather we worked on 1) and 2) below first :) > We'd still need to marry up the SPR numbers we get from the test to what's > actually being restored in Linux. > >> But there's a much simpler solution, we should 1) have a selftest for >> getcpu() and 2) we should be running the glibc (I think?) test suite >> that found this in the first place. It's frankly embarrassing that we >> didn't find this. > > Yeah, we should do that also, but how do we catch the next SPR we are missing. > I'd like some systematic way of doing that rather than wack-a-mole. Whack-a-mole 😂😂😂😂 We could also improve things by documenting how each SPR is handled, eg. is it saved/restored across idle, syscall, KVM etc. And possibly that could even become code that defines how SPRs are handled, rather than it all being done ad-hoc. cheers
On Fri, 2018-07-20 at 16:29 +1000, Michael Ellerman wrote: > Michael Neuling <mikey@neuling.org> writes: > > On Fri, 2018-07-20 at 12:32 +1000, Michael Ellerman wrote: > > > Michael Neuling <mikey@neuling.org> writes: > > > > On Wed, 2018-07-18 at 13:42 +0530, Gautham R Shenoy wrote: > > > > > On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote: > > > > > > > > > > > > > DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER); > > > > > > > diff --git a/arch/powerpc/kernel/idle_book3s.S > > > > > > > b/arch/powerpc/kernel/idle_book3s.S > > > > > > > index d85d551..5069d42 100644 > > > > > > > --- a/arch/powerpc/kernel/idle_book3s.S > > > > > > > +++ b/arch/powerpc/kernel/idle_book3s.S > > > > > > > @@ -120,6 +120,9 @@ power9_save_additional_sprs: > > > > > > > mfspr r4, SPRN_MMCR2 > > > > > > > std r3, STOP_MMCR1(r13) > > > > > > > std r4, STOP_MMCR2(r13) > > > > > > > + > > > > > > > + mfspr r3, SPRN_SPRG3 > > > > > > > + std r3, STOP_SPRG3(r13) > > > > > > > > > > > > We don't need to save it. Just restore it from paca->sprg_vdso > > > > > > which > > > > > > should > > > > > > never change. > > > > > > > > > > Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso. > > > > > > > > > > > > > > > > > How can we do better at catching these missing SPRGs? > > > > > > > > > > We can go through the list of SPRs from the POWER9 User Manual and > > > > > document explicitly why we don't have to save/restore certain SPRs > > > > > during the execution of the stop instruction. Does this sound ok ? > > > > > > > > > > (Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual > > > > > accessible from > > > > > https://openpowerfoundation.org/?resource_lib=power9-processor-users-m > > > > > anua > > > > > l) > > > > > > > > I was thinking of a boot time test case built into linux. linux has some > > > > boot > > > > time test cases which you can enable via CONFIG options. > > > > > > > > Firstly you could see if an SPR exists using the same trick xmon does in > > > > dump_one_spr(). Then once you have a list of usable SPRs, you could > > > > write > > > > all > > > > the known ones (I assume you'd have to leave out some, like the PSSCR), > > > > then > > > > set > > > > > > Write what value? > > > > > > Ideally you want to write a random bit pattern to reduce the chance > > > that only some bits are being restored. > > > > The xmon dump_one_spr() trick tries to work around that by writing one > > random > > value and then a different one to see if it really is a nop. > > > > > But you can't do that because writing a value to an SPRs has an effect. > > > > Sure that's a concern but xmon seems to get away with it. > > I don't think it writes, but maybe I'm reading the code wrong. You're right, sorry. It's the write the GPR that becomes a NOP when the SPR is not there. I misremembered how it worked. Maybe that won't work stop since we'd need to be able change the SPR value to ensure we don't hit the reset value after a stop state. We'd be able to detect SPRs that that change from it's reset value but not those that are already at their reset value. > Writing a random value to the MSR could be fun :) Fortunately the MSR is not an SPR :-P > > > > Yeah, I'm not convinced it'll work either but it would be a nice piece of > > test > > infrastructure to have if it does work. > > Yeah I guess I'd rather we worked on 1) and 2) below first :) ok > > We'd still need to marry up the SPR numbers we get from the test to what's > > actually being restored in Linux. > > > > > But there's a much simpler solution, we should 1) have a selftest for > > > getcpu() and 2) we should be running the glibc (I think?) test suite > > > that found this in the first place. It's frankly embarrassing that we > > > didn't find this. > > > > Yeah, we should do that also, but how do we catch the next SPR we are > > missing. > > I'd like some systematic way of doing that rather than wack-a-mole. > > Whack-a-mole 😂😂😂😂 I preferred waking them :-) > We could also improve things by documenting how each SPR is handled, eg. > is it saved/restored across idle, syscall, KVM etc. And possibly that > could even become code that defines how SPRs are handled, rather than it > all being done ad-hoc. Yeah. It's complicated by linux calling opal_slw_set_reg() to change what's saved. This was part of the reason I'd hoped doing a linux test case would help as we could do it after those calls. Mikey
diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h index e210a83..03fa904 100644 --- a/arch/powerpc/include/asm/cpuidle.h +++ b/arch/powerpc/include/asm/cpuidle.h @@ -77,6 +77,7 @@ struct stop_sprs { u64 mmcr1; u64 mmcr2; u64 mmcra; + u64 sprg3; }; extern u32 pnv_fastsleep_workaround_at_entry[]; diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c index 89cf155..a35ebfc 100644 --- a/arch/powerpc/kernel/asm-offsets.c +++ b/arch/powerpc/kernel/asm-offsets.c @@ -776,6 +776,7 @@ int main(void) STOP_SPR(STOP_MMCR1, mmcr1); STOP_SPR(STOP_MMCR2, mmcr2); STOP_SPR(STOP_MMCRA, mmcra); + STOP_SPR(STOP_SPRG3, sprg3); #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 d85d551..5069d42 100644 --- a/arch/powerpc/kernel/idle_book3s.S +++ b/arch/powerpc/kernel/idle_book3s.S @@ -120,6 +120,9 @@ power9_save_additional_sprs: mfspr r4, SPRN_MMCR2 std r3, STOP_MMCR1(r13) std r4, STOP_MMCR2(r13) + + mfspr r3, SPRN_SPRG3 + std r3, STOP_SPRG3(r13) blr power9_restore_additional_sprs: @@ -144,7 +147,9 @@ power9_restore_additional_sprs: mtspr SPRN_MMCR1, r4 ld r3, STOP_MMCR2(r13) + ld r4, STOP_SPRG3(r13) mtspr SPRN_MMCR2, r3 + mtspr SPRN_SPRG3, r4 blr /*