[PATCH] do firmware feature fixups after features are initialised

On pSeries the firmware features are not setup until ppc_md.init_early,
so we can't do the firmware feature sections fixups till after this.

Currently firmware feature sections is only used on iSeries which inits
the firmware features much earlier.  This is a bug in waiting on
pSeries. 

Signed-off-by: Michael Neuling <redacted>
---
paulus: since we aren't hitting this currently, it can wait for 2.6.23. 

 arch/powerpc/kernel/setup_64.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: linux-2.6-ozlabs/arch/powerpc/kernel/setup_64.c
===================================================================

--- linux-2.6-ozlabs.orig/arch/powerpc/kernel/setup_64.c
+++ linux-2.6-ozlabs/arch/powerpc/kernel/setup_64.c

@@ -350,13 +350,11 @@ void __init setup_system(void)
 {
 	DBG(" -> setup_system()\n");
 
-	/* Apply the CPUs-specific and firmware specific fixups to kernel
-	 * text (nop out sections not relevant to this CPU or this firmware)
+	/* Apply CPUs-specific fixups to kernel text (nop out sections
+	 * not relevant to this CPU)
 	 */
 	do_feature_fixups(cur_cpu_spec->cpu_features,
 			  &__start___ftr_fixup, &__stop___ftr_fixup);
-	do_feature_fixups(powerpc_firmware_features,
-			  &__start___fw_ftr_fixup, &__stop___fw_ftr_fixup);
 
 	/*
 	 * Unflatten the device-tree passed by prom_init or kexec

@@ -394,6 +392,12 @@ void __init setup_system(void)
 	if (ppc_md.init_early)
 		ppc_md.init_early();
 
+	/* Apply firmware specific fixups to kernel text (nop out
+	 * sections not relevant to this firmware)
+	 */
+	do_feature_fixups(powerpc_firmware_features,
+			  &__start___fw_ftr_fixup, &__stop___fw_ftr_fixup);
+
  	/*
 	 * We can discover serial ports now since the above did setup the
 	 * hash table management for us, thus ioremap works. We do that early

On Fri, 2007-06-08 at 14:00 +1000, Michael Neuling wrote:

On pSeries the firmware features are not setup until ppc_md.init_early,
so we can't do the firmware feature sections fixups till after this.

Currently firmware feature sections is only used on iSeries which inits
the firmware features much earlier.  This is a bug in waiting on
pSeries. 

Signed-off-by: Michael Neuling <redacted>
---
paulus: since we aren't hitting this currently, it can wait for 2.6.23. 

This patch will cause the kernel to blow up at boot on various machines,
I'm surprised we haven't hit that already.

The problem is that we can't service SLB miss on a !iseries machine if
CONFIG_PPC_ISERIES is set, before the fixup occurs. (Some iseries code
in there will not have been nop'ed out and SRR0 will be loaded with
crap).

Thus we die when unflattening the device-tree on some machines.

There are two possibly solutions I see in the long run:

 - We could set the FW features earlier on pseries, though that is a bit
annoying because that means doing it before the device-tree is
unflattened.

 - We could constraint lmb_alloc to the first segment until the FW fixup
occurs, either within lmb_alloc itself, or fixup the callers such as
unflatten_device_tree, to pass an explicit limit.

What do you think ?

Ben.

There are two possibly solutions I see in the long run:

 - We could set the FW features earlier on pseries, though that is  
a bit
annoying because that means doing it before the device-tree is
unflattened.

 - We could constraint lmb_alloc to the first segment until the FW  
fixup
occurs, either within lmb_alloc itself, or fixup the callers such as
unflatten_device_tree, to pass an explicit limit.

c) use a different SLB handler during early boot;
d) preload some SLB entries at very early boot to cover
    the first 4GB or so.

d) sounds nice and simple, but will it work?


Segher

On Wed, 2007-07-11 at 13:06 +0200, Segher Boessenkool wrote:

quoted

There are two possibly solutions I see in the long run:

 - We could set the FW features earlier on pseries, though that is  
a bit
annoying because that means doing it before the device-tree is
unflattened.

 - We could constraint lmb_alloc to the first segment until the FW  
fixup
occurs, either within lmb_alloc itself, or fixup the callers such as
unflatten_device_tree, to pass an explicit limit.

c) use a different SLB handler during early boot;
d) preload some SLB entries at very early boot to cover
    the first 4GB or so.

d) sounds nice and simple, but will it work?

Nah, best to limit everything at boot to SLB 0. Thing is, 4G is not
enough, you may have more RAM and have allocations from the top.

Ben.

On Wednesday 11 July 2007, Benjamin Herrenschmidt wrote:

There are two possibly solutions I see in the long run:
=20
=A0- We could set the FW features earlier on pseries, though that is a bit
annoying because that means doing it before the device-tree is
unflattened.
=20
=A0- We could constraint lmb_alloc to the first segment until the FW fixup
occurs, either within lmb_alloc itself, or fixup the callers such as
unflatten_device_tree, to pass an explicit limit.
=20
What do you think ?

If I'm understanding this right, the first solution should be something
along the lines of the patch below (not tested), which even removes
more lines than it adds. It doesn't seem that annoying to me, and it
makes sense to assume that the fw_features are set up after returning
from the ppc_md probe.

	Arnd <><

Index: linux-2.6/arch/powerpc/platforms/pseries/setup.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- linux-2.6.orig/arch/powerpc/platforms/pseries/setup.c

+++ linux-2.6/arch/powerpc/platforms/pseries/setup.c

@@ -327,8 +327,6 @@ static void __init pSeries_init_early(vo
 {
 	DBG(" -> pSeries_init_early()\n");

=20
=2D	fw_feature_init();
=2D
 	if (firmware_has_feature(FW_FEATURE_LPAR))
 		find_udbg_vterm();
=20

@@ -350,14 +348,21 @@ static int __init pSeries_probe_hypertas
 					 const char *uname, int depth,
 					 void *data)
 {
+	const char *hypertas;
+	unsigned long len;
+
 	if (depth !=3D 1 ||
 	    (strcmp(uname, "rtas") !=3D 0 && strcmp(uname, "rtas@0") !=3D 0))

=2D 		return 0;
+		return 0;
+
+	hypertas =3D of_get_flat_dt_prop(node, "ibm,hypertas-functions", &len);
+	if (!hypertas)
+		return 1;
=20
=2D	if (of_get_flat_dt_prop(node, "ibm,hypertas-functions", NULL) !=3D NULL)
=2D 		powerpc_firmware_features |=3D FW_FEATURE_LPAR;
+	powerpc_firmware_features |=3D FW_FEATURE_LPAR;
+	fw_feature_init(hypertas, len);
=20
=2D 	return 1;
+	return 1;
 }
=20
 static int __init pSeries_probe(void)
Index: linux-2.6/arch/powerpc/platforms/pseries/pseries.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- linux-2.6.orig/arch/powerpc/platforms/pseries/pseries.h

+++ linux-2.6/arch/powerpc/platforms/pseries/pseries.h

@@ -10,7 +10,7 @@
 #ifndef _PSERIES_PSERIES_H
 #define _PSERIES_PSERIES_H

=20
=2Dextern void __init fw_feature_init(void);
+extern void __init fw_feature_init(const char *, unsigned long);
=20
 struct pt_regs;
=20

diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platf=

orms/pseries/firmware.c
index 29bf83b..b0c8dbd 100644
=2D-- a/arch/powerpc/platforms/pseries/firmware.c

+++ b/arch/powerpc/platforms/pseries/firmware.c

@@ -66,24 +66,14 @@ firmware_features_table[FIRMWARE_MAX_FEATURES] =3D {
  * device-tree/ibm,hypertas-functions.  Ultimately this functionality may
  * be moved into prom.c prom_init().
  */

=2Dvoid __init fw_feature_init(void)
+void __init fw_feature_init(const char *hypertas, unsigned long len)
 {
 	struct device_node *dn;
 	const char *hypertas, *s;
=2D	int len, i;
+	int i;
=20
 	DBG(" -> fw_feature_init()\n");
=20
=2D	dn =3D of_find_node_by_path("/rtas");
=2D	if (dn =3D=3D NULL) {
=2D		printk(KERN_ERR "WARNING! Cannot find RTAS in device-tree!\n");
=2D		goto out;
=2D	}
=2D
=2D	hypertas =3D of_get_property(dn, "ibm,hypertas-functions", &len);
=2D	if (hypertas =3D=3D NULL)
=2D		goto out;
=2D
 	for (s =3D hypertas; s < hypertas + len; s +=3D strlen(s) + 1) {
 		for (i =3D 0; i < FIRMWARE_MAX_FEATURES; i++) {
 			/* check value against table of strings */

quoted

d) preload some SLB entries at very early boot to cover
    the first 4GB or so.

d) sounds nice and simple, but will it work?

Nah, best to limit everything at boot to SLB 0. Thing is, 4G is not
enough, you may have more RAM and have allocations from the top.

Yeah fair enough.  You could of course preload the stuff at
the top then, but that could quickly become a nasty hack, too.


Segher

In message [off-list ref] you wrote:

On Wednesday 11 July 2007, Benjamin Herrenschmidt wrote:

quoted

There are two possibly solutions I see in the long run:
=

quoted

=A0- We could set the FW features earlier on pseries, though that is a bit
annoying because that means doing it before the device-tree is
unflattened.
=

quoted

=A0- We could constraint lmb_alloc to the first segment until the FW fixup
occurs, either within lmb_alloc itself, or fixup the callers such as
unflatten_device_tree, to pass an explicit limit.
=

quoted

What do you think ?

If I'm understanding this right, the first solution should be something
along the lines of the patch below (not tested), which even removes
more lines than it adds. It doesn't seem that annoying to me, and it
makes sense to assume that the fw_features are set up after returning
from the ppc_md probe.

I'm not sure this patch is going to work as the do_feature_fixups isn't
called any earlier?

Mikey

)

On Thursday 12 July 2007, Michael Neuling wrote:

quoted

If I'm understanding this right, the first solution should be something
along the lines of the patch below (not tested), which even removes
more lines than it adds. It doesn't seem that annoying to me, and it
makes sense to assume that the fw_features are set up after returning
from the ppc_md probe.

I'm not sure this patch is going to work as the do_feature_fixups isn't
called any earlier?

Right, my patch still assumes that yours gets removed.

	Arnd <><

quoted

quoted

If I'm understanding this right, the first solution should be something
along the lines of the patch below (not tested), which even removes
more lines than it adds. It doesn't seem that annoying to me, and it
makes sense to assume that the fw_features are set up after returning
from the ppc_md probe.

I'm not sure this patch is going to work as the do_feature_fixups isn't
called any earlier?

Right, my patch still assumes that yours gets removed.

Arrh, cool.. that would better :-)

Mikey

Move firmware feature initialisation from pSeries_init_early to the
earlier pSeries_probe_hypertas so they are initialised before firmware
feature fixups are applied.

Currently firmware feature sections are only used for iSeries which
initialises the these features much earlier.  This is a bug in waiting
on pSeries.

Also adds some whitespace fixups.

Signed-off-by: Michael Neuling <redacted>
---

quoted

There are two possibly solutions I see in the long run:
=20
=A0- We could set the FW features earlier on pseries, though that is a bit
annoying because that means doing it before the device-tree is
unflattened.
=20
=A0- We could constraint lmb_alloc to the first segment until the FW fixup
occurs, either within lmb_alloc itself, or fixup the callers such as
unflatten_device_tree, to pass an explicit limit.
=20
What do you think ?

If I'm understanding this right, the first solution should be something
along the lines of the patch below (not tested), which even removes
more lines than it adds. It doesn't seem that annoying to me, and it
makes sense to assume that the fw_features are set up after returning
from the ppc_md probe.

I've cleaned this up and got it booting.  Seems to be doing the right
things as I can see fw feature sections being correctly NOPed out now
after boot.  

I've booted with pseries_defconfig and ppc64_defconfig.

Arnd, I've added a signed off by me, but it but this probably needs an
explicit one from you also before it heads up.

Mikey

 arch/powerpc/platforms/pseries/firmware.c |   19 +++----------------
 arch/powerpc/platforms/pseries/pseries.h  |    2 +-
 arch/powerpc/platforms/pseries/setup.c    |   17 +++++++++++------
 3 files changed, 15 insertions(+), 23 deletions(-)

Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/firmware.c
===================================================================

--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/firmware.c
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/firmware.c

@@ -66,24 +66,13 @@ firmware_features_table[FIRMWARE_MAX_FEA
  * device-tree/ibm,hypertas-functions.  Ultimately this functionality may
  * be moved into prom.c prom_init().
  */
-void __init fw_feature_init(void)
+void __init fw_feature_init(const char *hypertas, unsigned long len)
 {
-	struct device_node *dn;
-	const char *hypertas, *s;
-	int len, i;
+	const char *s;
+	int i;
 
 	DBG(" -> fw_feature_init()\n");
 
-	dn = of_find_node_by_path("/rtas");
-	if (dn == NULL) {
-		printk(KERN_ERR "WARNING! Cannot find RTAS in device-tree!\n");
-		goto out;
-	}
-
-	hypertas = of_get_property(dn, "ibm,hypertas-functions", &len);
-	if (hypertas == NULL)
-		goto out;
-
 	for (s = hypertas; s < hypertas + len; s += strlen(s) + 1) {
 		for (i = 0; i < FIRMWARE_MAX_FEATURES; i++) {
 			/* check value against table of strings */

@@ -98,7 +87,5 @@ void __init fw_feature_init(void)
 		}
 	}
 
-out:
-	of_node_put(dn);
 	DBG(" <- fw_feature_init()\n");
 }

Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/pseries.h
===================================================================

--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/pseries.h
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/pseries.h

@@ -10,7 +10,7 @@
 #ifndef _PSERIES_PSERIES_H
 #define _PSERIES_PSERIES_H
 
-extern void __init fw_feature_init(void);
+extern void __init fw_feature_init(const char *hypertas, unsigned long len);
 
 struct pt_regs;
 

Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/setup.c
===================================================================

--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/setup.c
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/setup.c

@@ -320,8 +320,6 @@ static void __init pSeries_init_early(vo
 {
 	DBG(" -> pSeries_init_early()\n");
 
-	fw_feature_init();
-
 	if (firmware_has_feature(FW_FEATURE_LPAR))
 		find_udbg_vterm();
 

@@ -343,14 +341,21 @@ static int __init pSeries_probe_hypertas
 					 const char *uname, int depth,
 					 void *data)
 {
+	const char *hypertas;
+	unsigned long len;
+
 	if (depth != 1 ||
 	    (strcmp(uname, "rtas") != 0 && strcmp(uname, "rtas@0") != 0))
- 		return 0;
+		return 0;
+
+	hypertas = of_get_flat_dt_prop(node, "ibm,hypertas-functions", &len);
+	if (!hypertas)
+		return 1;
 
-	if (of_get_flat_dt_prop(node, "ibm,hypertas-functions", NULL) != NULL)
- 		powerpc_firmware_features |= FW_FEATURE_LPAR;
+	powerpc_firmware_features |= FW_FEATURE_LPAR;
+	fw_feature_init(hypertas, len);
 
- 	return 1;
+	return 1;
 }
 
 static int __init pSeries_probe(void)

On Wednesday 18 July 2007, Michael Neuling wrote:

Move firmware feature initialisation from pSeries_init_early to the
earlier pSeries_probe_hypertas so they are initialised before firmware
feature fixups are applied.
=20
Currently firmware feature sections are only used for iSeries which
initialises the these features much earlier. =A0This is a bug in waiting
on pSeries.
=20
Also adds some whitespace fixups.
=20
Signed-off-by: Michael Neuling <redacted>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Haven't tested it myself, but it certainly looks good to me. It does
require reverting your previous patch though, are you submitting the
reversal patch as well?

	Arnd <><

On Wednesday 18 July 2007, Michael Neuling wrote:

quoted

Move firmware feature initialisation from pSeries_init_early to the
earlier pSeries_probe_hypertas so they are initialised before firmware
feature fixups are applied.
=20
Currently firmware feature sections are only used for iSeries which
initialises the these features much earlier. =A0This is a bug in waiting
on pSeries.
=20
Also adds some whitespace fixups.
=20
Signed-off-by: Michael Neuling <redacted>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Haven't tested it myself, but it certainly looks good to me. It does
require reverting your previous patch though, are you submitting the
reversal patch as well?

Yep, this got reverted yesterday in 
826ea8f22cf612d534f33c492c98f7895043bfd1 

Mikey