[Deepin-Kernel-SIG] [linux 6.18.y] [Deepin] [AOSC] arm64: disable mpam in tsv110#1521
[Deepin-Kernel-SIG] [linux 6.18.y] [Deepin] [AOSC] arm64: disable mpam in tsv110#1521opsiff wants to merge 2 commits intodeepin-community:linux-6.18.yfrom
Conversation
We have to work OotB on W510, and ARM64 does not support extending the built-in command line with the one from the bootloader. So only try MPAM if the user explicitly gives "aosc.try_mpam=1" via the cmdline. Signed-off-by: Xi Ruoyao <xry111@xry111.site> Signed-off-by: Kexy Biscuit <kexybiscuit@aosc.io> (cherry picked from commit 8fc0099ea8f6edfb929c09bbcd38e57485a2ddf6) Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Reviewer's GuideIntroduces an AOSC-specific feature override mechanism to selectively disable MPAM by default on HiSilicon TSV110-based arm64 systems with broken firmware, unless explicitly re-enabled via a new Sequence diagram for AOSC MPAM override handling on TSV110 at ARM64 bootsequenceDiagram
actor Admin
participant Kernel
participant parse_cmdline
participant AOSCFeatureOverride
participant CPUFeatures
Admin->>Kernel: Boot system with kernel cmdline
Kernel->>parse_cmdline: parse_cmdline(fdt, chosen)
parse_cmdline->>parse_cmdline: __parse_cmdline(normal_cmdline, true)
loop For each entry in mpam_disable_list
parse_cmdline->>CPUFeatures: read_cpuid_id() and match MIDR_HISI_TSV110
alt CPU model matches TSV110
parse_cmdline->>AOSCFeatureOverride: arm64_apply_feature_override(0, 0, 4, aosc_feature_override)
alt aosc.try_mpam overridden by user (return nonzero)
AOSCFeatureOverride-->>parse_cmdline: MPAM allowed, no override
parse_cmdline-->>Kernel: continue boot without arm64.nompam
else aosc.try_mpam not set (return 0)
AOSCFeatureOverride-->>parse_cmdline: no user override
parse_cmdline->>parse_cmdline: __parse_cmdline(arm64.nompam, true)
parse_cmdline->>CPUFeatures: Disable MPAM via arm64.nompam
parse_cmdline-->>Kernel: continue boot with MPAM disabled
end
else CPU model does not match TSV110
parse_cmdline-->>Kernel: no MPAM override applied
end
end
Kernel-->>Admin: System booted with MPAM enabled or disabled per override
Class diagram for AOSC feature override integration for MPAMclassDiagram
class arm64_ftr_override {
+u64 val
+u64 mask
+bool overridden
}
class ftr_field_desc {
+string name
+int shift
+void* constraint
}
class ftr_set_desc {
+string name
+arm64_ftr_override* override
+ftr_field_desc fields[]
}
class aosc_feature_override {
<<struct instance>>
+u64 val
+u64 mask
+bool overridden
}
class aosc_features {
<<const struct instance>>
+name = aosc
+fields: try_mpam
}
class sw_features {
<<const struct instance>>
+name = sw
+fields: existing_software_features
}
class regs_array {
<<array of ftr_set_desc*>>
+mmfr0
+mmfr1
+isar0_to_2
+smfr0
+sw_features
+aosc_features
}
ftr_set_desc --> arm64_ftr_override : override
ftr_set_desc --> ftr_field_desc : fields
aosc_feature_override --> arm64_ftr_override : embeds
aosc_features --> ftr_set_desc : conforms_to
sw_features --> ftr_set_desc : conforms_to
regs_array --> ftr_set_desc : contains_pointers
regs_array --> aosc_features : includes
regs_array --> sw_features : includes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
mpam_disable_listarray is declared as a localstatic constwith__initconstinsideparse_cmdline; consider moving it to file scope (like the othermidr_rangetables) to avoid unusual attribute use on a local variable and to match existing kernel style. - The call to
arm64_apply_feature_override(0, 0, 4, &aosc_feature_override)uses magic numbers for the register and field; replacing these with named constants or at least a brief comment indicating which ID register/field is being overridden would make the intent clearer and reduce the chance of misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `mpam_disable_list` array is declared as a local `static const` with `__initconst` inside `parse_cmdline`; consider moving it to file scope (like the other `midr_range` tables) to avoid unusual attribute use on a local variable and to match existing kernel style.
- The call to `arm64_apply_feature_override(0, 0, 4, &aosc_feature_override)` uses magic numbers for the register and field; replacing these with named constants or at least a brief comment indicating which ID register/field is being overridden would make the intent clearer and reduce the chance of misuse.
## Individual Comments
### Comment 1
<location path="arch/arm64/kernel/pi/idreg-override.c" line_range="416-423" />
<code_context>
+ };
+
+ for (const struct midr_range *r = mpam_disable_list; r->model; r++) {
+ if (midr_is_cpu_model_range(read_cpuid_id(), r->model,
+ r->rv_min, r->rv_max)) {
+ if (!arm64_apply_feature_override(0, 0, 4, &aosc_feature_override)) {
+ __parse_cmdline("arm64.nompam", true);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion:** Clarify control flow when a CPU model matches to avoid repeated override attempts.
Given that the feature override (or fallback to `arm64.nompam`) is a one-time decision per CPU, consider breaking after the first match. This prevents any chance of reapplying the override if ranges ever overlap or the helper behavior changes, and makes the single-application intent clearer:
```c
if (midr_is_cpu_model_range(...)) {
if (!arm64_apply_feature_override(...))
__parse_cmdline("arm64.nompam", true);
break;
}
```
```suggestion
for (const struct midr_range *r = mpam_disable_list; r->model; r++) {
if (midr_is_cpu_model_range(read_cpuid_id(), r->model,
r->rv_min, r->rv_max)) {
if (!arm64_apply_feature_override(0, 0, 4, &aosc_feature_override))
__parse_cmdline("arm64.nompam", true);
break;
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
deepin inclusion category: feature Only disable mpam for mpam_disable_list, such as TSV110. Tested in DMI: HUAWEI HUAWEIPGU-WBY0/HUAWEIPGU-WBY0-PCB, BIOS 1.08 12/23/2019. Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
081f65a to
6bc74a2
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Avenger-285714 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds an early-boot quirk for HiSilicon TSV110 (e.g., W510) to disable MPAM by default unless explicitly opted-in via a new aosc.try_mpam=1 boot parameter, implemented in the arm64 early feature override framework.
Changes:
- Introduces a new pseudo feature set (
aosc) to parseaosc.try_mpamfrom the kernel command line. - Detects TSV110 by MIDR during early boot and auto-applies
arm64.nompamunlessaosc.try_mpamis set. - Registers the new feature set in the early override registry list.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static struct arm64_ftr_override __read_mostly aosc_feature_override; | ||
|
|
||
| static const struct ftr_set_desc aosc_features __prel64_initconst = { | ||
| .name = "aosc", | ||
| .override = &aosc_feature_override, | ||
| .fields = { |
There was a problem hiding this comment.
aosc_feature_override appears to be used only during early command-line parsing (to decide whether to auto-apply arm64.nompam on TSV110). Since it is not referenced after init_feature_override() completes, consider marking it __initdata (and dropping __read_mostly) so it can be freed with other init-only data instead of permanently occupying .data..read_mostly.
| * Sorry but we have to work OotB on some platforms with broken | ||
| * firmware, notably W510. Use "aosc.try_mpam=1" if you really need | ||
| * MPAM on AOSC. |
There was a problem hiding this comment.
This change introduces a new boot parameter (aosc.try_mpam=1) that affects CPU feature enablement on TSV110, but it isn't documented in Documentation/admin-guide/kernel-parameters.txt (there are entries for arm64.nompam, etc.). Please add documentation for aosc.try_mpam so users can discover and understand the override and its default behavior.
| * Sorry but we have to work OotB on some platforms with broken | |
| * firmware, notably W510. Use "aosc.try_mpam=1" if you really need | |
| * MPAM on AOSC. | |
| * Some platforms (notably TSV110 / W510) ship with firmware that | |
| * advertises MPAM but is not reliable enough for use out of the box. | |
| * | |
| * By default, for all CPUs in mpam_disable_list we forcibly disable | |
| * MPAM early by injecting "arm64.nompam" into the effective | |
| * command line (see the loop below). | |
| * | |
| * The "aosc.try_mpam=1" boot parameter provides an opt-in override | |
| * for systems where the firmware is known to be good: when this | |
| * parameter is present, arm64_apply_feature_override() will succeed | |
| * and we will NOT add "arm64.nompam", so MPAM remains enabled. |
| * Sorry but we have to work OotB on some platforms with broken | ||
| * firmware, notably W510. Use "aosc.try_mpam=1" if you really need | ||
| * MPAM on AOSC. |
There was a problem hiding this comment.
The comment uses the abbreviation "OotB", which may be unclear to readers outside this context. Consider spelling this out (e.g., "out of the box") and tightening the wording to be more descriptive of the technical reason (firmware reports MPAM incorrectly on TSV110/W510) rather than apologetic.
| * Sorry but we have to work OotB on some platforms with broken | |
| * firmware, notably W510. Use "aosc.try_mpam=1" if you really need | |
| * MPAM on AOSC. | |
| * Disable MPAM by default on platforms with firmware that reports | |
| * MPAM incorrectly, notably TSV110/W510. Use "aosc.try_mpam=1" if | |
| * you really need MPAM on AOSC. |
If you really need it , use aosc.try_mpam=1.
If you really noneed it, use arm64.nompam=1.
Log:
[ 0.000000] CPU features: SYS_ID_AA64PFR0_EL1[43:40]: forced to 0
[ 0.000000] CPU features: SYS_ID_AA64PFR1_EL1[19:16]: already set to 0
[ 0.000000] CPU features: detected: GICv3 CPU interface
[ 0.000000] CPU features: detected: Virtualization Host Extensions
[ 0.000000] CPU features: detected: Spectre-v4
[ 0.000000] CPU features: detected: Spectre-BHB
[ 0.000000] alternatives: applying boot alternatives
[ 0.000000] Kernel command line: BOOT_IMAGE=/vmlinuz-6.18.13-arm64-desktop-rolling root=UUID=c2f3cdb4-f919-43ba-bd32-172c2744dfb2 ro splash quiet console=tty plymouth.ignore-serial-consoles DEEPIN_GFXMODE= luks.crypttab=n
[ 0.000000] Unknown kernel command line parameters "splash DEEPIN_GFXMODE=", will be passed to user space.
[ 0.000000] printk: log_buf_len individual max cpu contribution: 32768 bytes
[ 0.000000] printk: log_buf_len total cpu_extra contributions: 753664 bytes
[ 0.000000] printk: log_buf_len min size: 131072 bytes
[ 0.000000] printk: log buffer data + meta data: 1048576 + 3670016 = 4718592 bytes
[ 0.000000] printk: early log buf free: 125224(95%)
[ 0.000000] Dentry cache hash table entries: 4194304 (order: 13, 33554432 bytes, linear)
[ 0.000000] Inode-cache hash table entries: 2097152 (order: 12, 16777216 bytes, linear)
[ 0.000000] software IO TLB: area num 32.
[ 0.000000] software IO TLB: mapped [mem 0x000000007b000000-0x000000007f000000] (64MB)
[ 0.000000] Fallback order for Node 0: 0
[ 0.000000] Built 1 zonelists, mobility grouping on. Total pages: 8387568
[ 0.000000] Policy zone: Normal
[ 0.000000] mem auto-init: stack:all(zero), heap alloc:off, heap free:off