[Deepin-Kernel-SIG] [linux 6.18-y] [Fromlist] [AMD] x86/CPU/AMD: Print AGESA string from DMI additional information entry#1621
Conversation
Type 40 entries (Additional Information) are summarized in section 7.41 as part of the SMBIOS specification. Generally, these entries aren't interesting to save. However on some AMD Zen systems, the AGESA version is stored here. This is useful to save to the kernel message logs for debugging. It can be used to cross-reference issues. Implement an iterator for the Additional Information entries. Use this to find and print the AGESA string. Do so in AMD code, since the use case is AMD-specific. [ bp: Match only "AGESA". ] Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> Co-developed-by: "Mario Limonciello (AMD)" <superm1@kernel.org> Signed-off-by: "Mario Limonciello (AMD)" <superm1@kernel.org> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Reviewed-by: Jean Delvare <jdelvare@suse.de> Link: https://patch.msgid.link/20260307141024.819807-6-superm1@kernel.org [WangYuli: Fix conflicts] Link: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/platform&id=bc91133e260c8113c1119073c03b93c12aa41738 Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
Reviewer's GuideAdds support for scanning SMBIOS Type 40 (Additional Information) DMI entries to extract and log the AMD AGESA version string, by defining the Additional Information structures, exposing dmi_string_nosave for reuse, and registering a late init callback in the AMD x86 CPU code to walk and parse these entries. Sequence diagram for late init AGESA logging from DMI Additional InformationsequenceDiagram
actor KernelInit
participant AMD_CPU_Module as amd_cpu_module
participant DMI_Core as dmi_core
participant DMI_Scan as dmi_scan_module
participant KernelLog as pr_info
KernelInit->>AMD_CPU_Module: late_initcall(print_dmi_agesa)
AMD_CPU_Module->>DMI_Core: dmi_walk(dmi_scan_additional, NULL)
loop For each DMI record
DMI_Core->>AMD_CPU_Module: dmi_scan_additional(dmi_header*, NULL)
AMD_CPU_Module->>AMD_CPU_Module: validate type 40 and counts
alt Entry is valid
loop Iterate Type40 entries
AMD_CPU_Module->>DMI_Scan: dmi_string_nosave(&info->header, entry->str_num)
DMI_Scan-->>AMD_CPU_Module: const char* string_ptr
alt string_ptr starts with AGESA
AMD_CPU_Module->>KernelLog: pr_info("AGESA: %s", string_ptr)
AMD_CPU_Module-->>AMD_CPU_Module: break loop
else Non AGESA entry
AMD_CPU_Module->>AMD_CPU_Module: advance to next entry
end
end
else Non Type40 or invalid
AMD_CPU_Module->>AMD_CPU_Module: return
end
end
Class diagram for new DMI Additional Information structs and functionsclassDiagram
class dmi_header {
+u8 type
+u8 length
+u16 handle
}
class dmi_a_info_entry {
+u8 length
+u16 handle
+u8 offset
+u8 str_num
+u8 value[]
}
class dmi_a_info {
+dmi_header header
+u8 count
}
class dmi_core_api {
+const char* dmi_string_nosave(const dmi_header* dm, u8 s)
}
class amd_cpu_module {
+void dmi_scan_additional(const dmi_header* d, void* p)
+int print_dmi_agesa()
}
dmi_a_info --> dmi_header : embeds
amd_cpu_module --> dmi_a_info : parses
amd_cpu_module --> dmi_a_info_entry : iterates
amd_cpu_module --> dmi_core_api : calls dmi_string_nosave
dmi_core_api <.. amd_cpu_module : symbol exported
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
dmi_scan_additional(), you can preserve const-correctness by treating the callback argument asconst struct dmi_a_info *(e.g. viacontainer_of(d, struct dmi_a_info, header)) instead of casting awayconstfromstruct dmi_header *. - The explicit
IS_ENABLED(CONFIG_DMI)check insidedmi_scan_additional()is redundant given thatprint_dmi_agesa()already relies ondmi_walk(), which itself is a no-op whenCONFIG_DMIis disabled; you can simplify the code by removing this check.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `dmi_scan_additional()`, you can preserve const-correctness by treating the callback argument as `const struct dmi_a_info *` (e.g. via `container_of(d, struct dmi_a_info, header)`) instead of casting away `const` from `struct dmi_header *`.
- The explicit `IS_ENABLED(CONFIG_DMI)` check inside `dmi_scan_additional()` is redundant given that `print_dmi_agesa()` already relies on `dmi_walk()`, which itself is a no-op when `CONFIG_DMI` is disabled; you can simplify the code by removing this check.
## Individual Comments
### Comment 1
<location path="arch/x86/kernel/cpu/amd.c" line_range="1399-1400" />
<code_context>
+
+static void __init dmi_scan_additional(const struct dmi_header *d, void *p)
+{
+ struct dmi_a_info *info = (struct dmi_a_info *)d;
+ void *next, *end;
+
+ if (!IS_ENABLED(CONFIG_DMI))
</code_context>
<issue_to_address>
**suggestion:** Preserve const-correctness and use byte pointers instead of void* for offset arithmetic.
The cast from `const struct dmi_header *d` to `struct dmi_a_info *` drops `const`. Declaring `info` (and `entry`) as `const` would reflect that these structures are not modified. Also, prefer `const u8 *next, *end;` over `void *` to avoid non-standard `void *` arithmetic and clarify the size/offset calculations.
Suggested implementation:
```c
static void __init dmi_scan_additional(const struct dmi_header *d, void *p)
{
const struct dmi_a_info *info = (const struct dmi_a_info *)d;
const u8 *next, *end;
```
```c
next = (const u8 *)(info + 1);
end = (const u8 *)info + info->header.length;
```
If this function later declares and uses an `entry` pointer derived from `next` (e.g. `struct dmi_a_entry *entry = next;`), it should also be made `const`:
`const struct dmi_a_entry *entry = (const struct dmi_a_entry *)next;`
Any subsequent pointer arithmetic should continue to use `const u8 *` for offsets and casts back to the appropriate `const` struct types for interpretation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| struct dmi_a_info *info = (struct dmi_a_info *)d; | ||
| void *next, *end; |
There was a problem hiding this comment.
suggestion: Preserve const-correctness and use byte pointers instead of void* for offset arithmetic.
The cast from const struct dmi_header *d to struct dmi_a_info * drops const. Declaring info (and entry) as const would reflect that these structures are not modified. Also, prefer const u8 *next, *end; over void * to avoid non-standard void * arithmetic and clarify the size/offset calculations.
Suggested implementation:
static void __init dmi_scan_additional(const struct dmi_header *d, void *p)
{
const struct dmi_a_info *info = (const struct dmi_a_info *)d;
const u8 *next, *end; next = (const u8 *)(info + 1);
end = (const u8 *)info + info->header.length;If this function later declares and uses an entry pointer derived from next (e.g. struct dmi_a_entry *entry = next;), it should also be made const:
const struct dmi_a_entry *entry = (const struct dmi_a_entry *)next;
Any subsequent pointer arithmetic should continue to use const u8 * for offsets and casts back to the appropriate const struct types for interpretation.
There was a problem hiding this comment.
Pull request overview
Adds support for extracting and logging the AMD AGESA version string from SMBIOS Type 40 (“Additional Information”) records during boot, to aid debugging on affected Zen systems.
Changes:
- Introduces SMBIOS Type 40 (Additional Information) structure definitions in
include/linux/dmi.h. - Exposes
dmi_string_nosave()for reuse outsidedmi_scan.c. - Adds an x86/AMD late-init DMI walk that scans Type 40 entries and prints the first “AGESA…” string found.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| include/linux/dmi.h | Adds Type 40 structures/macros and a dmi_string_nosave() declaration/stub. |
| drivers/firmware/dmi_scan.c | Makes dmi_string_nosave() globally visible and exports it. |
| arch/x86/kernel/cpu/amd.c | Adds a DMI walker callback to locate and log the AGESA string from Type 40. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct dmi_a_info *info = (struct dmi_a_info *)d; | ||
| void *next, *end; | ||
|
|
||
| if (!IS_ENABLED(CONFIG_DMI)) | ||
| return; | ||
|
|
There was a problem hiding this comment.
next/end are void * but the code does arithmetic on them (next += ..., end - next). This relies on GCC void-pointer arithmetic and can trigger -Wpointer-arith warnings. Use u8 */const u8 * for these pointers (and consider keeping info/entry const) so the pointer math is well-defined and warning-free.
| if (!entry->length) | ||
| return; | ||
|
|
||
| string_ptr = dmi_string_nosave(&info->header, entry->str_num); | ||
|
|
||
| /* Sample string: AGESA!V9 StrixKrackanPI-FP8 1.1.0.0c */ | ||
| if (!strncmp(string_ptr, "AGESA", 5)) { | ||
| pr_info("AGESA: %s\n", string_ptr); | ||
| break; | ||
| } | ||
|
|
||
| next += entry->length; | ||
| } while (end - next >= DMI_A_INFO_ENT_MIN_SIZE); |
There was a problem hiding this comment.
The entry parsing loop should validate entry->length against both the minimum entry size and the remaining bytes in the structure before advancing next. As written, a bogus/firmware-corrupted entry->length (e.g. < min size or > remaining) can cause misparsing or pointer overflow. Add checks like entry->length >= DMI_A_INFO_ENT_MIN_SIZE and entry->length <= (end - next) before using it.
| return dmi_empty_string; | ||
| } | ||
| EXPORT_SYMBOL_GPL(dmi_string_nosave); |
There was a problem hiding this comment.
dmi_string_nosave() is now exported to modules via EXPORT_SYMBOL_GPL(), but this PR only needs it to be non-static for built-in callers (arch/x86). Exporting unnecessarily expands the module ABI/API surface. Consider dropping the export (and then the function can remain __init to avoid permanent text size growth).
Avenger-285714
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: opsiff 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 |
Type 40 entries (Additional Information) are summarized in section 7.41 as part of the SMBIOS specification. Generally, these entries aren't interesting to save.
However on some AMD Zen systems, the AGESA version is stored here. This is useful to save to the kernel message logs for debugging. It can be used to cross-reference issues.
Implement an iterator for the Additional Information entries. Use this to find and print the AGESA string. Do so in AMD code, since the use case is AMD-specific.
[ bp: Match only "AGESA". ]
Co-developed-by: "Mario Limonciello (AMD)" superm1@kernel.org
Reviewed-by: Jean Delvare jdelvare@suse.de
Link: https://patch.msgid.link/20260307141024.819807-6-superm1@kernel.org
[WangYuli: Fix conflicts]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/platform&id=bc91133e260c8113c1119073c03b93c12aa41738
Summary by Sourcery
Log the AGESA firmware version from SMBIOS DMI additional information entries on AMD x86 systems.
New Features:
Enhancements: