tianocore / edk2

EDK II
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
Other
4.52k stars 2.44k forks source link

ArmPkg: Remove Deprecated ArmPsciResetSystemLib #5922

Closed os-d closed 1 month ago

os-d commented 1 month ago

Description

ArmPsciResetSystemLib has been deprecated since commit b2c55e732888fd721f5235a820b1d1c45209992d in 2017. The lib itself has not been meaningfully updated in 10 years. This commit removes the library to complete the deprecation process and remove confusion over which library to use for resetting an ARM platform.

How This Was Tested

N/A.

Integration Instructions

All platforms should be using ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf instead of this ancient library. If a platform was using the old library, they will experience a build break and will need to update to the current library.

os-d commented 1 month ago

@ardbiesheuvel @leiflindholm @samimujawar can you please review?

ardbiesheuvel commented 1 month ago

I suspect this will break the Ampere and Phytium builds in edk2-platforms.

@leiflindholm ?

os-d commented 1 month ago

@ardbiesheuvel, looks like nothing in those platforms is using EfiResetSystemLib, I suspect it is not actually needed and they can just use the ArmSmcPsciResetSystemLib? The Ampere one is already using it for non-runtime DXE.

ardbiesheuvel commented 1 month ago

@ardbiesheuvel, looks like nothing in those platforms is using EfiResetSystemLib, I suspect it is not actually needed and they can just use the ArmSmcPsciResetSystemLib? The Ampere one is already using it for non-runtime DXE.

Those platforms use EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf and provide ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.inf as the library class resolution for EfiResetSystemLib.

os-d commented 1 month ago

@ardbiesheuvel, looks like nothing in those platforms is using EfiResetSystemLib, I suspect it is not actually needed and they can just use the ArmSmcPsciResetSystemLib? The Ampere one is already using it for non-runtime DXE.

Those platforms use EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf and provide ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.inf as the library class resolution for EfiResetSystemLib.

Do you think it is valid for them to use ArmSmcPsciResetSystemLib.inf instead? I'm keying off your deprecation statement :). Or do you still envision ArmPsciResetSystemLib.inf as valid to use?

leiflindholm commented 1 month ago

Pinged platform maintainers/reviewers in https://edk2.groups.io/g/devel/topic/remove_deprecated/107432712.

I note RPi also uses EmbeddedPkg/ResetRuntimeDxe although has its own implementation of EfiResetSystemLib.

ardbiesheuvel commented 1 month ago

Pinged platform maintainers/reviewers in https://edk2.groups.io/g/devel/topic/remove_deprecated/107432712.

I note RPi also uses EmbeddedPkg/ResetRuntimeDxe although has its own implementation of EfiResetSystemLib.

Yeah. EmbeddedPkg/ResetRuntimeDxe should be deprecated and removed too.

I will also note that we could get rid of ArmVirtPkg/Library/ArmVirtPsciResetSystemLib as well if the generic ARM version of ResetSystemLib (which is called ArmVirtPkg/Library/ArmVirtPsciResetSystemLib and uses SMC exclusively) is refactored to depend on ArmMonitorLib (of which ArmVirtPkg has an implementation that encapsulates the logic that required us to provide its own special implementation of ResetSystemLib)

leiflindholm commented 1 month ago

I will also note that we could get rid of ArmVirtPkg/Library/ArmVirtPsciResetSystemLib as well if the generic ARM version of ResetSystemLib (which is called ArmVirtPkg/Library/ArmVirtPsciResetSystemLib and uses SMC exclusively) is refactored to depend on ArmMonitorLib (of which ArmVirtPkg has an implementation that encapsulates the logic that required us to provide its own special implementation of ResetSystemLib)

I like that idea.

os-d commented 1 month ago

@ardbiesheuvel, I don't have enough context to do the ArmVirtPkg refactor, but I can also remove EmbeddedPkg/ResetRuntimeDxe in this PR, too, if you want.

ardbiesheuvel commented 1 month ago

@ardbiesheuvel, I don't have enough context to do the ArmVirtPkg refactor, but I can also remove EmbeddedPkg/ResetRuntimeDxe in this PR, too, if you want.

I've sent out a series to remove the existing references to ArmPsciResetSystemLib from edk2-platforms. With that applied, we can merge this PR.

I'd like to repurpose the 'ArmPsciResetSystemLib' name to implement ResetSystemLib based on ArmMonitorLib, which would allow us to drop ArmVirtPsciResetSystemLib as I suggested, so I'll wait for this PR to get merged first.

EmbeddedPkg/RuntimeDxe can be dropped once RaspberryPi moves to MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf. This requires some more elaborate refactoring as RPi's EfiResetSystemLib has its own implementation of reset notifiers, which need to be accommodated elsewhere. This is something I can look into as well.