linux-kernel - Re: [Question]: about 'cpuinfo_cur_freq' shown in sysfs when the CPU is in idle state

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200603075200.hbyofgcyiwocl565@vireshk-i7>
Date:   Wed, 3 Jun 2020 13:22:00 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Xiongfeng Wang <wangxiongfeng2@...wei.com>
Cc:     rjw@...ysocki.net, guohanjun@...wei.com, Sudeep.Holla@....com,
        ionela.voinescu@....com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Question]: about 'cpuinfo_cur_freq' shown in sysfs when the CPU
 is in idle state

On 02-06-20, 11:34, Xiongfeng Wang wrote:
> Hi Viresh,
> 
> Sorry to disturb you about another problem as follows.
> 
> CPPC use the increment of Desired Performance counter and Reference Performance
> counter to get the CPU frequency and show it in sysfs through
> 'cpuinfo_cur_freq'. But ACPI CPPC doesn't specifically define the behavior of
> these two counters when the CPU is in idle state, such as stop incrementing when
> the CPU is in idle state.
> 
> ARMv8.4 Extension inctroduced support for the Activity Monitors Unit (AMU). The
> processor frequency cycles and constant frequency cycles in AMU can be used as
> Delivered Performance counter and Reference Performance counter. These two
> counter in AMU does not increase when the PE is in WFI or WFE. So the increment
> is zero when the PE is in WFI/WFE. This cause no issue because
> 'cppc_get_rate_from_fbctrs()' in cppc_cpufreq driver will check the increment
> and return the desired performance if the increment is zero.
> 
> But when the CPU goes into power down idle state, accessing these two counters
> in AMU by memory-mapped address will return zero. Such as CPU1 went into power
> down idle state and CPU0 try to get the frequency of CPU1. In this situation,
> will display a very big value for 'cpuinfo_cur_freq' in sysfs. Do you have some
> advice about this problem ?
> 
> I was thinking about an idea as follows. We can run 'cppc_cpufreq_get_rate()' on
> the CPU to be measured, so that we can make sure the CPU is in C0 state when we
> access the two counters. Also we can return the actual frequency rather than
> desired performance when the CPU is in WFI/WFE. But this modification will
> change the existing logical and I am not sure if this will cause some bad effect.
> 
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 257d726..ded3bcc 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -396,9 +396,10 @@ static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
>         return cppc_cpufreq_perf_to_khz(cpu, delivered_perf);
>  }
> 
> -static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
> +static int cppc_cpufreq_get_rate_cpu(void *info)
>  {
>         struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> + unsigned int cpunum = *(unsigned int *)info;
>         struct cppc_cpudata *cpu = all_cpu_data[cpunum];
>         int ret;
> 
> @@ -418,6 +419,22 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>         return cppc_get_rate_from_fbctrs(cpu, fb_ctrs_t0, fb_ctrs_t1);
>  }
> 
> +static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
> +{
> + unsigned int ret;
> +
> + ret = smp_call_on_cpu(cpunum, cppc_cpufreq_get_rate_cpu, &cpunum, true);
> +
> + /*
> +  * convert negative error code to zero, otherwise we will display
> +  * an odd value for 'cpuinfo_cur_freq' in sysfs
> +  */
> + if (ret < 0)
> +         ret = 0;
> +
> + return ret;
> +}
> +
>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>  {
>         struct cppc_cpudata *cpudata;

I don't see any other sane solution, even if this brings the CPU back
to normal state and waste power. We should be able to reliably provide
value to userspace.

Rafael / Sudeep: What you do say ?

-- 
viresh