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;
Date:   Thu, 16 Apr 2020 19:20:05 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [Skiboot] [PATCH v7 0/4] Support for Self Save API in OPAL
To:     ego@linux.vnet.ibm.com, linux-kernel@vger.kernel.org,
        linuxppc-dev@ozlabs.org, linuxram@us.ibm.com, mpe@ellerman.id.au,
        oohall@gmail.com, pratik.r.sampat@gmail.com,
        Pratik Rajesh Sampat <psampat@linux.ibm.com>,
        skiboot@lists.ozlabs.org
References: <20200416075341.75268-1-psampat@linux.ibm.com>
In-Reply-To: <20200416075341.75268-1-psampat@linux.ibm.com>
MIME-Version: 1.0
Message-Id: <1587026695.tnsg4h9617.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Excerpts from Pratik Rajesh Sampat's message of April 16, 2020 5:53 pm:
> v6: https://lists.ozlabs.org/pipermail/skiboot/2020-March/016645.html
> Changelog
> v6 --> v7
> 1. Addressed comments from Gautham for reporting warnings and errors
>=20
> Background
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>=20
> The power management framework on POWER systems include core idle
> states that lose context. Deep idle states namely "winkle" on POWER8
> and "stop4" and "stop5" on POWER9 can be entered by a CPU to save
> different levels of power, as a consequence of which all the
> hypervisor resources such as SPRs and SCOMs are lost.
>=20
> For most SPRs, saving and restoration of content for SPRs and SCOMs
> is handled by the hypervisor kernel prior to entering an post exit
> from an idle state respectively. However, there is a small set of
> critical SPRs and XSCOMs that are expected to contain sane values even
> before the control is transferred to the hypervisor kernel at system
> reset vector.
>=20
> For this purpose, microcode firmware provides a mechanism to restore
> values on certain SPRs. The communication mechanism between the
> hypervisor kernel and the microcode is a standard interface called
> sleep-winkle-engine (SLW) on Power8 and Stop-API on Power9 which is
> abstracted by OPAL calls from the hypervisor kernel. The Stop-API
> provides an interface known as the self-restore API, to which the SPR
> number and a predefined value to be restored on wake-up from a deep
> stop state is supplied.
>=20
>=20
> Motivation to introduce a new Stop-API
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>=20
> The self-restore API expects not just the SPR number but also the
> value with which the SPR is restored. This is good for those SPRs such
> as HSPRG0 whose values do not change at runtime, since for them, the
> kernel can invoke the self-restore API at boot time once the values of
> these SPRs are determined.
>=20
> However, there are use-cases where-in the value to be saved cannot be
> known or cannot be updated in the layer it currently is.
> The shortcomings and the new use-cases which cannot be served by the
> existing self-restore API, serves as motivation for a new API:

Thanks for writing this up, it goes some way to help think about the=20
feature.

> Shortcoming1:
> ------------
> In a special wakeup scenario when a CPU is woken up in stop4/5 and
> after the task is done, the HCODE puts it back to stop. The value of
> PSSCR is passed to the HCODE via the self-restore API. The kernel
> currently provides the value of the deepest stop state due to being
> conservative. Thus if a core that was in stop4 was woken up due to
> special wakeup, the HCODE will now put it back to stop5 thus increasing
> the subsequent wakeup latency to ~200us.
> A mechanism is needed in place to update the PSSCR value each time the
> core is woken up due to special wakeup.

This seems like a shortcoming of the wakeup firmware that shouldn't need=20
any APIs to the kernel to solve, but the whole deep sleep wakeup seems=20
like a shortcoming so let's assume they won't do that for whatever=20
reason, then how much of a problem is this really? Are special wakeups=20
that frequent?

> Shortcoming2:
> ------------
> The value of LPCR is dynamic based on if the CPU is entered a stop
> state during cpu idle versus cpu hotplug.
> Today, an additional self-restore call is made before entering
> CPU-Hotplug to clear the PECE1 bit in stop-API so that if we are
> woken up by a special wakeup on an offlined CPU, we go back to stop
> with the the bit cleared.
> There is a overhead of an extra call

This is a self-restore call when we offline or online a CPU? That's not=20
a real problem either, is it?

> New Use-case:
> -------------
> In the case where the hypervisor is running on an
> ultravisor environment, the boot time is too late in the cycle to make
> the self-restore API calls, as these cannot be invoked from an
> non-secure context anymore
>=20
> To address these shortcomings, the firmware provides another API known
> as the self-save API. The self-save API only takes the SPR number as a
> parameter and will ensure that on wakeup from a deep-stop state the
> SPR is restored with the value that it contained prior to entering the
> deep-stop.
>=20

If the ultravisor is deployed in production only systems where we don't=20
use runtime deep-stop states, do we need to handle this case?

Thanks,
Nick