From: Gautham R. Shenoy <hidden> Date: 2018-07-17 11:27:50
From: "Gautham R. Shenoy" <redacted>
On 64-bit Servers, SPRN_SPRG3 and its userspace read-only mirror
SPRN_USPRG3 are used as userspace VDSO write and read registers
respectively.
SPRN_SPRG3 is lost when we enter stop4 and above, and is currently not
restored. As a result, any read from SPRN_USPRG3 returns zero on an
exit from stop4 and above.
Thus in this situation, any call from sched_getcpu() always returns
zero, as on powerpc, we call __kernel_getcpu() which relies upon
SPRN_USPRG3 to report the CPU and NUMA node information.
Fix this by saving the SPRN_SPRG3 before entering a deep stop state,
and restoring it back on wakeup from the stop state.
Reported-by: Florian Weimer <redacted>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Neuling <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Vaidyanathan Srinivasan <redacted>
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/cpuidle.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/idle_book3s.S | 5 +++++
3 files changed, 7 insertions(+)
From: Gautham R Shenoy <hidden> Date: 2018-07-17 11:47:33
On Tue, Jul 17, 2018 at 04:57:29PM +0530, Gautham R. Shenoy wrote:
From: "Gautham R. Shenoy" <redacted>
On 64-bit Servers, SPRN_SPRG3 and its userspace read-only mirror
SPRN_USPRG3 are used as userspace VDSO write and read registers
respectively.
SPRN_SPRG3 is lost when we enter stop4 and above, and is currently not
restored. As a result, any read from SPRN_USPRG3 returns zero on an
exit from stop4 and above.
Thus in this situation, any call from sched_getcpu() always returns
This happens only on POWER9.
zero, as on powerpc, we call __kernel_getcpu() which relies upon
SPRN_USPRG3 to report the CPU and NUMA node information.
Fix this by saving the SPRN_SPRG3 before entering a deep stop state,
and restoring it back on wakeup from the stop state.
I forgot to add that this fixes commit e1c1cfed5432 ("powerpc/powernv:
Save/Restore additional SPRs for stop4 cpuidle").
@mpe, do you want me to send fresh patch with these updates ?
From: Gautham R. Shenoy <hidden> Date: 2018-07-17 16:01:05
From: "Gautham R. Shenoy" <redacted>
On 64-bit servers, SPRN_SPRG3 and its userspace read-only mirror
SPRN_USPRG3 are used as userspace VDSO write and read registers
respectively.
SPRN_SPRG3 is lost when we enter stop4 and above, and is currently not
restored. As a result, any read from SPRN_USPRG3 returns zero on an
exit from stop4 and above.
Thus in this situation, on POWER9, any call from sched_getcpu() always
returns zero, as on powerpc, we call __kernel_getcpu() which relies
upon SPRN_USPRG3 to report the CPU and NUMA node information.
Fix this by saving the SPRN_SPRG3 before entering a deep stop state,
and restoring it back on wakeup from the stop state.
Fixes: e1c1cfed5432 ("powerpc/powernv: Save/Restore additional SPRs
for stop4 cpuidle")
Reported-by: Florian Weimer <redacted>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Neuling <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Vaidyanathan Srinivasan <redacted>
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/cpuidle.h | 1 +
arch/powerpc/kernel/asm-offsets.c | 1 +
arch/powerpc/kernel/idle_book3s.S | 5 +++++
3 files changed, 7 insertions(+)
We don't need to save it. Just restore it from paca->sprg_vdso which shoul=
d
never change.
How can we do better at catching these missing SPRGs?=20
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 a=
re
missing, rather than playing wake-a-mole.
Mikey=20
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.
=20
We don't need to save it. Just restore it from paca->sprg_vdso which s=
hould
quoted
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 and
document explicitly why we don't have to save/restore certain SPRs
during the execution of the stop instruction. Does this sound ok ?
=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-ma=
nual)
I was thinking of a boot time test case built into linux. linux has some bo=
ot
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 a=
ll
the known ones (I assume you'd have to leave out some, like the PSSCR), the=
n set
the appropriate stop level, make sure you got into that stop level, and the=
n see
if that register was changed. Then you'd have an automated list of register=
s you
need to make sure you save/restore at each stop level.
Could something like that work?
Mikey
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.
quoted
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
=20
We don't need to save it. Just restore it from paca->sprg_vdso whi=
ch
quoted
quoted
quoted
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 and
document explicitly why we don't have to save/restore certain SPRs
during the execution of the stop instruction. Does this sound ok ?
=20
(Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual
accessible from
https://openpowerfoundation.org/?resource_lib=3Dpower9-processor-user=
s-manua
quoted
quoted
l)
=20
I was thinking of a boot time test case built into linux. linux has som=
e
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 does i=
n
quoted
dump_one_spr(). Then once you have a list of usable SPRs, you could wri=
te
quoted
all
the known ones (I assume you'd have to leave out some, like the PSSCR),=
then
quoted
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.
The xmon dump_one_spr() trick tries to work around that by writing one rand=
om
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=
.
quoted
the appropriate stop level, make sure you got into that stop level, and=
then
quoted
see
if that register was changed. Then you'd have an automated list of regi=
sters
quoted
you
need to make sure you save/restore at each stop level.
=20
Could something like that work?
=20
Maybe.
=20
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 t=
est
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 missi=
ng.
I'd like some systematic way of doing that rather than wack-a-mole.
Mikey
=20
We don't need to save it. Just restore it from paca->sprg_vdso wh=
ich
quoted
quoted
quoted
quoted
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 and
document explicitly why we don't have to save/restore certain SPRs
during the execution of the stop instruction. Does this sound ok ?
=20
(Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual
accessible from
https://openpowerfoundation.org/?resource_lib=3Dpower9-processor-use=
rs-manua
quoted
quoted
quoted
l)
=20
I was thinking of a boot time test case built into linux. linux has so=
me
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 does =
in
quoted
quoted
dump_one_spr(). Then once you have a list of usable SPRs, you could wr=
ite
quoted
quoted
all
the known ones (I assume you'd have to leave out some, like the PSSCR)=
, then
quoted
quoted
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.
The xmon dump_one_spr() trick tries to work around that by writing one ra=
ndom
value and then a different one to see if it really is a nop.
quoted
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 :)
quoted
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 n=
ow.
Sure.
quoted
quoted
the appropriate stop level, make sure you got into that stop level, an=
d then
quoted
quoted
see
if that register was changed. Then you'd have an automated list of reg=
isters
quoted
quoted
you
need to make sure you save/restore at each stop level.
=20
Could something like that work?
=20
Maybe.
=20
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.
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.
Yeah, we should do that also, but how do we catch the next SPR we are mis=
sing.
I'd like some systematic way of doing that rather than wack-a-mole.
Whack-a-mole =F0=9F=98=82=F0=9F=98=82=F0=9F=98=82=F0=9F=98=82
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
=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.
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