Re: [RESEND][PATCH] powerpc/powernv : Save/Restore SPRG3 on entry/exit from stop.

On Fri, 2018-07-20 at 16:29 +1000, Michael Ellerman wrote:

Michael Neuling [off-list ref] writes:

quoted

On Fri, 2018-07-20 at 12:32 +1000, Michael Ellerman wrote:

quoted

Michael Neuling [off-list ref] writes:

quoted

On Wed, 2018-07-18 at 13:42 +0530, Gautham R Shenoy wrote:

quoted

On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote:

quoted

=20

quoted

 	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)

=20
We don't need to save it.  Just restore it from paca->sprg_vdso
which
should
never change.

=20
Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso.
=20

quoted

=20
How can we do better at catching these missing SPRGs?

=20
We can go through the list of SPRs from the POWER9 User Manual an=

d

quoted

quoted

quoted

quoted

document explicitly why we don't have to save/restore certain SPR=

s

quoted

quoted

quoted

quoted

during the execution of the stop instruction. Does this sound ok =

?

quoted

quoted

quoted

quoted

=20
(Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual
accessible from
https://openpowerfoundation.org/?resource_lib=3Dpower9-processor-=

users-m

quoted

quoted

quoted

quoted

anua
l)

=20
I was thinking of a boot time test case built into linux. linux has=

 some

quoted

quoted

quoted

boot
time test cases which you can enable via CONFIG options.
=20
Firstly you could see if an SPR exists using the same trick xmon do=

es in

quoted

quoted

quoted

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 PSS=

CR),

quoted

quoted

quoted

then
set

=20
Write what value?
=20
Ideally you want to write a random bit pattern to reduce the chance
that only some bits are being restored.

=20
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.
=20

quoted

But you can't do that because writing a value to an SPRs has an effec=

t.

quoted

=20
Sure that's a concern but xmon seems to get away with it.

=20
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.=20

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.=20

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

quoted

=20
Yeah, I'm not convinced it'll work either but it would be a nice piece =

of

quoted

test
infrastructure to have if it does work.

=20
Yeah I guess I'd rather we worked on 1) and 2) below first :)

ok

quoted

We'd still need to marry up the SPR numbers we get from the test to wha=

t's

quoted

actually being restored in Linux.
=20

quoted

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.

=20
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.

=20
Whack-a-mole =F0=9F=98=82=F0=9F=98=82=F0=9F=98=82=F0=9F=98=82

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