Re: [PATCH] ArmVirtPkg: switch to generic ResetSystemRuntimeDxe

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Jul 03, 2017 at 02:53:45PM +0100, Ard Biesheuvel wrote:
> For obscure reasons, ARM platforms use a different implementation of
> the ResetSystem() runtime service call than other platforms. So let's
> switch all ArmVirtPkg platforms to the generic version instead.
> 
> Given that all platforms use an implementation of EfiResetSystemLib [as
> consumed by the ResetRuntimeDxe in EmbeddedPkg that we are replacing]
> which is unlikely to be depended upon by out of tree platforms, let's
> simply modify this library into an implementation of ResetSystemLib
> instead [which is what the generic driver in MdeModulePkg consumes]
> 
> This does mean we need to update all clients at the same time, which
> is why all changes are part of the same patch.
> 
> As before, warm reset and platform specific reset are mapped onto
> cold reset (which is the only thing PSCI implements, at least the
> version we depend on). The new library function EnterS3WithImmediateWake()
> is left unimplemented, as permitted by the ResetSystemLib library class.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Looks good.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
> Note: this a no-whitespace diff
> 
>  ArmVirtPkg/ArmVirt.dsc.inc                                                 |   2 +-
>  ArmVirtPkg/ArmVirtQemu.dsc                                                 |   2 +-
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                       |   2 +-
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                                           |   2 +-
>  ArmVirtPkg/ArmVirtXen.dsc                                                  |   2 +-
>  ArmVirtPkg/ArmVirtXen.fdf                                                  |   2 +-
>  ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c   | 125 +++++++++++++-------
>  ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf |   4 +-
>  8 files changed, 89 insertions(+), 52 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 5bf537635488..0b9b457b5619 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -101,7 +101,7 @@ [LibraryClasses.common]
>  
>    PlatformPeiLib|ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
>    MemoryInitPeiLib|ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> -  EfiResetSystemLib|ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> +  ResetSystemLib|ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
>  
>    # ARM PL031 RTC Driver
>    RealTimeClockLib|ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 10de57a4149e..e23a6d17bc44 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -266,7 +266,7 @@ [Components.common]
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>    MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>    MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> -  EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf {
>      <LibraryClasses>
>        NULL|ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index ba2e7157cf2c..237b2d03a714 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -63,7 +63,7 @@ [FV.FvMain]
>    INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>  !endif
>    INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> -  INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  INF MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>    INF EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
>    INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 93ccb6df37e1..aa01debfda69 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -257,7 +257,7 @@ [Components.common]
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>    MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
>    MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> -  EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf {
>      <LibraryClasses>
>        NULL|ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
> index f832b3fb64f4..eb37137f271e 100644
> --- a/ArmVirtPkg/ArmVirtXen.dsc
> +++ b/ArmVirtPkg/ArmVirtXen.dsc
> @@ -167,7 +167,7 @@ [Components.common]
>    MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
>  
>    MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> -  EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>    EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
>  
> diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
> index b591818f0b42..67fde73e696d 100644
> --- a/ArmVirtPkg/ArmVirtXen.fdf
> +++ b/ArmVirtPkg/ArmVirtXen.fdf
> @@ -140,7 +140,7 @@ [FV.FvMain]
>    INF MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
>  
>    INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> -  INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
> +  INF MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>    INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
>    INF EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
>    INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> diff --git a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c
> index b94d1b0090c6..4b0db206e920 100644
> --- a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.c
> @@ -23,7 +23,7 @@
>  
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
> -#include <Library/EfiResetSystemLib.h>
> +#include <Library/ResetSystemLib.h>
>  #include <Library/ArmSmcLib.h>
>  #include <Library/ArmHvcLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> @@ -67,49 +67,75 @@ ArmPsciResetSystemLibConstructor (
>  }
>  
>  /**
> -  Resets the entire platform.
> -
> -  @param  ResetType             The type of reset to perform.
> -  @param  ResetStatus           The status code for the reset.
> -  @param  DataSize              The size, in bytes, of WatchdogData.
> -  @param  ResetData             For a ResetType of EfiResetCold, EfiResetWarm, or
> -                                EfiResetShutdown the data buffer starts with a Null-terminated
> -                                Unicode string, optionally followed by additional binary data.
> +  This function causes a system-wide reset (cold reset), in which
> +  all circuitry within the system returns to its initial state. This type of reset
> +  is asynchronous to system operation and operates without regard to
> +  cycle boundaries.
>  
> +  If this function returns, it means that the system does not support cold reset.
>  **/
> -EFI_STATUS
> +VOID
>  EFIAPI
> -LibResetSystem (
> -  IN EFI_RESET_TYPE   ResetType,
> -  IN EFI_STATUS       ResetStatus,
> -  IN UINTN            DataSize,
> -  IN CHAR16           *ResetData OPTIONAL
> +ResetCold (
> +  VOID
>    )
>  {
>    ARM_SMC_ARGS ArmSmcArgs;
>    ARM_HVC_ARGS ArmHvcArgs;
>  
> -  switch (ResetType) {
> -
> -  case EfiResetPlatformSpecific:
> -    // Map the platform specific reset as reboot
> -  case EfiResetWarm:
> -    // Map a warm reset into a cold reset
> -  case EfiResetCold:
>    // Send a PSCI 0.2 SYSTEM_RESET command
>    ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;
>    ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;
> +
> +  switch (mArmPsciMethod) {
> +  case 1:
> +    ArmCallHvc (&ArmHvcArgs);
>      break;
> -  case EfiResetShutdown:
> -    // Send a PSCI 0.2 SYSTEM_OFF command
> -    ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
> -    ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
> +
> +  case 2:
> +    ArmCallSmc (&ArmSmcArgs);
>      break;
> +
>    default:
> -    ASSERT (FALSE);
> -    return EFI_UNSUPPORTED;
> +    DEBUG ((EFI_D_ERROR, "%a: no PSCI method defined\n", __FUNCTION__));
> +  }
> +}
> +
> +/**
> +  This function causes a system-wide initialization (warm reset), in which all processors
> +  are set to their initial state. Pending cycles are not corrupted.
> +
> +  If this function returns, it means that the system does not support warm reset.
> +**/
> +VOID
> +EFIAPI
> +ResetWarm (
> +  VOID
> +  )
> +{
> +  // Map a warm reset into a cold reset
> +  ResetCold ();
>  }
>  
> +/**
> +  This function causes the system to enter a power state equivalent
> +  to the ACPI G2/S5 or G3 states.
> +
> +  If this function returns, it means that the system does not support shutdown reset.
> +**/
> +VOID
> +EFIAPI
> +ResetShutdown (
> +  VOID
> +  )
> +{
> +  ARM_SMC_ARGS ArmSmcArgs;
> +  ARM_HVC_ARGS ArmHvcArgs;
> +
> +  // Send a PSCI 0.2 SYSTEM_OFF command
> +  ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
> +  ArmHvcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_OFF;
> +
>    switch (mArmPsciMethod) {
>    case 1:
>      ArmCallHvc (&ArmHvcArgs);
> @@ -121,30 +147,41 @@ LibResetSystem (
>  
>    default:
>      DEBUG ((EFI_D_ERROR, "%a: no PSCI method defined\n", __FUNCTION__));
> -    return EFI_UNSUPPORTED;
>    }
> -
> -  // We should never be here
> -  DEBUG ((EFI_D_ERROR, "%a: PSCI Reset failed\n", __FUNCTION__));
> -  CpuDeadLoop ();
> -  return EFI_UNSUPPORTED;
>  }
>  
>  /**
> -  Initialize any infrastructure required for LibResetSystem () to function.
> +  This function causes the system to enter S3 and then wake up immediately.
>  
> -  @param  ImageHandle   The firmware allocated handle for the EFI image.
> -  @param  SystemTable   A pointer to the EFI System Table.
> -
> -  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
> +  If this function returns, it means that the system does not support S3 feature.
> +**/
> +VOID
> +EFIAPI
> +EnterS3WithImmediateWake (
> +  VOID
> +  )
> +{
> +  // not implemented
> +}
>  
> +/**
> +  This function causes a systemwide reset. The exact type of the reset is
> +  defined by the EFI_GUID that follows the Null-terminated Unicode string passed
> +  into ResetData. If the platform does not recognize the EFI_GUID in ResetData
> +  the platform must pick a supported reset type to perform.The platform may
> +  optionally log the parameters from any non-normal reset that occurs.
> +
> +  @param[in]  DataSize   The size, in bytes, of ResetData.
> +  @param[in]  ResetData  The data buffer starts with a Null-terminated string,
> +                         followed by the EFI_GUID.
>  **/
> -EFI_STATUS
> +VOID
>  EFIAPI
> -LibInitializeResetSystem (
> -  IN EFI_HANDLE        ImageHandle,
> -  IN EFI_SYSTEM_TABLE  *SystemTable
> +ResetPlatformSpecific (
> +  IN UINTN   DataSize,
> +  IN VOID    *ResetData
>    )
>  {
> -  return EFI_SUCCESS;
> +  // Map the platform specific reset as reboot
> +  ResetCold ();
>  }
> diff --git a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> index 9dc515aee52b..f28aed3fb1c2 100644
> --- a/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> +++ b/ArmVirtPkg/Library/ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf
> @@ -20,7 +20,7 @@ [Defines]
>    FILE_GUID                      = c81d76ed-66fa-44a3-ac4a-f163120187a9
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = EfiResetSystemLib|DXE_DRIVER DXE_RUNTIME_DRIVER
> +  LIBRARY_CLASS                  = ResetSystemLib|DXE_DRIVER DXE_RUNTIME_DRIVER
>    CONSTRUCTOR                    = ArmPsciResetSystemLibConstructor
>  
>  [Sources]
> @@ -29,8 +29,8 @@ [Sources]
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
> -  EmbeddedPkg/EmbeddedPkg.dec
>  
>  [LibraryClasses]
>    ArmSmcLib
> -- 
> 2.9.3
>