[Skiboot] [PATCH] OCC: Increase max pstate check on P9 to 255

[Michael Ellerman]

Stewart Smith <stewart at linux.vnet.ibm.com> writes:

> Vaidyanathan Srinivasan <svaidy at linux.vnet.ibm.com> writes:
>> * Stewart Smith <stewart at linux.vnet.ibm.com> [2017-11-17 09:34:03]:
>>
>>> This has changed from P8, we can now have > 127 pstates.
>>> 
>>> This was observed on Boston during WoF bringup.
>>> 
>>> Reported-by: Minda Wei <minda.wei at ibm.com>
>>
>> Reported-by: Francesco A Campisano <campisan at us.ibm.com>
>>>
>>>
>>> Signed-off-by: Stewart Smith <stewart at linux.vnet.ibm.com>
>>
>> Reviewed-by: Vaidyanathan Srinivasan <svaidy at linux.vnet.ibm.com>
>>
>>
>>> ---
>>>  hw/occ.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/hw/occ.c b/hw/occ.c
>>> index 78c6a6a356e3..8ad0dfe421d7 100644
>>> --- a/hw/occ.c
>>> +++ b/hw/occ.c
>>> @@ -612,13 +612,15 @@ static bool add_cpu_pstate_properties(int *pstate_nom)
>>>  	nr_pstates = labs(pmax - pmin) + 1;
>>>  	prlog(PR_DEBUG, "OCC: Version %x Min %d Nom %d Max %d Nr States %d\n",
>>>  	      occ_data->version, pmin, pnom, pmax, nr_pstates);
>>> -	if (nr_pstates <= 1 || nr_pstates > 128) {
>>> +	if ((occ_data->version == 0x90 && (nr_pstates <= 1)) ||
>>> +	    (occ_data->version <= 0x02 &&
>>> +	     (nr_pstates <= 1 || nr_pstates > 128))) {
>>>  		/**
>>>  		 * @fwts-label OCCInvalidPStateRange
>>>  		 * @fwts-advice The number of pstates is outside the valid
>>> -		 * range (currently <=1 or > 128), so OPAL has not added
>>> -		 * pstates to the device tree. This means that OCC (On Chip
>>> -		 * Controller) will be non-functional. This means
>>> +		 * range (currently <=1 or > 128 on p8, >255 on P9), so OPAL
>>> +		 * has not added pstates to the device tree. This means that
>>> +		 * OCC (On Chip Controller) will be non-functional. This means
>>>  		 * that CPU idle states and CPU frequency scaling
>>>  		 * will not be functional.
>>>  		 */
>>
>> Thanks for the fix to increase the limit.
>>
>> Checking for occ_data->version is the right check to decide permitted
>> range between P8 vs P9.
>>
>> In case occ_data->version is bumped up, then we do not know the range
>> and we will fail in an earlier check and not export any pstates.
>>
>> We need to update the right range for future versions here.
>>
>> However Linux could have reporting problems with PState IDs more than
>> 128 and also with large number of PStates.  This change does not as
>> such crash Linux, but more work is needed in Linux driver to support
>> large number of PStates.
>
> I'll pull the patch in and let's see how it goes for a bit... see if
> people see this as a real problem or not. We can always pull it out and
> do a dance with filtering pstates to limit to 128 and put the full list
> somewhere else that the kernel doesn't poke at by default.
>
> mpe, what are your thoughts here?

Hard to say.

The trade off seems to be unspecified "problems" in Linux if the large
number of states is exposed, vs having no CPU idle or CPU frequency
scaling.

The latter is clearly very bad, but it would be good to have more detail
from Vaidy on the issues we might see in Linux.

cheers