[PATCH] [v2] Add idle wait support for 44x platforms

[PATCH] [v2] Add idle wait support for 44x platforms

[Jerone Young]

# HG changeset patch
# User Jerone Young <jyoung5@us.ibm.com>
# Date 1207292108 18000
# Node ID afed3e5de82ab6c0ac8d6ceeb0292b6c41ece1ed
# Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
[v2] Add idle wait support for 44x platforms

This patch adds the ability for the CPU to go into wait state while in cpu_idle loop. This helps virtulization solutions know when the guest Linux kernel is in an idle state. There are two ways to do it.

1) Command line
	idle=spin <-- CPU will spin (this is the default)
	idle=wait <-- set CPU into wait state when idle

2) The device tree will be checked for the "/hypervisor" node
   If this node is seen it will use "wait" for idle, so that
   the hypervisor can know when guest Linux kernel it is in
   an idle state.

This patch, unlike the last, isolates the code to 44x platforms.

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

diff --git a/arch/powerpc/platforms/44x/Makefile b/arch/powerpc/platforms/44x/Makefile
--- a/arch/powerpc/platforms/44x/Makefile
+++ b/arch/powerpc/platforms/44x/Makefile
@@ -1,4 +1,4 @@ obj-$(CONFIG_44x)	:= misc_44x.o
-obj-$(CONFIG_44x)	:= misc_44x.o
+obj-$(CONFIG_44x)	:= misc_44x.o idle.o
 obj-$(CONFIG_EBONY)	+= ebony.o
 obj-$(CONFIG_TAISHAN)	+= taishan.o
 obj-$(CONFIG_BAMBOO)	+= bamboo.o
diff --git a/arch/powerpc/platforms/44x/idle.c b/arch/powerpc/platforms/44x/idle.c
new file mode 100644
--- /dev/null
+++ b/arch/powerpc/platforms/44x/idle.c
@@ -0,0 +1,84 @@
+/*
+ * Copyright 2008 IBM Corp. 
+ *
+ * Derived from pasemi/idle.c 
+ * 	by Olof Johansson <olof@lixom.net>
+ *
+ * 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 version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ */
+
+#include <linux/of.h>
+#include <linux/kernel.h>
+#include <asm/machdep.h>
+
+static int current_mode = 0;
+
+struct sleep_mode {
+	char *name;
+	void (*entry)(void);
+};
+
+static void ppc44x_idle(void)
+{
+	unsigned long msr_save;
+
+	msr_save = mfmsr();
+	/* set wait state MSR */
+	mtmsr(msr_save|MSR_WE|MSR_EE|MSR_CE);
+	/* return to initial state */
+	mtmsr(msr_save);
+}
+
+static struct sleep_mode modes[] = {
+	{ .name = "spin", .entry = NULL },
+	{ .name = "wait", .entry = &ppc44x_idle },
+};
+
+int __init ppc44x_idle_init(void)
+{
+	void *func = modes[current_mode].entry;
+	struct device_node *node;
+
+	node = of_find_node_by_path("/hypervisor");
+	if (node) {
+		/* if we find /hypervisor node is in device tree,
+		   set idle mode to wait */
+		func = &ppc44x_idle; /* wait */
+		of_node_put(node);
+	}
+
+	ppc_md.power_save = func;
+	return 0;
+}
+
+arch_initcall(ppc44x_idle_init);
+
+static int __init idle_param(char *p)
+{ 
+	int i;
+
+	for (i = 0; i < sizeof(modes)/ARRAY_SIZE(modes); i++) {
+		if (!strcmp(modes[i].name, p)) {
+			current_mode = i;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+early_param("idle", idle_param);

Re: [kvm-ppc-devel] [PATCH] [v2] Add idle wait support for 44x platforms

[Jimi Xenidis]

On Apr 4, 2008, at 3:06 AM, Jerone Young wrote:

> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1207292108 18000
> # Node ID afed3e5de82ab6c0ac8d6ceeb0292b6c41ece1ed
> # Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
> [v2] Add idle wait support for 44x platforms
>
> This patch adds the ability for the CPU to go into wait state while  
> in cpu_idle loop. This helps virtulization solutions know when the  
> guest Linux kernel is in an idle state. There are two ways to do it.
>
> 1) Command line
> 	idle=spin <-- CPU will spin (this is the default)
> 	idle=wait <-- set CPU into wait state when idle
>
> 2) The device tree will be checked for the "/hypervisor" node
>    If this node is seen it will use "wait" for idle, so that
>    the hypervisor can know when guest Linux kernel it is in
>    an idle state.
>
> This patch, unlike the last, isolates the code to 44x platforms.
>
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
>
> diff --git a/arch/powerpc/platforms/44x/Makefile b/arch/powerpc/ 
> platforms/44x/Makefile
> --- a/arch/powerpc/platforms/44x/Makefile
> +++ b/arch/powerpc/platforms/44x/Makefile
> @@ -1,4 +1,4 @@ obj-$(CONFIG_44x)	:= misc_44x.o
> -obj-$(CONFIG_44x)	:= misc_44x.o
> +obj-$(CONFIG_44x)	:= misc_44x.o idle.o
>  obj-$(CONFIG_EBONY)	+= ebony.o
>  obj-$(CONFIG_TAISHAN)	+= taishan.o
>  obj-$(CONFIG_BAMBOO)	+= bamboo.o
> diff --git a/arch/powerpc/platforms/44x/idle.c b/arch/powerpc/ 
> platforms/44x/idle.c
> new file mode 100644
> --- /dev/null
> +++ b/arch/powerpc/platforms/44x/idle.c
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright 2008 IBM Corp.
> + *
> + * Derived from pasemi/idle.c
> + * 	by Olof Johansson <olof@lixom.net>
> + *
> + * 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 version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA   
> 02111-1307 USA
> + *
> + */
> +
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <asm/machdep.h>
> +
> +static int current_mode = 0;

Doesn't matter if the 0 is functionally redundant or pleasing to your  
eye, it is the "Linux way" to leave out the " = 0", so just do it  
please.

> +
> +struct sleep_mode {
> +	char *name;
> +	void (*entry)(void);
> +};
> +
> +static void ppc44x_idle(void)
Perhaps "ppc44x_wait" is more appropriate?
> +{
> +	unsigned long msr_save;
> +
> +	msr_save = mfmsr();
> +	/* set wait state MSR */
> +	mtmsr(msr_save|MSR_WE|MSR_EE|MSR_CE);

Did we decide to drop MSR_DE?

> +	/* return to initial state */
> +	mtmsr(msr_save);

It may be my paranoia but I'm pretty sure you need the isync() after  
_both_ mtmsr()s
Certainly can't hurt.

> +}
> +
> +static struct sleep_mode modes[] = {
> +	{ .name = "spin", .entry = NULL },
> +	{ .name = "wait", .entry = &ppc44x_idle },
> +};
> +
> +int __init ppc44x_idle_init(void)
> +{
> +	void *func = modes[current_mode].entry;
> +	struct device_node *node;
> +
> +	node = of_find_node_by_path("/hypervisor");
> +	if (node) {
> +		/* if we find /hypervisor node is in device tree,
> +		   set idle mode to wait */
> +		func = &ppc44x_idle; /* wait */
> +		of_node_put(node);
> +	}
> +
> +	ppc_md.power_save = func;
> +	return 0;
> +}
> +
> +arch_initcall(ppc44x_idle_init);

IIRC, this would over-ride the idle_param() below, is that the  
intended behavior?

> +
> +static int __init idle_param(char *p)
> +{
> +	int i;
> +
> +	for (i = 0; i < sizeof(modes)/ARRAY_SIZE(modes); i++) {

It is supposed to be:
	for (i = 0; i < ARRAY_SIZE(modes); i++) {
What you have will actually eval to 0 and would have never checked  
anything :)

> +		if (!strcmp(modes[i].name, p)) {
> +			current_mode = i;
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +early_param("idle", idle_param);
>
> ---------------------------------------------------------------------- 
> ---
> 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] [v2] Add idle wait support for 44x platforms

[Jerone Young]

On Fri, 2008-04-04 at 08:36 -0400, Jimi Xenidis wrote:
> On Apr 4, 2008, at 3:06 AM, Jerone Young wrote:
> 
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1207292108 18000
> > # Node ID afed3e5de82ab6c0ac8d6ceeb0292b6c41ece1ed
> > # Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
> > [v2] Add idle wait support for 44x platforms
> >
> > This patch adds the ability for the CPU to go into wait state while  
> > in cpu_idle loop. This helps virtulization solutions know when the  
> > guest Linux kernel is in an idle state. There are two ways to do it.
> >
> > 1) Command line
> > 	idle=spin <-- CPU will spin (this is the default)
> > 	idle=wait <-- set CPU into wait state when idle
> >
> > 2) The device tree will be checked for the "/hypervisor" node
> >    If this node is seen it will use "wait" for idle, so that
> >    the hypervisor can know when guest Linux kernel it is in
> >    an idle state.
> >
> > This patch, unlike the last, isolates the code to 44x platforms.
> >
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> >
> > diff --git a/arch/powerpc/platforms/44x/Makefile b/arch/powerpc/ 
> > platforms/44x/Makefile
> > --- a/arch/powerpc/platforms/44x/Makefile
> > +++ b/arch/powerpc/platforms/44x/Makefile
> > @@ -1,4 +1,4 @@ obj-$(CONFIG_44x)	:= misc_44x.o
> > -obj-$(CONFIG_44x)	:= misc_44x.o
> > +obj-$(CONFIG_44x)	:= misc_44x.o idle.o
> >  obj-$(CONFIG_EBONY)	+= ebony.o
> >  obj-$(CONFIG_TAISHAN)	+= taishan.o
> >  obj-$(CONFIG_BAMBOO)	+= bamboo.o
> > diff --git a/arch/powerpc/platforms/44x/idle.c b/arch/powerpc/ 
> > platforms/44x/idle.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/44x/idle.c
> > @@ -0,0 +1,84 @@
> > +/*
> > + * Copyright 2008 IBM Corp.
> > + *
> > + * Derived from pasemi/idle.c
> > + * 	by Olof Johansson <olof@lixom.net>
> > + *
> > + * 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 version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA   
> > 02111-1307 USA
> > + *
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/kernel.h>
> > +#include <asm/machdep.h>
> > +
> > +static int current_mode = 0;
> 
> Doesn't matter if the 0 is functionally redundant or pleasing to your  
> eye, it is the "Linux way" to leave out the " = 0", so just do it  
> please.
> 
Ok this can be removed.

> > +
> > +struct sleep_mode {
> > +	char *name;
> > +	void (*entry)(void);
> > +};
> > +
> > +static void ppc44x_idle(void)
> Perhaps "ppc44x_wait" is more appropriate?
> > +{
> > +	unsigned long msr_save;
> > +
> > +	msr_save = mfmsr();
> > +	/* set wait state MSR */
> > +	mtmsr(msr_save|MSR_WE|MSR_EE|MSR_CE);
> 
> Did we decide to drop MSR_DE?
Hollis argued it down on an earlier email on kvm-ppc-devel. Though I did
have it in one of the earlier patches.

> 
> > +	/* return to initial state */
> > +	mtmsr(msr_save);
> 
> It may be my paranoia but I'm pretty sure you need the isync() after  
> _both_ mtmsr()s
> Certainly can't hurt.

I could add this back. Though everyone had already agreed on it without
it. So figured it was safer to go witout it. But I can add it back.

> 
> > +}
> > +
> > +static struct sleep_mode modes[] = {
> > +	{ .name = "spin", .entry = NULL },
> > +	{ .name = "wait", .entry = &ppc44x_idle },
> > +};
> > +
> > +int __init ppc44x_idle_init(void)
> > +{
> > +	void *func = modes[current_mode].entry;
> > +	struct device_node *node;
> > +
> > +	node = of_find_node_by_path("/hypervisor");
> > +	if (node) {
> > +		/* if we find /hypervisor node is in device tree,
> > +		   set idle mode to wait */
> > +		func = &ppc44x_idle; /* wait */
> > +		of_node_put(node);
> > +	}
> > +
> > +	ppc_md.power_save = func;
> > +	return 0;
> > +}
> > +
> > +arch_initcall(ppc44x_idle_init);
> 
> IIRC, this would over-ride the idle_param() below, is that the  
> intended behavior?

Yes. At the moment if it detects a hypervisor in the kernel tree it
overrides what the command line says.

> 
> > +
> > +static int __init idle_param(char *p)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < sizeof(modes)/ARRAY_SIZE(modes); i++) {
> 
> It is supposed to be:
> 	for (i = 0; i < ARRAY_SIZE(modes); i++) {
> What you have will actually eval to 0 and would have never checked  
> anything :)

Ouch. My bad. Wasn't thinking when I added that.

> 
> > +		if (!strcmp(modes[i].name, p)) {
> > +			current_mode = i;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +early_param("idle", idle_param);
> >
> > ---------------------------------------------------------------------- 
> > ---
> > 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] [v2] Add idle wait support for 44x platforms

[Hollis Blanchard]

On Friday 04 April 2008 10:42:41 Jerone Young wrote:
> > > +{
> > > +=A0=A0=A0unsigned long msr_save;
> > > +
> > > +=A0=A0=A0msr_save =3D mfmsr();
> > > +=A0=A0=A0/* set wait state MSR */
> > > +=A0=A0=A0mtmsr(msr_save|MSR_WE|MSR_EE|MSR_CE);
> >
> > Did we decide to drop MSR_DE?
>
> Hollis argued it down on an earlier email on kvm-ppc-devel. Though I did
> have it in one of the earlier patches.

I didn't comment either way. However, looking at the 440 user manual now, i=
t=20
seems clear that DE should be set to allow JTAG debugger events.

=2D-=20
Hollis Blanchard
IBM Linux Technology Center

Re: [kvm-ppc-devel] [PATCH] [v2] Add idle wait support for 44x platforms

[Arnd Bergmann]

On Friday 04 April 2008, Jerone Young wrote:
> > > +int __init ppc44x_idle_init(void)
> > > +{
> > > +=A0=A0=A0void *func =3D modes[current_mode].entry;
> > > +=A0=A0=A0struct device_node *node;
> > > +
> > > +=A0=A0=A0node =3D of_find_node_by_path("/hypervisor");
> > > +=A0=A0=A0if (node) {
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* if we find /hypervisor node is i=
n device tree,
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 set idle mode to wait */
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0func =3D &ppc44x_idle; /* wait */
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0of_node_put(node);
> > > +=A0=A0=A0}
> > > +
> > > +=A0=A0=A0ppc_md.power_save =3D func;
> > > +=A0=A0=A0return 0;
> > > +}
> > > +
> > > +arch_initcall(ppc44x_idle_init);
> >=20
> > IIRC, this would over-ride the idle_param() below, is that the =A0
> > intended behavior?
>=20
> Yes. At the moment if it detects a hypervisor in the kernel tree it
> overrides what the command line says.
>=20

This is unusual behavior. Normally, we try to come up with reasonable
defaults and give the user the chance to override it.

	Arnd <><

Re: [kvm-ppc-devel] [PATCH] [v2] Add idle wait support for 44x platforms

[Josh Boyer]

On Tue, 8 Apr 2008 04:21:44 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Friday 04 April 2008, Jerone Young wrote:
> > > > +int __init ppc44x_idle_init(void)
> > > > +{
> > > > +=C2=A0=C2=A0=C2=A0void *func =3D modes[current_mode].entry;
> > > > +=C2=A0=C2=A0=C2=A0struct device_node *node;
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0node =3D of_find_node_by_path("/hypervisor");
> > > > +=C2=A0=C2=A0=C2=A0if (node) {
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
/* if we find /hypervisor node is in device tree,
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 =C2=A0 set idle mode to wait */
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
func =3D &ppc44x_idle; /* wait */
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
of_node_put(node);
> > > > +=C2=A0=C2=A0=C2=A0}
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0ppc_md.power_save =3D func;
> > > > +=C2=A0=C2=A0=C2=A0return 0;
> > > > +}
> > > > +
> > > > +arch_initcall(ppc44x_idle_init);
> > >=20
> > > IIRC, this would over-ride the idle_param() below, is that the =C2=A0
> > > intended behavior?
> >=20
> > Yes. At the moment if it detects a hypervisor in the kernel tree it
> > overrides what the command line says.
> >=20
>=20
> This is unusual behavior. Normally, we try to come up with reasonable
> defaults and give the user the chance to override it.

The /hypervisor check is going away completely in the next version of
the patch.  I've spoken to the HW guys and am more comfortable with
wait being the default.  The spin version should still be an option,
specified by the kernel parameter.

josh

Re: [kvm-ppc-devel] [PATCH] [v2] Add idle wait support for 44x platforms

[Segher Boessenkool]

>> +	msr_save = mfmsr();
>> +	/* set wait state MSR */
>> +	mtmsr(msr_save|MSR_WE|MSR_EE|MSR_CE);
>
> Did we decide to drop MSR_DE?
>
>> +	/* return to initial state */
>> +	mtmsr(msr_save);
>
> It may be my paranoia but I'm pretty sure you need the isync() after
> _both_ mtmsr()s
> Certainly can't hurt.

It hurts because it spreads the paranoia.  Cargo cult.

Please add a comment that explains _why_ you need a context 
synchronising
instruction here.  The POWER ISA 2.05 says that the requirements for
changing the WE bit are implementation dependent, both before and after
the mtmsr, so the safe thing to do would be to use the "big hammer" 
isync
both before and after each of these mtmsrs.  A sync before entering wait
state might be needed as well, dunno.  Either way, comment please :-)


Segher