Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 14 Apr 2020 12:41:49 +0530
From:   Gautham R Shenoy <ego@linux.vnet.ibm.com>
To:     Pratik Rajesh Sampat <psampat@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org,
        mpe@ellerman.id.au, skiboot@lists.ozlabs.org, oohall@gmail.com,
        ego@linux.vnet.ibm.com, linuxram@us.ibm.com,
        pratik.r.sampat@gmail.com
Subject: Re: [PATCH v6 0/3] powerpc/powernv: Introduce interface for
 self-restore support
Message-ID: <20200414071149.GD24277@in.ibm.com>
Reply-To: ego@linux.vnet.ibm.com
References: <20200326071034.12838-1-psampat@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200326071034.12838-1-psampat@linux.ibm.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Pratik,

On Thu, Mar 26, 2020 at 12:40:31PM +0530, Pratik Rajesh Sampat wrote:
> v5: https://lkml.org/lkml/2020/3/17/944
> Changelog
> v5-->v6
> 1. Updated background, motivation and illuminated potential design
> choices
> 2. Re-organization of patch-set
>   a. Split introducing preference for optimization from 1/1 to patch 3/3
>   b. Merge introducing self-save API and parsing the device-tree
>   c. Introduce a supported mode called KERNEL_SAVE_RESTORE which
>      outlines and makes kernel supported SPRs for save-restore more
>      explicit
> 
[..snip..]

> Presenting the design choices in front of us:
> 
> Design-Choice 1:
> ----------------
> A simple implementation is to just replace self-restore calls with
> self-save as it is direct super-set.
> 
> Pros:
> A simple design, quick to implement
> 
> 
> Cons:
> * Breaks backward compatibility. Self-restore has historically been
>   supported in the firmware and an old firmware running on an new
>   kernel will be incompatible and deep stop states will be cut.
> * Furthermore, critical SPRs which need to be restored
>   before 0x100 vector like HID0 are not supported by self-save.
> 
> Design-Choice 2:
> ----------------
> Advertise both self-restore and self-save from OPAL including the set
> of registers that each support. The kernel can then choose which API
> to go with.
> For the sake of simplicity, in case both modes are supported for an
> SPR by default self-save would be called for it.
> 
> Pros:
> * Backwards compatible
> 
> Cons:
> Overhead in parsing device tree with the SPR list
> 
> Possible optimization with Approach2:
> -------------------------------------
> There are SPRs whose values don't tend to change over time and invoking
> self-save on them, where the values are gotten each time may turn out to
> be inefficient. In that case calling a self-restore where passing the
> value makes more sense as, if the value is same, the memory location
> is not updated.
> SPRs that dont change are as follows:
> SPRN_HSPRG0,
> SPRN_LPCR,
> SPRN_PTCR,
> SPRN_HMEER,
> SPRN_HID0,

We can just pick self-save wherever available and fallback to
self-restore when self-save support is not avaiable for any SPR.
The optimization that you mention here can be revisited if the
additional latency due to self-save becomes observable (Note that both
stop4 and stop5 have wakeup latency between 200-500us).

> 
> The values of PSSCR and MSR change at runtime and hence, the kernel
> cannot determine during boot time what their values will be before
> entering a particular deep-stop state.
> 
> Therefore, a preference based interface is introduced for choosing
> between self-save or self-restore between for each SPR.
> The per-SPR preference is only a refinement of
> approach 2 purely for performance reasons. It can be dropped if the
> complexity is not deemed worth the returns.
> 
> Patches Organization
> ====================
> Design Choice 2 has been chosen as an implementation to demonstrate in
> the patch series.
> 
> Patch1:
> Devises an interface which lists all the interested SPRs, along with
> highlighting the support of mode.
> It is an isomorphic patch to replicate the functionality of the older
> self-restore firmware for the new interface
> 
> Patch2:
> Introduces the self-save API and leverages upon the struct interface to
> add another supported mode in the mix of saving and restoring. It also
> enforces that in case both modes are supported self-save is chosen over
> self-restore
> 
> The commit also parses the device-tree and populate support for
> self-save and self-restore in the supported mask
> 
> Patch3:
> Introduce an optimization to allow preference to choose between one more
> over the one when both both modes are supported. This optimization can
> allow for better performance for the SPRs that don't change in value and
> hence self-restore is a better alternative, and in cases when it is
> known for values to change self-save is more convenient.
> 
> Pratik Rajesh Sampat (3):
>   powerpc/powernv: Introduce interface for self-restore support
>   powerpc/powernv: Introduce support and parsing for self-save API
>   powerpc/powernv: Preference optimization for SPRs with constant values
> 
>  .../bindings/powerpc/opal/power-mgt.txt       |  18 +
>  arch/powerpc/include/asm/opal-api.h           |   3 +-
>  arch/powerpc/include/asm/opal.h               |   1 +
>  arch/powerpc/platforms/powernv/idle.c         | 385 +++++++++++++++---
>  arch/powerpc/platforms/powernv/opal-call.c    |   1 +
>  5 files changed, 351 insertions(+), 57 deletions(-)
> 
> -- 
> 2.17.1
>