From: Gautham R. Shenoy <hidden> Date: 2015-08-04 08:31:14
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create a function name update_hid0() which follows this recipe and
invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/kvm_ppc.h | 11 +++++++++++
arch/powerpc/platforms/powernv/subcore.c | 4 ++--
2 files changed, 13 insertions(+), 2 deletions(-)
@@ -227,7 +227,7 @@ static void split_core(int new_mode)/* Write new mode */hid0=mfspr(SPRN_HID0);hid0|=HID0_POWER8_DYNLPARDIS|split_parms[i].value;-mtspr(SPRN_HID0,hid0);+update_hid0(hid0);update_hid_in_slw(hid0);/* Wait for it to happen */
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-08-04 10:09:01
On Tue, 2015-04-08 at 08:30:58 UTC, "Gautham R. Shenoy" wrote:
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create a function name update_hid0() which follows this recipe and
invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/kvm_ppc.h | 11 +++++++++++
Why is it in there? It's not KVM related per se.
Where should it go? I think reg.h would be best, ideally near the definition
for HID0, though that's probably not possible because of ASSEMBLY requirements.
So at the bottom of reg.h ?
@@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)externvoidxics_wake_cpu(intcpu);+staticinlinevoidupdate_hid0(unsignedlonghid0)+{+/*+*TheHID0updateshouldattheveryleastbeprecededbya+*aSYNCinstructionfollowedbyanISYNCinstruction+*/+mb();+mtspr(SPRN_HID0,hid0);+isync();
That's going to turn into three separate inline asm blocks, which is maybe a
bit unfortunate. Have you checked the generated code is what we want, ie. just
sync, mtspr, isync ?
cheers
From: Gautham R Shenoy <hidden> Date: 2015-08-04 10:57:57
Hi Michael,
On Tue, Aug 04, 2015 at 08:08:58PM +1000, Michael Ellerman wrote:
On Tue, 2015-04-08 at 08:30:58 UTC, "Gautham R. Shenoy" wrote:
quoted
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create a function name update_hid0() which follows this recipe and
invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/kvm_ppc.h | 11 +++++++++++
Why is it in there? It's not KVM related per se.
Ok. Will fix this.
Where should it go? I think reg.h would be best, ideally near the definition
for HID0, though that's probably not possible because of ASSEMBLY requirements.
So at the bottom of reg.h ?
@@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)externvoidxics_wake_cpu(intcpu);+staticinlinevoidupdate_hid0(unsignedlonghid0)+{+/*+*TheHID0updateshouldattheveryleastbeprecededbya+*aSYNCinstructionfollowedbyanISYNCinstruction+*/+mb();+mtspr(SPRN_HID0,hid0);+isync();
That's going to turn into three separate inline asm blocks, which is maybe a
bit unfortunate. Have you checked the generated code is what we want, ie. just
sync, mtspr, isync ?
Yes, the objdump of subcore.o shows exactly these three instructions:
7c 00 04 ac sync
7c 70 fb a6 mtspr 1008,r3
4c 00 01 2c isync
From: Gautham R. Shenoy <hidden> Date: 2015-08-04 11:06:45
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create an inline function name update_hid0() which follows this recipe
and invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
[v1--> v2: Moved defn of update_hid0 to reg.h from kvm_ppc.h]
arch/powerpc/include/asm/reg.h | 13 +++++++++++++
arch/powerpc/platforms/powernv/subcore.c | 4 ++--
2 files changed, 15 insertions(+), 2 deletions(-)
@@ -12,6 +12,8 @@#include<linux/stringify.h>#include<asm/cputable.h>+#include<asm/barrier.h>+#include<asm/synch.h>/* Pickup Book E specific registers. */#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
@@ -227,7 +227,7 @@ static void split_core(int new_mode)/* Write new mode */hid0=mfspr(SPRN_HID0);hid0|=HID0_POWER8_DYNLPARDIS|split_parms[i].value;-mtspr(SPRN_HID0,hid0);+update_hid0(hid0);update_hid_in_slw(hid0);/* Wait for it to happen */
On Tuesday 04 August 2015 03:38 PM, Michael Ellerman wrote:
On Tue, 2015-04-08 at 08:30:58 UTC, "Gautham R. Shenoy" wrote:
quoted
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create a function name update_hid0() which follows this recipe and
invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/kvm_ppc.h | 11 +++++++++++
Why is it in there? It's not KVM related per se.
Where should it go? I think reg.h would be best, ideally near the definition
for HID0, though that's probably not possible because of ASSEMBLY requirements.
So at the bottom of reg.h ?
just to understand, Something like this will not do?
#define update_hid0(x) __asm__ __volatile__(
"sync\n"\
"mtspr "
__stringify(SPRN_HID0)", %0\n"\
"isync"::"r"(x));
Maddy
@@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)externvoidxics_wake_cpu(intcpu);+staticinlinevoidupdate_hid0(unsignedlonghid0)+{+/*+*TheHID0updateshouldattheveryleastbeprecededbya+*aSYNCinstructionfollowedbyanISYNCinstruction+*/+mb();+mtspr(SPRN_HID0,hid0);+isync();
That's going to turn into three separate inline asm blocks, which is maybe a
bit unfortunate. Have you checked the generated code is what we want, ie. just
sync, mtspr, isync ?
cheers
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-08-05 02:00:10
On Tue, 2015-08-04 at 19:36 +0530, Madhavan Srinivasan wrote:
On Tuesday 04 August 2015 03:38 PM, Michael Ellerman wrote:
quoted
On Tue, 2015-04-08 at 08:30:58 UTC, "Gautham R. Shenoy" wrote:
quoted
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create a function name update_hid0() which follows this recipe and
invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/kvm_ppc.h | 11 +++++++++++
Why is it in there? It's not KVM related per se.
Where should it go? I think reg.h would be best, ideally near the definition
for HID0, though that's probably not possible because of ASSEMBLY requirements.
So at the bottom of reg.h ?
just to understand, Something like this will not do?
#define update_hid0(x) __asm__ __volatile__( "sync\n"\
"mtspr " __stringify(SPRN_HID0)", %0\n"\
"isync"::"r"(x));
Yeah we could do that also.
The static inline is less ugly though.
cheers
On Tue, Aug 04, 2015 at 08:08:58PM +1000, Michael Ellerman wrote:
quoted
+static inline void update_hid0(unsigned long hid0)
+{
+ /*
+ * The HID0 update should at the very least be preceded by a
+ * a SYNC instruction followed by an ISYNC instruction
+ */
+ mb();
+ mtspr(SPRN_HID0, hid0);
+ isync();
That's going to turn into three separate inline asm blocks, which is maybe a
bit unfortunate. Have you checked the generated code is what we want, ie. just
sync, mtspr, isync ?
The "mb()" is not such a great name anyway: you don't want a memory
barrier, you want an actual sync instruction ("sync 0", "hwsync",
whatever the currently preferred spelling is).
The function name should also say this is for POWER8 (the required
sequences are different for some other processors; and some others
might not even _have_ a HID0, or not at 1008). power8_write_hid0
or such?
For writing it as one asm, why not just
asm volatile("sync ; mtspr %0,%1 ; isync" : : "i"(SPRN_HID0), "r"(hid0));
instead of the stringify stuff?
Segher
From: Gautham R Shenoy <hidden> Date: 2015-08-05 06:54:10
Hi Segher,
Thanks for the suggestions. I will rename the function to
update_power8_hid0() and use asm volatile.
On Tue, Aug 04, 2015 at 09:30:57PM -0500, Segher Boessenkool wrote:
On Tue, Aug 04, 2015 at 08:08:58PM +1000, Michael Ellerman wrote:
quoted
quoted
+static inline void update_hid0(unsigned long hid0)
+{
+ /*
+ * The HID0 update should at the very least be preceded by a
+ * a SYNC instruction followed by an ISYNC instruction
+ */
+ mb();
+ mtspr(SPRN_HID0, hid0);
+ isync();
That's going to turn into three separate inline asm blocks, which is maybe a
bit unfortunate. Have you checked the generated code is what we want, ie. just
sync, mtspr, isync ?
The "mb()" is not such a great name anyway: you don't want a memory
barrier, you want an actual sync instruction ("sync 0", "hwsync",
whatever the currently preferred spelling is).
The function name should also say this is for POWER8 (the required
sequences are different for some other processors; and some others
might not even _have_ a HID0, or not at 1008). power8_write_hid0
or such?
For writing it as one asm, why not just
asm volatile("sync ; mtspr %0,%1 ; isync" : : "i"(SPRN_HID0), "r"(hid0));
instead of the stringify stuff?
Segher
From: Gautham R. Shenoy <hidden> Date: 2015-08-05 07:08:51
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create an inline function name update_power8_hid0() which follows this
recipe and invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
[v1 --> v2: Moved defn of update_hid0 to reg.h from kvm_ppc.h]
[v2 --> v3: Renamed to update_power8_hid0 and used asm volatile]
arch/powerpc/include/asm/reg.h | 9 +++++++++
arch/powerpc/platforms/powernv/subcore.c | 4 ++--
2 files changed, 11 insertions(+), 2 deletions(-)
@@ -227,7 +227,7 @@ static void split_core(int new_mode)/* Write new mode */hid0=mfspr(SPRN_HID0);hid0|=HID0_POWER8_DYNLPARDIS|split_parms[i].value;-mtspr(SPRN_HID0,hid0);+update_power8_hid0(hid0);update_hid_in_slw(hid0);/* Wait for it to happen */
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2015-08-09 02:30:23
On Tue, 2015-08-04 at 20:08 +1000, Michael Ellerman wrote:
On Tue, 2015-04-08 at 08:30:58 UTC, "Gautham R. Shenoy" wrote:
quoted
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create a function name update_hid0() which follows this recipe and
invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/kvm_ppc.h | 11 +++++++++++
Why is it in there? It's not KVM related per se.
Where should it go? I think reg.h would be best, ideally near the definition
for HID0, though that's probably not possible because of ASSEMBLY requirements.
So at the bottom of reg.h ?
@@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)externvoidxics_wake_cpu(intcpu);+staticinlinevoidupdate_hid0(unsignedlonghid0)+{+/*+*TheHID0updateshouldattheveryleastbeprecededbya+*aSYNCinstructionfollowedbyanISYNCinstruction+*/+mb();+mtspr(SPRN_HID0,hid0);+isync();
That's going to turn into three separate inline asm blocks, which is maybe a
bit unfortunate. Have you checked the generated code is what we want, ie. just
sync, mtspr, isync ?
It depends on the processor, I'd rather we make this out of line in
misc.S or similar and use the appropriate CPU table bits and pieces.
Some older CPUs require whacking it N times for example.
Cheers,
Ben.
From: Sam Bobroff <hidden> Date: 2015-08-14 04:55:41
On Wed, Aug 05, 2015 at 12:38:31PM +0530, Gautham R. Shenoy wrote:
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create an inline function name update_power8_hid0() which follows this
recipe and invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
Hi Gautham,
I've tested this on a Power 8 machine and verified that it is able to change
split modes and that when doing so the new code is used.
Reviewed-by: Sam Bobroff <redacted>
Tested-by: Sam Bobroff <redacted>
From: Shreyas B Prabhu <hidden> Date: 2015-08-14 09:00:36
On 08/05/2015 12:38 PM, Gautham R. Shenoy wrote:
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create an inline function name update_power8_hid0() which follows this
recipe and invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-08-17 08:03:34
On Wed, 2015-05-08 at 07:08:31 UTC, "Gautham R. Shenoy" wrote:
Section 3.7 of Version 1.2 of the Power8 Processor User's Manual
prescribes that updates to HID0 be preceded by a SYNC instruction and
followed by an ISYNC instruction (Page 91).
Create an inline function name update_power8_hid0() which follows this
recipe and invoke it from the static split core path.
Signed-off-by: Gautham R. Shenoy <redacted>
Reviewed-by: Sam Bobroff <redacted>
Tested-by: Sam Bobroff <redacted>