Re: [RFC PATCH 2/2] cpufreq/amd-pstate: Correctly reset bios_min_perf during suspend and offlining

[Mario Limonciello]

On 7/15/26 02:48, K Prateek Nayak wrote:
> msr_init_perf() accepts the configured bios_min_perf in CPPC_REQ MSR
> only if it finds the other fields (bits[63:16] + bits[7:0]) to be 0.
>
> This is intentional: a kernel unaware of bios_min_perf can leave the
> last perf request programmed in the CPPC_REQ MSR before kexec and the
> newer kernel, which is aware of bios_min_perf, should avoid incorrectly
> interpreting this stale value form last CPPC_REQ as the bios_min_perf.
>
> A kernel aware of bios_min_perf is expected to reprogram the CPPC_REQ as
> seen at the time of reset during suspend / onlining. This allows for
> kexeced kernel to pick the bios_min_freq during boot without needing
> complex mechanisms like KHO.
>
> Although suspend / offline callbacks correctly restore min_perf back to
> bios_min_perf, they fail to set other fields to 0s which causes the
> kexeced kernel to think the CPPC_REQ is configured with a stale value
> from the last boot.
>
> Restore the CPPC_REQ seen at the time of the boot during suspend /
> offline.
>
> The "cpudata->cppc_req_cached" is temporarily clobbered when the CPU is
> suspended / offlined to reflect the perf state at that time. A
> subsequent resume / onlining will use this clobbered data to reconstruct
> the CPPC request and force a reprogramming of hardware to restore the
> state back.
>
> Since cpufreq is suspended at the time of clobbering, the state will
> persist, undisturbed, until the CPU resumes activity.

Another idea; how about if you use kexec_in_progress to control behavior?

> Fixes: 85d7dda5a9f6 ("cpufreq/amd-pstate: Fix a regression leading to EPP 0 after hibernate")
> Fixes: ba3319e59057 ("cpufreq/amd-pstate: Fix a regression leading to EPP 0 after resume")
> Signed-off-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>
> ---
>   drivers/cpufreq/amd-pstate.c | 61 +++++++++++++++++++++++++++++-------
>   1 file changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index a6e43db6efefe..6ab769d4bd2da 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -2044,6 +2044,8 @@ static int amd_pstate_cpu_online(struct cpufreq_policy *policy)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
>   	union perf_cached perf = READ_ONCE(cpudata->perf);
> +	u64 cppc_req_cached = cpudata->cppc_req_cached;
> +	u8 min_perf, max_perf, des_perf, epp;
>   	u8 cached_floor_perf;
>   	int ret;
>   
> @@ -2051,6 +2053,22 @@ static int amd_pstate_cpu_online(struct cpufreq_policy *policy)
>   	if (ret)
>   		return ret;
>   
> +
> +	max_perf = FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cppc_req_cached);
> +	des_perf = FIELD_GET(AMD_CPPC_DES_PERF_MASK, cppc_req_cached);
> +	min_perf = FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cppc_req_cached);
> +	epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cppc_req_cached);
> +
> +	/*
> +	 * During offline, cppc_req_cached is clobbered with the state at the time of
> +	 * hotplug. Retrieve the relevant fields and then force a mismatch for the H/W
> +	 * to update with the correct values.
> +	 */
> +	cpudata->cppc_req_cached++;
> +	ret = amd_pstate_update_perf(policy, min_perf, des_perf, max_perf, epp, false);
> +	if (ret)
> +		return ret;
> +
>   	cached_floor_perf = freq_to_perf(perf, cpudata->nominal_freq, cpudata->floor_freq);
>   	return amd_pstate_set_floor_perf(policy, cached_floor_perf);
>   }
> @@ -2059,6 +2077,7 @@ static int amd_pstate_cpu_offline(struct cpufreq_policy *policy)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
>   	union perf_cached perf = READ_ONCE(cpudata->perf);
> +	u64 cppc_req_cached = cpudata->cppc_req_cached;
>   	int ret;
>   
>   	/*
> @@ -2066,14 +2085,13 @@ static int amd_pstate_cpu_offline(struct cpufreq_policy *policy)
>   	 * min_perf value across kexec reboots. If this CPU is just onlined normally after this, the
>   	 * limits, epp and desired perf will get reset to the cached values in cpudata struct
>   	 */
> -	ret = amd_pstate_update_perf(policy, perf.bios_min_perf,
> -				     FIELD_GET(AMD_CPPC_DES_PERF_MASK, cpudata->cppc_req_cached),
> -				     FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached),
> -				     FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached),
> -				     false);
> +	ret = amd_pstate_update_perf(policy, perf.bios_min_perf, 0U, 0U, 0U, false);
>   	if (ret)
>   		return ret;
>   
> +	/* HACK: See the comment in amd_pstate_suspend() below. */
> +	cpudata->cppc_req_cached = cppc_req_cached;
> +
>   	return amd_pstate_set_floor_perf(policy, cpudata->bios_floor_perf);
>   }
>   
> @@ -2081,6 +2099,7 @@ static int amd_pstate_suspend(struct cpufreq_policy *policy)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
>   	union perf_cached perf = READ_ONCE(cpudata->perf);
> +	u64 cppc_req_cached = cpudata->cppc_req_cached;
>   	int ret;
>   
>   	/*
> @@ -2088,14 +2107,17 @@ static int amd_pstate_suspend(struct cpufreq_policy *policy)
>   	 * min_perf value across kexec reboots. If this CPU is just resumed back without kexec,
>   	 * the limits, epp and desired perf will get reset to the cached values in cpudata struct
>   	 */
> -	ret = amd_pstate_update_perf(policy, perf.bios_min_perf,
> -				     FIELD_GET(AMD_CPPC_DES_PERF_MASK, cpudata->cppc_req_cached),
> -				     FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached),
> -				     FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached),
> -				     false);
> +	ret = amd_pstate_update_perf(policy, perf.bios_min_perf, 0U, 0U, 0U, false);
>   	if (ret)
>   		return ret;
>   
> +	/*
> +	 * HACK: Restore the cppc_req_cached at the time of suspend. This is
> +	 * used during amd_pstate_epp_resume() to restore the correct EPP value
> +	 * when this CPU resumes.
> +	 */
> +	cpudata->cppc_req_cached = cppc_req_cached;
> +
>   	ret = amd_pstate_set_floor_perf(policy, cpudata->bios_floor_perf);
>   	if (ret)
>   		return ret;
> @@ -2131,9 +2153,26 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
>   	u8 cached_floor_perf;
>   
>   	if (cpudata->suspended) {
> +		u64 cppc_req_cached = cpudata->cppc_req_cached;
> +		u8 min_perf, max_perf, des_perf, epp;
>   		int ret;
>   
> -		/* enable amd pstate from suspend state*/
> +		max_perf = FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cppc_req_cached);
> +		des_perf = FIELD_GET(AMD_CPPC_DES_PERF_MASK, cppc_req_cached);
> +		min_perf = FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cppc_req_cached);
> +		epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cppc_req_cached);
> +
> +		/*
> +		 * Enable amd-pstate from suspend state. During suspend, cppc_req_cached
> +		 * is clobbered with the state at the time of suspend. Retrieve the
> +		 * relevant fields and then force a mismatch for the H/W to update with
> +		 * the correct values.
> +		 */
> +		cpudata->cppc_req_cached++;
> +		ret = amd_pstate_update_perf(policy, min_perf, des_perf, max_perf, epp, false);
> +		if (ret)
> +			return ret;
> +
>   		ret = amd_pstate_epp_update_limit(policy, false);
>   		if (ret)
>   			return ret;