On 06/29/15 09:42, Tian, Hot wrote:
> I think you'd better do more comments clean up, e.g.:
> AcpiS3Save.h (maybe we can totally remove this .h):  This is an 
> implementation of the ACPI S3 Save protocol.  This is defined in S3 boot path 
> specification 0.9.
> AcpiS3Save.c: This is an implementation of the ACPI S3 Save protocol.  This 
> is defined in S3 boot path specification 0.9.
>               Comments for InstallAcpiS3Save(): The function is the driver 
> Entry point which will produce AcpiS3SaveProtocol.
> AcpiS3SaveDxe.uni: #string STR_MODULE_ABSTRACT             #language en-US 
> "Installs ACPI S3 Save protocol to prepare S3 boot data"
>                        #string STR_MODULE_DESCRIPTION          #language 
> en-US "AcpiS3Save module installs ACPI S3 Save protocol to prepare S3 boot 
> data."
> etc.

Will do, thanks.

> 
> Thanks,
> Hot
> 
> -----Original Message-----
> From: Yao, Jiewen [mailto:jiewen....@intel.com] 
> Sent: Saturday, June 27, 2015 19:56
> To: Laszlo Ersek; edk2-devel@lists.sourceforge.net
> Cc: He, Tim; Fan, Jeff
> Subject: Re: [edk2] [PATCH 8/9] IntelFrameworkModulePkg: AcpiS3SaveDxe: drop 
> EFI_ACPI_S3_SAVE_PROTOCOL
> 
> Reviewed by: Yao, Jiewen <jiewen....@intel.com>
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Saturday, June 27, 2015 9:08 AM
> To: edk2-devel@lists.sourceforge.net
> Cc: Yao, Jiewen; Fan, Jeff; Wei, David; He, Tim
> Subject: [PATCH 8/9] IntelFrameworkModulePkg: AcpiS3SaveDxe: drop 
> EFI_ACPI_S3_SAVE_PROTOCOL
> 
> At this point, nothing in the edk2 tree calls EFI_ACPI_S3_SAVE_PROTOCOL 
> member functions; simplify the code by dropping this protocol interface.
> 
> Cc: Yao Jiewen <jiewen....@intel.com>
> Cc: Jeff Fan <jeff....@intel.com>
> Cc: David Wei <david....@intel.com>
> Cc: Tim He <tim...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
>  IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf |  4 
> +-
>  IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h      | 28 
> +------
>  IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c      | 78 
> +-------------------
>  3 files changed, 6 insertions(+), 104 deletions(-)
> 
> diff --git 
> a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf 
> b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> index e5fb92e..c6fd1dc 100644
> --- a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> +++ b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe
> +++ .inf
> @@ -1,5 +1,5 @@
>  ## @file
> -# AcpiS3Save module installs ACPI S3 Save protocol to prepare S3 boot data.
> +# AcpiS3Save module installs EndOfDxe callback to prepare S3 boot data.
>  #
>  # Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>  # 
> @@ -63,14 +63,12 @@ [Guids]
>    gEfiEndOfDxeEventGroupGuid                    ## CONSUMES  ## Event
>  
>  [Protocols]
> -  gEfiAcpiS3SaveProtocolGuid                    ## PRODUCES
>    gFrameworkEfiMpServiceProtocolGuid            ## SOMETIMES_CONSUMES
>    ## NOTIFY
>    ## SOMETIMES_CONSUMES
>    gEdkiiVariableLockProtocolGuid
>  
>  [FeaturePcd]
> -  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPlatformCsmSupport          
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode                 
> ## CONSUMES
>  
> diff --git 
> a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h 
> b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h
> index 65974a3..6d8a9bb 100644
> --- a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h
> +++ b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h
> @@ -19,41 +19,15 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #define _ACPI_S3_SAVE_H_
>  
>  /**
> -  Gets the buffer of legacy memory below 1 MB
> -  This function is to get the buffer in legacy memory below 1MB that is 
> required during S3 resume.
> -
> -  @param This           A pointer to the EFI_ACPI_S3_SAVE_PROTOCOL instance.
> -  @param Size           The returned size of legacy memory below 1 MB.
> -
> -  @retval EFI_SUCCESS           Size is successfully returned.
> -  @retval EFI_INVALID_PARAMETER The pointer Size is NULL.
> -
> -**/
> -EFI_STATUS
> -EFIAPI
> -LegacyGetS3MemorySize (
> -  IN  EFI_ACPI_S3_SAVE_PROTOCOL    * This,
> -  OUT UINTN                        * Size
> -  );
> -
> -/**
>    Prepares all information that is needed in the S3 resume boot path.
>    
>    Allocate the resources or prepare informations and save in ACPI variable 
> set for S3 resume boot path  
>    
> -  @param This                 A pointer to the EFI_ACPI_S3_SAVE_PROTOCOL 
> instance.
> -  @param LegacyMemoryAddress  The base address of legacy memory.
> -
> -  @retval EFI_NOT_FOUND         Some necessary information cannot be found.
>    @retval EFI_SUCCESS           All information was saved successfully.
> -  @retval EFI_OUT_OF_RESOURCES  Resources were insufficient to save all the 
> information.
> -  @retval EFI_INVALID_PARAMETER The memory range is not located below 1 MB.
> -
>  **/
>  EFI_STATUS
>  EFIAPI
>  S3Ready (
> -  IN EFI_ACPI_S3_SAVE_PROTOCOL     *This,
> -  IN VOID                          *LegacyMemoryAddress   
> +  VOID
>    );
>  #endif
> diff --git 
> a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c 
> b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
> index e0fa866..3d7d3cd 100644
> --- a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
> +++ b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
> @@ -29,7 +29,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #include <Guid/AcpiS3Context.h>
>  #include <Guid/Acpi.h>
>  #include <Guid/EventGroup.h>
> -#include <Protocol/AcpiS3Save.h>
>  #include <IndustryStandard/Acpi.h>
>  
>  #include "AcpiS3Save.h"
> @@ -52,13 +51,6 @@ S3ReadyThunkPlatform (
>    IN ACPI_S3_CONTEXT      *AcpiS3Context
>    );
>  
> -UINTN     mLegacyRegionSize;
> -
> -EFI_ACPI_S3_SAVE_PROTOCOL mS3Save = {
> -  LegacyGetS3MemorySize,
> -  S3Ready,
> -};
> -
>  EFI_GUID              mAcpiS3IdtrProfileGuid = {
>    0xdea652b0, 0xd587, 0x4c54, { 0xb5, 0xb4, 0xc6, 0x82, 0xe7, 0xa0, 0xaa, 
> 0x3d }  }; @@ -407,52 +399,16 @@ S3CreateIdentityMappingPageTables (  }
>  
>  /**
> -  Gets the buffer of legacy memory below 1 MB
> -  This function is to get the buffer in legacy memory below 1MB that is 
> required during S3 resume.
> -
> -  @param This           A pointer to the EFI_ACPI_S3_SAVE_PROTOCOL instance.
> -  @param Size           The returned size of legacy memory below 1 MB.
> -
> -  @retval EFI_SUCCESS           Size is successfully returned.
> -  @retval EFI_INVALID_PARAMETER The pointer Size is NULL.
> -
> -**/
> -EFI_STATUS
> -EFIAPI
> -LegacyGetS3MemorySize (
> -  IN  EFI_ACPI_S3_SAVE_PROTOCOL   *This,
> -  OUT UINTN                       *Size
> -  )
> -{
> -  ASSERT (FALSE);
> -
> -  if (Size == NULL) {
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
> -  *Size = mLegacyRegionSize;
> -  return EFI_SUCCESS;
> -}
> -
> -/**
>    Prepares all information that is needed in the S3 resume boot path.
>    
>    Allocate the resources or prepare informations and save in ACPI variable 
> set for S3 resume boot path  
>    
> -  @param This                 A pointer to the EFI_ACPI_S3_SAVE_PROTOCOL 
> instance.
> -  @param LegacyMemoryAddress  The base address of legacy memory.
> -
> -  @retval EFI_NOT_FOUND         Some necessary information cannot be found.
>    @retval EFI_SUCCESS           All information was saved successfully.
> -  @retval EFI_OUT_OF_RESOURCES  Resources were insufficient to save all the 
> information.
> -  @retval EFI_INVALID_PARAMETER The memory range is not located below 1 MB.
> -
>  **/
>  EFI_STATUS
>  EFIAPI
>  S3Ready (
> -  IN EFI_ACPI_S3_SAVE_PROTOCOL    *This,
> -  IN VOID                         *LegacyMemoryAddress
> +  VOID
>    )
>  {
>    EFI_STATUS                                    Status;
> @@ -464,17 +420,12 @@ S3Ready (
>  
>    DEBUG ((EFI_D_INFO, "S3Ready!\n"));
>  
> -  //
> -  // Platform may invoke AcpiS3Save->S3Save() before ExitPmAuth, because we 
> need save S3 information there, while BDS ReadyToBoot may invoke it again.
> -  // So if 2nd S3Save() is triggered later, we need ignore it.
> -  //
> +  ASSERT (!AlreadyEntered);
>    if (AlreadyEntered) {
>      return EFI_SUCCESS;
>    }
>    AlreadyEntered = TRUE;
>  
> -  ASSERT (LegacyMemoryAddress == NULL);
> -
>    AcpiS3Context = AllocateMemoryBelow4G (EfiReservedMemoryType, 
> sizeof(*AcpiS3Context));
>    ASSERT (AcpiS3Context != NULL);
>    AcpiS3ContextBuffer = (EFI_PHYSICAL_ADDRESS)(UINTN)AcpiS3Context;
> @@ -567,13 +518,9 @@ OnEndOfDxe (
>    EFI_STATUS Status;
>  
>    //
> -  // Our S3Ready() function ignores both of its parameters, and always
> -  // succeeds.
> +  // Our S3Ready() function always succeeds.
>    //
> -  Status = S3Ready (
> -             NULL, // This
> -             NULL  // LegacyMemoryAddress
> -             );
> +  Status = S3Ready ();
>    ASSERT_EFI_ERROR (Status);
>  
>    //
> @@ -607,27 +554,10 @@ InstallAcpiS3Save (
>    EFI_STATUS        Status;
>    EFI_EVENT         EndOfDxeEvent;
>  
> -  if (!FeaturePcdGet(PcdPlatformCsmSupport)) {
> -    //
> -    // More memory for no CSM tip, because GDT need relocation
> -    //
> -    mLegacyRegionSize = 0x250;
> -  } else {
> -    mLegacyRegionSize = 0x100;
> -  }
> -
>    if (FeaturePcdGet(PcdFrameworkCompatibilitySupport)) {
>      InstallAcpiS3SaveThunk ();
>    }
>  
> -  Status = gBS->InstallProtocolInterface (
> -                  &ImageHandle,
> -                  &gEfiAcpiS3SaveProtocolGuid,
> -                  EFI_NATIVE_INTERFACE,
> -                  &mS3Save
> -                  );
> -  ASSERT_EFI_ERROR (Status);
> -
>    Status = gBS->CreateEventEx (
>                    EVT_NOTIFY_SIGNAL,
>                    TPL_CALLBACK,
> --
> 1.8.3.1
> 
> 
> 
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors 
> network devices and physical & virtual servers, alerts via email & sms 
> for fault. Monitor 25 devices for free with no restriction. Download now
> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
> 


------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to