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

[Pratik Sampat]

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

Well this is a list that the firmware currently supports. There is nothing to
stop them from making changes to the support.

The solution is that our self save now has support for versioning. So whenever
something changes, the versions are updated and the compatibility of the SPRs
can be determined and updated accordingly.

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

Sure. I'll try that out

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

Your case is perfectly valid. However, isn't this what happens currently too?
If for some reason, the self-restore call fails today, the kernel does handle
the consequences.
The try-catch sort of an approach is inevitable in this scenario as there is no
way from which the kernel can know.

I don't mind getting rid of it. I'll do a through test once to see if there's a
corner case I'm missing and I'll remove it then.

Thanks
Pratik