<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="width: 72ex;" vlink="#551A8B" text="#000000"
link="#0B6CDA" bgcolor="#FFFFFF" alink="#EE0000">
<pre>Hello Oliver,
</pre>
<br>
<div class="moz-cite-prefix">On 14/10/19 4:39 PM, Oliver O'Halloran
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAOSf1CGPPh1NYygJ6qTCV5N3pKTa3mqc4F-AOf8-e-B90Z63vQ@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">On Mon, Oct 14, 2019 at 8:33 PM Pratik Sampat <a class="moz-txt-link-rfc2396E" href="mailto:psampat@linux.ibm.com"><psampat@linux.ibm.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
<pre>For the self-restore, the register set is complete of what the power management engine
supports.
However, for self save it is a subset to only what Linux currently cares about.
This is because, A few registers namely HID0, URMOR and HRMOR although available,
but are known to be faulty. Finally leaving behind only 3 other registers DAWR,
LDBAR and PMCR which Linux particularly does not care about.
I can advertise the remaining 3 registers in self save list. That should not be a problem.
</pre>
<blockquote type="cite"
cite="mid:CAOSf1CGPPh1NYygJ6qTCV5N3pKTa3mqc4F-AOf8-e-B90Z63vQ@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre>Currently, the way this has been designed in the kernel is to support legacy self restore,
unless explicitly said that self restore is not supported (This would happen most likely in
a PEF environment) and by default support for self save has been stripped.
In the way the skiboot-kernel interaction has been designed; the kernel looks
for the self restore node and if it is present, looks for active and subsequently populates
the SPR list. In a case the self restore node is absent, the kernel does not strip self
restore support for an SPR if it does not find the node itself. It assumes that this should
be a legacy scenario.
If we remove the status property and only compare based on the presence/absence of a node,
then there will be no difference between the a new skiboot explictly asking us to disable
self restore and an older skiboot which has no way of telling us it supports self restore
but we know that it does because it did so in legacy systems.
</pre>
<blockquote type="cite"
cite="mid:CAOSf1CGPPh1NYygJ6qTCV5N3pKTa3mqc4F-AOf8-e-B90Z63vQ@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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
</pre>
</blockquote>
<pre>This is actually a good idea from an engineering point of view to know before hand our
support for something rather than this try-except approach of throwing something at the
firmware and hope to handle failure later if any.
However, such a check does not currently exist and we got in touch with the firmware team
and they said they'll let us know if something like this can be prioritized and supported.
</pre>
<br>
</body>
</html>