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

[Pratik Sampat]

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.

>> +       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?

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

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