| 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: <c3fd7249-3cba-43e9-85c6-eadd711c0527@nvidia.com>
Date: Tue, 9 Dec 2025 23:40:41 +0530
From: Sumit Gupta <sumitg@...dia.com>
To: Pierre Gondois <pierre.gondois@....com>, mario.limonciello@....com
Cc: linux-kernel@...r.kernel.org, acpica-devel@...ts.linux.dev,
linux-doc@...r.kernel.org, linux-acpi@...r.kernel.org,
linux-pm@...r.kernel.org, zhanjie9@...ilicon.com, ionela.voinescu@....com,
perry.yuan@....com, gautham.shenoy@....com, ray.huang@....com,
zhenglifeng1@...wei.com, corbet@....net, lenb@...nel.org,
robert.moore@...el.com, viresh.kumar@...aro.org, rafael@...nel.org,
linux-tegra@...r.kernel.org, treding@...dia.com, jonathanh@...dia.com,
vsethi@...dia.com, ksitaraman@...dia.com, sanjayc@...dia.com,
nhartman@...dia.com, bbasu@...dia.com, rdunlap@...radead.org,
sumitg@...dia.com
Subject: Re: [PATCH v4 3/8] ACPI: CPPC: extend APIs to support auto_sel and
epp
On 27/11/25 20:24, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Sumit, Mario,
>
> On 11/5/25 12:38, Sumit Gupta wrote:
>> - Add auto_sel read support in cppc_get_perf_caps().
>> - Add write of both auto_sel and energy_perf in cppc_set_epp_perf().
>> - Remove redundant energy_perf field from 'struct cppc_perf_caps' as
>> the same is available in 'struct cppc_perf_ctrls' which is used.
>>
>> Signed-off-by: Sumit Gupta<sumitg@...dia.com>
>> ---
>> drivers/acpi/cppc_acpi.c | 42 ++++++++++++++++++++++++++++++++--------
>> include/acpi/cppc_acpi.h | 1 -
>> 2 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 05672c30187c..757e8ce87e9b 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1344,8 +1344,8 @@ int cppc_get_perf_caps(int cpunum, struct
>> cppc_perf_caps *perf_caps)
>> struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> struct cpc_register_resource *highest_reg, *lowest_reg,
>> *lowest_non_linear_reg, *nominal_reg, *guaranteed_reg,
>> - *low_freq_reg = NULL, *nom_freq_reg = NULL;
>> - u64 high, low, guaranteed, nom, min_nonlinear, low_f = 0, nom_f
>> = 0;
>> + *low_freq_reg = NULL, *nom_freq_reg = NULL,
>> *auto_sel_reg = NULL;
>> + u64 high, low, guaranteed, nom, min_nonlinear, low_f = 0, nom_f
>> = 0, auto_sel = 0;
>
> I am not sure this is really useful to get the auto_sel value register
> in this function as:
> - All of the other registers read are read-only
> - The name of the function doesn't match: the autonomous selection is
> not really
> related to perf. capabilities
>
> I assume this change comes from the presence of the auto_sel register
> in the
> 'struct cppc_perf_caps', but IMO this register should be placed in
> another structure.
>
> I assume this is ok to let it in 'struct cppc_perf_caps' for now, but I
> think we should not
> fetch the value with all the other perf. capabilities values.
>
Yes, reading it here in cppc_get_perf_caps() as it was part of
'struct cppc_perf_caps' in the existing code which i agree looks wrong.
I am adding cppc_get_perf() api to read performance controls in 'patch
2/8'.
Additionally, I can move the 'auto_sel' field from 'struct cppc_perf_caps'
to 'struct cppc_perf_ctrls' and read it within cppc_get_perf() with other
performance controls.
>
>> int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>> struct cppc_pcc_data *pcc_ss_data = NULL;
>> int ret = 0, regs_in_pcc = 0;
>> @@ -1362,11 +1362,12 @@ int cppc_get_perf_caps(int cpunum, struct
>> cppc_perf_caps *perf_caps)
>> low_freq_reg = &cpc_desc->cpc_regs[LOWEST_FREQ];
>> nom_freq_reg = &cpc_desc->cpc_regs[NOMINAL_FREQ];
>> guaranteed_reg = &cpc_desc->cpc_regs[GUARANTEED_PERF];
>> + auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>>
>> /* Are any of the regs PCC ?*/
>> if (CPC_IN_PCC(highest_reg) || CPC_IN_PCC(lowest_reg) ||
>> CPC_IN_PCC(lowest_non_linear_reg) ||
>> CPC_IN_PCC(nominal_reg) ||
>> - CPC_IN_PCC(low_freq_reg) || CPC_IN_PCC(nom_freq_reg)) {
>> + CPC_IN_PCC(low_freq_reg) || CPC_IN_PCC(nom_freq_reg) ||
>> CPC_IN_PCC(auto_sel_reg)) {
>> if (pcc_ss_id < 0) {
>> pr_debug("Invalid pcc_ss_id\n");
>> return -ENODEV;
>> @@ -1414,6 +1415,9 @@ int cppc_get_perf_caps(int cpunum, struct
>> cppc_perf_caps *perf_caps)
>> perf_caps->lowest_freq = low_f;
>> perf_caps->nominal_freq = nom_f;
>>
>> + if (CPC_SUPPORTED(auto_sel_reg))
>> + cpc_read(cpunum, auto_sel_reg, &auto_sel);
>> + perf_caps->auto_sel = (bool)auto_sel;
>>
>> out_err:
>> if (regs_in_pcc)
>> @@ -1555,6 +1559,8 @@ int cppc_set_epp_perf(int cpu, struct
>> cppc_perf_ctrls *perf_ctrls, bool enable)
>> struct cpc_register_resource *auto_sel_reg;
>> struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>> struct cppc_pcc_data *pcc_ss_data = NULL;
>> + bool autosel_support_in_ffh_or_sysmem;
>> + bool epp_support_in_ffh_or_sysmem;
>> int ret;
>>
>> if (!cpc_desc) {
>> @@ -1565,6 +1571,11 @@ int cppc_set_epp_perf(int cpu, struct
>> cppc_perf_ctrls *perf_ctrls, bool enable)
>> auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>> epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
>>
>> + epp_support_in_ffh_or_sysmem = CPC_SUPPORTED(epp_set_reg) &&
>> + (CPC_IN_FFH(epp_set_reg) ||
>> CPC_IN_SYSTEM_MEMORY(epp_set_reg));
>> + autosel_support_in_ffh_or_sysmem = CPC_SUPPORTED(auto_sel_reg) &&
>> + (CPC_IN_FFH(auto_sel_reg) ||
>> CPC_IN_SYSTEM_MEMORY(auto_sel_reg));
>> +
>> if (CPC_IN_PCC(epp_set_reg) || CPC_IN_PCC(auto_sel_reg)) {
>> if (pcc_ss_id < 0) {
>> pr_debug("Invalid pcc_ss_id forCPU:%d\n", cpu);
>> @@ -1589,14 +1600,29 @@ int cppc_set_epp_perf(int cpu, struct
>> cppc_perf_ctrls *perf_ctrls, bool enable)
>> /* after writing CPC, transfer the ownership of PCC to
>> platform */
>> ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>> up_write(&pcc_ss_data->pcc_lock);
>> - } else if (osc_cpc_flexible_adr_space_confirmed &&
>> - CPC_SUPPORTED(epp_set_reg) &&
>> CPC_IN_FFH(epp_set_reg)) {
>> - ret = cpc_write(cpu, epp_set_reg,
>> perf_ctrls->energy_perf);
>
> I think this is a bit out of the scope of this patchset, but I'm not
> sure this is necessary
> to check the value of "osc_cpc_flexible_adr_space_confirmed" here.
> Indeed, acpi_cppc_processor_probe() already loops over the CPPC fields
> and should detect when a field is using an address space that is not
> allowed by "osc_cpc_flexible_adr_space_confirmed".
>
> From what I understand:
>
> - osc_cpc_flexible_adr_space_confirmed was introduced to check that CPPC
> registers are in the correct address space
>
> - this broke some amd platforms that didn't configure the _OSC method
> correctly
>
> - 8b356e536e69 ("ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is
> supported") introduced cpc_supported_by_cpu() to bypass the check of
> osc_cpc_flexible_adr_space_confirmed. Indeed, the broken amd platforms
> don't configure the _OSC method, but it is possible to check if there is
> CPPC support by reading an MSR register.
>
> - an amd platform failed to set the EPP register. This seems to be due
> to the EPP register being located in FFH and not in PCC. However the
> handler only supported PCC at that time: 7bc1fcd39901 ("ACPI: CPPC: Add
> AMD pstate energy performance preference cppc control") The bug report
> thread: bugzilla.kernel.org/show_bug.cgi?id=218686
>
> - to allow setting the EPP value when it is located in FFH, the
> following patch was done: aaf21ac93909 ("ACPI: CPPC: Add support for
> setting EPP register in FFH") This patch seems to have added a check
> over the _OSC flexible bit value due to this comment:
> https://bugzilla.kernel.org/show_bug.cgi?id=218686#c83 However the CPPC
> registers are always allowed to be located in the FFH and PCC address
> space. Cf: 0651ab90e4ad ("ACPI: CPPC: Check _OSC for flexible address
> space")
>
> ------------
>
> Just to summarize, I think the check over
> osc_cpc_flexible_adr_space_confirmed could/should be removed. Ideally in
> a separate patch.
>
> If Mario could confirm this is correct this would be nice.
>
Can send a separate patch with this change.
Thank you,
Sumit Gupta
>
>> + } else if (osc_cpc_flexible_adr_space_confirmed) {
>> + if (!epp_support_in_ffh_or_sysmem &&
>> !autosel_support_in_ffh_or_sysmem) {
>> + ret = -EOPNOTSUPP;
>> + } else {
>> + if (autosel_support_in_ffh_or_sysmem) {
>> + ret = cpc_write(cpu, auto_sel_reg,
>> enable);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + if (epp_support_in_ffh_or_sysmem) {
>> + ret = cpc_write(cpu, epp_set_reg,
>> perf_ctrls->energy_perf);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> } else {
>> - ret = -ENOTSUPP;
>> - pr_debug("_CPC in PCC and _CPC in FFH are not
>> supported\n");
>> + ret = -EOPNOTSUPP;
>> }
>>
>> + if (ret == -EOPNOTSUPP)
>> + pr_debug("_CPC in PCC and _CPC in FFH are not
>> supported\n");
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 7190afeead8b..42e37a84cac9 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -119,7 +119,6 @@ struct cppc_perf_caps {
>> u32 lowest_nonlinear_perf;
>> u32 lowest_freq;
>> u32 nominal_freq;
>> - u32 energy_perf;
>> bool auto_sel;
>> };
>>
Powered by blists - more mailing lists