[PATCH] Add idle power save for ppc 4xx

# HG changeset patch
# User Jerone Young [off-list ref]
# 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 <redacted>

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

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

7 more quoted lines

void ppc4xx_idle(void)

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.

This belongs in the platform setup code.

-- 
Hollis Blanchard
IBM Linux Technology Center

On Mon, 2008-03-31 at 11:27 -0500, Hollis Blanchard wrote:

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref]
# 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 <redacted>

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

20 more quoted lines

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.

quoted

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

quoted

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.

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

# HG changeset patch
# User Jerone Young [off-list ref]
# 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?

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

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

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

josh

On Mon, 31 Mar 2008 11:52:02 -0500
Jerone Young [off-list ref] wrote:

quoted

void ppc4xx_idle(void)

quoted

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

On Mon, 31 Mar 2008 12:07:17 -0500
Josh Boyer [off-list ref] wrote:

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref]
# 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

On Mon, 2008-03-31 at 13:05 -0500, Josh Boyer wrote:

On Mon, 31 Mar 2008 12:07:17 -0500
Josh Boyer [off-list ref] wrote:

quoted

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref]
# 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

On Mon, 2008-03-31 at 12:07 -0500, Josh Boyer wrote:

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref]
# 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?

quoted

Signed-off-by: Jerone Young <redacted>

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).

quoted

+#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.

quoted

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

On Mon, 2008-03-31 at 13:23 -0500, Jerone Young wrote:

quoted

quoted

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

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

On Mon, 31 Mar 2008 12:07:17 -0500
Josh Boyer [off-list ref] wrote:

quoted

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref] # 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?

quoted

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

On Mon, 2008-03-31 at 19:24 +0000, Hollis Blanchard wrote:

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

quoted

On Mon, 31 Mar 2008 12:07:17 -0500
Josh Boyer [off-list ref] wrote:

quoted

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref] # 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."

quoted

quoted

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

On Mon, 2008-03-31 at 15:28 -0500, Josh Boyer wrote:

On Mon, 2008-03-31 at 19:24 +0000, Hollis Blanchard wrote:

quoted

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

quoted

On Mon, 31 Mar 2008 12:07:17 -0500
Josh Boyer [off-list ref] wrote:

quoted

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref] # 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?

quoted

quoted

quoted

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

On Mon, 2008-03-31 at 13:19 -0500, Jerone Young wrote:

On Mon, 2008-03-31 at 13:05 -0500, Josh Boyer wrote:

quoted

On Mon, 31 Mar 2008 12:07:17 -0500
Josh Boyer [off-list ref] wrote:

quoted

On Mon, 2008-03-31 at 08:12 -0500, Jerone Young wrote:

quoted

# HG changeset patch
# User Jerone Young [off-list ref]
# 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

On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:

quoted

quoted

quoted

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

On Mon, 2008-03-31 at 22:15 -0500, Josh Boyer wrote:

On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:

quoted

quoted

quoted

quoted

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

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.

On Tue, 1 Apr 2008 15:00:38 +1100
Paul Mackerras [off-list ref] wrote:

Josh Boyer writes:

quoted

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

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

On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:

quoted

quoted

quoted

quoted

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

On Tue, 2008-04-01 at 08:01 -0400, Jimi Xenidis wrote:

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

quoted

On Tue, 2008-04-01 at 12:04 +1100, Michael Ellerman wrote:

quoted

quoted

quoted

quoted

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

On Monday 31 March 2008, Jerone Young wrote:

quoted

=20

quoted

+{
+=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.

quoted

=20

quoted

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