| 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
| ||
|
Message-ID: <20160801203156.GV4605@ubuntu>
Date: Mon, 1 Aug 2016 13:31:56 -0700
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Al Stone <ahs3@...hat.com>, ashwinch@...gle.com
Cc: rjw@...ysocki.net, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org,
Prashanth Prakash <pprakash@...eaurora.org>
Subject: Re: [PATCH v5] Force cppc_cpufreq to report values in KHz to fix
user space reporting
[+ Ashwin's new email id..]
On 20-07-16, 15:10, Al Stone wrote:
> When CPPC is being used by ACPI on arm64, user space tools such as
> cpupower report CPU frequency values from sysfs that are incorrect.
>
> What the driver was doing was reporting the values given by ACPI tables
> in whatever scale was used to provide them. However, the ACPI spec
> defines the CPPC values as unitless abstract numbers. Internal kernel
> structures such as struct perf_cap, in contrast, expect these values
> to be in KHz. When these struct values get reported via sysfs, the
> user space tools also assume they are in KHz, causing them to report
> incorrect values (for example, reporting a CPU frequency of 1MHz when
> it should be 1.8GHz).
>
> The downside is that this approach has some assumptions:
>
> (1) It relies on SMBIOS3 being used, *and* that the Max Frequency
> value for a processor is set to a non-zero value.
>
> (2) It assumes that all processors run at the same speed, or that
> the CPPC values have all been scaled to reflect relative speed.
> This patch retrieves the largest CPU Max Frequency from a type 4 DMI
> record that it can find. This may not be an issue, however, as a
> sampling of DMI data on x86 and arm64 indicates there is often only
> one such record regardless. Since CPPC is relatively new, it is
> unclear if the ACPI ASL will always be written to reflect any sort
> of relative performance of processors of differing speeds.
>
> (3) It assumes that performance and frequency both scale linearly.
>
> For arm64 servers, this may be sufficient, but it does rely on
> firmware values being set correctly. Hence, other approaches will
> be considered in the future.
>
> This has been tested on three arm64 servers, with and without DMI, with
> and without CPPC support.
>
> Changes for v5:
> -- Move code to cpufreq/cppc_cpufreq.c from acpi/cppc_acpi.c to keep
> frequency-related code together, and keep the CPPC abstract scale
> in ACPI (Prashanth Prakash)
> -- Fix the scaling to remove the incorrect assumption that frequency
> was always a range from zero to max; as a practical matter, it is
> not (Prasanth Prakash); this also allowed us to remove an over-
> engineered function to do this math.
>
> Changes for v4:
> -- Replaced magic constants with #defines (Rafael Wysocki)
> -- Renamed cppc_unitless_to_khz() to cppc_to_khz() (Rafael Wysocki)
> -- Replaced hidden initialization with a clearer form (Rafael Wysocki)
> -- Instead of picking up the first Max Speed value from DMI, we will
> now get the largest Max Speed; still an approximation, but slightly
> less subject to error (Rafael Wysocki)
> -- Kconfig for cppc_cpufreq now depends on DMI, instead of selecting
> it, in order to make sure DMI is set up properly (Rafael Wysocki)
>
> Changes for v3:
> -- Added clarifying commentary re short-term vs long-term fix (Alexey
> Klimov)
> -- Added range checking code to ensure proper arithmetic occurs,
> especially no division by zero (Alexey Klimov)
>
> Changes for v2:
> -- Corrected thinko: needed to have DEPENDS on DMI in Kconfig.arm,
> not SELECT DMI (found by build daemon)
>
> Signed-off-by: Al Stone <ahs3@...hat.com>
> Signed-off-by: Prashanth Prakash <pprakash@...eaurora.org>
> ---
> drivers/cpufreq/cppc_cpufreq.c | 53 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 8882b8e..6debc18 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -19,10 +19,19 @@
> #include <linux/delay.h>
> #include <linux/cpu.h>
> #include <linux/cpufreq.h>
> +#include <linux/dmi.h>
> #include <linux/vmalloc.h>
>
> +#include <asm/unaligned.h>
> +
> #include <acpi/cppc_acpi.h>
>
> +/* Minimum struct length needed for the DMI processor entry we want */
> +#define DMI_ENTRY_PROCESSOR_MIN_LENGTH 48
> +
> +/* Offest in the DMI processor structure for the max frequency */
> +#define DMI_PROCESSOR_MAX_SPEED 0x14
> +
> /*
> * These structs contain information parsed from per CPU
> * ACPI _CPC structures.
> @@ -32,6 +41,39 @@
> */
> static struct cpudata **all_cpu_data;
>
> +/* Capture the max KHz from DMI */
> +static u64 cppc_dmi_max_khz;
> +
> +/* Callback function used to retrieve the max frequency from DMI */
> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
> +{
> + const u8 *dmi_data = (const u8 *)dm;
> + u16 *mhz = (u16 *)private;
> +
> + if (dm->type == DMI_ENTRY_PROCESSOR &&
> + dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
> + u16 val = (u16)get_unaligned((const u16 *)
> + (dmi_data + DMI_PROCESSOR_MAX_SPEED));
> + *mhz = val > *mhz ? val : *mhz;
> + }
> +}
> +
> +/* Look up the max frequency in DMI */
> +static u64 cppc_get_dmi_max_khz(void)
> +{
> + u16 mhz = 0;
> +
> + dmi_walk(cppc_find_dmi_mhz, &mhz);
> +
> + /*
> + * Real stupid fallback value, just in case there is no
> + * actual value set.
> + */
> + mhz = mhz ? mhz : 1;
> +
> + return (1000 * mhz);
> +}
> +
> static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
> unsigned int target_freq,
> unsigned int relation)
> @@ -42,7 +84,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>
> cpu = all_cpu_data[policy->cpu];
>
> - cpu->perf_ctrls.desired_perf = target_freq;
> + cpu->perf_ctrls.desired_perf = target_freq * policy->max / cppc_dmi_max_khz;
> freqs.old = policy->cur;
> freqs.new = target_freq;
>
> @@ -94,8 +136,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> return ret;
> }
>
> - policy->min = cpu->perf_caps.lowest_perf;
> - policy->max = cpu->perf_caps.highest_perf;
> + cppc_dmi_max_khz = cppc_get_dmi_max_khz();
> +
> + policy->min = cpu->perf_caps.lowest_perf * cppc_dmi_max_khz / cpu->perf_caps.highest_perf;
> + policy->max = cppc_dmi_max_khz;
> policy->cpuinfo.min_freq = policy->min;
> policy->cpuinfo.max_freq = policy->max;
> policy->shared_type = cpu->shared_type;
> @@ -112,7 +156,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> cpu->cur_policy = policy;
>
> /* Set policy->cur to max now. The governors will adjust later. */
> - policy->cur = cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf;
> + policy->cur = cppc_dmi_max_khz;
> + cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf;
>
> ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
> if (ret)
> --
> 2.7.4
--
viresh
Powered by blists - more mailing lists