Re: [RFC] Support stop state version quirk and firmware enabled stop

Pratik Sampat
2020-04-12T12:06:49Z

Hello Gautham,

On 08/04/20 3:20 pm, Gautham R Shenoy wrote:
> Hi Pratik,
>
> On Wed, Mar 04, 2020 at 09:26:48PM +0530, Pratik Rajesh Sampat wrote:
>> A concept patch in Skiboot to illustrate the case wherein handling of
>> stop states for different DD versions of a CPU can be achieved by a
>> simple modification in the list of cpu_features.
>> As an example idle-stop1 is defined which uses P9_CPU_DD1 to define the
>> cpu feature.
>>
>> Along with that, an implementation is being worked upon the LE OPAL
>> series which helps OPAL handle the stop state entry and exit.
>>
>> This patch advertises this capability of the firmware which can be
>> availed if the quirk-version-setting is not cognizable.
>>
>> The firmware-enabled stop is being worked by Abhishek Goel
>> <huntbag@linux.vnet.ibm.com> building upon the LE OPAL series.
>>
>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
>> ---
>> core/cpufeatures.c | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/core/cpufeatures.c b/core/cpufeatures.c
>> index ec30c975..b9875e7b 100644
>> --- a/core/cpufeatures.c
>> +++ b/core/cpufeatures.c
>> @@ -510,6 +510,25 @@ static const struct cpu_feature cpu_features_table[] = {
>> -1, -1, -1,
>> NULL, },
>>
>> + /*
>> + * QUIRK for ISAv3.0B stop idle instructions and registers
>> + * Helps us determine if there are any quirks
>> + * XXX: Same of idle-stop
>> + */
>> + { "idle-stop-v1",
>> + CPU_P9_DD1,
>> + ISA_V3_0B, USABLE_HV|USABLE_OS,
>> + HV_CUSTOM, OS_CUSTOM,
>> + -1, -1, -1,
>> + NULL, },
>
> So, at this point, we don't need any such quirk for any of the DD
> version right ? This is to demonstrate that if say P9_DD1 had a quirk
> w.r.t stop-state handling, then this is how we would advertise it to
> the kernel.

Absolutely, A dummy property has been added to show how quirk handling
with stop-states.

>> +
>> + { "firmware-stop-supported",
>> + CPU_P9,
>> + ISA_V3_0B, USABLE_HV|USABLE_OS,
>> + HV_CUSTOM, OS_CUSTOM,
>> + -1, -1, -1,
>> + NULL, },
>> +
>
> I suppose this is for the opal-cpuidle driver support posted here:
> https://lists.ozlabs.org/pipermail/skiboot/2020-April/016726.html

Right, this complements in usage of the opal-cpuidle driver

>> /*
>> * ISAv3.0B Hypervisor Virtualization Interrupt
>> * Also associated system registers, LPCR EE, HEIC, HVICE,
>> @@ -883,6 +902,9 @@ static void add_cpufeatures(struct dt_node *cpus,
>> const struct cpu_feature *f = &cpu_features_table[i];
>>
>> if (f->cpus_supported & cpu_feature_cpu) {
>> + if (!strcmp(f->name, "firmware-stop-supported") &&
>> + HAVE_BIG_ENDIAN)
>> + continue;
> In OPAL do we have an macro defining BIG_ENDIAN ? If yes, you could
> wrap the "firmware-stop-supported" in cpu_features_table[] within
> #ifndef BIG_ENDIAN. That way you won't need a special case here.
>
HAVE_BIG_ENDIAN is actually a macro. Its a good idea to wrap it in
the declaration itself.

>
>> DBG(" '%s'\n", f->name);
>> add_cpu_feature_nodeps(features, f);
>> }
>> --
>> 2.24.1
>>
> --
> Thanks and Regards
> gautham.