Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Deepin Kernel SIG] [Intel] intel: backport GNR and SRF intel_idle for 6.6 #538

Merged

Conversation

Avenger-285714
Copy link
Collaborator

Backport Intel_idle driver for GNR and SRF and important fix for 6.6

there are 3 patches in this PR.

Test:
idle status is listed as expected

Link: https://gitee.com/OpenCloudOS/OpenCloudOS-Kernel/pulls/256

commit 92813fd upstream.

Add Sierra Forest SoC C-states, which are C1, C1E, C6S, and C6SP.

Sierra Forest SoC is built with modules, each module includes 4 cores
(Crestmont microarchitecture). There is one L2 cache per module, shared
between the 4 cores.

There is no core C6 state, but there is C6S state, which has module scope:
when all 4 cores request C6S, the entire module (4 cores + L2 cache)
enters the low power state.

C6SP state has package scope - when all modules in the package enter C6S,
the package enters the power state mode.

Intel-SIG: commit 92813fd intel_idle: add Sierra Forest SoC support.
Backport Intel_idle Sierra Forest SoC support.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[ Yingbao Jia: amend commit log ]
Signed-off-by: Yingbao Jia <yingbao.jia@intel.com>
commit 370406bf5738dade8ac95a2ee95c29299d4ac902 upstream.

Add Granite Rapids Xeon C-states, which are C1, C1E, C6, and C6P.

Comparing to previous Xeon Generations (e.g., Emerald Rapids), C6
requests end up only in core C6 state, and no package C-state promotion
takes place even if all cores in the package are in core C6.

C6P requests also end up in core C6, but if all cores have requested
C6P, the SoC will enter the package C6 state.

Intel-SIG: commit 370406bf5738 intel_idle: add Granite Rapids Xeon support.
Backport intel_idle GNR support

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Link: https://patch.msgid.link/20240806160310.3719205-1-artem.bityutskiy@linux.intel.com
[ rjw: Changelog edits ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[ Yingbao Jia: amend commit log ]
Signed-off-by: Yingbao Jia <yingbao.jia@intel.com>
commit 4c411cca33cf1c21946b710b2eb59aca9f646703 upstream.

Background
~~~~~~~~~~

The driver uses 'use_acpi = true' in C-state custom table for all Xeon
platforms. The meaning of this flag is as follows.

 1. If a C-state from the custom table is defined in ACPI _CST (matched
    by the mwait hint), then enable this C-state.

 2. Otherwise, disable this C-state, unless the C-sate definition in the
    custom table has the 'CPUIDLE_FLAG_ALWAYS_ENABLE' flag set, in which
    case enabled it.

The goal is to honor BIOS C6 settings - If BIOS disables C6, disable it
by default in the OS too (but it can be enabled via sysfs).

This works well on Xeons that expose only one flavor of C6. This are all
Xeons except for the newest Granite Rapids (GNR) and Sierra Forest (SRF).

The problem
~~~~~~~~~~~

GNR and SRF have 2 flavors of C6: C6/C6P on GNR, C6S/C6SP on SRF. The
the "P" flavor allows for the package C6, while the "non-P" flavor
allows only for core/module C6.

As far as this patch is concerned, both GNR and SRF platforms are
handled the same way. Therefore, further discussion is focused on GNR,
but it applies to SRF as well.

On Intel Xeon platforms, BIOS exposes only 2 ACPI C-states: C1 and C2.
Well, depending on BIOS settings, C2 may be named as C3. But there still
will be only 2 states - C1 and C3. But this is a non-essential detail,
so further discussion is focused on the ACPI C1 and C2 case.

On pre-GNR/SRF Xeon platforms, ACPI C1 is mapped to C1 or C1E, and ACPI
C2 is mapped to C6. The 'use_acpi' flag works just fine:

 * If ACPI C2 enabled, enable C6.
 * Otherwise, disable C6.

However, on GNR there are 2 flavors of C6, so BIOS maps ACPI C2 to
either C6 or C6P, depending on the user settings. As a result, due to
the 'use_acpi' flag, 'intel_idle' disables least one of the C6 flavors.

BIOS                   | OS                         | Verdict
----------------------------------------------------|---------
ACPI C2 disabled       | C6 disabled, C6P disabled  | OK
ACPI C2 mapped to C6   | C6 enabled,  C6P disabled  | Not OK
ACPI C2 mapped to C6P  | C6 disabled, C6P enabled   | Not OK

The goal of 'use_acpi' is to honor BIOS ACPI C2 disabled case, which
works fine. But if ACPI C2 is enabled, the goal is to enable all flavors
of C6, not just one of the flavors. This was overlooked when enabling
GNR/SRF platforms.

In other words, before GNR/SRF, the ACPI C2 status was binary - enabled
or disabled. But it is not binary on GNR/SRF, however the goal is to
continue treat it as binary.

The fix
~~~~~~~

Notice, that current algorithm matches ACPI and custom table C-states
by the mwait hint. However, mwait hint consists of the 'state' and
'sub-state' parts, and all C6 flavors have the same state value of 0x20,
but different sub-state values.

Introduce new C-state table flag - CPUIDLE_FLAG_PARTIAL_HINT_MATCH and
add it to both C6 flavors of the GNR/SRF platforms.

When matching ACPI _CST and custom table C-states, match only the start
part if the C-state has CPUIDLE_FLAG_PARTIAL_HINT_MATCH, other wise
match both state and sub-state parts (as before).

With this fix, GNR C-states enabled/disabled status looks like this.

BIOS                   | OS
----------------------------------------------------
ACPI C2 disabled       | C6 disabled, C6P disabled
ACPI C2 mapped to C6   | C6 enabled, C6P enabled
ACPI C2 mapped to C6P  | C6 enabled, C6P enabled

Possible alternative
~~~~~~~~~~~~~~~~~~~~

The alternative would be to remove 'use_acpi' flag for GNR and SRF.
This would be a simpler solution, but it would violate the principle of
least surprise - users of Xeon platforms are used to the fact that
intel_idle honors C6 enabled/disabled flag. It is more consistent user
experience if GNR/SRF continue doing so.

How tested
~~~~~~~~~~

Tested on GNR and SRF platform with all the 3 BIOS configurations: ACPI
C2 disabled, mapped to C6/C6S, mapped to C6P/C6SP.

Tested on Ice lake Xeon and Sapphire Rapids Xeon platforms with ACPI C2
enabled and disabled, just to verify that the patch does not break older
Xeons.

Intel-SIG: Intel-SIG: commit 4c411cca33cf intel_idle: fix ACPI _CST matching for newer Xeon platforms.
Backport intel_idle GNR and SRF support and fix

Fixes: 92813fd ("intel_idle: add Sierra Forest SoC support")
Fixes: 370406bf5738 ("intel_idle: add Granite Rapids Xeon support")
Cc: 6.8+ <stable@vger.kernel.org> # 6.8+
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Link: https://patch.msgid.link/20240913165143.4140073-1-dedekind1@gmail.com
[ rjw: Changelog edits ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
[ Yingbao Jia: amend commit log ]
Signed-off-by: Yingbao Jia <yingbao.jia@intel.com>
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 代码重复gnr_cstatessrf_cstates数组中的状态定义存在重复代码,可以考虑提取公共部分到一个函数中,减少代码冗余。

  2. 宏定义使用MWAIT2flg宏的使用在gnr_cstatessrf_cstates数组中重复,建议将其提取为常量或宏定义,以减少代码重复。

  3. 逻辑判断:在intel_idle_off_by_default函数中,flags & CPUIDLE_FLAG_PARTIAL_HINT_MATCH的逻辑判断应该放在if语句的起始位置,以提高代码的可读性。

  4. 注释清晰度intel_idle_off_by_default函数的注释应该更详细地解释flags参数的使用,以及CPUIDLE_FLAG_PARTIAL_HINT_MATCH标志的具体作用。

  5. 性能考虑:在intel_idle_off_by_default函数中,对acpi_state_table.states数组的遍历可能会影响性能,如果acpi_state_table.states数组很大,可以考虑使用更高效的数据结构(如哈希表)来存储和查找状态。

  6. 代码风格intel_idle_off_by_default函数中的变量命名应该遵循一致的命名规范,例如mwait_hint应该改为mwaitHint以符合常见的命名习惯。

  7. 错误处理:在intel_idle_off_by_default函数中,如果acpi_state_table.states数组为空,应该添加相应的错误处理逻辑,以避免潜在的空指针访问。

  8. 代码一致性intel_idle_off_by_default函数在CONFIG_ACPI_PROCESSOR_CSTATE配置下被禁用,建议在函数定义中添加相应的注释,说明为什么在这个配置下禁用该函数。

  9. 代码维护:如果gnr_cstatessrf_cstates数组中的状态定义在未来需要更新,建议将这些状态定义提取到一个单独的文件中,以方便维护和更新。

  10. 代码安全性:没有明显的安全漏洞,但是建议在处理外部输入(如mwait_hint)时,添加输入验证,以防止潜在的注入攻击。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from avenger-285714. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Avenger-285714
Copy link
Collaborator Author

测试通过

@Avenger-285714 Avenger-285714 merged commit f74b2c5 into deepin-community:linux-6.6.y Dec 27, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants