Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC 1/3] Interface for an idle-stop dependency structure
To:     ego@linux.vnet.ibm.com
Cc:     linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org,
        mpe@ellerman.id.au, mikey@neuling.org, npiggin@gmail.com,
        vaidy@linux.ibm.com, skiboot@lists.ozlabs.org, oohall@gmail.com,
        pratik.r.sampat@gmail.com
References: <cover.1583332695.git.psampat@linux.ibm.com>
 <80c7a6ae66ab25bd088beaa3bd5c69f3b1352ac2.1583332695.git.psampat@linux.ibm.com>
 <20200408105140.GD950@in.ibm.com>
From:   Pratik Sampat <psampat@linux.ibm.com>
Date:   Sun, 12 Apr 2020 17:48:43 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20200408105140.GD950@in.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Message-Id: <d2af70ee-6927-c29e-a7cb-a5dbd7c05c31@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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