[Skiboot] [PATCH] [v3] Fast reboot for P8

[Oliver O'Halloran]

On Wed, Sep 28, 2016 at 2:54 PM, Stewart Smith
<stewart at linux.vnet.ibm.com> wrote:
> Benjamin Herrenschmidt <benh at kernel.crashing.org> writes:
>> On Tue, 2016-09-27 at 18:00 +1000, Stewart Smith wrote:
>>>
>>> what would be your criteria for going to signed-off-by ?
>>>
>>> i'm thinking:
>>> 1) behind a config switch (I'm thinking
>>>    nvram_query("experimental-fast-reboot")=="Danger-is-my-middle-
>>> name" or some
>>>    such check)
>>> 2) CAPI disable ptaches from Andrew
>>> 3) Check for checkstops
>>>
>>> Although skipping 2 and 3 because there's (1) (and not having it
>>> enabled by default) could be a way forward.
>>
>> Yup, I generally agree. Also more testing.
>
> rebooted for me: SHIP IT! :)
>
>>> Couple of quick thoughts below, not near complete review of course :)
>>>
>>> >
>>> > --- a/hw/fsp/fsp-leds.c
>>> > +++ b/hw/fsp/fsp-leds.c
>>> > @@ -1570,6 +1570,9 @@ void create_led_device_nodes(void)
>>> >    if (!pled)
>>> >            return;
>>> >
>>> > +  /* Check if already populated (fast-reboot) */
>>> > +  if (dt_has_node_property(pled, "compatible", NULL))
>>> > +          return;
>>>
>>> Any thoughts on a consistent way to do this that makes it relatively
>>> obvious we're in fast reboot rather than IPL?
>>
>> Not off hand. Maybe a different init function alltogether ?
>
> Yeah... maybe.
>
>>> >    dt_add_property_strings(pled, "compatible",
>>> > DT_PROPERTY_LED_COMPATIBLE);
>>> >
>>> >    led_mode = dt_prop_get(pled, DT_PROPERTY_LED_MODE);
>>> > diff --git a/hw/occ.c b/hw/occ.c
>>> > index b606a67..3d86f7a 100644
>>> > --- a/hw/occ.c
>>> > +++ b/hw/occ.c
>>> > @@ -517,10 +517,14 @@ void occ_pstates_init(void)
>>> >    struct proc_chip *chip;
>>> >    struct cpu_thread *c;
>>> >    s8 pstate_nom;
>>> > +  static bool occ_pstates_initialized;
>>> >
>>> >    /* OCC is P8 only */
>>> >    if (proc_gen != proc_gen_p8)
>>> >            return;
>>> > +  /* Handle fast reboots */
>>> > +  if (occ_pstates_initialized)
>>> > +          return;
>>> >
>>> >    chip = next_chip(NULL);
>>> >    if (!chip->homer_base) {
>>> > @@ -558,6 +562,7 @@ void occ_pstates_init(void)
>>> >    for_each_chip(chip)
>>> >            chip->throttle = 0;
>>> >    opal_add_poller(occ_throttle_poll, NULL);
>>> > +  occ_pstates_initialized = true;
>>> >  }
>>>
>>> We could reset the OCCs on fast reboot, as there is a PRD command to
>>> disable them.. although nobody should *ever* use them....
>>
>> We could ... should we ?
>
> Probably? I may have to think about it a bit more.
>
>>> FWIW I've been looking at moving OCC init even on openpower into
>>> skiboot
>>> for the purposes of speeding up boot, so this could be "easy".
>>
>> I was under the impression that OCC inits requires going through
>> 223648726434 lines of horrible TMGT code in HostBoot (or HBRT) ?
>
> I was thinking of just calling HBRT to do it.
>
>>> > --- a/include/config.h
>>> > +++ b/include/config.h
>>> > @@ -73,7 +73,7 @@
>>> >  //#define FORCE_DUMMY_CONSOLE 1
>>> >
>>> >  /* Enable this to do fast resets. Currently unreliable... */
>>> > -//#define ENABLE_FAST_RESET       1
>>> > +#define ENABLE_FAST_REBOOT        1
>>>
>>> I think we could remove the config var and just do it via NVRAM
>>> setting.
>>
>> If we have those, sure.
>
> we do as of f0ef4e2d26a89deff97d50942f40a77edcc40867.

There's a slight problem with those since I figured we would only be
using nvram_query() in the boot path. The host OS can modify the nvram
contents so some extra checking is necessary to make it usable for
this. I've posted a series to fix it.

>
> --
> Stewart Smith
> OPAL Architect, IBM.
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot