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

[Stewart Smith]

Oliver <oohall at gmail.com> writes:
> On Tue, May 14, 2019 at 6:59 PM Vasant Hegde
> <hegdevasant at 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 at 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 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");
>> >> +               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".

-- 
Stewart Smith
OPAL Architect, IBM.