[Skiboot] [PATCH v3 3/4] Advertise the self-save and self-restore attributes in the device tree

[Oliver O'Halloran]

On Mon, Feb 10, 2020 at 4:41 PM Pratik Sampat <psampat at linux.ibm.com> wrote:
>
> Hello Oliver, Thanks for the review.
>
>
> On 10/02/20 7:43 am, Oliver O'Halloran wrote:
> > On Fri, Feb 7, 2020 at 3:21 PM Pratik Rajesh Sampat
> > <psampat at linux.ibm.com> wrote:
> >> Support for self save and self restore interface is advertised in the
> >> device tree, along with the list of SPRs it supports for each.
> >>
> >> The Special Purpose Register identification is encoded in a 2048 bitmask
> >> structure, where each bit signifies the identification key of that SPR
> >> which is consistent with that of the POWER architecture set for that
> >> register.
> > Please answer the questions I asked the last time I reviewed this:
> > https://patchwork.ozlabs.org/patch/1174835/#2278701
>
> Sure, I'll address them as they come further in the patch.
>
> > Since then I've also forgotten what the difference between
> > self-restore and the existing API is. So can you add something to
> > doc/power-management.rst to describe the relationship between the
> > current API, self-restore, and self-save? The new device-tree bindings
> > need to be documented in doc/device-tree/ibm,opal/power-mgt/ too. The
> > cover letter of the series covers some of that, but actual
> > documentation is sorely needed.
>
> Right, I'll add more description in the documentation and make sure it's
> complete
>
> >> +#define MAX_RESET_PATCH_SIZE   64
> > unused?
>
> Yeah, I got rid of it. Sorry about that
>
> >
> >> +
> >> +       const uint64_t self_restore_regs[] = {
> >> +               P8_SPR_HRMOR,
> >> +               P8_SPR_HMEER,
> >> +               P8_SPR_PMICR,
> >> +               P8_SPR_PMCR,
> >> +               P8_SPR_HID0,
> >> +               P8_SPR_HID1,
> >> +               P8_SPR_HID4,
> >> +               P8_SPR_HID5,
> >> +               P8_SPR_HSPRG0,
> >> +               P8_SPR_LPCR,
> >> +               P8_SPR_PSSCR,
> >> +               P8_MSR_MSR
> >> +       };
> >> +
> >> +       const uint64_t self_save_regs[] = {
> >> +               P9_STOP_SPR_DAWR,
> >> +               P9_STOP_SPR_HSPRG0,
> >> +               P9_STOP_SPR_LDBAR,
> >> +               P9_STOP_SPR_LPCR,
> >> +               P9_STOP_SPR_PSSCR,
> >> +               P9_STOP_SPR_MSR,
> >> +               P9_STOP_SPR_HRMOR,
> >> +               P9_STOP_SPR_HMEER,
> >> +               P9_STOP_SPR_PMCR,
> >> +               P9_STOP_SPR_PTCR
> >> +       };
> > Again, where did these sets of registers come from?
> >
> These are the set of registers that are supported by the pore engine. Although
> our kernel only needs a subset of them, the whole list is sent out to indicate
> what is supported.

That doesn't really help. I'll assume that by "pore" you mean what's
in libpore/p9_*.[CH] which writes a pile of stuff into the HOMER, but
what consumes that? What I want to know is whether updates to some
other bit of firmware can change the list of supported registers or
not.

> >> +       self_save_map = zalloc(BITMAP_BYTES(SPR_BITMAP_LENGTH));
> >> +       self_restore_map = zalloc(BITMAP_BYTES(SPR_BITMAP_LENGTH));
> > In general I prefer people avoid doing allocations until they're
> > actually needed. If any of the checks below fail then we've done a
> > bunch of work for no reason so...      do the checks first?
>
> Yeah makes sense, I can move the allocations down later when I really need them.
> It will also get rid of all the goto bail and I can just use a return instead.
>
> >
> >> +       dt_add_property_string(self_restore, "status", "enabled");
> >> +
> >> +       dt_add_property(self_restore, "sprn-bitmask", *self_restore_map,
> >> +                       SPR_BITMAP_LENGTH / 8);
> >> +
> >> +       self_save = dt_new(power_mgt, "self-save");
> >> +       if (!self_save) {
> >> +               prerror("OCC: Failed to create self save node");
> >> +               goto bail;
> >> +       }
> >> +       if (proc_gen == proc_gen_p9) {
> >> +               dt_add_property_string(self_save, "status", "enabled");
> >> +
> >> +               dt_add_property(self_save, "sprn-bitmask", *self_save_map,
> >> +                               SPR_BITMAP_LENGTH / 8);
> > If skiboot is compiled as little endian this is going to break. It
> > works today because we always build skiboot BE, but that may not
> > always be true.
> >
> Okay, I think I understand what you mean. I should run my bitmask through a
> loop 32 bits at a time with a be32_to_cpu while populating it?

Pretty much. You can check if it works by building skiboot in LE mode
using: make clean && make LITTLE_ENDIAN=1

>
> >> +       } else {
> >> +               dt_add_property_string(self_save, "status", "disabled");
> > I still don't understand why we're adding anything to the DT if the
> > feature isn't supported.
>
> Today in the kernel, if the node doesn't exist we assume that we are running an
> older firmware on a newer kernel and make the self-restore call as it was
> always supported in legacy.
>
> Now let's take an example of a case where self-restore is actually unsupported,
> in that case if the node does not exist the kernel will assume it does and make
> the call anyways to see it fail later.

The case I'm concerned about is old-kernel / new-firmware since the
legacy API is going to be broken if there's no self-restore
capability. This series seems to be ignoring that fact when we're
populating the DT so an older kernel is going to assume the legacy API
is supported. What is the fallout from that?

> Not that it is a bad thing for it to fail later and then cut support for stop
> states, however, I'm trying to avoid this try-catch scenario. If you feel that
> I should let it fail and handle it later, I'll do that instead.
>
> If you consider opposite scenario instead, that we allow self-restore only if
> the node is present, then using legacy skiboot deep stop states will be
> disabled which is again an undesired behavior.
>
> --
>
> Pratik
>