[Skiboot] [PATCH Skiboot v1.2 3/3] Advertise the self-save and self-restore attributes in the device tree

[Oliver O'Halloran]

On Mon, Oct 14, 2019 at 8:33 PM Pratik Sampat <psampat at linux.ibm.com> wrote:
>
> > Currently, when the system goes into a deep state, these are the minimum set of
> > registers that are needed whose values need to be restored from when they entered the
> > state of loss.

Sure, but who decided this was the minimum set? Is it the set of
registers that the power management engine supports restoring? Or just
the set of registers that Linux currently cares about? We want to
advertise to Linux what firmware is capable of doing.

> > I don't see the point of this property. If firmware doesn't support
> > the feature then don't add the node. We need to handle that case from
> > the OS side anyway since old firmware won't be providing it.
> >
> > If you're super-convinced that we need something like this, then use
> > the more standard device-tree "status" property.
>
> This property is needed for a scenario when we are running an older firmware on a newer
> kernel. In that case we would want the legacy self restore to work as-is.
> However, In case we remove the active (or now re-named as status) property, the issue is
> that in case we don't support self restore, the node will be absent and the kernel will
> strip the legacy self restore functionality from the system which will cause regression
> as the functionality is actually supported in the older firmware.

Old firmware isn't going to put any of the new properties in the DT,
including the active property, so how does this help? If the kernel
changes to support this feature break using the old API, then the
kernel changes are broken.

> > Nitpicks aside, how do we know libpore supports the self-restore? The
> > commit message says that it's a new feature but you're assuming that
> > all P9 systems support it. Is there some way to determine if it's
> > supported at runtime?
>
> As far as I know, Skiboot tries to make a stop API call with the highest version in mind,
> if it fails it falls back to older version.The way we determine today if self-restore
> works is by checking if the HOMER is populated in the system.
> Ideally we want to do a version check but we leave it to the stop API as it would do it
> anyways and give us an error if something went wrong.

Sorry, when I said self-restore I mean self-save. Checking for the
existance of the HOMER region might be ok for self-restore since I
think we've supported that since forever, but I'm not sure that's true
for self-save.

I'm pretty skeptical of just reporting errors when Linux attempts to
call into firmware. The whole point of advertising features in the DT
is to avoid the OS just throwing stuff at firmware and seeing what
works.

Oliver