[PATCH] Don't access HID registers if running on a Hypervisor.

[PATCH] Don't access HID registers if running on a Hypervisor.

[Jimi Xenidis]

The following patch avoids accessing Hypervisor privilege HID
registers when running on a Hypervisor (MSR[HV]=0).

Signed-off-by: Jimi Xenidis <jimix@watson.ibm.com>


diff -r 87d019e8edc2 arch/powerpc/kernel/cpu_setup_power4.S
--- a/arch/powerpc/kernel/cpu_setup_power4.S	Tue Jun 20 13:50:49 2006 -0400
+++ b/arch/powerpc/kernel/cpu_setup_power4.S	Wed Jun 21 19:10:22 2006 -0400
@@ -149,7 +149,12 @@ _GLOBAL(__save_cpu_setup)
 	cmpwi	r0,0x44
 	bne	2f
 
-1:	/* Save HID0,1,4 and 5 */
+1:	/* skip if not running in HV mode */
+	mfmsr	r0
+	rldicl.	r0,r0,4,63
+	beq	2f
+
+	/* Save HID0,1,4 and 5 */
 	mfspr	r3,SPRN_HID0
 	std	r3,CS_HID0(r5)
 	mfspr	r3,SPRN_HID1
@@ -183,7 +188,12 @@ _GLOBAL(__restore_cpu_setup)
 	cmpwi	r0,0x44
 	bnelr
 
-1:	/* Before accessing memory, we make sure rm_ci is clear */
+1:	/* skip if not running in HV mode */
+	mfmsr	r0
+	rldicl.	r0,r0,4,63
+	beqlr
+
+	/* Before accessing memory, we make sure rm_ci is clear */
 	li	r0,0
 	mfspr	r3,SPRN_HID4
 	rldimi	r3,r0,40,23	/* clear bit 23 (rm_ci) */

Re: [PATCH] Don't access HID registers if running on a Hypervisor.

[Olof Johansson]

On Wed, Jun 21, 2006 at 07:15:55PM -0400, Jimi Xenidis wrote:
> 
> The following patch avoids accessing Hypervisor privilege HID
> registers when running on a Hypervisor (MSR[HV]=0).

I'm curious, why is this causing problems now? JS20 has been running
that code with a hypervisor since a long time back.


-Olof

Re: [PATCH] Don't access HID registers if running on a Hypervisor.

[Jimi Xenidis]

On Jun 21, 2006, at 7:51 PM, Olof Johansson wrote:

> On Wed, Jun 21, 2006 at 07:15:55PM -0400, Jimi Xenidis wrote:
>>
>> The following patch avoids accessing Hypervisor privilege HID
>> registers when running on a Hypervisor (MSR[HV]=0).
>
> I'm curious, why is this causing problems now?
Every once in a while I run Xen on one of our simulators thats is  
sensitive to this code that does this.
First time we hit it (a while ago) we discovered that mtsdr1 was used  
(even though a no-op)

> JS20 has been running
> that code with a hypervisor since a long time back.

970s consider this a no-op (I believe an expensive one) especially  
when you consider some of the synchronization issues around the mtspr  
HIDx.
And though this is 970 specific code, Cell and future processors,  
will no longer allow writing to to these registers if !(MSR[0]=1 &&  
MSR[PR]=0)

-JX

Re: [PATCH] Don't access HID registers if running on a Hypervisor.

[Benjamin Herrenschmidt]

On Thu, 2006-06-22 at 06:58 -0400, Jimi Xenidis wrote:

> 970s consider this a no-op (I believe an expensive one) especially  
> when you consider some of the synchronization issues around the mtspr  
> HIDx.
> And though this is 970 specific code, Cell and future processors,  
> will no longer allow writing to to these registers if !(MSR[0]=1 &&  
> MSR[PR]=0)

Fair enough, let's fix it.

Ben.

Re: [PATCH] Don't access HID registers if running on a Hypervisor.

[Paul Mackerras]

Jimi Xenidis writes:

> The following patch avoids accessing Hypervisor privilege HID
> registers when running on a Hypervisor (MSR[HV]=0).

Why?  Aren't the writes defined to be no-ops when not in hypervisor
mode?

Paul.

Re: [PATCH] Don't access HID registers if running on a Hypervisor.

[Jimi Xenidis]

On Jun 21, 2006, at 7:58 PM, Paul Mackerras wrote:

> Jimi Xenidis writes:
>
>> The following patch avoids accessing Hypervisor privilege HID
>> registers when running on a Hypervisor (MSR[HV]=0).
>
> Why?  Aren't the writes defined to be no-ops when not in hypervisor
> mode?

The write are no-ops but look at the instructions required to be  
around the write for some of them.
I believe read may have performance issues as well.

I juts think its good practice and as Xell and newer processes come  
along we may want to start using the PACA(?) rather then peeking at  
the msr all the time.

-JX

Re: [PATCH] Don't access HID registers if running on a Hypervisor.

[Olof Johansson]

On Wed, Jun 21, 2006 at 10:35:28PM -0400, Jimi Xenidis wrote:
> 
> On Jun 21, 2006, at 7:58 PM, Paul Mackerras wrote:
> 
> > Jimi Xenidis writes:
> >
> >> The following patch avoids accessing Hypervisor privilege HID
> >> registers when running on a Hypervisor (MSR[HV]=0).
> >
> > Why?  Aren't the writes defined to be no-ops when not in hypervisor
> > mode?
> 
> The write are no-ops but look at the instructions required to be  
> around the write for some of them.
> I believe read may have performance issues as well.

Are you aware that this code runs only once per processor during
boot (and/or during cpu hotplug)? A few extra cycles there won't kill
anyone.

That being said, not touching the registers makes sense, but I don't buy
the performance motivation. :-)

> I juts think its good practice and as Xell and newer processes come  
> along we may want to start using the PACA(?) rather then peeking at  
> the msr all the time.

PACA means it's a mfspr + dependent load + test instead of mfmsr + mask,
right? It'd need benchmarking on several processors to show if it's
better or not.


-Olof

Re: [PATCH] Don't access HID registers if running on a Hypervisor.

[Jimi Xenidis]

On Jun 22, 2006, at 12:23 PM, Olof Johansson wrote:
>
> Are you aware that this code runs only once per processor during
> boot (and/or during cpu hotplug)? A few extra cycles there won't kill
> anyone.

Hey, I never said it was a hot path, only that it had performance  
issues.
I agree about the a "few extra cycles"

> That being said, not touching the registers makes sense, but I  
> don't buy
> the performance motivation. :-)
>
>> I juts think its good practice and as Xell and newer processes come
>> along we may want to start using the PACA(?) rather then peeking at
>> the msr all the time.
>
> PACA means it's a mfspr + dependent load + test instead of mfmsr +  
> mask,
> right? It'd need benchmarking on several processors to show if it's
> better or not.
>
>
> -Olof