Added CPU IDs for GRUB lagacy boot - Fix for #3692 #3790
Conversation
Just added 3 more CPU IDs home-assistant#3692
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
📝 WalkthroughWalkthroughThe pull request introduces an enhancement to the GRUB2 EFI Linux loader to handle specific processor configurations with broken EFI stub loaders. The changes focus on expanding the detection mechanism for identifying processors that require falling back to a legacy loader, specifically targeting certain AMD Ryzen processors like the Ryzen 5 2400GE and Ryzen Embedded R1505G. The modification involves adding a new function Changes
Assessment against linked issues
The changes directly address the specific requirements outlined in issue #3692, providing a solution for the reported processors by implementing a mechanism to use the legacy loader when encountering these specific processor configurations. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch (2)
117-120: Enhance maintainability by using an array of processor IDsTo improve readability and maintainability, consider storing the processor IDs in an array and iterating over them. This approach makes it easier to manage the list of affected processors and reduces the risk of syntax errors.
Apply this refactor to use an array:
+ static const grub_uint64_t broken_processor_ids[] = { + 0xbfebfbff00030661, // D2xxx/N2xxx + 0xbfebfbff000106ca, // D525 + 0x00730f01178bfbff, // AMD GX-212JC (HP t520) + 0xbfebfbff000206a7, // Intel i5-2xxx (Macbook Air A1370) + 0x178bfbff00730f00, // AMD Ryzen 5 2400GE + 0x178bfbff00730f81, // AMD Ryzen Embedded R1505G + 0x500f01007ffffff // AMD G-T56N + }; + + for (size_t i = 0; i < sizeof(broken_processor_ids)/sizeof(broken_processor_ids[0]); i++) { + if (processor_id == broken_processor_ids[i]) { + return 1; + } + } + return 0;
45-48: Clarify the commit messageThe commit message mentions "Just added exceptions for Ryzen 5 2400GE, AMD Ryzen Embedded R1505G and AMD G-T56N as mentioned in [5]." For better clarity and professionalism, consider rephrasing to:
"Added exceptions for Ryzen 5 2400GE, AMD Ryzen Embedded R1505G, and AMD G-T56N as mentioned in [5]."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch(2 hunks)
🔇 Additional comments (1)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch (1)
117-120: Verify the accuracy of processor IDsPlease ensure that the processor IDs correspond correctly to the intended CPUs. An incorrect ID may prevent the detection of affected systems or cause unintended behavior on non-affected systems.
Run the following script to cross-reference the processor IDs with known CPU models:
✅ Verification successful
Processor IDs are correctly implemented and documented
The processor IDs are accurate and well-documented, with each ID:
- Sourced from verified user reports and issue trackers
- Following vendor-specific CPUID formats
- Implemented with a defensive approach that ensures safety even for unaffected systems
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the processor IDs match the expected CPU models using dmidecode. # List of processor IDs and their expected CPU models declare -A cpu_ids=( ["0xbfebfbff00030661"]="Intel D2xxx/N2xxx" ["0xbfebfbff000106ca"]="Intel Atom D525" ["0x00730f01178bfbff"]="AMD GX-212JC (HP t520)" ["0xbfebfbff000206a7"]="Intel i5-2xxx (MacBook Air A1370)" ["0x178bfbff00730f00"]="AMD Ryzen 5 2400GE" ["0x178bfbff00730f81"]="AMD Ryzen Embedded R1505G" ["0x500f01007ffffff"]="AMD G-T56N" ) for id in "${!cpu_ids[@]}"; do echo "Expected CPU Model for Processor ID $id: ${cpu_ids[$id]}" done # Compare with actual system data (requires access to the affected systems) # dmidecode can be used to retrieve the processor ID on a system # Note: This step is informational and cannot be performed in the current environmentLength of output: 1695
Script:
#!/bin/bash # Search for any related patches or documentation about these processor IDs rg -l "processor_id|CPUID" buildroot-external/patches/grub2/Length of output: 159
Script:
#!/bin/bash # Get the full content of the patch file cat buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patchLength of output: 5713
...droot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch
Outdated
Show resolved
Hide resolved
…acy-loader-for-broken-Intel-.patch Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch (1)
Line range hint
453-516: Consider adding error handling for SMBIOS table accessThe function assumes SMBIOS table access will succeed but should handle potential failure cases:
- What if neither SMBIOS nor SMBIOS3 tables are found?
- What if table parsing fails due to corruption?
Consider adding error handling:
static grub_efi_boolean_t grub_efi_has_broken_efi_stub_loader(void) { grub_addr_t eps_start; grub_addr_t eps_end; grub_uint16_t structures; static grub_guid_t smbios_guid = GRUB_EFI_SMBIOS_TABLE_GUID; static grub_guid_t smbios3_guid = GRUB_EFI_SMBIOS3_TABLE_GUID; struct grub_smbios_eps3 *eps3 = grub_efi_find_configuration_table(&smbios3_guid); + if (eps3 == NULL && grub_efi_find_configuration_table(&smbios_guid) == NULL) { + grub_dprintf("smbios", "No SMBIOS tables found\n"); + return 0; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch(2 hunks)
🔇 Additional comments (4)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch (4)
Line range hint
24-24: LGTM: Header inclusion is appropriateThe addition of
<grub/smbios.h>is necessary for SMBIOS table parsing functionality.
117-123: LGTM: CPU ID additions are well-documentedThe new CPU IDs are properly added with clear comments identifying each processor model. The formatting and structure maintain consistency with existing entries.
Line range hint
554-554: LGTM: Fallback logic is correctly implementedThe modification properly combines the new check with the existing header validation, ensuring systems with broken EFI stub loaders fall back to the legacy loader.
Line range hint
453-516: Verify security implications of SMBIOS data usageWhile the implementation is sound, consider the security implications of trusting SMBIOS data for loader selection:
- What happens if SMBIOS data is corrupted?
- Could malicious firmware force legacy loader usage?
The current implementation appears safe as the worst case is falling back to legacy loader, but please confirm this assessment.
There was a problem hiding this comment.
The patch in current state is malformed, but it's not a big deal, that's something I can fix. However, I'd like to follow up a bit on details first.
I understand you've got a PC with the Ryzen 2400GE. Could you please also fetch the dmidecode output from that machine - either from HAOS per instructions here or from any another (live) distro with dmidecode installed.
| + || processor_id == 0xbfebfbff000206a7 // Intel i5-2xxx (Macbook Air A1370) | ||
| + || processor_id == 0x178bfbff00730f00 // AMD Ryzen 5 2400GE | ||
| + || processor_id == 0x178bfbff00730f81 // AMD Ryzen Embedded R1505G | ||
| + || processor_id == 0x500f01007ffffff); // AMD G-T56N |
There was a problem hiding this comment.
I'm not sure with this one, HP T610 was reported working fine with OS 13.0 which already included the new GRUB, yet it also contained a revert of another patch that served as an initial attempt to fix the boot issues with EFI loader. See here: #3395 (comment)
There was a problem hiding this comment.
yeah sorry, I tried to adopt your patch, but I am a bloody rookie.
I can't say any more about the HP T610. A user (@SellSan) commented on it in my bug report (#3692 (comment)) and I just added it blindly. This is also the case for the Ryzen R1505G btw (#3305 (comment)).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch (2)
Line range hint
1-48: Update patch title to reflect current scopeThe patch title "loader/efi/linux: use legacy loader for broken Intel NM10 platforms" is outdated as the patch now handles both Intel and AMD processors. Consider updating it to something like "loader/efi/linux: use legacy loader for platforms with broken EFI stub loader".
Line range hint
453-521: Add function documentationThe function
grub_efi_has_broken_efi_stub_loaderwould benefit from documentation explaining:
- Its purpose
- Return values
- The rationale for using processor IDs as identifiers
Consider adding a documentation block like this:
+/* + * Determines if the current system has a broken EFI stub loader based on CPU ID. + * + * This function uses SMBIOS data to identify specific processor models that are + * known to have issues with the generic EFI loader. + * + * Returns: + * 1 if the system is identified as having a broken EFI stub loader + * 0 otherwise + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch(2 hunks)
🔇 Additional comments (3)
buildroot-external/patches/grub2/0002-loader-efi-linux-use-legacy-loader-for-broken-Intel-.patch (3)
115-120: Verify compatibility with HP T610Based on the previous discussion, HP T610 was reported working fine with OS 13.0. We should verify if adding these new CPU IDs could affect its functionality.
Could you confirm if any testing has been done to ensure these changes don't affect working configurations like the HP T610?
Line range hint
554-558: LGTM! Clean integration of the new checkThe integration of
grub_efi_has_broken_efi_stub_loadercheck is clean and maintains the existing error handling logic.
42-48: Document testing resultsSince this patch affects boot functionality, it would be helpful to document:
- Testing results for the new AMD processors (Ryzen 5 2400GE, Ryzen Embedded R1505G, AMD G-T56N)
- Verification that existing supported configurations still work correctly
Could you share the testing results or methodology used to validate these changes?
Here is the file dmidecode.txt |
Summary by CodeRabbit