linux-kernel - Re: [v2 PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170719190913.18ca21e5@roar.ozlabs.ibm.com>
Date:   Wed, 19 Jul 2017 19:09:13 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        Michael Neuling <mikey@...ling.org>,
        Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
        Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>,
        Akshay Adiga <akshay.adiga@...ux.vnet.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [v2 PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via
 stop-api only on Hotplug

On Wed, 19 Jul 2017 13:48:50 +0530
"Gautham R. Shenoy" <ego@...ux.vnet.ibm.com> wrote:

> From: "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>
> 
> Currently we use the stop-api provided by the firmware to program the
> SLW engine to restore the values of hypervisor resources that get lost
> on deeper idle states (such as winkle). Since the deep states were
> only used for CPU-Hotplug on POWER8 systems, we would program the LPCR
> to have the PECE1 bit since Hotplugged CPUs shouldn't be spuriously
> woken up by decrementer.
> 
> On POWER9, some of the deep platform idle states such as stop4 can be
> used in cpuidle as well. In this case, we want the CPU in stop4 to be
> woken up by the decrementer when some timer on the CPU expires.
> 
> In this patch, for POWER9, we program the stop-api for LPCR with PECE1
> bit cleared only when we are offlining the CPU.
> 
> Signed-off-by: Gautham R. Shenoy <ego@...ux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/idle.c | 45 ++++++++++++++++++++++++++++++++++-
>  arch/powerpc/platforms/powernv/smp.c  | 10 --------
>  2 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 2abee07..a6c69b7 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -68,7 +68,7 @@ static int pnv_save_sprs_for_deep_states(void)
>  	 * all cpus at boot. Get these reg values of current cpu and use the
>  	 * same across all cpus.
>  	 */
> -	uint64_t lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
> +	uint64_t lpcr_val = mfspr(SPRN_LPCR);
>  	uint64_t hid0_val = mfspr(SPRN_HID0);
>  	uint64_t hid1_val = mfspr(SPRN_HID1);
>  	uint64_t hid4_val = mfspr(SPRN_HID4);
> @@ -85,6 +85,16 @@ static int pnv_save_sprs_for_deep_states(void)
>  		if (rc != 0)
>  			return rc;
>  
> +		/*
> +		 * On POWER8, the only state that uses SLW engine is
> +		 * winkle.  This is only used for CPU-Hotplug. So we
> +		 * clear the decrementer bit from LPCR since we
> +		 * don't want to be woken up on decrementer when in
> +		 * winkle.
> +		 */
> +		if (!cpu_has_feature(CPU_FTR_ARCH_300))
> +			lpcr_val = lpcr_val & ~(u64)LPCR_PECE1;
> +
>  		rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
>  		if (rc != 0)
>  			return rc;
> @@ -355,6 +365,15 @@ void power9_idle(void)
>  }
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> +static void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val)
> +{
> +	u64 pir = get_hard_smp_processor_id(cpu);
> +
> +	mtspr(SPRN_LPCR, lpcr_val);
> +	if (cpu_has_feature(CPU_FTR_ARCH_300))
> +		opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);

Is opal_slw_set_reg very heavyweight? If not, I would just remove the
POWER9 special casing entirely from both these functions, which follows
the principle of least surprise. Otherwise I guess it's reasonable.

Thanks for making those other changes, it looks good.

Reviewed-by: Nicholas Piggin <npiggin@...il.com>