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

[Pratik Sampat <psampat@linux.ibm.com>]

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.