<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body width="72ex" style="width: 70ex;" vlink="#551A8B" text="#000000"
    link="#0B6CDA" bgcolor="#FFFFFF" alink="#EE0000">
    <pre>Hello Oliver,

Thanks for the review.
</pre>
    <div class="moz-cite-prefix">On 11/10/19 6:33 AM, Oliver O'Halloran
      wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">On Thu, Oct 10, 2019 at 11:10 PM Pratik Rajesh 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="">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 Linux kernel for that register.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">Where did 2048 come from and why do we care about being consistent
with Linux? SPRs are either defined by the Power architecture or in
the CPU user manual. Whatever happens to be in Linux is *not*
authoritative and from the point of view of firmware we shouldn't be
relying on any specific behaviour from the OS.
</pre>
    </blockquote>
    <pre><tt>I might have used a wrong terminology here by calling out the Linux kernel. The right
term would be the POWER special purpose register set, where the maximum that are used
now are upto 2048.
We don't rely on any specific expectations set from the OS. 
</tt></pre>
    <blockquote type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Signed-off-by: Pratik Rajesh Sampat <a class="moz-txt-link-rfc2396E" href="mailto:psampat@linux.ibm.com"><psampat@linux.ibm.com></a>
---
 hw/slw.c          | 72 +++++++++++++++++++++++++++++++++++++++++++++++
 include/skiboot.h |  1 +
 2 files changed, 73 insertions(+)

diff --git a/hw/slw.c b/hw/slw.c
index b79aaab3..d9c2d091 100644
--- a/hw/slw.c
+++ b/hw/slw.c
@@ -22,6 +22,7 @@
 #include <opal-api.h>
 #include <nvram.h>
 #include <sbe-p8.h>
+#include <bitmap.h>

 #include <p9_stop_api.H>
 #include <p8_pore_table_gen_api.H>
@@ -753,6 +754,70 @@ static void slw_late_init_p9(struct proc_chip *chip)
        }
 }

+/* Add device tree properties to determine self-save | restore */
+void add_cpu_self_save_properties(struct dt_node *power_mgt)
+{
+       int i;
+       struct dt_node *self_restore, *self_save;
+       bitmap_t *self_restore_map, *self_save_map;
</pre>
      </blockquote>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+       /* 32 times 64 bits needed to store a 2048 bits bitmask*/
+       const int bits_nr = 32;
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">If you're going to call something "bits_nr" then it had better be a
number of bits, and this isn't.

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+       const uint64_t self_restore_regs[] = {
+               0x130, // HSPRG0
+               0x13E, // LPCR
+               0x151, // HMEER
+               0x3F0, // HID0
+               0x3F1, // HID1
+               0x3F4, // HID4
+               0x3F6, // HID5
+               0x7D0, // MSR
+               0x357 // PSCCR
+       };
+
+       const uint64_t self_save_regs[] = {
+               0x130, // HSPRG0
+               0x13E, // LPCR
+               0x151, // HMEER
+               0x7D0, // MSR
+               0x357 // PSCCR
+       };
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">How'd you decide on these registers?</pre>
    </blockquote>
    <pre>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 type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+       const int self_save_regs_nr     =       ARRAY_SIZE(self_save_regs);
+       const int self_restore_regs_nr  =       ARRAY_SIZE(self_restore_regs);
+
+       self_save_map = zalloc(BITMAP_BYTES(0x800));
+       self_restore_map = zalloc(BITMAP_BYTES(0x800));
+
+       for (i = 0; i < self_save_regs_nr; i++)
+               bitmap_set_bit(*self_save_map, self_save_regs[i]);
+
+       for (i = 0; i < self_restore_regs_nr; i++)
+               bitmap_set_bit(*self_restore_map, self_restore_regs[i]);
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">Just use ARRAY_SIZE() in the loop tests. The constants aren't making
anything better.</pre>
    </blockquote>
    <pre>Alright thanks! I'll get rid of them.
</pre>
    <blockquote type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+       self_restore = dt_new(power_mgt, "self-restore");
+       if (!self_restore) {
+               prerror("OCC: Failed to create self restore node");
+               return;
+       }
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">This leaks the memory allocated for self_save_map and
self_restore_map. dt_add_property() allocates it's own space and
copies the contents rather than re-using the existing storage. You
need to free them.
</pre>
    </blockquote>
    <pre>Right, I had missed this. Taken care of now.
</pre>
    <blockquote type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+       dt_add_property_cells(self_restore, "active", 0x1);
</pre>
      </blockquote>
      <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>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 type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+
+       dt_add_property(self_restore, "sprn-bitmask", *self_restore_map,
+                       bits_nr * sizeof(uint64_t));
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">Write the size as: 2048 / 8 or SPR_BITMAP_LENGTH / 8, or similar, and
use the same expression in the bitmap allocations above.
</pre>
    </blockquote>
    <pre>Yes, this makes it a lot more cleaner. Thanks.
</pre>
    <blockquote type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+
+       self_save = dt_new(power_mgt, "self-save");
+       if (!self_save) {
+               prerror("OCC: Failed to create self save node");
+               return;
+       }
+       if (proc_gen == proc_gen_p9) {
+               dt_add_property_cells(self_save, "active", 0x1);
+
+               dt_add_property(self_save, "sprn-bitmask", *self_save_map,
+                               bits_nr * sizeof(uint64_t));
+       } else
+               dt_add_property_cells(self_save, "active", 0x0);
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">Skiboot style is to use {} around all the arms of an if statement if
any of them have a a pair of braces.

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>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>
    <pre><span style="color: rgb(29, 28, 29); font-family: Slack-Lato, appleLogo, sans-serif; font-size: 15px; font-style: normal; font-variant-ligatures: common-ligatures; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(248, 248, 248); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"><span></span></span><span style="color: rgb(29, 28, 29); font-family: Slack-Lato, appleLogo, sans-serif; font-size: 15px; font-style: normal; font-variant-ligatures: common-ligatures; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(248, 248, 248); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"></span><wbr style="box-sizing: inherit; color: rgb(29, 28, 29); font-family: Slack-Lato, appleLogo, sans-serif; font-size: 15px; font-style: normal; font-variant-ligatures: common-ligatures; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(248, 248, 248); text-decoration-style: initial; text-decoration-color: initial;"><wbr style="box-sizing: inherit; color: rgb(29, 28, 29); font-family: Slack-Lato, appleLogo, sans-serif; font-size: 15px; font-style: normal; font-variant-ligatures: common-ligatures; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(248, 248, 248); text-decoration-style: initial; text-decoration-color: initial;"></pre>
    <blockquote type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+}
+
 /* Add device tree properties to describe idle states */
 void add_cpu_idle_state_properties(void)
 {
@@ -1543,6 +1608,7 @@ opal_call(OPAL_SLW_SELF_SAVE_REG, opal_slw_self_save_reg, 2);
 void slw_init(void)
 {
        struct proc_chip *chip;
+       struct dt_node *power_mgt;

        if (proc_chip_quirks & QUIRK_MAMBO_CALLOUTS) {
                wakeup_engine_state = WAKEUP_ENGINE_NOT_PRESENT;
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">@@ -1568,4 +1634,10 @@ void slw_init(void)
                }
        }
        add_cpu_idle_state_properties();
+       power_mgt = dt_find_by_path(dt_root, "/ibm,opal/power-mgt");
+       if (!power_mgt) {
+               prerror("OCC: dt node /ibm,opal/power-mgt not found\n");
+               return;
+       }
+       add_cpu_self_save_properties(power_mgt);
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">Either move the DT node lookup into add_cpu_self_save_properties() to
make it self-contained, or rework it to something like:

add_spr_bitmap(power_mgt, "self-save", self_save_spr_array);
add_spr_bitmap(power_mgt, "self-restore", self_restore_spr_array);

I don't really mind which you do, but right now it's a bit all over the place.
</pre>
    </blockquote>
    <pre>Got it, I'll make sure I streamline this in a cleaner way before I submit the next 
iteration.
</pre>
    <blockquote type="cite"
cite="mid:CAOSf1CHsM81MxidzLkXYnBKp=rWGas9ic1kcmfrszY5_qHdYHg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap=""> }
diff --git a/include/skiboot.h b/include/skiboot.h
index 1aa8bf7c..e8f0f755 100644
--- a/include/skiboot.h
+++ b/include/skiboot.h
@@ -202,6 +202,7 @@ extern void early_uart_init(void);
 extern void homer_init(void);
 extern void slw_init(void);
 extern void add_cpu_idle_state_properties(void);
+extern void add_cpu_self_save_properties(struct dt_node *power_mgt);
 extern void lpc_rtc_init(void);

 /* flash support */
--
2.21.0

</pre>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>