[PATCH] powerpc: fix os-term usage on kernel panic

[PATCH] powerpc: fix os-term usage on kernel panic

[Linas Vepstas]

The rtas_os_term() routine was being called at the wrong time.
The actual rtas call "os-term" will not ever return, and so
calling it from the panic notifier is too early.  Instead,
call it from the machine_reset() call.

The patch splits the rtas_os_term() routine into two: one 
part to capture the kernel panic message, invoked during
the panic notifier, and another part that is invoked during 
machine_reset().

Prior to this patch, the os-term call was never being made,
because panic_timeout was always non-zero. Calling os-term 
helps keep the hypervisor happy! We have to keep the hypervisor
happy to avoid service, dump and error reporting problems.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
One could make a strong argument to move all of this code 
from kernel/rtas.c to platforms/pseries/setup.c  I did not
do this, just so as to make the changes minimal.

 arch/powerpc/kernel/rtas.c             |   12 ++++++------
 arch/powerpc/platforms/pseries/setup.c |    3 ++-
 include/asm-powerpc/rtas.h             |    3 ++-
 3 files changed, 10 insertions(+), 8 deletions(-)

Index: linux-2.6.24-rc2-git4/arch/powerpc/kernel/rtas.c
===================================================================
--- linux-2.6.24-rc2-git4.orig/arch/powerpc/kernel/rtas.c	2007-11-19 18:58:53.000000000 -0600
+++ linux-2.6.24-rc2-git4/arch/powerpc/kernel/rtas.c	2007-11-19 19:01:10.000000000 -0600
@@ -631,18 +631,18 @@ void rtas_halt(void)
 /* Must be in the RMO region, so we place it here */
 static char rtas_os_term_buf[2048];
 
-void rtas_os_term(char *str)
+void rtas_panic_msg(char *str)
 {
-	int status;
+	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
+}
 
-	if (panic_timeout)
-		return;
+void rtas_os_term(void)
+{
+	int status;
 
 	if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term"))
 		return;
 
-	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
-
 	do {
 		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
 				   __pa(rtas_os_term_buf));
Index: linux-2.6.24-rc2-git4/arch/powerpc/platforms/pseries/setup.c
===================================================================
--- linux-2.6.24-rc2-git4.orig/arch/powerpc/platforms/pseries/setup.c	2007-11-19 18:58:53.000000000 -0600
+++ linux-2.6.24-rc2-git4/arch/powerpc/platforms/pseries/setup.c	2007-11-19 19:01:10.000000000 -0600
@@ -507,7 +507,8 @@ define_machine(pseries) {
 	.restart		= rtas_restart,
 	.power_off		= pSeries_power_off,
 	.halt			= rtas_halt,
-	.panic			= rtas_os_term,
+	.panic			= rtas_panic_msg,
+	.machine_shutdown	= rtas_os_term,
 	.get_boot_time		= rtas_get_boot_time,
 	.get_rtc_time		= rtas_get_rtc_time,
 	.set_rtc_time		= rtas_set_rtc_time,
Index: linux-2.6.24-rc2-git4/include/asm-powerpc/rtas.h
===================================================================
--- linux-2.6.24-rc2-git4.orig/include/asm-powerpc/rtas.h	2007-11-19 18:58:53.000000000 -0600
+++ linux-2.6.24-rc2-git4/include/asm-powerpc/rtas.h	2007-11-19 19:01:10.000000000 -0600
@@ -164,7 +164,8 @@ extern int rtas_call(int token, int, int
 extern void rtas_restart(char *cmd);
 extern void rtas_power_off(void);
 extern void rtas_halt(void);
-extern void rtas_os_term(char *str);
+extern void rtas_panic_msg(char *str);
+extern void rtas_os_term(void);
 extern int rtas_get_sensor(int sensor, int index, int *state);
 extern int rtas_get_power_level(int powerdomain, int *level);
 extern int rtas_set_power_level(int powerdomain, int level, int *setlevel);

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Will Schmidt]

(resending with the proper "from" addr this time). 


I'm seeing some funky behavior on power5/power6 partitions with this
patch.    A "/sbin/reboot" is now behaving much more like a
"/sbin/halt".

Anybody else seeing this, or is it time for me to call an exorcist for
my boxes? 

-Will

On Mon, 2007-11-19 at 19:28 -0600, Linas Vepstas wrote:
> The rtas_os_term() routine was being called at the wrong time.
> The actual rtas call "os-term" will not ever return, and so
> calling it from the panic notifier is too early.  Instead,
> call it from the machine_reset() call.
> 
> The patch splits the rtas_os_term() routine into two: one 
> part to capture the kernel panic message, invoked during
> the panic notifier, and another part that is invoked during 
> machine_reset().
> 
> Prior to this patch, the os-term call was never being made,
> because panic_timeout was always non-zero. Calling os-term 
> helps keep the hypervisor happy! We have to keep the hypervisor
> happy to avoid service, dump and error reporting problems.
> 
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> 
> ----
> One could make a strong argument to move all of this code 
> from kernel/rtas.c to platforms/pseries/setup.c  I did not
> do this, just so as to make the changes minimal.
> 
>  arch/powerpc/kernel/rtas.c             |   12 ++++++------
>  arch/powerpc/platforms/pseries/setup.c |    3 ++-
>  include/asm-powerpc/rtas.h             |    3 ++-
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6.24-rc2-git4/arch/powerpc/kernel/rtas.c
> ===================================================================
> --- linux-2.6.24-rc2-git4.orig/arch/powerpc/kernel/rtas.c	2007-11-19 18:58:53.000000000 -0600
> +++ linux-2.6.24-rc2-git4/arch/powerpc/kernel/rtas.c	2007-11-19 19:01:10.000000000 -0600
> @@ -631,18 +631,18 @@ void rtas_halt(void)
>  /* Must be in the RMO region, so we place it here */
>  static char rtas_os_term_buf[2048];
> 
> -void rtas_os_term(char *str)
> +void rtas_panic_msg(char *str)
>  {
> -	int status;
> +	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
> +}
> 
> -	if (panic_timeout)
> -		return;
> +void rtas_os_term(void)
> +{
> +	int status;
> 
>  	if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term"))
>  		return;
> 
> -	snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str);
> -
>  	do {
>  		status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL,
>  				   __pa(rtas_os_term_buf));
> Index: linux-2.6.24-rc2-git4/arch/powerpc/platforms/pseries/setup.c
> ===================================================================
> --- linux-2.6.24-rc2-git4.orig/arch/powerpc/platforms/pseries/setup.c	2007-11-19 18:58:53.000000000 -0600
> +++ linux-2.6.24-rc2-git4/arch/powerpc/platforms/pseries/setup.c	2007-11-19 19:01:10.000000000 -0600
> @@ -507,7 +507,8 @@ define_machine(pseries) {
>  	.restart		= rtas_restart,
>  	.power_off		= pSeries_power_off,
>  	.halt			= rtas_halt,
> -	.panic			= rtas_os_term,
> +	.panic			= rtas_panic_msg,
> +	.machine_shutdown	= rtas_os_term,
>  	.get_boot_time		= rtas_get_boot_time,
>  	.get_rtc_time		= rtas_get_rtc_time,
>  	.set_rtc_time		= rtas_set_rtc_time,
> Index: linux-2.6.24-rc2-git4/include/asm-powerpc/rtas.h
> ===================================================================
> --- linux-2.6.24-rc2-git4.orig/include/asm-powerpc/rtas.h	2007-11-19 18:58:53.000000000 -0600
> +++ linux-2.6.24-rc2-git4/include/asm-powerpc/rtas.h	2007-11-19 19:01:10.000000000 -0600
> @@ -164,7 +164,8 @@ extern int rtas_call(int token, int, int
>  extern void rtas_restart(char *cmd);
>  extern void rtas_power_off(void);
>  extern void rtas_halt(void);
> -extern void rtas_os_term(char *str);
> +extern void rtas_panic_msg(char *str);
> +extern void rtas_os_term(void);
>  extern int rtas_get_sensor(int sensor, int index, int *state);
>  extern int rtas_get_power_level(int powerdomain, int *level);
>  extern int rtas_set_power_level(int powerdomain, int level, int *setlevel);
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Olaf Hering]

On Tue, Nov 27, Will Schmidt wrote:

> > -void rtas_os_term(char *str)
> > +void rtas_panic_msg(char *str)

> > -	if (panic_timeout)
> > -		return;

This change is wrong. Booting with panic=123 really means the system
has to reboot in 123 seconds after a panic.
But, maybe this panic_timeout check was moved elsewhere.

Please revert this part of commit
a2b51812a4dc5db09ab4d4638d4d8ed456e2457e before 2.6.24 is released.

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Linas Vepstas]

On Wed, Nov 28, 2007 at 12:00:37PM +0100, Olaf Hering wrote:
> On Tue, Nov 27, Will Schmidt wrote:
> 
> > > -void rtas_os_term(char *str)
> > > +void rtas_panic_msg(char *str)
> 
> > > -	if (panic_timeout)
> > > -		return;
> 
> This change is wrong. Booting with panic=123 really means the system
> has to reboot in 123 seconds after a panic.

And it does.

> But, maybe this panic_timeout check was moved elsewhere.

It was *always* somewhere else; the check here was always wrong.

This change makes the os-term call happen after the the panic
timeout amount of time has elapsed.

--linas

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Linas Vepstas]

On Tue, Nov 27, 2007 at 06:15:59PM -0600, Will Schmidt wrote:
> (resending with the proper "from" addr this time). 
> 
> 
> I'm seeing some funky behavior on power5/power6 partitions with this
> patch.    A "/sbin/reboot" is now behaving much more like a
> "/sbin/halt".
> 
> Anybody else seeing this, or is it time for me to call an exorcist for
> my boxes? 

I beleive the patch
http://www.nabble.com/-PATCH--powerpc-pseries:-tell-phyp-to-auto-restart-t4847604.html

will cure this problem.

>From that patch:

+/**
+ * pSeries_auto_restart - tell hypervisor that boot succeeded.
+ *
+ * The pseries hypervisor attempts to detect and prevent an
+ * infinite loop of kernel crashes and auto-reboots. It does
+ * so by refusing to auto-reboot unless we indicate that the
+ * current boot was sucessful.  So, indicate success late in
+ * the boot sequence.
+ */ 

FYI, I am leaving IBM in just a few days now, and won't really
have much of a chance to debug this, if there are other problems.

This pair of patches was required to make hypervisor-assisted 
dump work, viz, we need to tell the hypervisor about when we 
crashed, or didn't crash, so that if we crashed, the dump can 
be taken appropriately.

It occurs to me that, as I write this, that maybe xmon 'zr'
command should be modified to call pSeries_auto_restart just
in case, so that it actually reboots.  There might be another
funky code path that I can't think of right now.

--linas

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Olaf Hering]

On Wed, Nov 28, Linas Vepstas wrote:

> On Wed, Nov 28, 2007 at 12:00:37PM +0100, Olaf Hering wrote:
> > On Tue, Nov 27, Will Schmidt wrote:

> > > > -	if (panic_timeout)
> > > > -		return;
> > 
> > This change is wrong. Booting with panic=123 really means the system
> > has to reboot in 123 seconds after a panic.
> 
> And it does.

Have you ever tried it? Current state is that the JS20 hangs after
panic, simply because it calls into the hypervisor (or whatever).
To reproduce:
 mkdir ../O-os-term
 cp arch/powerpc/configs/pseries_defconfig ../O-os-term/.config
 yes '' | make -kj6 O=../O-os-term zImage
 scp -4pr ../O-os-term/arch/powerpc/boot/zImage root@tftp:/tftpboot/js20


 boot /pci@8000000f8000000/pci@0/ethernet@1,1 debug panic=1 root=/dev/sdx42


Whoever relies on the string passing to the HMC can boot with panic=0 or
use some sysctl to write to /proc/sys/kernel/panic
So, please restore the panic_timeout check.

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Will Schmidt]

On Wed, 2007-11-28 at 14:18 -0600, Linas Vepstas wrote:
> On Tue, Nov 27, 2007 at 06:15:59PM -0600, Will Schmidt wrote:
> > (resending with the proper "from" addr this time). 
> > 
> > 
> > I'm seeing some funky behavior on power5/power6 partitions with this
> > patch.    A "/sbin/reboot" is now behaving much more like a
> > "/sbin/halt".
> > 
> > Anybody else seeing this, or is it time for me to call an exorcist for
> > my boxes? 
> 
> I beleive the patch
> http://www.nabble.com/-PATCH--powerpc-pseries:-tell-phyp-to-auto-restart-t4847604.html
> 
> will cure this problem.

It does not.   this code is getting called, but still turns the box into
a doorstop at /sbin/reboot. 

It does clear up if I apply this patch, which is a revert of part of
your earlier patch.

My js2X also turns into a doorstop after /sbin/reboot..     Though I'm
not going through a panic path, I wonder if the panic portion is OK and
this is what Olaf is hitting.  

diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index fdeefe5..c9fac5a 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -508,7 +508,7 @@ define_machine(pseries) {
 	.power_off		= pSeries_power_off,
 	.halt			= rtas_halt,
 	.panic			= rtas_panic_msg,
-	.machine_shutdown	= rtas_os_term,
+/*	.machine_shutdown	= rtas_os_term,*/
 	.get_boot_time		= rtas_get_boot_time,
 	.get_rtc_time		= rtas_get_rtc_time,
 	.set_rtc_time		= rtas_set_rtc_time,

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Stephen Rothwell]

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

On Tue, 27 Nov 2007 18:15:59 -0600 Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
>
> (resending with the proper "from" addr this time). 
> 
> 
> I'm seeing some funky behavior on power5/power6 partitions with this
> patch.    A "/sbin/reboot" is now behaving much more like a
> "/sbin/halt".
> 
> Anybody else seeing this, or is it time for me to call an exorcist for
> my boxes? 

On my Power5+ box, I get an error (code B200A101) logged every time I
reboot and about half the time, the machine does not reboot but need to
be power cycled. Removing the cited patch makes the error not by logged
and reboots work fine.

Paul and I have been having a look at this and Paul has asked the
architects for clarification of when os-term should be used.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Will Schmidt]

On Fri, 2007-11-30 at 16:56 +1100, Stephen Rothwell wrote:
> On Tue, 27 Nov 2007 18:15:59 -0600 Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
> >
> > (resending with the proper "from" addr this time). 
> > 
> > 
> > I'm seeing some funky behavior on power5/power6 partitions with this
> > patch.    A "/sbin/reboot" is now behaving much more like a
> > "/sbin/halt".
> > 
> > Anybody else seeing this, or is it time for me to call an exorcist for
> > my boxes? 
> 
> On my Power5+ box, I get an error (code B200A101) logged every time I
> reboot and about half the time, the machine does not reboot but need to
> be power cycled. Removing the cited patch makes the error not by logged
> and reboots work fine.
> 
> Paul and I have been having a look at this and Paul has asked the
> architects for clarification of when os-term should be used.

The architects will have the correct answer of course.   

>From my reading of the papr, I've got the impression that there are two
possibilities.    

First, the os-term never returns, and it's up to the service processor
to do whatever it's going to do.  (call home, dump, something else).
Nothing we can do there. 

Second, os-term returns, and it's up to the OS to call into power-off or
system-reboot.
I think this is more likely.   I havn't followed the path back from
machine_restart to see exactly how we got there, but probably means a
bit more logic to decide whether to call into rtas_restart() or
pSeries_power_off() after the call to machine_shutdown.

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Mike Strosaker]

Will Schmidt wrote:
>>From my reading of the papr, I've got the impression that there are two
> possibilities.    
> 
> First, the os-term never returns, and it's up to the service processor
> to do whatever it's going to do.  (call home, dump, something else).
> Nothing we can do there. 
> 
> Second, os-term returns, and it's up to the OS to call into power-off or
> system-reboot.
> I think this is more likely.   I havn't followed the path back from
> machine_restart to see exactly how we got there, but probably means a
> bit more logic to decide whether to call into rtas_restart() or
> pSeries_power_off() after the call to machine_shutdown.

My understanding was that os-term is supposed to indicate an "abnormal" 
termination, which is why it was initially only associated with a panic.

The os-term behavior is kind of complex; there is both a standard behavior, and 
an "extended" behavior.  You can determine if the extended behavior will be used 
by looking for the ibm,extended-os-term property.  I think POWER5 largely uses 
the older behavior, and POWER6 uses the extended behavior.

I've never seen os-term return using the old behavior.  Whether the partition 
reboots after calling os-term depends on the setting of the 
partition_auto_restart RTAS parameter.  If partition_auto_restart is 1 when 
os-term is called, the hypervisor will restart the partition and reset 
partition_auto_restart to 0.  The OS is supposed to set partition_auto_restart 
back to 1 after it boots; that way, if the OS is failing to boot, the hypervisor 
won't continually try to restart the partition without any hope of success. 
That's why Linas sent a patch with a pSeries_auto_restart routine.

If the extended os-term behavior is used, the os-term call is supposed to return 
unless the ibm,configure-kernel-dump call was previously used (to prepare for a 
PHYP assisted dump).  Would that explain why Linas and Will are seeing different 
behaviors?  I may have missed it, but I don't see a check for 
ibm,extended-os-term anywhere.

- Mike

Re: [PATCH] powerpc: fix os-term usage on kernel panic

[Linas Vepstas]

On Thu, Nov 29, 2007 at 11:41:47AM +0100, Olaf Hering wrote:
> On Wed, Nov 28, Linas Vepstas wrote:
> 
> > On Wed, Nov 28, 2007 at 12:00:37PM +0100, Olaf Hering wrote:
> > > On Tue, Nov 27, Will Schmidt wrote:
> 
> > > > > -	if (panic_timeout)
> > > > > -		return;
> > > 
> > > This change is wrong. Booting with panic=123 really means the system
> > > has to reboot in 123 seconds after a panic.
> > 
> > And it does.
> 
> Have you ever tried it? Current state is that the JS20 hangs after
> panic, 

It should printout the "Rebooting in timeout_wait seconds ..."
Then it should wait timeout_wait number of seconds, as usual,
and *then* call the hypervisor.

> simply because it calls into the hypervisor (or whatever).

The hypervisor is not supposed to return at this point. Its supposed
to reboot. Appearently, its not rebooting. Either we are using it 
wrong, or the hypervisor is buggy on some systems. It did work on 
the machines I was on; but I did not try power5's or blades.

> So, please restore the panic_timeout check.

The problem with this check was that was that the value was never 
ever set, and so the branch was never ever taken. 

--linas