diff mbox series

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

Message ID 20190413091548.14329-16-hegdevasant@linux.vnet.ibm.com
State Changes Requested
Headers show
Series MPIPL support | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (ff79070d1c4cdc38f2ecb42e45b8322cb1efb819)
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present

Commit Message

Vasant Hegde April 13, 2019, 9:15 a.m. UTC 
Enhance reboot2 call to support FADUMP. Payload will call this interface
to initiate fadump.

Signed-off-by: Vasant Hegde <hegdevasant@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(-)

Comments

Michael Neuling May 9, 2019, 3:38 a.m. UTC | #1 
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@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

the error message doesn't match the opal call name... This is a bit confusing 

> +		console_complete_flush();
> +		assert(false);

Won't assert generate other prints we don't want? 

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) 

> +		break;
>  	default:
>  		prlog(PR_NOTICE, "OPAL: Unsupported reboot request %d\n",
> reboot_type);
>  		return OPAL_UNSUPPORTED;
> diff --git a/doc/opal-api/opal-cec-reboot-6-116.rst b/doc/opal-api/opal-cec-
> reboot-6-116.rst
> index 516d4fc01..9ac7f9f69 100644
> --- a/doc/opal-api/opal-cec-reboot-6-116.rst
> +++ b/doc/opal-api/opal-cec-reboot-6-116.rst
> @@ -63,6 +63,13 @@ OPAL_REBOOT_FULL_IPL = 2
>  	On platforms that don't support fast reboot, this is equivalent to a
>  	normal reboot.
>  
> +OPAL_REBOOT_OS_ERROR = 3
> +	Request for fadump reboot. Firmware will reboot the system and collect
> +	dump.
> +
> +	On platforms that don't support fadump, this is equivalent to a
> +	normal assert.
> +
>  Unsupported Reboot type
>  	For unsupported reboot type, this function will return with
>  	OPAL_UNSUPPORTED and no reboot will be triggered.
> diff --git a/include/opal-api.h b/include/opal-api.h
> index 702e70841..1884850a9 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -1239,6 +1239,7 @@ enum {
>  	OPAL_REBOOT_NORMAL = 0,
>  	OPAL_REBOOT_PLATFORM_ERROR,
>  	OPAL_REBOOT_FULL_IPL,
> +	OPAL_REBOOT_OS_ERROR,
>  };
>  
>  /* Argument to OPAL_PCI_TCE_KILL */
Oliver O'Halloran May 10, 2019, 1:33 a.m. UTC | #2 
On Sat, Apr 13, 2019 at 7:19 PM Vasant Hegde
<hegdevasant@linux.vnet.ibm.com> wrote:
>
> Enhance reboot2 call to support FADUMP. Payload will call this interface
> to initiate fadump.

Every other bit of bare metal firmware calls it MPIPL so I'd rather we
just stuck with that terminology for consistency. Think of it this
way: FAdump/fadump is a linux feature that we're implementing on top
of MPIPL.

> Signed-off-by: Vasant Hegde <hegdevasant@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");
> +               console_complete_flush();
> +               assert(false);
> +               break;
>         default:
>                 prlog(PR_NOTICE, "OPAL: Unsupported reboot request %d\n", reboot_type);
>                 return OPAL_UNSUPPORTED;
> diff --git a/doc/opal-api/opal-cec-reboot-6-116.rst b/doc/opal-api/opal-cec-reboot-6-116.rst
> index 516d4fc01..9ac7f9f69 100644
> --- a/doc/opal-api/opal-cec-reboot-6-116.rst
> +++ b/doc/opal-api/opal-cec-reboot-6-116.rst
> @@ -63,6 +63,13 @@ OPAL_REBOOT_FULL_IPL = 2
>         On platforms that don't support fast reboot, this is equivalent to a
>         normal reboot.
>
> +OPAL_REBOOT_OS_ERROR = 3
> +       Request for fadump reboot. Firmware will reboot the system and collect
> +       dump.
> +
> +       On platforms that don't support fadump, this is equivalent to a
> +       normal assert.
> +
>  Unsupported Reboot type
>         For unsupported reboot type, this function will return with
>         OPAL_UNSUPPORTED and no reboot will be triggered.
> diff --git a/include/opal-api.h b/include/opal-api.h
> index 702e70841..1884850a9 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -1239,6 +1239,7 @@ enum {
>         OPAL_REBOOT_NORMAL = 0,
>         OPAL_REBOOT_PLATFORM_ERROR,
>         OPAL_REBOOT_FULL_IPL,

> +       OPAL_REBOOT_OS_ERROR,

Having a "PLATFORM_ERROR" reboot type was always kind of dumb IMO so
I'd rather we didn't make that a pattern. Call it MPIPL or
MEMORY_PRESERVING_IPL. That way people might actually know what it
does.

>  };
>
>  /* Argument to OPAL_PCI_TCE_KILL */
> --
> 2.14.3
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Vasant Hegde May 13, 2019, 3:25 p.m. UTC | #3 
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@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
> 
>> +		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.



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


-Vasant
Michael Neuling May 14, 2019, 7:54 a.m. UTC | #4 
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@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
Vasant Hegde May 14, 2019, 8:48 a.m. UTC | #5 
On 05/14/2019 01:24 PM, Michael Neuling wrote:
> 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@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".

Yes. Makes sense. Will fix it.


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

Yeah. assert() is bit confusing.

May be ew can just call _abort() here.. it will print opal backtraces to console.
May be that's fine.

> 
> I think the assert() is just confusing me.

Agree.

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

Agree. if /else condition in unnecessary.

-Vasant
Vasant Hegde May 14, 2019, 8:59 a.m. UTC | #6 
On 05/10/2019 07:03 AM, Oliver wrote:
> On Sat, Apr 13, 2019 at 7:19 PM Vasant Hegde
> <hegdevasant@linux.vnet.ibm.com> wrote:
>>
>> Enhance reboot2 call to support FADUMP. Payload will call this interface
>> to initiate fadump.
> 
> Every other bit of bare metal firmware calls it MPIPL so I'd rather we
> just stuck with that terminology for consistency. Think of it this
> way: FAdump/fadump is a linux feature that we're implementing on top
> of MPIPL.

Agreed. Will fix it.

> 
>> Signed-off-by: Vasant Hegde <hegdevasant@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");
>> +               console_complete_flush();
>> +               assert(false);
>> +               break;
>>          default:
>>                  prlog(PR_NOTICE, "OPAL: Unsupported reboot request %d\n", reboot_type);
>>                  return OPAL_UNSUPPORTED;
>> diff --git a/doc/opal-api/opal-cec-reboot-6-116.rst b/doc/opal-api/opal-cec-reboot-6-116.rst
>> index 516d4fc01..9ac7f9f69 100644
>> --- a/doc/opal-api/opal-cec-reboot-6-116.rst
>> +++ b/doc/opal-api/opal-cec-reboot-6-116.rst
>> @@ -63,6 +63,13 @@ OPAL_REBOOT_FULL_IPL = 2
>>          On platforms that don't support fast reboot, this is equivalent to a
>>          normal reboot.
>>
>> +OPAL_REBOOT_OS_ERROR = 3
>> +       Request for fadump reboot. Firmware will reboot the system and collect
>> +       dump.
>> +
>> +       On platforms that don't support fadump, this is equivalent to a
>> +       normal assert.
>> +
>>   Unsupported Reboot type
>>          For unsupported reboot type, this function will return with
>>          OPAL_UNSUPPORTED and no reboot will be triggered.
>> diff --git a/include/opal-api.h b/include/opal-api.h
>> index 702e70841..1884850a9 100644
>> --- a/include/opal-api.h
>> +++ b/include/opal-api.h
>> @@ -1239,6 +1239,7 @@ enum {
>>          OPAL_REBOOT_NORMAL = 0,
>>          OPAL_REBOOT_PLATFORM_ERROR,
>>          OPAL_REBOOT_FULL_IPL,
> 
>> +       OPAL_REBOOT_OS_ERROR,
> 
> Having a "PLATFORM_ERROR" reboot type was always kind of dumb IMO so
> I'd rather we didn't make that a pattern. Call it MPIPL or
> MEMORY_PRESERVING_IPL. That way people might actually know what it
> does.

I wanted to have something OPAL_REBOOT_*. So I added it as "OPAL_REBOOT_MPIPL".
Then based on Stewart suggestion I changed it to OPAL_REBOOT_OS_ERROR.

-Vasant
Oliver O'Halloran May 14, 2019, 10:53 a.m. UTC | #7 
On Tue, May 14, 2019 at 6:59 PM Vasant Hegde
<hegdevasant@linux.vnet.ibm.com> wrote:
>
> On 05/10/2019 07:03 AM, Oliver wrote:
> > On Sat, Apr 13, 2019 at 7:19 PM Vasant Hegde
> > <hegdevasant@linux.vnet.ibm.com> wrote:
> >>
> >> Enhance reboot2 call to support FADUMP. Payload will call this interface
> >> to initiate fadump.
> >
> > Every other bit of bare metal firmware calls it MPIPL so I'd rather we
> > just stuck with that terminology for consistency. Think of it this
> > way: FAdump/fadump is a linux feature that we're implementing on top
> > of MPIPL.
>
> Agreed. Will fix it.
>
> >
> >> Signed-off-by: Vasant Hegde <hegdevasant@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");
> >> +               console_complete_flush();
> >> +               assert(false);
> >> +               break;
> >>          default:
> >>                  prlog(PR_NOTICE, "OPAL: Unsupported reboot request %d\n", reboot_type);
> >>                  return OPAL_UNSUPPORTED;
> >> diff --git a/doc/opal-api/opal-cec-reboot-6-116.rst b/doc/opal-api/opal-cec-reboot-6-116.rst
> >> index 516d4fc01..9ac7f9f69 100644
> >> --- a/doc/opal-api/opal-cec-reboot-6-116.rst
> >> +++ b/doc/opal-api/opal-cec-reboot-6-116.rst
> >> @@ -63,6 +63,13 @@ OPAL_REBOOT_FULL_IPL = 2
> >>          On platforms that don't support fast reboot, this is equivalent to a
> >>          normal reboot.
> >>
> >> +OPAL_REBOOT_OS_ERROR = 3
> >> +       Request for fadump reboot. Firmware will reboot the system and collect
> >> +       dump.
> >> +
> >> +       On platforms that don't support fadump, this is equivalent to a
> >> +       normal assert.
> >> +
> >>   Unsupported Reboot type
> >>          For unsupported reboot type, this function will return with
> >>          OPAL_UNSUPPORTED and no reboot will be triggered.
> >> diff --git a/include/opal-api.h b/include/opal-api.h
> >> index 702e70841..1884850a9 100644
> >> --- a/include/opal-api.h
> >> +++ b/include/opal-api.h
> >> @@ -1239,6 +1239,7 @@ enum {
> >>          OPAL_REBOOT_NORMAL = 0,
> >>          OPAL_REBOOT_PLATFORM_ERROR,
> >>          OPAL_REBOOT_FULL_IPL,
> >
> >> +       OPAL_REBOOT_OS_ERROR,
> >
> > Having a "PLATFORM_ERROR" reboot type was always kind of dumb IMO so
> > I'd rather we didn't make that a pattern. Call it MPIPL or
> > MEMORY_PRESERVING_IPL. That way people might actually know what it
> > does.
>
> I wanted to have something OPAL_REBOOT_*. So I added it as "OPAL_REBOOT_MPIPL".
> Then based on Stewart suggestion I changed it to OPAL_REBOOT_OS_ERROR.

Well, in that case stewart needs to stop being wrong. ;)

>
> -Vasant
>
Stewart Smith May 21, 2019, 5:25 a.m. UTC | #8 
Oliver <oohall@gmail.com> writes:
> On Tue, May 14, 2019 at 6:59 PM Vasant Hegde
> <hegdevasant@linux.vnet.ibm.com> wrote:
>>
>> On 05/10/2019 07:03 AM, Oliver wrote:
>> > On Sat, Apr 13, 2019 at 7:19 PM Vasant Hegde
>> > <hegdevasant@linux.vnet.ibm.com> wrote:
>> >>
>> >> Enhance reboot2 call to support FADUMP. Payload will call this interface
>> >> to initiate fadump.
>> >
>> > Every other bit of bare metal firmware calls it MPIPL so I'd rather we
>> > just stuck with that terminology for consistency. Think of it this
>> > way: FAdump/fadump is a linux feature that we're implementing on top
>> > of MPIPL.
>>
>> Agreed. Will fix it.
>>
>> >
>> >> Signed-off-by: Vasant Hegde <hegdevasant@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");
>> >> +               console_complete_flush();
>> >> +               assert(false);
>> >> +               break;
>> >>          default:
>> >>                  prlog(PR_NOTICE, "OPAL: Unsupported reboot request %d\n", reboot_type);
>> >>                  return OPAL_UNSUPPORTED;
>> >> diff --git a/doc/opal-api/opal-cec-reboot-6-116.rst b/doc/opal-api/opal-cec-reboot-6-116.rst
>> >> index 516d4fc01..9ac7f9f69 100644
>> >> --- a/doc/opal-api/opal-cec-reboot-6-116.rst
>> >> +++ b/doc/opal-api/opal-cec-reboot-6-116.rst
>> >> @@ -63,6 +63,13 @@ OPAL_REBOOT_FULL_IPL = 2
>> >>          On platforms that don't support fast reboot, this is equivalent to a
>> >>          normal reboot.
>> >>
>> >> +OPAL_REBOOT_OS_ERROR = 3
>> >> +       Request for fadump reboot. Firmware will reboot the system and collect
>> >> +       dump.
>> >> +
>> >> +       On platforms that don't support fadump, this is equivalent to a
>> >> +       normal assert.
>> >> +
>> >>   Unsupported Reboot type
>> >>          For unsupported reboot type, this function will return with
>> >>          OPAL_UNSUPPORTED and no reboot will be triggered.
>> >> diff --git a/include/opal-api.h b/include/opal-api.h
>> >> index 702e70841..1884850a9 100644
>> >> --- a/include/opal-api.h
>> >> +++ b/include/opal-api.h
>> >> @@ -1239,6 +1239,7 @@ enum {
>> >>          OPAL_REBOOT_NORMAL = 0,
>> >>          OPAL_REBOOT_PLATFORM_ERROR,
>> >>          OPAL_REBOOT_FULL_IPL,
>> >
>> >> +       OPAL_REBOOT_OS_ERROR,
>> >
>> > Having a "PLATFORM_ERROR" reboot type was always kind of dumb IMO so
>> > I'd rather we didn't make that a pattern. Call it MPIPL or
>> > MEMORY_PRESERVING_IPL. That way people might actually know what it
>> > does.
>>
>> I wanted to have something OPAL_REBOOT_*. So I added it as "OPAL_REBOOT_MPIPL".
>> Then based on Stewart suggestion I changed it to OPAL_REBOOT_OS_ERROR.
>
> Well, in that case stewart needs to stop being wrong. ;)

My logic behind it was that MPIPL is a mechanism for doing something
sensible in the event of the OS being unable to continue. e.g. dumping
out a bunch of debug data to somewhere useful.
We don't have to restrict MPIPL to be the *only* thing we could do in that
situation. e.g. there'd be nothing stopping us from having the behaviour of
"kick off the BMC to do a bunch of things with pdbg and then do a full
IPL", which could especially be useful during bringup where MPIPL isn't
ready yet but pdbg/cronus could be triggered through some inband IPMI or
something.

I guess there is an argument to have both though, so it's possible to
force the *specific* action of MPIPL rather than "Whatever firmware
thinks is a good idea".
diff mbox series

Patch

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");
+		console_complete_flush();
+		assert(false);
+		break;
 	default:
 		prlog(PR_NOTICE, "OPAL: Unsupported reboot request %d\n", reboot_type);
 		return OPAL_UNSUPPORTED;
diff --git a/doc/opal-api/opal-cec-reboot-6-116.rst b/doc/opal-api/opal-cec-reboot-6-116.rst
index 516d4fc01..9ac7f9f69 100644
--- a/doc/opal-api/opal-cec-reboot-6-116.rst
+++ b/doc/opal-api/opal-cec-reboot-6-116.rst
@@ -63,6 +63,13 @@  OPAL_REBOOT_FULL_IPL = 2
 	On platforms that don't support fast reboot, this is equivalent to a
 	normal reboot.
 
+OPAL_REBOOT_OS_ERROR = 3
+	Request for fadump reboot. Firmware will reboot the system and collect
+	dump.
+
+	On platforms that don't support fadump, this is equivalent to a
+	normal assert.
+
 Unsupported Reboot type
 	For unsupported reboot type, this function will return with
 	OPAL_UNSUPPORTED and no reboot will be triggered.
diff --git a/include/opal-api.h b/include/opal-api.h
index 702e70841..1884850a9 100644
--- a/include/opal-api.h
+++ b/include/opal-api.h
@@ -1239,6 +1239,7 @@  enum {
 	OPAL_REBOOT_NORMAL = 0,
 	OPAL_REBOOT_PLATFORM_ERROR,
 	OPAL_REBOOT_FULL_IPL,
+	OPAL_REBOOT_OS_ERROR,
 };
 
 /* Argument to OPAL_PCI_TCE_KILL */