linux-kernel - Re: [PATCH] acpi-cpufreq: Fix max-frequency computation

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] [day] [month] [year] [list]

Message-ID: <98bcf90d-0bbc-4bb5-b678-67296e476680@amd.com>
Date: Mon, 13 Jan 2025 08:22:54 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: "Gautham R. Shenoy" <gautham.shenoy@....com>,
 Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>,
 "Rafael J . Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Adam Clark <Adam.Clark@....com>
Subject: Re: [PATCH] acpi-cpufreq: Fix max-frequency computation

On 1/12/2025 22:41, Gautham R. Shenoy wrote:
> commit 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover
> boost frequencies") introduces an assumption in
> acpi_cpufreq_cpu_init() that the first entry in the P-state table is
> the nominal frequency. This assumption is incorrect. The frequency
> corresponding to the P0 P-State need not be the same as the nominal
> frequency advertised via CPPC.
> 
> Since the driver is using the CPPC.highest_perf and CPPC.nominal_perf
> to compute the boost-ratio, it makes sense to use CPPC.nominal_freq to
> compute the max-frequency. CPPC.nominal_freq is advertised on
> platforms supporting CPPC revisions 3 or higher.
> 
> Hence, fallback to using the first entry in the P-State table only on
> platforms that do not advertise CPPC.nominal_freq.
> 
> Fixes: 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover boost frequencies")
> Tested-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
> Signed-off-by: Gautham R. Shenoy <gautham.shenoy@....com>

Reviewed-by: Mario Limonciello <mario.limonciello@....com>

> ---
>   drivers/cpufreq/acpi-cpufreq.c | 36 +++++++++++++++++++++++++---------
>   1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index c9ebacf5c88e..744fcdeab173 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -623,7 +623,14 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c)
>   #endif
>   
>   #ifdef CONFIG_ACPI_CPPC_LIB
> -static u64 get_max_boost_ratio(unsigned int cpu)
> +/*
> + * get_max_boost_ratio: Computes the max_boost_ratio as the ratio
> + * between the highest_perf and the nominal_perf.
> + *
> + * Returns the max_boost_ratio for @cpu. Returns the CPPC nominal
> + * frequency via @nominal_freq if it is non-NULL pointer.
> + */
> +static u64 get_max_boost_ratio(unsigned int cpu, u64 *nominal_freq)
>   {
>   	struct cppc_perf_caps perf_caps;
>   	u64 highest_perf, nominal_perf;
> @@ -652,6 +659,9 @@ static u64 get_max_boost_ratio(unsigned int cpu)
>   
>   	nominal_perf = perf_caps.nominal_perf;
>   
> +	if (nominal_freq)
> +		*nominal_freq = perf_caps.nominal_freq;
> +
>   	if (!highest_perf || !nominal_perf) {
>   		pr_debug("CPU%d: highest or nominal performance missing\n", cpu);
>   		return 0;
> @@ -664,8 +674,12 @@ static u64 get_max_boost_ratio(unsigned int cpu)
>   
>   	return div_u64(highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
>   }
> +
>   #else
> -static inline u64 get_max_boost_ratio(unsigned int cpu) { return 0; }
> +static inline u64 get_max_boost_ratio(unsigned int cpu, u64 *nominal_freq)
> +{
> +	return 0;
> +}
>   #endif
>   
>   static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> @@ -677,7 +691,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>   	struct cpuinfo_x86 *c = &cpu_data(cpu);
>   	unsigned int valid_states = 0;
>   	unsigned int result = 0;
> -	u64 max_boost_ratio;
> +	u64 max_boost_ratio, nominal_freq = 0;
>   	unsigned int i;
>   #ifdef CONFIG_SMP
>   	static int blacklisted;
> @@ -827,16 +841,20 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>   	}
>   	freq_table[valid_states].frequency = CPUFREQ_TABLE_END;
>   
> -	max_boost_ratio = get_max_boost_ratio(cpu);
> +	max_boost_ratio = get_max_boost_ratio(cpu, &nominal_freq);
>   	if (max_boost_ratio) {
> -		unsigned int freq = freq_table[0].frequency;
> +		unsigned int freq = nominal_freq;
>   
>   		/*
> -		 * Because the loop above sorts the freq_table entries in the
> -		 * descending order, freq is the maximum frequency in the table.
> -		 * Assume that it corresponds to the CPPC nominal frequency and
> -		 * use it to set cpuinfo.max_freq.
> +		 * The loop above sorts the freq_table entries in the
> +		 * descending order. If ACPI CPPC has not advertised
> +		 * the nominal frequency (this is possible in CPPC
> +		 * revisions prior to 3), then use the first entry in
> +		 * the pstate table as a proxy for nominal frequency.
>   		 */
> +		if (!freq)
> +			freq = freq_table[0].frequency;
> +
>   		policy->cpuinfo.max_freq = freq * max_boost_ratio >> SCHED_CAPACITY_SHIFT;
>   	} else {
>   		/*