Re: [RFC 1/3] Interface for an idle-stop dependency structure

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

Hello Gautham

On 08/04/20 4:21 pm, Gautham R Shenoy wrote:
> Hi Pratik,
>
> On Wed, Mar 04, 2020 at 09:31:21PM +0530, Pratik Rajesh Sampat wrote:
>> Design patch to introduce the idea of having a dependency structure for
>> idle-stop. The structure encapsulates the following:
>> 1. Bitmask for version of idle-stop
>> 2. Bitmask for propterties like ENABLE/DISABLE
>> 3. Function pointer which helps handle how the stop must be invoked
>>
>> The commit lays a foundation for other idle-stop versions to be added
>> and handled cleanly based on their specified requirments.
>> Currently it handles the existing "idle-stop" version by setting the
>> discovery bits and the function pointer.
> So, if this patch is applied, and we are running with an OPAL that
> doesn't publish the "idle-stop" dt-cpu-feature, then the goal is to
> not enable any stop states. Is this correct ?
>
Yes, all states will be disabled with no power saving.

>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/processor.h  | 17 +++++++++++++++++
>>   arch/powerpc/kernel/dt_cpu_ftrs.c     |  5 +++++
>>   arch/powerpc/platforms/powernv/idle.c | 17 +++++++++++++----
>>   drivers/cpuidle/cpuidle-powernv.c     |  3 ++-
>>   4 files changed, 37 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
>> index eedcbfb9a6ff..da59f01a5c09 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -429,6 +429,23 @@ extern void power4_idle_nap(void);
>>   extern unsigned long cpuidle_disable;
>>   enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
>>
>> +#define STOP_ENABLE		0x00000001
>> +
>> +#define STOP_VERSION_P9       0x1
>> +
>> +/*
>> + * Classify the dependencies of the stop states
>> + * @idle_stop: function handler to handle the quirk stop version
>> + * @cpuidle_prop: Signify support for stop states through kernel and/or firmware
>> + * @stop_version: Classify quirk versions for stop states
>> + */
>> +typedef struct {
>> +	unsigned long (*idle_stop)(unsigned long, bool);
>> +	uint8_t cpuidle_prop;
>> +	uint8_t stop_version;
> Why do we need both cpuidle_prop and stop_version ?

The idea is that each stop_version has house multitude of overlapping properties.
So the idea is to give a clean distinction. However, I can see now that the
versioning and properties could be embedded in a single bitmask


>> @@ -657,6 +659,9 @@ static void __init cpufeatures_setup_start(u32 isa)
>>   	}
>>   }
>>
>> +stop_deps_t stop_dep = {NULL, 0x0, 0x0};
>> +EXPORT_SYMBOL(stop_dep);
>> +
>>   static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
>>   {
>>   	const struct dt_cpu_feature_match *m;
>> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
>> index 78599bca66c2..c32cdc37acf4 100644
>> --- a/arch/powerpc/platforms/powernv/idle.c
>> +++ b/arch/powerpc/platforms/powernv/idle.c
>> @@ -812,7 +812,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
>>
>>   #ifndef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>>   	__ppc64_runlatch_off();
>> -	srr1 = power9_idle_stop(psscr, true);
>> +	srr1 = stop_dep.idle_stop(psscr, true);
>>   	__ppc64_runlatch_on();
>>   #else
>>   	/*
>> @@ -828,7 +828,7 @@ static unsigned long power9_offline_stop(unsigned long psscr)
>>   	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_IDLE;
>>
>>   	__ppc64_runlatch_off();
>> -	srr1 = power9_idle_stop(psscr, false);
>> +	srr1 = stop_dep.idle_stop(psscr, true);
>>   	__ppc64_runlatch_on();
>>
>>   	local_paca->kvm_hstate.hwthread_state = KVM_HWTHREAD_IN_KERNEL;
>> @@ -856,7 +856,7 @@ void power9_idle_type(unsigned long stop_psscr_val,
>>   	psscr = (psscr & ~stop_psscr_mask) | stop_psscr_val;
>>
>>   	__ppc64_runlatch_off();
>> -	srr1 = power9_idle_stop(psscr, true);
>> +	srr1 = stop_dep.idle_stop(psscr, true);
>>   	__ppc64_runlatch_on();
>>
> There is one other place in arch/powerpc/kvm/book3s_hv_rmhandlers.S
> where call isa300_idle_stop_mayloss (this is kvm_nap_sequence).
>
> So, if stop states are not supported, then, KVM subsystem should know
> about it. Some KVM configurations depend on putting the secondary
> threads of the core offline into an idle state whose wakeup is from
> 0x100 vector. Your patch doesn't address that part.
>
Sure, I'll make sure to address it there too.

>
>>   		goto out;
>> +	switch(stop_dep.stop_version) {
>> +	case STOP_VERSION_P9:
>> +		stop_dep.idle_stop = power9_idle_stop;
>> +		break;
>> +	default:
>> +		stop_dep.idle_stop = NULL;
> You should add a pr_warn() here that stop state isn't supported
> because the kernel doesn't know about the version.
>
Sure


Thanks
Pratik