Re: [PATCH 4/6] OvmfPkg/ResetSystemLib: factor out ResetShutdown()

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/17/20 5:37 PM, Laszlo Ersek wrote:
> Move the ResetShutdown() definition to its own file. This will help us
> introduce:
> 
> - a new library instance that is not broken in runtime modules (the
>    current library instance is broken in runtime modules),
> 
> - another new library instance for bhyve support.
> 
> While at it, squash AcpiPmControl() into ResetShutdown(), open-coding
> SuspendType=0. This is justified because we've had no other callers for
> AcpiPmControl() since commit 2d9950a2bff8 ("OvmfPkg: remove
> EnterS3WithImmediateWake () from ResetSystemLib", 2020-01-10).
> 
> Tested with the "reset -s" UEFI shell command, on both i440fx and q35.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2675
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf |  1 +
>   OvmfPkg/Library/ResetSystemLib/ResetShutdown.c    | 51 ++++++++++++++++++++
>   OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c   | 49 -------------------
>   3 files changed, 52 insertions(+), 49 deletions(-)
> 
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> index af20f516c035..9362f884f124 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> @@ -22,6 +22,7 @@ [Defines]
>   #
>   
>   [Sources]
> +  ResetShutdown.c
>     ResetSystemLib.c
>   
>   [Packages]
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c b/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
> new file mode 100644
> index 000000000000..971d94fa5766
> --- /dev/null
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetShutdown.c
> @@ -0,0 +1,51 @@
> +/** @file
> +  Reset System Library Shutdown API implementation for OVMF.
> +
> +  Copyright (C) 2020, Red Hat, Inc.
> +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Base.h>                   // BIT13
> +
> +#include <Library/BaseLib.h>        // CpuDeadLoop()
> +#include <Library/DebugLib.h>       // ASSERT()
> +#include <Library/IoLib.h>          // IoOr16()
> +#include <Library/PciLib.h>         // PciRead16()
> +#include <Library/ResetSystemLib.h> // ResetShutdown()
> +#include <OvmfPlatforms.h>          // OVMF_HOSTBRIDGE_DID
> +
> +/**
> +  Calling this function causes the system to enter a power state equivalent
> +  to the ACPI G2/S5 or G3 states.
> +
> +  System shutdown should not return, if it returns, it means the system does
> +  not support shut down reset.
> +**/
> +VOID
> +EFIAPI
> +ResetShutdown (
> +  VOID
> +  )
> +{
> +  UINT16 AcpiPmBaseAddress;
> +  UINT16 HostBridgeDevId;
> +
> +  AcpiPmBaseAddress = 0;
> +  HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> +  switch (HostBridgeDevId) {
> +  case INTEL_82441_DEVICE_ID:
> +    AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
> +    break;
> +  case INTEL_Q35_MCH_DEVICE_ID:
> +    AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
> +    break;
> +  default:
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +
> +  IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, 0);
> +  IoOr16 (AcpiPmBaseAddress + 4, BIT13);
> +  CpuDeadLoop ();
> +}
> diff --git a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> index 0fc142325ece..fe51f53d1df2 100644
> --- a/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> +++ b/OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c
> @@ -9,41 +9,9 @@
>   #include <Base.h>                   // BIT1
>   
>   #include <Library/BaseLib.h>        // CpuDeadLoop()
> -#include <Library/DebugLib.h>       // ASSERT()
>   #include <Library/IoLib.h>          // IoWrite8()
> -#include <Library/PciLib.h>         // PciRead16()
>   #include <Library/ResetSystemLib.h> // ResetCold()
>   #include <Library/TimerLib.h>       // MicroSecondDelay()
> -#include <OvmfPlatforms.h>          // OVMF_HOSTBRIDGE_DID
> -
> -VOID
> -AcpiPmControl (
> -  UINTN SuspendType
> -  )
> -{
> -  UINT16 AcpiPmBaseAddress;
> -  UINT16 HostBridgeDevId;
> -
> -  ASSERT (SuspendType < 6);
> -
> -  AcpiPmBaseAddress = 0;
> -  HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
> -  switch (HostBridgeDevId) {
> -  case INTEL_82441_DEVICE_ID:
> -    AcpiPmBaseAddress = PIIX4_PMBA_VALUE;
> -    break;
> -  case INTEL_Q35_MCH_DEVICE_ID:
> -    AcpiPmBaseAddress = ICH9_PMBASE_VALUE;
> -    break;
> -  default:
> -    ASSERT (FALSE);
> -    CpuDeadLoop ();
> -  }
> -
> -  IoBitFieldWrite16 (AcpiPmBaseAddress + 4, 10, 13, (UINT16) SuspendType);
> -  IoOr16 (AcpiPmBaseAddress + 4, BIT13);
> -  CpuDeadLoop ();
> -}
>   
>   /**
>     Calling this function causes a system-wide reset. This sets
> @@ -84,23 +52,6 @@ ResetWarm (
>     CpuDeadLoop ();
>   }
>   
> -/**
> -  Calling this function causes the system to enter a power state equivalent
> -  to the ACPI G2/S5 or G3 states.
> -
> -  System shutdown should not return, if it returns, it means the system does
> -  not support shut down reset.
> -**/
> -VOID
> -EFIAPI
> -ResetShutdown (
> -  VOID
> -  )
> -{
> -  AcpiPmControl (0);
> -  ASSERT (FALSE);
> -}
> -
>   
>   /**
>     This function causes a systemwide reset. The exact type of the reset is
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>