Move CpuDxe from ArmPkg to UefiCpuPkg #10836

AjanZhong · 2025-03-10T12:28:38Z

Description

As continues work to this discussion thread:
https://edk2.groups.io/g/devel/topic/patch_v5_2_6/102725178

This PR moves CpuDxe from ArmPkg/Drivers/CpuDxe to UefiCpuPkg/CpuDxeArm.

UefiCpuPkg contains CpuDxe for I386/X64/LoongArch64/RISCV64 architectures, move CpuDxe for Arm/AArch64 architectures to UefiCpuPkg making CpuDxe for all architectures which supported by EDK2 are easy to maintain.

Also, libraries, Guids, PCDs which are required by CpuDxe are all moved out of ArmPkg to MdePkg or UefiCpuPkg.

Breaking change?
- Breaking change - Does this PR cause a break in build or boot behavior?
- N/A
Impacts security?
- Security - Does this PR have a direct security impact?
- N/A
Includes tests?
- Tests - Does this PR include any explicit test code?
- N/A

How This Was Tested

Packages depends on ArmPkg/Drivers/CpuDxe.inf should pass.
AArch64 QEMU environment.

Integration Instructions

Need to create a new PR in edk2-platform for this PR.

os-d · 2025-03-10T17:27:37Z

ArmPkg/ArmPkg.dsc

@@ -119,7 +119,7 @@
  ArmPkg/Library/SemihostLib/SemihostLib.inf
  ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf

-  ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+  UefiCpuPkg/CpuDxeArm/CpuDxe.inf


This is odd to have ArmPkg build a component that lives in UefiCpuPkg. I would expect UefiCpuPkg.dsc to build all the components that live within its package. I understand that the library dependencies are still within ArmPkg, but it doesn't seem to me that there is much value in moving the driver if the libraries aren't correctly dealt with. Untangling the ArmPkg libraries is a monumental task, but it is also the right direction to go

Decoupled UefiCpuPkg/CpuDxeArm/CpuDxe.inf with ArmPkg/ArmPkg.dsc in latest commit.

os-d · 2025-03-10T17:42:37Z

MdePkg/MdePkg.dec

@@ -362,6 +362,10 @@
  #
  ArmMmuLib|Include/Library/ArmMmuLib.h

+  ##  @libraryclass  Provides a default exception handler.
+  #
+  DefaultExceptionHandlerLib|Include/Library/DefaultExceptionHandlerLib.h


This is gated under [LibraryClasses.ARM, LibraryClasses.AARCH64], but still being in MdePkg.dec I would consider renaming this to be more specific or merge it with other existing libraries classes, if applicable

3 commits has been created to address this commit:

[CpuExceptionHandlerLib: Move DumpImageAndCpuContent to library header file]

[ArmPkg: Merge DefaultExceptionHandlerLib with ArmExceptionLib]

[ArmPkg,UefiCpuPkg: Move ArmExceptionLib from ArmPkg to UefiCpuPkg]

os-d · 2025-03-10T17:43:31Z

MdePkg/MdePkg.dec

@@ -935,6 +935,9 @@
  ## Include/Guid/Cper.h
  gEfiIa32X64ErrorTypeMsCheckGuid    = { 0x48AB7F57, 0xDC34, 0x4f6c, { 0xA7, 0xD3, 0xB0, 0xB5, 0xB0, 0xA7, 0x43, 0x14 }}

+[Guids.ARM, Guids.AARCH64]
+  gArmTokenSpaceGuid       = { 0xBB11ECFE, 0x820F, 0x4968, { 0xBB, 0xA6, 0xF7, 0x6A, 0xFE, 0x30, 0x25, 0x96 } }


This might be okay as an interim step, but I think only if we have a plan for removing gArmTokenSpaceGuid in the future

No, this is not okay even as an interim step.

Any suggestion for Guids belongs to ArmPkg? There are plenty of Guids need to be moved or removed, for instance, ArmMmuLib depends on gArmMmuRelaceLiveTransaltaionEntryFuncGuid.

A token space Guid defines a token space. The token space in question is ArmPkg's token space. It makes absolutely no sense to duplicate that token space definition in another package. If we're very lucky, we don't end up with conflicting Pcd definitions in that token space. But that's only luck.

So let's rewind.

Where is gArmMmuReplaceLiveTranslationEntryFuncGuid used?
It is used only in ArmMmuLib. Nothing else cares about that Pcd.
Hence, if the library moves to a different package, then the Pcd definition should move with it, to a token space defined by that package.

Get your point, thanks Leif. Let me move PCD definition from ArmPKg to MdePkg. I will update this PR later.

2 commits updated to fix Guid issue:

[ArmPkg,MdePkg: Move PCD PcdRemapUnusedMemoryNx from ArmPkg to MdePkg]

[ArmPkg,MdePkg: Move PCD PcdVFPEnabled from ArmPkg to MdePkg]

2 commits updated to move ArmMmuLib from ArmPkg to UefiCpuPkg:

[ArmPkg,MdePkg: Move ArmMmuLib.h from ArmPkg to MdePkg]

[ArmPkg,UefiCpuPkg: Move ArmMmuLib from ArmPkg to UefiCpuPkg]

AjanZhong · 2025-03-11T15:13:53Z

Thanks @os-d , let me try to address your comments one by one. And will update this PR once I get good process.

AjanZhong · 2025-03-14T12:36:56Z

Now we have one issue need to be fixed: ERROR - Dependency Check: UefiCpuPkg/CpuDxeArm/CpuDxe.inf depends on pkg EmbeddedPkg/EmbeddedPkg.dec but pkg is not listed in AcceptableDependencies.
This is caused by Protocol: gHardwareInterruptProtocolGuid is created by Arm CpuDxe. Let me refine this GUID in MdePkg to fix this issue.

Move the ArmMmuLib interface definition to MdePkg, as discussed in https://edk2.groups.io/g/devel/topic/patch_v5_2_6/102725178 Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Move the PcdNormalMemoryNonshareableOverride definition to MdePkg, as discussed in https://edk2.groups.io/g/devel/topic/patch_v5_2_6/102725178 Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Move ArmMmuLib from ArmPkg to UefiCpuPkg for easy maintaining. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

DumpImageAndCpuContent dumps both module info and Cpu Context when exception triggered. Default exception handler for ARM and AArch64 architectures utilize same logic as DumpImageAndCpuContent. Move DumpImageAndCpuContent from local header to library header file. With this changes, ARM and AArch64 architectures can provide exception library CpuExceptionHandlerLib as same as other architectures, and drop DefaultExceptionHandlerLib. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Merge DefaultExceptionHandlerLib with ArmExceptionLib to make sure ArmExceptionLib align with definitions of CpuExceptionHandlerLib. Rename method DefaultExceptionHandler to DumpImageAndCpuContent in ArmExceptionLib. With this changes, Exception Libraries for ARM and AArch64 keep align with other architectures. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Exception libraries for Ia32, X64, LoongArch, RiscV64 architectures are all placed at UefiCpuPkg/Library. Move ArmExceptionLib to same place for ease maintaining Exception library for all architectures. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Move PCD PcdRemapUnusedMemoryNx definition from ArmPkg to MdePkg as discussed in: https://edk2.groups.io/g/devel/topic/patch_v5_2_6/102725178 Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Move PCD PcdVFPEnabled definition from ArmPkg to MdePkg as discussed in: https://edk2.groups.io/g/devel/topic/patch_v5_2_6/102725178 Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Move gHardwareInterruptProtocolGuid out of EmbeddedPkg, define a new Protocol Guid gEfiHardwareInterruptProtocolGuid to cover Hardware Protocol Guid in MdePkg. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

Move CpuDxe from ArmPkg to CpuDxeArm in UefiCpuPkg as discussed in: https://edk2.groups.io/g/devel/topic/patch_v5_2_6/102725178 Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

AjanZhong · 2025-03-21T07:25:57Z

@leiflindholm @os-d @ardbiesheuvel Hi all, PR is ready for review now. Please help review this PR when you are available. Thanks.

AjanZhong force-pushed the cpudxe_armpkg_ueficpupkg branch from bf471d6 to 9e4c0de Compare March 10, 2025 14:05

os-d reviewed Mar 10, 2025

View reviewed changes

leiflindholm requested a review from ardbiesheuvel March 11, 2025 17:14

AjanZhong force-pushed the cpudxe_armpkg_ueficpupkg branch 5 times, most recently from 3c75c8d to 6d4eb5d Compare March 14, 2025 08:19

AjanZhong force-pushed the cpudxe_armpkg_ueficpupkg branch 2 times, most recently from 422f529 to a0a3486 Compare March 17, 2025 12:45

AjanZhong force-pushed the cpudxe_armpkg_ueficpupkg branch 2 times, most recently from 8bbadf6 to a492423 Compare March 18, 2025 04:05

AjanZhong force-pushed the cpudxe_armpkg_ueficpupkg branch from a492423 to c9ecc09 Compare March 18, 2025 04:25

AjanZhong added 8 commits March 18, 2025 13:30

ArmPkg,UefiCpuPkg: Move ArmMmuLib from ArmPkg to UefiCpuPkg

78cc52c

Move ArmMmuLib from ArmPkg to UefiCpuPkg for easy maintaining. Continuous-integration-options: PatchCheck.ignore-multi-package Signed-off-by: Ajan Zhong <[email protected]>

AjanZhong force-pushed the cpudxe_armpkg_ueficpupkg branch from c9ecc09 to 2c0482b Compare March 18, 2025 05:33

AjanZhong marked this pull request as ready for review March 18, 2025 06:34

tianocore-assign-reviewers bot requested a review from benjamindoron March 18, 2025 06:34

tianocore-assign-reviewers bot requested review from changab, ChaselChiu, gdong1, gguo11837463, jameslu8, jiaxinwu, kraxel, lgao4, LinusvPelt, LiuZhiguang001, lzeng14, mdkinney, niruiyu and rahul1-kumar March 18, 2025 06:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move CpuDxe from ArmPkg to UefiCpuPkg #10836

Move CpuDxe from ArmPkg to UefiCpuPkg #10836

AjanZhong commented Mar 10, 2025 •

edited

Loading

os-d Mar 10, 2025

AjanZhong Mar 14, 2025

os-d Mar 10, 2025

AjanZhong Mar 14, 2025

os-d Mar 10, 2025

leiflindholm Mar 11, 2025

AjanZhong Mar 12, 2025

leiflindholm Mar 12, 2025 •

edited

Loading

AjanZhong Mar 12, 2025

AjanZhong Mar 14, 2025

AjanZhong Mar 18, 2025

AjanZhong commented Mar 11, 2025

AjanZhong commented Mar 14, 2025

AjanZhong commented Mar 21, 2025

Move CpuDxe from ArmPkg to UefiCpuPkg #10836

Are you sure you want to change the base?

Move CpuDxe from ArmPkg to UefiCpuPkg #10836

Conversation

AjanZhong commented Mar 10, 2025 • edited Loading

Description

How This Was Tested

Integration Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leiflindholm Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AjanZhong commented Mar 11, 2025

AjanZhong commented Mar 14, 2025

AjanZhong commented Mar 21, 2025

AjanZhong commented Mar 10, 2025 •

edited

Loading

leiflindholm Mar 12, 2025 •

edited

Loading