Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v6 0/3] powerpc/powernv: Introduce interface for
 self-restore support
To:     ego@linux.vnet.ibm.com
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org,
        mpe@ellerman.id.au, skiboot@lists.ozlabs.org, oohall@gmail.com,
        linuxram@us.ibm.com, pratik.r.sampat@gmail.com
References: <20200326071034.12838-1-psampat@linux.ibm.com>
 <20200414071149.GD24277@in.ibm.com>
From:   Pratik Sampat <psampat@linux.ibm.com>
Date:   Tue, 14 Apr 2020 17:40:40 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20200414071149.GD24277@in.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Message-Id: <f0e6d1c2-e888-2ff6-ea80-95f753e89051@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Gautham

On 14/04/20 12:41 pm, Gautham R Shenoy wrote:
> 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).

Yes, Patch2 introduces picking self-save when available and using
self-restore as fallback.
Sure. The optimization of preference does certainly add an additional
layer of complexity. As you mentioned, in the case the latency really
begins to hurt us we can revisit this discussion.

Thanks
Pratik

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