On Thu, Jun 07, 2018 at 08:57:04AM +0200, Ard Biesheuvel wrote: > On 6 June 2018 at 15:29, Laszlo Ersek <[email protected]> wrote: > > On 06/06/18 14:37, Ard Biesheuvel wrote: > >> Implement ResetSystemLib's EnterS3WithImmediateWake() routine using > >> a jump back to the PEI entry point with interrupts and MMU+caches > >> disabled. This is only possible at boot time, when we are sure that > >> the current CPU is the only one up and running. Also, it depends on > >> the platform whether the PEI code is preserved in memory (it may be > >> copied to DRAM rather than execute in place), so also add a feature > >> PCD to selectively enable this feature. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Ard Biesheuvel <[email protected]> > >> --- > >> ArmPkg/ArmPkg.dec | 4 > >> ++++ > >> ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c | 21 > >> ++++++++++++++++++-- > >> ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf | 9 > >> +++++++++ > >> 3 files changed, 32 insertions(+), 2 deletions(-) > >> > >> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec > >> index debe066b6f7b..3aa229fe2ec9 100644 > >> --- a/ArmPkg/ArmPkg.dec > >> +++ b/ArmPkg/ArmPkg.dec > >> @@ -85,6 +85,10 @@ [PcdsFeatureFlag.common] > >> # Define if the GICv3 controller should use the GICv2 legacy > >> gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042 > >> > >> + # Whether to implement warm reboot for capsule update using a jump back > >> to the > >> + # PEI entry point with caches and interrupts disabled. > >> + > >> gArmTokenSpaceGuid.PcdArmReenterPeiForCapsuleWarmReboot|FALSE|BOOLEAN|0x0000001F > >> + > >> [PcdsFeatureFlag.ARM] > >> # Whether to map normal memory as non-shareable. FALSE is the safe > >> choice, but > >> # TRUE may be appropriate to fix performance problems if you don't care > >> about > >> diff --git > >> a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > >> b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > >> index d6d26bce5009..cb75e32771c2 100644 > >> --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > >> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c > >> @@ -15,10 +15,13 @@ > >> > >> #include <PiDxe.h> > >> > >> +#include <Library/ArmMmuLib.h> > >> +#include <Library/ArmSmcLib.h> > >> #include <Library/BaseLib.h> > >> #include <Library/DebugLib.h> > >> #include <Library/ResetSystemLib.h> > >> -#include <Library/ArmSmcLib.h> > >> +#include <Library/UefiBootServicesTableLib.h> > >> +#include <Library/UefiRuntimeLib.h> > >> > >> #include <IndustryStandard/ArmStdSmc.h> > >> > >> @@ -89,7 +92,21 @@ EnterS3WithImmediateWake ( > >> VOID > >> ) > >> { > >> - // Not implemented > >> + VOID (*Reset)(VOID); > >> + > >> + if (FeaturePcdGet (PcdArmReenterPeiForCapsuleWarmReboot) && > >> + !EfiAtRuntime ()) { > >> + // > >> + // At boot time, we are the only core running, so we can implement the > >> + // immediate wake (which is used by capsule update) by disabling the > >> MMU > >> + // and interrupts, and jumping to the PEI entry point. > >> + // > >> + Reset = (VOID *)(UINTN)FixedPcdGet64 (PcdFvBaseAddress); > > > > ISO C99 6.3.2.3 Pointers > > > > 1 A pointer to void may be converted to or from a pointer to any > > incomplete or object type. [...] > > > > [...] > > > > 5 An integer may be converted to any pointer type. Except as > > previously specified, the result is implementation-defined, might > > not be correctly aligned, might not point to an entity of the > > referenced type, and might be a trap representation. [...] > > > > [...] > > > > 8 A pointer to a function of one type may be converted to a pointer to > > a function of another type and back again; [...] > > > > The point is, converting pointer-to-void to pointer-to-function is > > undefined, and I expect at least clang will yell at us for it. However, > > converting an integer to pointer-to-function is implementation-defined; > > i.e., the C language implementation must support it to an extent, and > > must document how it works. (The "previously specified" part is about > > null pointer constants, in paragraph 3, which I'm not quoting now.) > > > > Thus, I suggest either > > > > typedef VOID (*RESET_FUNCTION_PTR) (VOID); > > RESET_FUNCTION_PTR Reset; > > > > Reset = (RESET_FUNCTION_PTR)(UINTN)FixedPcdGet64 (PcdFvBaseAddress); > > > > or else (for kicks and giggles regarding the syntax), just > > > > Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress); > > Thanks Laszlo > > I will go for option #2 unless Leif has any objections to that?
I think my preference would be the first simply becauase it feels more TianoCore. But I'm relaly happy with either option. Regards, Leif _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
