Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 23 Apr 2020 23:45:23 +0530
From:   Gautham R Shenoy <ego@linux.vnet.ibm.com>
To:     Pratik Rajesh Sampat <psampat@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org,
        mpe@ellerman.id.au, skiboot@lists.ozlabs.org, oohall@gmail.com,
        ego@linux.vnet.ibm.com, linuxram@us.ibm.com,
        pratik.r.sampat@gmail.com
Subject: Re: [PATCH v8 1/1] powerpc/powernv: Introduce support and parsing
 for self-save API
Message-ID: <20200423181523.GA15514@in.ibm.com>
Reply-To: ego@linux.vnet.ibm.com
References: <20200423105557.29108-1-psampat@linux.ibm.com>
 <20200423105557.29108-2-psampat@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200423105557.29108-2-psampat@linux.ibm.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Apr 23, 2020 at 04:25:57PM +0530, Pratik Rajesh Sampat wrote:
> This commit introduces and leverages the Self save API. The difference
> between self-save and self-restore is that the value to be saved for the
> SPR does not need to be passed to the call.
> 
> Add the new Self Save OPAL API call in the list of OPAL calls.
> 
> The device tree is parsed looking for the property "ibm,opal-self-save"
> If self-save is supported then for all SPRs self-save is invoked for all
> P9 supported registers. In the case self-save fails corresponding
> self-restore call is invoked as a fallback.
> 
> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>


A suggestion from the bisectability point of view though.

Since in this patch you are also invoking self_save API for a new SPR,
namely PTCR which was previously not present, I would suggest that you
move the PTCR changes to a different patch.

Otherwise, the patchset looks good to me

Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

> ---
>  arch/powerpc/include/asm/opal-api.h        |  3 +-
>  arch/powerpc/include/asm/opal.h            |  1 +
>  arch/powerpc/platforms/powernv/idle.c      | 73 ++++++++++++++++++----
>  arch/powerpc/platforms/powernv/opal-call.c |  1 +
>  4 files changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
> index 1dffa3cb16ba..7ba698369083 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -214,7 +214,8 @@
>  #define OPAL_SECVAR_GET				176
>  #define OPAL_SECVAR_GET_NEXT			177
>  #define OPAL_SECVAR_ENQUEUE_UPDATE		178
> -#define OPAL_LAST				178
> +#define OPAL_SLW_SELF_SAVE_REG			181
> +#define OPAL_LAST				181
> 
>  #define QUIESCE_HOLD			1 /* Spin all calls at entry */
>  #define QUIESCE_REJECT			2 /* Fail all calls with OPAL_BUSY */
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 9986ac34b8e2..a370b0e8d899 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -204,6 +204,7 @@ int64_t opal_handle_hmi2(__be64 *out_flags);
>  int64_t opal_register_dump_region(uint32_t id, uint64_t start, uint64_t end);
>  int64_t opal_unregister_dump_region(uint32_t id);
>  int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
> +int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
>  int64_t opal_config_cpu_idle_state(uint64_t state, uint64_t flag);
>  int64_t opal_pci_set_phb_cxl_mode(uint64_t phb_id, uint64_t mode, uint64_t pe_number);
>  int64_t opal_pci_get_pbcq_tunnel_bar(uint64_t phb_id, uint64_t *addr);
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 78599bca66c2..ada7ece24521 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -32,6 +32,11 @@
>  #define P9_STOP_SPR_MSR 2000
>  #define P9_STOP_SPR_PSSCR      855
> 
> +/* Caching the self-save functionality, lpcr, ptcr support */
> +DEFINE_STATIC_KEY_FALSE(self_save_available);
> +DEFINE_STATIC_KEY_FALSE(is_lpcr_self_save);
> +DEFINE_STATIC_KEY_FALSE(is_ptcr_self_save);
> +
>  static u32 supported_cpuidle_states;
>  struct pnv_idle_states_t *pnv_idle_states;
>  int nr_pnv_idle_states;
> @@ -61,6 +66,35 @@ static bool deepest_stop_found;
> 
>  static unsigned long power7_offline_type;
> 
> +/*
> + * Cache support for SPRs that support self-save as well as kernel save restore
> + * so that kernel does not duplicate efforts in saving and restoring SPRs
> + */
> +static void cache_spr_self_save_support(u64 sprn)
> +{
> +	switch (sprn) {
> +	case SPRN_LPCR:
> +		static_branch_enable(&is_lpcr_self_save);
> +		break;
> +	case SPRN_PTCR:
> +		static_branch_enable(&is_ptcr_self_save);
> +		break;
> +	}
> +}
> +
> +static int pnv_save_one_spr(u64 pir, u64 sprn, u64 val)
> +{
> +	if (static_branch_likely(&self_save_available)) {
> +		int rc = opal_slw_self_save_reg(pir, sprn);
> +
> +		if (!rc) {
> +			cache_spr_self_save_support(sprn);
> +			return rc;
> +		}
> +	}
> +	return opal_slw_set_reg(pir, sprn, val);
> +}
> +
>  static int pnv_save_sprs_for_deep_states(void)
>  {
>  	int cpu;
> @@ -72,6 +106,7 @@ static int pnv_save_sprs_for_deep_states(void)
>  	 * same across all cpus.
>  	 */
>  	uint64_t lpcr_val	= mfspr(SPRN_LPCR);
> +	uint64_t ptcr_val	= mfspr(SPRN_PTCR);
>  	uint64_t hid0_val	= mfspr(SPRN_HID0);
>  	uint64_t hid1_val	= mfspr(SPRN_HID1);
>  	uint64_t hid4_val	= mfspr(SPRN_HID4);
> @@ -84,30 +119,34 @@ static int pnv_save_sprs_for_deep_states(void)
>  		uint64_t pir = get_hard_smp_processor_id(cpu);
>  		uint64_t hsprg0_val = (uint64_t)paca_ptrs[cpu];
> 
> -		rc = opal_slw_set_reg(pir, SPRN_HSPRG0, hsprg0_val);
> +		rc = pnv_save_one_spr(pir, SPRN_HSPRG0, hsprg0_val);
>  		if (rc != 0)
>  			return rc;
> 
> -		rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> +		rc = pnv_save_one_spr(pir, SPRN_LPCR, lpcr_val);
>  		if (rc != 0)
>  			return rc;
> 
> +		/*
> +		 * No need to check for failure, if firmware fails to save then
> +		 * kernel handles save-restore for PTCR
> +		 */
> +		pnv_save_one_spr(pir, SPRN_PTCR, ptcr_val);
> +
>  		if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> -			rc = opal_slw_set_reg(pir, P9_STOP_SPR_MSR, msr_val);
> +			rc = pnv_save_one_spr(pir, P9_STOP_SPR_MSR, msr_val);
>  			if (rc)
>  				return rc;
> 
> -			rc = opal_slw_set_reg(pir,
> +			rc = pnv_save_one_spr(pir,
>  					      P9_STOP_SPR_PSSCR, psscr_val);
> -
>  			if (rc)
>  				return rc;
>  		}
> 
>  		/* HIDs are per core registers */
>  		if (cpu_thread_in_core(cpu) == 0) {
> -
> -			rc = opal_slw_set_reg(pir, SPRN_HMEER, hmeer_val);
> +			rc = pnv_save_one_spr(pir, SPRN_HMEER, hmeer_val);
>  			if (rc != 0)
>  				return rc;
> 
> @@ -658,7 +697,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
>  		mmcr0		= mfspr(SPRN_MMCR0);
>  	}
>  	if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
> -		sprs.lpcr	= mfspr(SPRN_LPCR);
> +		if (!static_branch_unlikely(&is_lpcr_self_save))
> +			sprs.lpcr	= mfspr(SPRN_LPCR);
>  		sprs.hfscr	= mfspr(SPRN_HFSCR);
>  		sprs.fscr	= mfspr(SPRN_FSCR);
>  		sprs.pid	= mfspr(SPRN_PID);
> @@ -672,7 +712,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
>  		sprs.mmcr1	= mfspr(SPRN_MMCR1);
>  		sprs.mmcr2	= mfspr(SPRN_MMCR2);
> 
> -		sprs.ptcr	= mfspr(SPRN_PTCR);
> +		if (!static_branch_unlikely(&is_ptcr_self_save))
> +			sprs.ptcr	= mfspr(SPRN_PTCR);
>  		sprs.rpr	= mfspr(SPRN_RPR);
>  		sprs.tscr	= mfspr(SPRN_TSCR);
>  		if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> @@ -756,7 +797,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
>  		goto core_woken;
> 
>  	/* Per-core SPRs */
> -	mtspr(SPRN_PTCR,	sprs.ptcr);
> +	if (!static_branch_unlikely(&is_ptcr_self_save))
> +		mtspr(SPRN_PTCR,	sprs.ptcr);
>  	mtspr(SPRN_RPR,		sprs.rpr);
>  	mtspr(SPRN_TSCR,	sprs.tscr);
> 
> @@ -777,7 +819,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
>  	atomic_unlock_and_stop_thread_idle();
> 
>  	/* Per-thread SPRs */
> -	mtspr(SPRN_LPCR,	sprs.lpcr);
> +	if (!static_branch_unlikely(&is_lpcr_self_save))
> +		mtspr(SPRN_LPCR,	sprs.lpcr);
>  	mtspr(SPRN_HFSCR,	sprs.hfscr);
>  	mtspr(SPRN_FSCR,	sprs.fscr);
>  	mtspr(SPRN_PID,		sprs.pid);
> @@ -956,8 +999,10 @@ void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val)
>  	 * Program the LPCR via stop-api only if the deepest stop state
>  	 * can lose hypervisor context.
>  	 */
> -	if (supported_cpuidle_states & OPAL_PM_LOSE_FULL_CONTEXT)
> -		opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> +	if (supported_cpuidle_states & OPAL_PM_LOSE_FULL_CONTEXT) {
> +		if (!static_branch_unlikely(&is_lpcr_self_save))
> +			opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> +	}
>  }
> 
>  /*
> @@ -1298,6 +1343,8 @@ static int pnv_parse_cpuidle_dt(void)
>  		}
>  		for (i = 0; i < nr_idle_states; i++)
>  			pnv_idle_states[i].psscr_mask = temp_u64[i];
> +		if (of_property_read_bool(np, "ibm,opal-self-save"))
> +			static_branch_enable(&self_save_available);
>  	}
> 
>  	/*
> diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
> index 5cd0f52d258f..11e0ceb90de0 100644
> --- a/arch/powerpc/platforms/powernv/opal-call.c
> +++ b/arch/powerpc/platforms/powernv/opal-call.c
> @@ -223,6 +223,7 @@ OPAL_CALL(opal_handle_hmi,			OPAL_HANDLE_HMI);
>  OPAL_CALL(opal_handle_hmi2,			OPAL_HANDLE_HMI2);
>  OPAL_CALL(opal_config_cpu_idle_state,		OPAL_CONFIG_CPU_IDLE_STATE);
>  OPAL_CALL(opal_slw_set_reg,			OPAL_SLW_SET_REG);
> +OPAL_CALL(opal_slw_self_save_reg,		OPAL_SLW_SELF_SAVE_REG);
>  OPAL_CALL(opal_register_dump_region,		OPAL_REGISTER_DUMP_REGION);
>  OPAL_CALL(opal_unregister_dump_region,		OPAL_UNREGISTER_DUMP_REGION);
>  OPAL_CALL(opal_pci_set_phb_cxl_mode,		OPAL_PCI_SET_PHB_CAPI_MODE);
> -- 
> 2.17.1
>