Messages in this thread | | | From | Michael Ellerman <> | Subject | Re: [RESEND][PATCH] powerpc/powernv : Save/Restore SPRG3 on entry/exit from stop. | Date | Fri, 20 Jul 2018 16:29:19 +1000 |
| |
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
| |