Re: [RFC PATCH 0/2] cpufreq/amd-pstate: Fixes for "Requested CPU Min frequency" support

[Mario Limonciello]

On 7/16/26 21:54, K Prateek Nayak wrote:
> Hello Mario,
>
> Thank you for reviewing the series.
>
> On 7/17/2026 1:20 AM, Mario Limonciello wrote:
> > On 7/15/26 02:48, K Prateek Nayak wrote:
> > > It was noted during internal testing that amd-pstate active mode with
> > > performance governor always sets the min_perf to nominal_perf despite
> > > BIOS having supplied bios_min_perf as the preferred idling perf.
> > >
> > > Users who set the "Requested CPU Min frequency" from BIOS are known to
> > > have profiled their workload at different operating frequency to know
> > > the best configuration and amd-pstate should use the same as the lower
> > > limit when configured.
> > >
> > > While testing the fix for above, it was noted that kexec fails to
> > > persist bios_min_freq even when both, the old and new kernel are aware
> > > of bios_min_perf.
> > >
> > > The suspend, offlining paths switched to persisting the CPPC_REQ MSR
> > > state at the time of suspend / offlining with only min_perf being reset
> > > to bios_min_perf.
> > >
> > > msr_init() path only accepts the configured min_perf as bios_min_perf
> > > when it finds rest of the bits in CPPC_REQ MSR to be 0. This is
> > > intentional to prevent stale value of last CPPC_REQ from kernels that
> > > are not aware of bios_min_perf to be mistakenly interpreted as the
> > > bios_min_perf during kexec boot.
> >
> > Is it a real valid case we need to worry about for someone kexec'ing between kernels that are aware of this vs not aware of it?
>
> Since there are defensive measure in place I thought it might have
> been a concern but as you said, I doubt anyone is running one of
> those flavors and kexecing back and forth that often.
>
>
> > During kernel development; sure this might happen.  But I would think once this is available in mainline any of the distro kernels will have picked this up and people will start with a distro kernel with support, or they'll start with a distro kernel without and upgrade to one with.
> >
> > I can't imagine a case people will go the other way.
>
> Ack! So I'm thinking of changing Patch 2 to:
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 27cfacd283be..707e1485eb1c 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -462,7 +462,6 @@ static int msr_init_perf(struct amd_cpudata *cpudata)
>   {
>   	union perf_cached perf = READ_ONCE(cpudata->perf);
>   	u64 cap1, numerator, cppc_req;
> -	u8 min_perf;
>   
>   	int ret = rdmsrq_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
>   				     &cap1);
> @@ -478,16 +477,6 @@ static int msr_init_perf(struct amd_cpudata *cpudata)
>   		return ret;
>   
>   	WRITE_ONCE(cpudata->cppc_req_cached, cppc_req);
> -	min_perf = FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cppc_req);
> -
> -	/*
> -	 * Clear out the min_perf part to check if the rest of the MSR is 0, if yes, this is an
> -	 * indication that the min_perf value is the one specified through the BIOS option
> -	 */
> -	cppc_req &= ~(AMD_CPPC_MIN_PERF_MASK);
> -
> -	if (!cppc_req)
> -		perf.bios_min_perf = min_perf;
>   
>   	perf.highest_perf = numerator;
>   	perf.max_limit_perf = numerator;
> @@ -495,6 +484,7 @@ static int msr_init_perf(struct amd_cpudata *cpudata)
>   	perf.nominal_perf = FIELD_GET(AMD_CPPC_NOMINAL_PERF_MASK, cap1);
>   	perf.lowest_nonlinear_perf = FIELD_GET(AMD_CPPC_LOWNONLIN_PERF_MASK, cap1);
>   	perf.lowest_perf = FIELD_GET(AMD_CPPC_LOWEST_PERF_MASK, cap1);
> +	perf.bios_min_perf = FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cppc_req);
>   	WRITE_ONCE(cpudata->perf, perf);
>   	WRITE_ONCE(cpudata->prefcore_ranking, FIELD_GET(AMD_CPPC_HIGHEST_PERF_MASK, cap1));
>   	WRITE_ONCE(cpudata->floor_perf_cnt, FIELD_GET(AMD_CPPC_FLOOR_PERF_CNT_MASK, cap1));
> @@ -1046,6 +1036,13 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   		return -EINVAL;
>   	}
>   
> +	if (perf.bios_min_perf) {
> +		u32 bios_min_freq = perf_to_freq(perf, cpudata->nominal_freq, perf.bios_min_perf);
> +
> +		pr_debug("Found Requested CPU Min Frequency of %u on CPU%d\n",
> +			 bios_min_freq, cpudata->cpu);
> +	}
> +
>   	return 0;
>   }
>   
> ---
>
> It gets rid of that defensive check and leaves enough breadcrumbs to
> debug any issues that might arise during a kexec. Thoughts?

That looks better to me, yes.