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

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

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

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 <fweimer@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 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(+)

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
 
 /*
-- 
1.9.4

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

[Gautham R Shenoy]

On Tue, Jul 17, 2018 at 04:57:29PM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> 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 ?


> Reported-by: Florian Weimer <fweimer@redhat.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Michael Neuling <mikey@neuling.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  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(+)
> 
> 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
> 
>  /*
> -- 
> 1.9.4
> 

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

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

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 <fweimer@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 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(+)

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
 
 /*
-- 
1.9.4

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

[Michael Neuling]

>  	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
>  
>  /*

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

[Michael Neuling]

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

>  	blr
> =20
>  power9_restore_additional_sprs:
> @@ -144,7 +147,9 @@ power9_restore_additional_sprs:
>  	mtspr	SPRN_MMCR1, r4
> =20
>  	ld	r3, STOP_MMCR2(r13)
> +	ld	r4, STOP_SPRG3(r13)
>  	mtspr	SPRN_MMCR2, r3
> +	mtspr	SPRN_SPRG3, r4
>  	blr
> =20
>  /*

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

[Gautham R Shenoy]

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
> >  
> >  /*

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

[Michael Neuling]

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

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

[Michael Neuling]

On Wed, 2018-07-18 at 13:42 +0530, Gautham R Shenoy wrote:
> Hello Mikey,
>=20
> On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote:
> >=20
> > >  	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 s=
hould
> > never change.
>=20
> Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso.
>=20
> >=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

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

[Michael Ellerman]

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

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

[Michael Neuling]

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

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

[Michael Neuling]

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:
> > > >=20
> > > > >  	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 whi=
ch
> > > > should
> > > > never change.
> > >=20
> > > Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso.
> > >=20
> > > >=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
> > > l)
> >=20
> > I was thinking of a boot time test case built into linux. linux has som=
e
> > 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
> > dump_one_spr(). Then once you have a list of usable SPRs, you could wri=
te
> > all
> > the known ones (I assume you'd have to leave out some, like the PSSCR),=
 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.

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

> > 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 regi=
sters
> > 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

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

[Michael Ellerman]

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

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

[Michael Ellerman]

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:
>> > > >=20
>> > > > >  	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 wh=
ich
>> > > > should
>> > > > never change.
>> > >=20
>> > > Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso.
>> > >=20
>> > > >=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
>> > > l)
>> >=20
>> > I was thinking of a boot time test case built into linux. linux has so=
me
>> > 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
>> > dump_one_spr(). Then once you have a list of usable SPRs, you could wr=
ite
>> > all
>> > the known ones (I assume you'd have to leave out some, like the PSSCR)=
, 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.
>
> 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.
>
>> 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 n=
ow.

Sure.

>> > the appropriate stop level, make sure you got into that stop level, an=
d then
>> > see
>> > if that register was changed. Then you'd have an automated list of reg=
isters
>> > 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.
>
>> 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

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

[Michael Neuling]

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

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

[Michael Neuling]

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:
> > > > > >=20
> > > > > > >  	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
> > > > > >=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
> > > > > document explicitly why we don't have to save/restore certain SPR=
s
> > > > > 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-m
> > > > > anua
> > > > > l)
> > > >=20
> > > > 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.
> > > >=20
> > > > Firstly you could see if an SPR exists using the same trick xmon do=
es 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 PSS=
CR),
> > > > 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
> > > But you can't do that because writing a value to an SPRs has an effec=
t.
> >=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

> >=20
> > 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.
>=20
> 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 wha=
t's
> > actually being restored in Linux.
> >=20
> > > 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