Re: [kvm-ppc-devel] [PATCH 1 of 2] Add MSR Wait state to

Re: [kvm-ppc-devel] [PATCH 1 of 2] Add MSR Wait state to

[Hollis Blanchard]

On Fri, 2008-03-28 at 16:34 -0500, Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1206720510 18000
> # Node ID e48cf2ad6c85c457ff64c04b83960fc305420842
> # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> Add MSR Wait state to cpu_idle loop
> 
> This patch sets the wait state MSR when in the cpu_idle loop. This is
> mainly to help out virtualization solutions such as KVM. This way the
> virtualization soultion is able to tell if the guest kernel is idle.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> --- a/arch/powerpc/kernel/idle.c
> +++ b/arch/powerpc/kernel/idle.c
> @@ -32,6 +32,8 @@
>  #include <asm/time.h>
>  #include <asm/machdep.h>
>  #include <asm/smp.h>
> +
> +static unsigned long msr_save;

Why is this global?

>  #ifdef CONFIG_HOTPLUG_CPU
>  /* this is used for software suspend, and that shuts down
> @@ -88,6 +90,10 @@ void cpu_idle(void)
>  				HMT_low();
>  				HMT_very_low();
>  			}
> +
> +			/* set wait state MSR */
> +			msr_save = mfmsr();
> +			mtmsr(msr_save|MSR_WE);
>  		}
> 
>  		HMT_medium();

The code you are modifying is common to all PowerPC kernels. Why didn't
you use the power_save() hook provided to you?

-- 
Hollis Blanchard
IBM Linux Technology Center


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 1 of 2] Add MSR Wait state to

[Jerone Young]

On Fri, 2008-03-28 at 16:43 -0500, Hollis Blanchard wrote:
> On Fri, 2008-03-28 at 16:34 -0500, Jerone Young wrote:
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1206720510 18000
> > # Node ID e48cf2ad6c85c457ff64c04b83960fc305420842
> > # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> > Add MSR Wait state to cpu_idle loop
> > 
> > This patch sets the wait state MSR when in the cpu_idle loop. This is
> > mainly to help out virtualization solutions such as KVM. This way the
> > virtualization soultion is able to tell if the guest kernel is idle.
> > 
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> > 
> > diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> > --- a/arch/powerpc/kernel/idle.c
> > +++ b/arch/powerpc/kernel/idle.c
> > @@ -32,6 +32,8 @@
> >  #include <asm/time.h>
> >  #include <asm/machdep.h>
> >  #include <asm/smp.h>
> > +
> > +static unsigned long msr_save;
> 
> Why is this global?

I'll change this to declare it in cpu_idle().

> 
> >  #ifdef CONFIG_HOTPLUG_CPU
> >  /* this is used for software suspend, and that shuts down
> > @@ -88,6 +90,10 @@ void cpu_idle(void)
> >  				HMT_low();
> >  				HMT_very_low();
> >  			}
> > +
> > +			/* set wait state MSR */
> > +			msr_save = mfmsr();
> > +			mtmsr(msr_save|MSR_WE);
> >  		}
> > 
> >  		HMT_medium();
> 
> The code you are modifying is common to all PowerPC kernels. Why didn't
> you use the power_save() hook provided to you?
> 

power_save() is called with interrupts disabled. So we would never wake
up.




-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel

Re: [kvm-ppc-devel] [PATCH 1 of 2] Add MSR Wait state to

[Jerone Young]

On Fri, 2008-03-28 at 17:10 -0500, Nathan Lynch wrote:
> Jerone Young wrote:
> > On Fri, 2008-03-28 at 16:43 -0500, Hollis Blanchard wrote:
> > > >  #ifdef CONFIG_HOTPLUG_CPU
> > > >  /* this is used for software suspend, and that shuts down
> > > > @@ -88,6 +90,10 @@ void cpu_idle(void)
> > > >  				HMT_low();
> > > >  				HMT_very_low();
> > > >  			}
> > > > +
> > > > +			/* set wait state MSR */
> > > > +			msr_save = mfmsr();
> > > > +			mtmsr(msr_save|MSR_WE);
> > > >  		}
> > > > 
> > > >  		HMT_medium();
> > > 
> > > The code you are modifying is common to all PowerPC kernels. Why didn't
> > > you use the power_save() hook provided to you?
> > > 
> > 
> > power_save() is called with interrupts disabled. So we would never wake
> > up.
> 
> MSR_WE/POW is implementation-dependent and modifying it in generic
> code is not okay.

I figured this would come out as an issue. Once I got to linuxppc-dev

> 
> You can re-enable interrupts in your power_save implementation before
> setting MSR_WE (in fact power4_idle does this).

OK great .. I'll do this then.



-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel