| 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: <e29f1d7e-943d-4286-a92c-3db04f9e60ae@huawei.com>
Date: Wed, 15 Jan 2025 17:16:42 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: <lenb@...nel.org>, <robert.moore@...el.com>, <viresh.kumar@...aro.org>,
<mario.limonciello@....com>, <gautham.shenoy@....com>, <ray.huang@....com>,
<pierre.gondois@....com>, <acpica-devel@...ts.linux.dev>,
<linux-acpi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-pm@...r.kernel.org>, <linuxarm@...wei.com>,
<jonathan.cameron@...wei.com>, <zhanjie9@...ilicon.com>,
<lihuisong@...wei.com>, <hepeng68@...wei.com>, <fanghao11@...wei.com>
Subject: Re: [PATCH v4 5/6] ACPI: CPPC: Add autonomous selection ABIs
On 2025/1/15 2:24, Rafael J. Wysocki wrote:
> On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@...wei.com> wrote:
>
> This should mention the specification revision and section number(s)
> for the specification material the code is based on.
Will mention it. Thanks.
>
>> cppc_set_epp - write energy performance preference register value
>>
>> cppc_get_auto_act_window - read autonomous activity window register value
>>
>> cppc_set_auto_act_window - write autonomous activity window register value
>>
>> cppc_get_auto_sel - read autonomous selection enable register value,
>> modified from cppc_get_auto_sel_caps()
>
> It would be better to move the modification part into a separate patch.
Yes, good point. Thanks.
>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@...wei.com>
>> ---
>> drivers/acpi/cppc_acpi.c | 82 ++++++++++++++++++++++++++++++++----
>> drivers/cpufreq/amd-pstate.c | 3 +-
>> include/acpi/cppc_acpi.h | 30 +++++++++++--
>> 3 files changed, 103 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 03134613311d..7bfe30f7b40f 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1568,23 +1568,89 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>> EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>>
>> /**
>> - * cppc_get_auto_sel_caps - Read autonomous selection register.
>> - * @cpunum : CPU from which to read register.
>> - * @perf_caps : struct where autonomous selection register value is updated.
>> + * cppc_set_epp() - Write the EPP register.
>> + * @cpu: CPU on which to write register.
>> + * @epp_val: Value to write to the EPP register.
>> */
>> -int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> +int cppc_set_epp(int cpu, u64 epp_val)
>> {
>> - u64 auto_sel;
>> + if (epp_val > CPPC_ENERGY_PERF_MAX)
>> + return -EINVAL;
>> +
>> + return cppc_set_reg_val(cpu, ENERGY_PERF, epp_val);
>> +}
>> +EXPORT_SYMBOL_GPL(cppc_set_epp);
>> +
>> +/**
>> + * cppc_get_auto_act_window() - Read autonomous activity window register.
>> + * @cpu: CPU from which to read register.
>> + * @auto_act_window: Return address.
>
> It would be good to describe the autonomous activity window encoding.
Will add. Thanks.
>
>> + */
>> +int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
>> +{
>> + unsigned int exp;
>> + u64 val, sig;
>> + int ret;
>> +
>> + ret = cppc_get_reg_val(cpu, AUTO_ACT_WINDOW, &val);
>> + if (ret)
>> + return ret;
>> +
>> + sig = val & CPPC_AUTO_ACT_WINDOW_MAX_SIG;
>> + exp = (val >> CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) & CPPC_AUTO_ACT_WINDOW_MAX_EXP;
>> + *auto_act_window = sig * int_pow(10, exp);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(cppc_get_auto_act_window);
>> +
>> +/**
>> + * cppc_set_auto_act_window() - Write autonomous activity window register.
>> + * @cpu: CPU on which to write register.
>> + * @auto_act_window: usec value to write to the autonomous activity window register.
>> + */
>> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
>> +{
>> + u64 max_val = CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP);
>> + int digits = 0;
>> + u64 val;
>> +
>> + if (auto_act_window > max_val)
>> + return -EINVAL;
>> +
>> + while (auto_act_window > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
>> + auto_act_window /= 10;
>> + digits += 1;
>> + }
>> +
>> + if (auto_act_window > CPPC_AUTO_ACT_WINDOW_MAX_SIG)
>> + auto_act_window = CPPC_AUTO_ACT_WINDOW_MAX_SIG;
>
> It looks like this may clobber the most significant bit, or am I mistaken?
Actually, after the while loop above, auto_act_window is not larger than
CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH, which is 129. So this if condition
is valid only when auto_act_window is 128 or 129. Since we only have 7 bits
to store this value, 128 and 129 can only be cut to 127.
>
>> +
>> + val = (digits << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) + auto_act_window;
>> +
>> + return cppc_set_reg_val(cpu, AUTO_ACT_WINDOW, val);
>> +}
>> +EXPORT_SYMBOL_GPL(cppc_set_auto_act_window);
>> +
>> +/**
>> + * cppc_get_auto_sel() - Read autonomous selection register.
>> + * @cpu: CPU from which to read register.
>> + * @enable: Return address.
>> + */
>> +int cppc_get_auto_sel(int cpu, bool *enable)
>> +{
>> + u64 val;
>> int ret;
>>
>> - ret = cppc_get_reg_val(cpunum, AUTO_SEL_ENABLE, &auto_sel);
>> + ret = cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, &val);
>> if (ret)
>> return ret;
>>
>> - perf_caps->auto_sel = (bool)auto_sel;
>> + *enable = (bool)val;
>> +
>> return 0;
>> }
>> -EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
>> +EXPORT_SYMBOL_GPL(cppc_get_auto_sel);
>>
>> /**
>> * cppc_set_auto_sel - Write autonomous selection register.
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 66e5dfc711c0..8bc11d0618f8 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -399,6 +399,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
>> {
>> struct cppc_perf_caps cppc_perf;
>> u64 numerator;
>> + bool auto_sel;
>>
>> int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
>> if (ret)
>> @@ -420,7 +421,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
>> if (cppc_state == AMD_PSTATE_ACTIVE)
>> return 0;
>>
>> - ret = cppc_get_auto_sel_caps(cpudata->cpu, &cppc_perf);
>> + ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);
>> if (ret) {
>> pr_warn("failed to get auto_sel, ret: %d\n", ret);
>> return 0;
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 62d368bcd9ec..325e9543e08f 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -32,6 +32,15 @@
>> #define CMD_READ 0
>> #define CMD_WRITE 1
>>
>> +#define CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE (7)
>> +#define CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE (3)
>> +#define CPPC_AUTO_ACT_WINDOW_MAX_SIG ((1 << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) - 1)
>> +#define CPPC_AUTO_ACT_WINDOW_MAX_EXP ((1 << CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE) - 1)
>> +/* CPPC_AUTO_ACT_WINDOW_MAX_SIG is 127, so 128 and 129 will decay to 127 when writing */
>> +#define CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH 129
>> +
>> +#define CPPC_ENERGY_PERF_MAX (0xFF)
>> +
>> /* Each register has the folowing format. */
>> struct cpc_reg {
>> u8 descriptor;
>> @@ -159,7 +168,10 @@ extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
>> extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
>> extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
>> extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
>> -extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
>> +extern int cppc_set_epp(int cpu, u64 epp_val);
>> +extern int cppc_get_auto_act_window(int cpu, u64 *auto_act_window);
>> +extern int cppc_set_auto_act_window(int cpu, u64 auto_act_window);
>> +extern int cppc_get_auto_sel(int cpu, bool *enable);
>> extern int cppc_set_auto_sel(int cpu, bool enable);
>> extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>> extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>> @@ -229,11 +241,23 @@ static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>> {
>> return -EOPNOTSUPP;
>> }
>> -static inline int cppc_set_auto_sel(int cpu, bool enable)
>> +static inline int cppc_set_epp(int cpu, u64 epp_val)
>> {
>> return -EOPNOTSUPP;
>> }
>> -static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>> +static inline int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +static inline int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +static inline int cppc_get_auto_sel(int cpu, bool *enable)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +static inline int cppc_set_auto_sel(int cpu, bool enable)
>> {
>> return -EOPNOTSUPP;
>> }
>> --
>> 2.33.0
>>
>>
>
Powered by blists - more mailing lists