All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powerpc: Add an inline function to update HID0
@ 2015-08-04  8:30 Gautham R. Shenoy
  2015-08-04 10:08 ` Michael Ellerman
  0 siblings, 1 reply; 14+ messages in thread
From: Gautham R. Shenoy @ 2015-08-04  8:30 UTC (permalink / raw)
  To: Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev, mikey
  Cc: Gautham R. Shenoy

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 <ego@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/kvm_ppc.h       | 11 +++++++++++
 arch/powerpc/platforms/powernv/subcore.c |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index c6ef05b..325f1d6 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)
 
 extern void xics_wake_cpu(int cpu);
 
+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();
+}
+
 #endif /* __POWERPC_KVM_PPC_H__ */
diff --git a/arch/powerpc/platforms/powernv/subcore.c b/arch/powerpc/platforms/powernv/subcore.c
index f60f80a..37e77bf 100644
--- a/arch/powerpc/platforms/powernv/subcore.c
+++ b/arch/powerpc/platforms/powernv/subcore.c
@@ -190,7 +190,7 @@ static void unsplit_core(void)
 
 	hid0 = mfspr(SPRN_HID0);
 	hid0 &= ~HID0_POWER8_DYNLPARDIS;
-	mtspr(SPRN_HID0, hid0);
+	update_hid0(hid0);
 	update_hid_in_slw(hid0);
 
 	while (mfspr(SPRN_HID0) & mask)
@@ -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 */
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
  2015-08-04  8:30 [PATCH] powerpc: Add an inline function to update HID0 Gautham R. Shenoy
@ 2015-08-04 10:08 ` Michael Ellerman
  2015-08-04 10:57   ` Gautham R Shenoy
                     ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Michael Ellerman @ 2015-08-04 10:08 UTC (permalink / raw)
  To: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev, mikey
  Cc: Gautham R. Shenoy

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 <ego@linux.vnet.ibm.com>
> ---
>  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 ?

> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index c6ef05b..325f1d6 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)
>  
>  extern void xics_wake_cpu(int cpu);
>  
> +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 ?

cheers

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
  2015-08-04 10:08 ` Michael Ellerman
@ 2015-08-04 10:57   ` Gautham R Shenoy
  2015-08-04 11:06     ` [PATCH v2] " Gautham R. Shenoy
  2015-08-04 14:06   ` Madhavan Srinivasan
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Gautham R Shenoy @ 2015-08-04 10:57 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev, mikey

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:
> > 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 <ego@linux.vnet.ibm.com>
> > ---
> >  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 ?
> 
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> > index c6ef05b..325f1d6 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)
> >  
> >  extern void xics_wake_cpu(int cpu);
> >  
> > +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 ?
> 

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

> cheers
>
--
Thanks and Regards
gautham. 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] powerpc: Add an inline function to update HID0
  2015-08-04 10:57   ` Gautham R Shenoy
@ 2015-08-04 11:06     ` Gautham R. Shenoy
  0 siblings, 0 replies; 14+ messages in thread
From: Gautham R. Shenoy @ 2015-08-04 11:06 UTC (permalink / raw)
  To: Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev, mikey
  Cc: Gautham R. Shenoy

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 <ego@linux.vnet.ibm.com>
---
[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(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index af56b5c..beb98ac 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -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)
@@ -1281,6 +1283,17 @@ struct pt_regs;
 
 extern void ppc_save_regs(struct pt_regs *regs);
 
+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();
+}
+
 #endif /* __ASSEMBLY__ */
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_REG_H */
diff --git a/arch/powerpc/platforms/powernv/subcore.c b/arch/powerpc/platforms/powernv/subcore.c
index f60f80a..37e77bf 100644
--- a/arch/powerpc/platforms/powernv/subcore.c
+++ b/arch/powerpc/platforms/powernv/subcore.c
@@ -190,7 +190,7 @@ static void unsplit_core(void)
 
 	hid0 = mfspr(SPRN_HID0);
 	hid0 &= ~HID0_POWER8_DYNLPARDIS;
-	mtspr(SPRN_HID0, hid0);
+	update_hid0(hid0);
 	update_hid_in_slw(hid0);
 
 	while (mfspr(SPRN_HID0) & mask)
@@ -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 */
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
  2015-08-04 10:08 ` Michael Ellerman
  2015-08-04 10:57   ` Gautham R Shenoy
@ 2015-08-04 14:06   ` Madhavan Srinivasan
  2015-08-05  2:00       ` Michael Ellerman
  2015-08-05  2:30   ` Segher Boessenkool
  2015-08-09  2:29   ` powerpc: Add an inline function to update HID0 Benjamin Herrenschmidt
  3 siblings, 1 reply; 14+ messages in thread
From: Madhavan Srinivasan @ 2015-08-04 14:06 UTC (permalink / raw)
  To: Michael Ellerman, Gautham R. Shenoy, Paul Mackerras,
	linux-kernel, linuxppc-dev, mikey



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:
>> 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 <ego@linux.vnet.ibm.com>
>> ---
>>  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

>
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>> index c6ef05b..325f1d6 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)
>>  
>>  extern void xics_wake_cpu(int cpu);
>>  
>> +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 ?
>
> cheers
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
  2015-08-04 14:06   ` Madhavan Srinivasan
@ 2015-08-05  2:00       ` Michael Ellerman
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Ellerman @ 2015-08-05  2:00 UTC (permalink / raw)
  To: Madhavan Srinivasan
  Cc: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev, mikey

On Tue, 2015-08-04 at 19:36 +0530, Madhavan Srinivasan wrote:
> 
> 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:
> >> 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 <ego@linux.vnet.ibm.com>
> >> ---
> >>  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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
@ 2015-08-05  2:00       ` Michael Ellerman
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Ellerman @ 2015-08-05  2:00 UTC (permalink / raw)
  To: Madhavan Srinivasan
  Cc: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev, mikey

On Tue, 2015-08-04 at 19:36 +0530, Madhavan Srinivasan wrote:
> 
> 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:
> >> 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 <ego@linux.vnet.ibm.com>
> >> ---
> >>  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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
  2015-08-04 10:08 ` Michael Ellerman
  2015-08-04 10:57   ` Gautham R Shenoy
  2015-08-04 14:06   ` Madhavan Srinivasan
@ 2015-08-05  2:30   ` Segher Boessenkool
  2015-08-05  6:54     ` Gautham R Shenoy
  2015-08-09  2:29   ` powerpc: Add an inline function to update HID0 Benjamin Herrenschmidt
  3 siblings, 1 reply; 14+ messages in thread
From: Segher Boessenkool @ 2015-08-05  2:30 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev, mikey

On Tue, Aug 04, 2015 at 08:08:58PM +1000, Michael Ellerman wrote:
> > +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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
  2015-08-05  2:30   ` Segher Boessenkool
@ 2015-08-05  6:54     ` Gautham R Shenoy
  2015-08-05  7:08       ` [PATCH v3] powerpc: Add an inline function to update POWER8 HID0 Gautham R. Shenoy
  0 siblings, 1 reply; 14+ messages in thread
From: Gautham R Shenoy @ 2015-08-05  6:54 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Michael Ellerman, Gautham R. Shenoy, Paul Mackerras,
	linux-kernel, linuxppc-dev, mikey

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

--
Thanks and Regards
gautham.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3] powerpc: Add an inline function to update POWER8 HID0
  2015-08-05  6:54     ` Gautham R Shenoy
@ 2015-08-05  7:08       ` Gautham R. Shenoy
  2015-08-14  4:54         ` Sam Bobroff
                           ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Gautham R. Shenoy @ 2015-08-05  7:08 UTC (permalink / raw)
  To: Paul Mackerras, linux-kernel, linuxppc-dev, Michael Ellerman
  Cc: mikey, Segher Boessenkool, Madhavan Srinivasan, Shreyas B Prabhu,
	Gautham R. Shenoy

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 <ego@linux.vnet.ibm.com>
---
[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(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index af56b5c..1245d99 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1281,6 +1281,15 @@ struct pt_regs;
 
 extern void ppc_save_regs(struct pt_regs *regs);
 
+static inline void update_power8_hid0(unsigned long hid0)
+{
+	/*
+	 *  The HID0 update on Power8 should at the very least be
+	 *  preceded by a a SYNC instruction followed by an ISYNC
+	 *  instruction
+	 */
+	asm volatile("sync; mtspr %0,%1; isync":: "i"(SPRN_HID0), "r"(hid0));
+}
 #endif /* __ASSEMBLY__ */
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_REG_H */
diff --git a/arch/powerpc/platforms/powernv/subcore.c b/arch/powerpc/platforms/powernv/subcore.c
index f60f80a..503a73f 100644
--- a/arch/powerpc/platforms/powernv/subcore.c
+++ b/arch/powerpc/platforms/powernv/subcore.c
@@ -190,7 +190,7 @@ static void unsplit_core(void)
 
 	hid0 = mfspr(SPRN_HID0);
 	hid0 &= ~HID0_POWER8_DYNLPARDIS;
-	mtspr(SPRN_HID0, hid0);
+	update_power8_hid0(hid0);
 	update_hid_in_slw(hid0);
 
 	while (mfspr(SPRN_HID0) & mask)
@@ -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 */
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: powerpc: Add an inline function to update HID0
  2015-08-04 10:08 ` Michael Ellerman
                     ` (2 preceding siblings ...)
  2015-08-05  2:30   ` Segher Boessenkool
@ 2015-08-09  2:29   ` Benjamin Herrenschmidt
  3 siblings, 0 replies; 14+ messages in thread
From: Benjamin Herrenschmidt @ 2015-08-09  2:29 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev, mikey

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:
> > 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 <ego@linux.vnet.ibm.com>
> > ---
> >  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 ?
> 
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> > index c6ef05b..325f1d6 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -685,4 +685,15 @@ static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int ra, int rb)
> >  
> >  extern void xics_wake_cpu(int cpu);
> >  
> > +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 ?

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.

> cheers
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3] powerpc: Add an inline function to update POWER8 HID0
  2015-08-05  7:08       ` [PATCH v3] powerpc: Add an inline function to update POWER8 HID0 Gautham R. Shenoy
@ 2015-08-14  4:54         ` Sam Bobroff
  2015-08-14  8:59         ` Shreyas B Prabhu
  2015-08-17  8:03         ` [v3] " Michael Ellerman
  2 siblings, 0 replies; 14+ messages in thread
From: Sam Bobroff @ 2015-08-14  4:54 UTC (permalink / raw)
  To: Gautham R. Shenoy
  Cc: Paul Mackerras, linux-kernel, linuxppc-dev, Michael Ellerman,
	Shreyas B Prabhu, mikey, Madhavan Srinivasan

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 <ego@linux.vnet.ibm.com>

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 <sam.bobroff@au1.ibm.com>
Tested-by: Sam Bobroff <sam.bobroff@au1.ibm.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3] powerpc: Add an inline function to update POWER8 HID0
  2015-08-05  7:08       ` [PATCH v3] powerpc: Add an inline function to update POWER8 HID0 Gautham R. Shenoy
  2015-08-14  4:54         ` Sam Bobroff
@ 2015-08-14  8:59         ` Shreyas B Prabhu
  2015-08-17  8:03         ` [v3] " Michael Ellerman
  2 siblings, 0 replies; 14+ messages in thread
From: Shreyas B Prabhu @ 2015-08-14  8:59 UTC (permalink / raw)
  To: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev,
	Michael Ellerman
  Cc: mikey, Segher Boessenkool, Madhavan Srinivasan



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 <ego@linux.vnet.ibm.com>

Reviewed-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [v3] powerpc: Add an inline function to update POWER8 HID0
  2015-08-05  7:08       ` [PATCH v3] powerpc: Add an inline function to update POWER8 HID0 Gautham R. Shenoy
  2015-08-14  4:54         ` Sam Bobroff
  2015-08-14  8:59         ` Shreyas B Prabhu
@ 2015-08-17  8:03         ` Michael Ellerman
  2 siblings, 0 replies; 14+ messages in thread
From: Michael Ellerman @ 2015-08-17  8:03 UTC (permalink / raw)
  To: Gautham R. Shenoy, Paul Mackerras, linux-kernel, linuxppc-dev
  Cc: Shreyas B Prabhu, mikey, Madhavan Srinivasan, Gautham R. Shenoy

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 <ego@linux.vnet.ibm.com>
> Reviewed-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> Tested-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/e63dbd16ab7be41f5b66

cheers

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-08-17  8:03 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-04  8:30 [PATCH] powerpc: Add an inline function to update HID0 Gautham R. Shenoy
2015-08-04 10:08 ` Michael Ellerman
2015-08-04 10:57   ` Gautham R Shenoy
2015-08-04 11:06     ` [PATCH v2] " Gautham R. Shenoy
2015-08-04 14:06   ` Madhavan Srinivasan
2015-08-05  2:00     ` Michael Ellerman
2015-08-05  2:00       ` Michael Ellerman
2015-08-05  2:30   ` Segher Boessenkool
2015-08-05  6:54     ` Gautham R Shenoy
2015-08-05  7:08       ` [PATCH v3] powerpc: Add an inline function to update POWER8 HID0 Gautham R. Shenoy
2015-08-14  4:54         ` Sam Bobroff
2015-08-14  8:59         ` Shreyas B Prabhu
2015-08-17  8:03         ` [v3] " Michael Ellerman
2015-08-09  2:29   ` powerpc: Add an inline function to update HID0 Benjamin Herrenschmidt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.