[Skiboot] [PATCH v7 15/22] fadump: Introduce new reboot type

[Michael Neuling]

On Mon, 2019-05-13 at 20:55 +0530, Vasant Hegde wrote:
> On 05/09/2019 09:08 AM, Michael Neuling wrote:
> > On Sat, 2019-04-13 at 14:45 +0530, Vasant Hegde wrote:
> > > Enhance reboot2 call to support FADUMP. Payload will call this interface
> > > to initiate fadump.
> > > 
> > > Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> > > ---
> > >   core/platform.c                        | 7 ++++++-
> > >   doc/opal-api/opal-cec-reboot-6-116.rst | 7 +++++++
> > >   include/opal-api.h                     | 1 +
> > >   3 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/core/platform.c b/core/platform.c
> > > index 570a4309a..5283fce0a 100644
> > > --- a/core/platform.c
> > > +++ b/core/platform.c
> > > @@ -1,4 +1,4 @@
> > > -/* Copyright 2013-2016 IBM Corp.
> > > +/* Copyright 2013-2019 IBM Corp.
> > >    *
> > >    * Licensed under the Apache License, Version 2.0 (the "License");
> > >    * you may not use this file except in compliance with the License.
> > > @@ -103,6 +103,11 @@ static int64_t opal_cec_reboot2(uint32_t reboot_type,
> > > char *diag)
> > >   	case OPAL_REBOOT_FULL_IPL:
> > >   		disable_fast_reboot("full IPL reboot requested");
> > >   		return opal_cec_reboot();
> > > +	case OPAL_REBOOT_OS_ERROR:
> > > +		prlog(PR_ERR, "Kernel requested for fadump\n");
> > 
> > PR_INFO
> 
> Yep! PR_ERR is not nice. Changed it to PR_NOTICE so that log comes to console.
> 
> > the error message doesn't match the opal call name... This is a bit
> > confusing

I'd like the call name and print to be consistent.  The print says "kernel
requested fadump", but it didn't. The kernel actually did a call saying
OS_ERROR.

The print should be something like "Reboot: OS reported error. Performing dump".

> > 
> > > +		console_complete_flush();
> > > +		assert(false);
> > 
> > Won't assert generate other prints we don't want?
> 
> You are right.. May be we should avoid unnecessary back traces. How about this
> one?
> 
> +               if (platform.terminate)
> +                       platform.terminate("Kernel requested for fadump");
> +               else
> +                       assert(false);
> 
> Note that terminate path checks for MPIPL support and makes appropriate call.

I think we can just fix all the terminate calls to do the right thing. Then
maybe could just call _abort() here (which calls .terminate + fallback for us).

I think the assert() is just confusing me.

Sorry to be a pain, but this code just seems a bit confusing in something that
should be easy to make clear.

> > What happens on other platforms when this happens? I'm pretty sure an assert
> > will not generate a reboot (I guess it says so below)
> 
> You mean platform where we don't support MPIPL? Then it will fall back to
> normal
> reboot.

ok.

Mikey