[PATCH] Add idle power save for ppc 4xx

[PATCH] Add idle power save for ppc 4xx

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1206969060 18000
# Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
# Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
Add idle power save for ppc 4xx

This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.

I have tested this on hardware & KVM virtual guest.

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
 obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
+obj-$(CONFIG_4xx)		+= idle_4xx.o
 obj-$(CONFIG_TAU)		+= tau_6xx.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
 				   swsusp_$(CONFIG_WORD_SIZE).o
diff --git a/arch/powerpc/kernel/idle_4xx.c b/arch/powerpc/kernel/idle_4xx.c
new file mode 100644
--- /dev/null
+++ b/arch/powerpc/kernel/idle_4xx.c
@@ -0,0 +1,24 @@
+/*
+ * This file contains the power_save function for 4xx CPUs
+ *
+ * added by Jerone Young <jyoung5@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/processor.h>
+#include <asm/machdep.h>
+
+void ppc4xx_idle()
+{
+	unsigned long msr_save;
+
+	/* set wait state MSR */
+	local_irq_enable();
+	msr_save = mfmsr();
+	mtmsr(msr_save|MSR_WE);
+	local_irq_disable();
+}
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -132,6 +132,10 @@ void __init machine_init(unsigned long d
 	if (cpu_has_feature(CPU_FTR_CAN_DOZE) ||
 	    cpu_has_feature(CPU_FTR_CAN_NAP))
 		ppc_md.power_save = ppc6xx_idle;
+#endif
+
+#ifdef CONFIG_4xx
+	ppc_md.power_save = ppc4xx_idle;
 #endif
 
 	if (ppc_md.progress)
diff --git a/include/asm-powerpc/machdep.h b/include/asm-powerpc/machdep.h
--- a/include/asm-powerpc/machdep.h
+++ b/include/asm-powerpc/machdep.h
@@ -266,6 +266,7 @@ extern void power4_idle(void);
 extern void power4_idle(void);
 extern void power4_cpu_offline_powersave(void);
 extern void ppc6xx_idle(void);
+extern void ppc4xx_idle(void);
 
 /*
  * ppc_md contains a copy of the machine description structure for the

Re: [kvm-ppc-devel] [PATCH] Add idle power save for ppc 4xx

[Hollis Blanchard]

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1206969060 18000
> # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
> # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> Add idle power save for ppc 4xx
> 
> This patch sets the wait state MSR when power_save is called in
> cpu_idle loop for ppc4xx. This is mainly to help out virtualization
> solutions such as KVM. This way the virtualization soultions are able
> to tell if the guest kernel is idle.
> 
> I have tested this on hardware & KVM virtual guest.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
>  obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
>  obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
>  obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
> +obj-$(CONFIG_4xx)		+= idle_4xx.o
>  obj-$(CONFIG_TAU)		+= tau_6xx.o
>  obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
>  				   swsusp_$(CONFIG_WORD_SIZE).o
> diff --git a/arch/powerpc/kernel/idle_4xx.c b/arch/powerpc/kernel/idle_4xx.c
> new file mode 100644
> --- /dev/null
> +++ b/arch/powerpc/kernel/idle_4xx.c
> @@ -0,0 +1,24 @@
> +/*
> + * This file contains the power_save function for 4xx CPUs
> + *
> + * added by Jerone Young <jyoung5@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <asm/processor.h>
> +#include <asm/machdep.h>
> +
> +void ppc4xx_idle()

void ppc4xx_idle(void)

> +{
> +	unsigned long msr_save;
> +
> +	/* set wait state MSR */
> +	local_irq_enable();
> +	msr_save = mfmsr();
> +	mtmsr(msr_save|MSR_WE);

Why don't you |MSR_WE|MSR_EE at the same time?

> +	local_irq_disable();
> +}

None of the other power_save() implementations need this. In fact many
of them don't even seem to return; they just loop around mtmsr.

> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> --- a/arch/powerpc/kernel/setup_32.c
> +++ b/arch/powerpc/kernel/setup_32.c
> @@ -132,6 +132,10 @@ void __init machine_init(unsigned long d
>  	if (cpu_has_feature(CPU_FTR_CAN_DOZE) ||
>  	    cpu_has_feature(CPU_FTR_CAN_NAP))
>  		ppc_md.power_save = ppc6xx_idle;
> +#endif
> +
> +#ifdef CONFIG_4xx
> +	ppc_md.power_save = ppc4xx_idle;
>  #endif
> 
>  	if (ppc_md.progress)

This belongs in the platform setup code.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [kvm-ppc-devel] [PATCH] Add idle power save for ppc 4xx

[Jerone Young]

On Mon, 2008-03-31 at 11:27 -0500, Hollis Blanchard wrote:
> On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1206969060 18000
> > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
> > # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> > Add idle power save for ppc 4xx
> > 
> > This patch sets the wait state MSR when power_save is called in
> > cpu_idle loop for ppc4xx. This is mainly to help out virtualization
> > solutions such as KVM. This way the virtualization soultions are able
> > to tell if the guest kernel is idle.
> > 
> > I have tested this on hardware & KVM virtual guest.
> > 
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> > 
> > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> > --- a/arch/powerpc/kernel/Makefile
> > +++ b/arch/powerpc/kernel/Makefile
> > @@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
> >  obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
> >  obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
> >  obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
> > +obj-$(CONFIG_4xx)		+= idle_4xx.o
> >  obj-$(CONFIG_TAU)		+= tau_6xx.o
> >  obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
> >  				   swsusp_$(CONFIG_WORD_SIZE).o
> > diff --git a/arch/powerpc/kernel/idle_4xx.c b/arch/powerpc/kernel/idle_4xx.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/arch/powerpc/kernel/idle_4xx.c
> > @@ -0,0 +1,24 @@
> > +/*
> > + * This file contains the power_save function for 4xx CPUs
> > + *
> > + * added by Jerone Young <jyoung5@us.ibm.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <asm/processor.h>
> > +#include <asm/machdep.h>
> > +
> > +void ppc4xx_idle()
> 
> void ppc4xx_idle(void)
> 
> > +{
> > +	unsigned long msr_save;
> > +
> > +	/* set wait state MSR */
> > +	local_irq_enable();
> > +	msr_save = mfmsr();
> > +	mtmsr(msr_save|MSR_WE);
> 
> Why don't you |MSR_WE|MSR_EE at the same time?

You technically can do this. But the question is do all 4xx cpus use
MSR_EE to enable interrupts? I can assume they do (from what I know),
but figured it would be safer to make the local_irq_enable() call.
I can change it to just set the MSR_EE bit though, since all 4xx cpus
(as far as I know) use it.


> 
> > +	local_irq_disable();
> > +}
> 
> None of the other power_save() implementations need this. In fact many
> of them don't even seem to return; they just loop around mtmsr.

Sure it can be removed. Though with the comment in the mach_dep
structure about power_save. It specifically says that interrupts are off
when it is called. So I was following it here mainly. But I can remove
the disabling of interrupts, since mtmsr is the only used under
power_save.

> 
> > diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> > --- a/arch/powerpc/kernel/setup_32.c
> > +++ b/arch/powerpc/kernel/setup_32.c
> > @@ -132,6 +132,10 @@ void __init machine_init(unsigned long d
> >  	if (cpu_has_feature(CPU_FTR_CAN_DOZE) ||
> >  	    cpu_has_feature(CPU_FTR_CAN_NAP))
> >  		ppc_md.power_save = ppc6xx_idle;
> > +#endif
> > +
> > +#ifdef CONFIG_4xx
> > +	ppc_md.power_save = ppc4xx_idle;
> >  #endif
> > 
> >  	if (ppc_md.progress)
> 
> This belongs in the platform setup code.

I'll move this to the 4xx platform setup code.

> 

Re: [kvm-ppc-devel] [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Mon, 31 Mar 2008 11:52:02 -0500
Jerone Young <jyoung5@us.ibm.com> wrote:

> > void ppc4xx_idle(void)
> > 
> > > +{
> > > +	unsigned long msr_save;
> > > +
> > > +	/* set wait state MSR */
> > > +	local_irq_enable();
> > > +	msr_save = mfmsr();
> > > +	mtmsr(msr_save|MSR_WE);
> > 
> > Why don't you |MSR_WE|MSR_EE at the same time?
> 
> You technically can do this. But the question is do all 4xx cpus use
> MSR_EE to enable interrupts? I can assume they do (from what I know),

They do for enabling external interrupts of normal priority.  MSR_CE
might also be used/needed if someone has the watchdog enabled or has an
external device with the UIC pin mapped as a CE.

josh

Re: [kvm-ppc-devel] [PATCH] Add idle power save for ppc 4xx

[Arnd Bergmann]

On Monday 31 March 2008, Jerone Young wrote:
> >=20
> > > +{
> > > +=A0=A0=A0unsigned long msr_save;
> > > +
> > > +=A0=A0=A0/* set wait state MSR */
> > > +=A0=A0=A0local_irq_enable();
> > > +=A0=A0=A0msr_save =3D mfmsr();
> > > +=A0=A0=A0mtmsr(msr_save|MSR_WE);
> >=20
> > Why don't you |MSR_WE|MSR_EE at the same time?
>=20
> You technically can do this. But the question is do all 4xx cpus use
> MSR_EE to enable interrupts? I can assume they do (from what I know),
> but figured it would be safer to make the local_irq_enable() call.
> I can change it to just set the MSR_EE bit though, since all 4xx cpus
> (as far as I know) use it.

=46or correctness reasons, you actually have to set both EE and WE
simultaneously. Otherwise, an interrupt can come in between the two
mtmsr(), mark some thread as runnable and then go to sleep here
without ever checking need_resched() until the next interrupt,
which may take an indefinite time.

> >=20
> > > +=A0=A0=A0local_irq_disable();
> > > +}
> >=20
> > None of the other power_save() implementations need this. In fact many
> > of them don't even seem to return; they just loop around mtmsr.
>=20
> Sure it can be removed. Though with the comment in the mach_dep
> structure about power_save. It specifically says that interrupts are off
> when it is called. So I was following it here mainly. But I can remove
> the disabling of interrupts, since mtmsr is the only used under
> power_save.

With the current code, it shouldn't matter, because cpu_idle enables
the interrupts right after calling ppc_md->power_save(), but it would
be reasonable to disable the interrupts here anyway, if only to make
the function return with the same state that it was entered.

IMHO, the function should be

void ppc4xx_idle()
{
        unsigned long msr_save;

        msr_save =3D mfmsr();
	/* enter idle mode */
        mtmsr(msr_save|MSR_WE|MSR_EE);
	/* disable interrupts again */
        mtmsr(msr_save);
}

	Arnd <><

Re: [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1206969060 18000
> # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
> # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> Add idle power save for ppc 4xx
> 
> This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.
> 
> I have tested this on hardware & KVM virtual guest.

I'm not overly thrilled with adding this to all of 4xx.  It doesn't
actually save much power at all (1% on a project that actually measured
it with an amp meter recently) and there's really no other benefit to
doing it outside of the virtual guest case.

I'm assuming you pass a dtb to the virtual guest when you start it up.
Could you define a property in the CPU node there that can be parsed to
use the power_save function instead of always making it the default?

> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
>  obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
>  obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
>  obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
> +obj-$(CONFIG_4xx)		+= idle_4xx.o
>  obj-$(CONFIG_TAU)		+= tau_6xx.o
>  obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
>  				   swsusp_$(CONFIG_WORD_SIZE).o
> diff --git a/arch/powerpc/kernel/idle_4xx.c b/arch/powerpc/kernel/idle_4xx.c
> new file mode 100644
> --- /dev/null
> +++ b/arch/powerpc/kernel/idle_4xx.c

Can this be added to sysdev/ppc4xx_soc.c instead?

> +#include <asm/processor.h>
> +#include <asm/machdep.h>
> +
> +void ppc4xx_idle()
> +{
> +	unsigned long msr_save;
> +
> +	/* set wait state MSR */
> +	local_irq_enable();
> +	msr_save = mfmsr();
> +	mtmsr(msr_save|MSR_WE);
> +	local_irq_disable();
> +}

I agree with Hollis on both the MSR_WE|MSR_EE and removing the
local_irq_disable changes.

> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> --- a/arch/powerpc/kernel/setup_32.c
> +++ b/arch/powerpc/kernel/setup_32.c
> @@ -132,6 +132,10 @@ void __init machine_init(unsigned long d
>  	if (cpu_has_feature(CPU_FTR_CAN_DOZE) ||
>  	    cpu_has_feature(CPU_FTR_CAN_NAP))
>  		ppc_md.power_save = ppc6xx_idle;
> +#endif
> +
> +#ifdef CONFIG_4xx
> +	ppc_md.power_save = ppc4xx_idle;
>  #endif

I agree this belongs in platform setup code.  Finding the right spot for
it might be a bit of a challenge.

josh

Re: [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Mon, 31 Mar 2008 12:07:17 -0500
Josh Boyer <jwboyer@gmail.com> wrote:

> On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1206969060 18000
> > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
> > # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> > Add idle power save for ppc 4xx
> > 
> > This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.
> > 
> > I have tested this on hardware & KVM virtual guest.
> 
> I'm not overly thrilled with adding this to all of 4xx.  It doesn't
> actually save much power at all (1% on a project that actually measured
> it with an amp meter recently) and there's really no other benefit to
> doing it outside of the virtual guest case.
> 
> I'm assuming you pass a dtb to the virtual guest when you start it up.
> Could you define a property in the CPU node there that can be parsed to
> use the power_save function instead of always making it the default?

Actually, you probably don't want this as a property in the device
tree.  It doesn't describe hardware.  A Kconfig option might be
warranted though.

josh

Re: [PATCH] Add idle power save for ppc 4xx

[Jerone Young]

On Mon, 2008-03-31 at 13:05 -0500, Josh Boyer wrote:
> On Mon, 31 Mar 2008 12:07:17 -0500
> Josh Boyer <jwboyer@gmail.com> wrote:
> 
> > On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> > > # HG changeset patch
> > > # User Jerone Young <jyoung5@us.ibm.com>
> > > # Date 1206969060 18000
> > > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
> > > # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> > > Add idle power save for ppc 4xx
> > > 
> > > This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.
> > > 
> > > I have tested this on hardware & KVM virtual guest.
> > 
> > I'm not overly thrilled with adding this to all of 4xx.  It doesn't
> > actually save much power at all (1% on a project that actually measured
> > it with an amp meter recently) and there's really no other benefit to
> > doing it outside of the virtual guest case.
> > 
> > I'm assuming you pass a dtb to the virtual guest when you start it up.
> > Could you define a property in the CPU node there that can be parsed to
> > use the power_save function instead of always making it the default?
> 
> Actually, you probably don't want this as a property in the device
> tree.  It doesn't describe hardware.  A Kconfig option might be
> warranted though.

I'll go with the Kconfig option. 

> 
> josh

Re: [PATCH] Add idle power save for ppc 4xx

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

On Mon, 2008-03-31 at 13:19 -0500, Jerone Young wrote:
> On Mon, 2008-03-31 at 13:05 -0500, Josh Boyer wrote:
> > On Mon, 31 Mar 2008 12:07:17 -0500
> > Josh Boyer <jwboyer@gmail.com> wrote:
> > 
> > > On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> > > > # HG changeset patch
> > > > # User Jerone Young <jyoung5@us.ibm.com>
> > > > # Date 1206969060 18000
> > > > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
> > > > # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> > > > Add idle power save for ppc 4xx
> > > > 
> > > > This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.
> > > > 
> > > > I have tested this on hardware & KVM virtual guest.
> > > 
> > > I'm not overly thrilled with adding this to all of 4xx.  It doesn't
> > > actually save much power at all (1% on a project that actually measured
> > > it with an amp meter recently) and there's really no other benefit to
> > > doing it outside of the virtual guest case.
> > > 
> > > I'm assuming you pass a dtb to the virtual guest when you start it up.
> > > Could you define a property in the CPU node there that can be parsed to
> > > use the power_save function instead of always making it the default?
> > 
> > Actually, you probably don't want this as a property in the device
> > tree.  It doesn't describe hardware.  A Kconfig option might be
> > warranted though.
> 
> I'll go with the Kconfig option. 

Go with a device-tree check. The pseries kernel supports both bare-metal
and hypervisor in the same kernel image, and it works out which it's
running on by looking at the device-tree. This seems equivalent to me?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:
> > > > I'm assuming you pass a dtb to the virtual guest when you start it up.
> > > > Could you define a property in the CPU node there that can be parsed to
> > > > use the power_save function instead of always making it the default?
> > > 
> > > Actually, you probably don't want this as a property in the device
> > > tree.  It doesn't describe hardware.  A Kconfig option might be
> > > warranted though.
> > 
> > I'll go with the Kconfig option. 
> 
> Go with a device-tree check. The pseries kernel supports both bare-metal
> and hypervisor in the same kernel image, and it works out which it's
> running on by looking at the device-tree. This seems equivalent to me?

After some back and forth on IRC, we decided on that as well.  I love
being right, then wrong, then right again.

/me needs more sleep.

josh

Re: [PATCH] Add idle power save for ppc 4xx

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On Mon, 2008-03-31 at 22:15 -0500, Josh Boyer wrote:
> On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:
> > > > > I'm assuming you pass a dtb to the virtual guest when you start it up.
> > > > > Could you define a property in the CPU node there that can be parsed to
> > > > > use the power_save function instead of always making it the default?
> > > > 
> > > > Actually, you probably don't want this as a property in the device
> > > > tree.  It doesn't describe hardware.  A Kconfig option might be
> > > > warranted though.
> > > 
> > > I'll go with the Kconfig option. 
> > 
> > Go with a device-tree check. The pseries kernel supports both bare-metal
> > and hypervisor in the same kernel image, and it works out which it's
> > running on by looking at the device-tree. This seems equivalent to me?
> 
> After some back and forth on IRC, we decided on that as well.  I love
> being right, then wrong, then right again.
> 
> /me needs more sleep.

Better to be right twice rather than once, no?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [kvm-ppc-devel] [PATCH] Add idle power save for ppc 4xx

[Jimi Xenidis]

On Mar 31, 2008, at 11:15 PM, Josh Boyer wrote:

> On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:
>>>>> I'm assuming you pass a dtb to the virtual guest when you start  
>>>>> it up.
>>>>> Could you define a property in the CPU node there that can be  
>>>>> parsed to
>>>>> use the power_save function instead of always making it the  
>>>>> default?
>>>>
>>>> Actually, you probably don't want this as a property in the device
>>>> tree.  It doesn't describe hardware.  A Kconfig option might be
>>>> warranted though.
>>>
>>> I'll go with the Kconfig option.
>>
>> Go with a device-tree check. The pseries kernel supports both bare- 
>> metal
>> and hypervisor in the same kernel image, and it works out which it's
>> running on by looking at the device-tree. This seems equivalent to  
>> me?
>
> After some back and forth on IRC, we decided on that as well.  I love
> being right, then wrong, then right again.

Awesome, can you summarize for the rest of us?
-JX

Re: [kvm-ppc-devel] [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Tue, 2008-04-01 at 08:01 -0400, Jimi Xenidis wrote:
> On Mar 31, 2008, at 11:15 PM, Josh Boyer wrote:
> 
> > On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:
> >>>>> I'm assuming you pass a dtb to the virtual guest when you start  
> >>>>> it up.
> >>>>> Could you define a property in the CPU node there that can be  
> >>>>> parsed to
> >>>>> use the power_save function instead of always making it the  
> >>>>> default?
> >>>>
> >>>> Actually, you probably don't want this as a property in the device
> >>>> tree.  It doesn't describe hardware.  A Kconfig option might be
> >>>> warranted though.
> >>>
> >>> I'll go with the Kconfig option.
> >>
> >> Go with a device-tree check. The pseries kernel supports both bare- 
> >> metal
> >> and hypervisor in the same kernel image, and it works out which it's
> >> running on by looking at the device-tree. This seems equivalent to  
> >> me?
> >
> > After some back and forth on IRC, we decided on that as well.  I love
> > being right, then wrong, then right again.
> 
> Awesome, can you summarize for the rest of us?

A node or property will be in the device tree that the guests can use to
determine if it's running under a hypervisor or not.

The details are yet to be worked out, so there isn't much more of a
summary yet.  There was also some conversation about having an "idle="
kernel parameter similar to the PA-Semi port that would allow toggling
of it as well.

josh

Re: [PATCH] Add idle power save for ppc 4xx

[Hollis Blanchard]

On Mon, 31 Mar 2008 13:05:18 -0500, Josh Boyer wrote:

> On Mon, 31 Mar 2008 12:07:17 -0500
> Josh Boyer <jwboyer@gmail.com> wrote:
> 
>> On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
>> > # HG changeset patch
>> > # User Jerone Young <jyoung5@us.ibm.com> # Date 1206969060 18000
>> > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 
>> > 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc
>> > 4xx
>> > 
>> > This patch sets the wait state MSR when power_save is called in
>> > cpu_idle loop for ppc4xx. This is mainly to help out virtualization
>> > solutions such as KVM. This way the virtualization soultions are able
>> > to tell if the guest kernel is idle.
>> > 
>> > I have tested this on hardware & KVM virtual guest.
>> 
>> I'm not overly thrilled with adding this to all of 4xx.  It doesn't
>> actually save much power at all (1% on a project that actually measured
>> it with an amp meter recently) and there's really no other benefit to
>> doing it outside of the virtual guest case.

So it slightly helps hardware, and it helps virtualization a *lot*. 
What's the problem?

>> I'm assuming you pass a dtb to the virtual guest when you start it up.
>> Could you define a property in the CPU node there that can be parsed to
>> use the power_save function instead of always making it the default?
> 
> Actually, you probably don't want this as a property in the device tree.
>  It doesn't describe hardware.  A Kconfig option might be warranted
> though.

There will be a device tree binding for hypervisor properties, so if it's 
not always enabled, having a hypervisor node (for any hypervisor) in the 
device tree would be an indicator. Far better than a Kconfig option, at 
any rate.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Mon, 2008-03-31 at 19:24 +0000, Hollis Blanchard wrote:
> On Mon, 31 Mar 2008 13:05:18 -0500, Josh Boyer wrote:
> 
> > On Mon, 31 Mar 2008 12:07:17 -0500
> > Josh Boyer <jwboyer@gmail.com> wrote:
> > 
> >> On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> >> > # HG changeset patch
> >> > # User Jerone Young <jyoung5@us.ibm.com> # Date 1206969060 18000
> >> > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 
> >> > 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc
> >> > 4xx
> >> > 
> >> > This patch sets the wait state MSR when power_save is called in
> >> > cpu_idle loop for ppc4xx. This is mainly to help out virtualization
> >> > solutions such as KVM. This way the virtualization soultions are able
> >> > to tell if the guest kernel is idle.
> >> > 
> >> > I have tested this on hardware & KVM virtual guest.
> >> 
> >> I'm not overly thrilled with adding this to all of 4xx.  It doesn't
> >> actually save much power at all (1% on a project that actually measured
> >> it with an amp meter recently) and there's really no other benefit to
> >> doing it outside of the virtual guest case.
> 
> So it slightly helps hardware, and it helps virtualization a *lot*. 
> What's the problem?

There's 0 publicly available documentation on exactly what "Wait State
Enable" means other than the description for the MSR register bit in the
4xx UM.  I'm a very paranoid person.

Explain to me what it really provides with some kind of concrete numbers
on real hardware and I'll think about it as the default.  Until then, I
think a Kconfig option (or DT property) is acceptable for now.  I didn't
say "no", I just said "make it optional."

> >> I'm assuming you pass a dtb to the virtual guest when you start it up.
> >> Could you define a property in the CPU node there that can be parsed to
> >> use the power_save function instead of always making it the default?
> > 
> > Actually, you probably don't want this as a property in the device tree.
> >  It doesn't describe hardware.  A Kconfig option might be warranted
> > though.
> 
> There will be a device tree binding for hypervisor properties, so if it's 
> not always enabled, having a hypervisor node (for any hypervisor) in the 
> device tree would be an indicator. Far better than a Kconfig option, at 
> any rate.

But you want this in the guests, right?  Not the hypervisor...

josh

Re: [PATCH] Add idle power save for ppc 4xx

[Hollis Blanchard]

On Mon, 2008-03-31 at 15:28 -0500, Josh Boyer wrote:
> On Mon, 2008-03-31 at 19:24 +0000, Hollis Blanchard wrote:
> > On Mon, 31 Mar 2008 13:05:18 -0500, Josh Boyer wrote:
> > 
> > > On Mon, 31 Mar 2008 12:07:17 -0500
> > > Josh Boyer <jwboyer@gmail.com> wrote:
> > > 
> > >> On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> > >> > # HG changeset patch
> > >> > # User Jerone Young <jyoung5@us.ibm.com> # Date 1206969060 18000
> > >> > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f # Parent 
> > >> > 1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc Add idle power save for ppc
> > >> > 4xx
> > >> > 
> > >> > This patch sets the wait state MSR when power_save is called in
> > >> > cpu_idle loop for ppc4xx. This is mainly to help out virtualization
> > >> > solutions such as KVM. This way the virtualization soultions are able
> > >> > to tell if the guest kernel is idle.
> > >> > 
> > >> > I have tested this on hardware & KVM virtual guest.
> > >> 
> > >> I'm not overly thrilled with adding this to all of 4xx.  It doesn't
> > >> actually save much power at all (1% on a project that actually measured
> > >> it with an amp meter recently) and there's really no other benefit to
> > >> doing it outside of the virtual guest case.
> > 
> > So it slightly helps hardware, and it helps virtualization a *lot*. 
> > What's the problem?
> 
> There's 0 publicly available documentation on exactly what "Wait State
> Enable" means other than the description for the MSR register bit in the
> 4xx UM.  I'm a very paranoid person.
> 
> Explain to me what it really provides with some kind of concrete numbers
> on real hardware and I'll think about it as the default.  Until then, I
> think a Kconfig option (or DT property) is acceptable for now.  I didn't
> say "no", I just said "make it optional."

You can be paranoid about all new features, and then new development
ceases.

Did your project that measured it report any suspicious problems?

> > >> I'm assuming you pass a dtb to the virtual guest when you start it up.
> > >> Could you define a property in the CPU node there that can be parsed to
> > >> use the power_save function instead of always making it the default?
> > > 
> > > Actually, you probably don't want this as a property in the device tree.
> > >  It doesn't describe hardware.  A Kconfig option might be warranted
> > > though.
> > 
> > There will be a device tree binding for hypervisor properties, so if it's 
> > not always enabled, having a hypervisor node (for any hypervisor) in the 
> > device tree would be an indicator. Far better than a Kconfig option, at 
> > any rate.
> 
> But you want this in the guests, right?  Not the hypervisor...

Not sure what you mean. The hypervisor will create device tree
properties for the guests, so the guest could use that property to
initialize the power_save hook.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] Add idle power save for ppc 4xx

[Paul Mackerras]

Josh Boyer writes:

> Actually, you probably don't want this as a property in the device
> tree.  It doesn't describe hardware.  A Kconfig option might be
> warranted though.

In general it is valid to have properties in the device tree that
describe the hypervisor or the kernel's interface to it, since they
are aspects of the platform, i.e. the virtual "hardware" that the
kernel is running on.

Paul.

Re: [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Tue, 1 Apr 2008 15:00:38 +1100
Paul Mackerras <paulus@samba.org> wrote:

> Josh Boyer writes:
> 
> > Actually, you probably don't want this as a property in the device
> > tree.  It doesn't describe hardware.  A Kconfig option might be
> > warranted though.
> 
> In general it is valid to have properties in the device tree that
> describe the hypervisor or the kernel's interface to it, since they
> are aspects of the platform, i.e. the virtual "hardware" that the
> kernel is running on.

Yep, we reached that conclusion already.

josh

Re: [PATCH] Add idle power save for ppc 4xx

[Jerone Young]

On Mon, 2008-03-31 at 12:07 -0500, Josh Boyer wrote:
> On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1206969060 18000
> > # Node ID 10aea37177130bbe5de7bee6ec06d9010bc5da1f
> > # Parent  1506aa38ddabb0bf73fff3ac3f3db5f9ef6458cc
> > Add idle power save for ppc 4xx
> > 
> > This patch sets the wait state MSR when power_save is called in cpu_idle loop for ppc4xx. This is mainly to help out virtualization solutions such as KVM. This way the virtualization soultions are able to tell if the guest kernel is idle.
> > 
> > I have tested this on hardware & KVM virtual guest.
> 
> I'm not overly thrilled with adding this to all of 4xx.  It doesn't
> actually save much power at all (1% on a project that actually measured
> it with an amp meter recently) and there's really no other benefit to
> doing it outside of the virtual guest case.
> 
> I'm assuming you pass a dtb to the virtual guest when you start it up.
> Could you define a property in the CPU node there that can be parsed to
> use the power_save function instead of always making it the default?
> 
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> > 
> > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> > --- a/arch/powerpc/kernel/Makefile
> > +++ b/arch/powerpc/kernel/Makefile
> > @@ -39,6 +39,7 @@ obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsy
> >  obj-$(CONFIG_GENERIC_TBSYNC)	+= smp-tbsync.o
> >  obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
> >  obj-$(CONFIG_6xx)		+= idle_6xx.o l2cr_6xx.o cpu_setup_6xx.o
> > +obj-$(CONFIG_4xx)		+= idle_4xx.o
> >  obj-$(CONFIG_TAU)		+= tau_6xx.o
> >  obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o \
> >  				   swsusp_$(CONFIG_WORD_SIZE).o
> > diff --git a/arch/powerpc/kernel/idle_4xx.c b/arch/powerpc/kernel/idle_4xx.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/arch/powerpc/kernel/idle_4xx.c
> 
> Can this be added to sysdev/ppc4xx_soc.c instead?

Probably. Though the other platforms have there power_save code in
idle_<platform>.S files, as they are in assembly.

Also I don't appear to have ppc4xx_soc.c in my source (using
2.6.25-rc6).

> 
> > +#include <asm/processor.h>
> > +#include <asm/machdep.h>
> > +
> > +void ppc4xx_idle()
> > +{
> > +	unsigned long msr_save;
> > +
> > +	/* set wait state MSR */
> > +	local_irq_enable();
> > +	msr_save = mfmsr();
> > +	mtmsr(msr_save|MSR_WE);
> > +	local_irq_disable();
> > +}
> 
> I agree with Hollis on both the MSR_WE|MSR_EE and removing the
> local_irq_disable changes.

You mention adding MSR_CE in a followup email. I'll add that to and
remove local_irq_disable.

> 
> > diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> > --- a/arch/powerpc/kernel/setup_32.c
> > +++ b/arch/powerpc/kernel/setup_32.c
> > @@ -132,6 +132,10 @@ void __init machine_init(unsigned long d
> >  	if (cpu_has_feature(CPU_FTR_CAN_DOZE) ||
> >  	    cpu_has_feature(CPU_FTR_CAN_NAP))
> >  		ppc_md.power_save = ppc6xx_idle;
> > +#endif
> > +
> > +#ifdef CONFIG_4xx
> > +	ppc_md.power_save = ppc4xx_idle;
> >  #endif
> 
> I agree this belongs in platform setup code.  Finding the right spot for
> it might be a bit of a challenge.
> 
> josh
> 

Re: [PATCH] Add idle power save for ppc 4xx

[Josh Boyer]

On Mon, 2008-03-31 at 13:23 -0500, Jerone Young wrote:
> > > diff --git a/arch/powerpc/kernel/idle_4xx.c b/arch/powerpc/kernel/idle_4xx.c
> > > new file mode 100644
> > > --- /dev/null
> > > +++ b/arch/powerpc/kernel/idle_4xx.c
> > 
> > Can this be added to sysdev/ppc4xx_soc.c instead?
> 
> Probably. Though the other platforms have there power_save code in
> idle_<platform>.S files, as they are in assembly.

No, just two of them do.  Cell, pseries, and pasemi all have them under
the platform directory.

> Also I don't appear to have ppc4xx_soc.c in my source (using
> 2.6.25-rc6).

You're working against a very old tree, relatively speaking.  Use either
my for-2.6.26 branch in my git tree, or the linux-next tree.

josh