[edk2] [PATCH 0/9] multiple packages: save S3 state at EndOfDxe

[Laszlo Ersek]

In
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146>,

And in the future, I believe we could remove EFI_ACPI_S3_SAVE_PROTOCOL
protocol completely. (This is Framework 0.9 defined API, not PI
defined API)
1) AcpiS3Save does not install EFI_ACPI_S3_SAVE_PROTOCOL.
2) AcpiS3Save registers EndOfDxe callback function.
3) AcpiS3Save does S3Save() in EndOfDxe callback function.
So everything could be self-contained. There is no need to rely on
platform BDS.
Then this module eliminates IntelFrameworkPkg dependency and can be
moved to MdeModulePkg later.

This series implements the idea, and "on the side", it changes OVMF to
kick the EndOfDxe event group (which is where the entire discussion has
come from -- Ard started it because ArmVirtPkg lacked EndOfDxe).

The series is organized carefully. It is bisectable, plus it has
another, "orthogonal" dimension (see below). The structure is:

- Patches #1 and #2 make IntelFrameworkModulePkg/AcpiS3SaveDxe act upon
EndOfDxe *in addition to* the usual S3Save() protocol member call.

- Patches #3 and #4 do the same to OvmfPkg's clone of the driver.
(Before anyone asks: yes, that clone needs to remain separate.)

- Patch #5 modifies Vlv2TbltDevicePkg. That package is the only one
in the edk2 tree that uses IntelFrameworkModulePkg/AcpiS3SaveDxe.

Because I'm implementing Jiewen's idea in
IntelFrameworkModulePkg/AcpiS3SaveDxe, I must migrate the driver's
clients, and Vlv2TbltDevicePkg is one client. (The only one.)

So patch #5 flips Vlv2TbltDevicePkg from calling the S3Save() protocol
member to kicking the EndOfDxe event group. Unfortunately, I couldn't
even *compile* this package, let alone test it. Thus, I need help with
this patch.

Importantly, because Vlv2TbltDevicePkg has its own clone of
GenericBdsLib, Ard's patch (patch #7) below does *not* affect it.
Therefore the Vlv2TbltDevicePkg changes are completely isolated to
patch #5.

- Patch #6 switches over OvmfPkg from depending on the S3Save() protocol
member call (inside IntelFrameworkModulePkg/GenericBdsLib) to kicking
EndOfDxe explicitly.

- Patch #7 is the one from Ard. It eliminates the S3Save() protocol
member call in IntelFrameworkModulePkg/GenericBdsLib.

- After patch #5, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by
IntelFrameworkModulePkg/AcpiS3SaveDxe, so that interface is removed in
patch #8.

- After patches #6 and #7, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by OvmfPkg/AcpiS3SaveDxe.
Therefore that interface is removed in patch #9.

Now here comes the "orthogonal" dimension: the sub-series consisting of
patches #3, #4, #6, #7 and #9 implements Jiewen's idea for OvmfPkg
*only*. It does affect central code in one patch (in Ard's patch #7),
but that central code is actually not used by anything except OvmfPkg,
because Vlv2TbltDevicePkg has its own clone of GenericBdsLib.

So, if patches #1, #2, #5 and #8 look problematic, then I'm completely
fine dropping them. Then IntelFrameworkModulePkg/AcpiS3SaveDxe and
Vlv2TbltDevicePkg won't be touched at all, and Jiewen's idea (and the
kicking of EndOfDxe) will only be implemented for OvmfPkg.

I regression tested OVMF with these changes: S3 continues to work fine
with Fedora and Windows Server 2012 R2, and they also boot okay (and
reject S3 suspend attempts) when S3 is disabled in QEMU.

Public branch:
https://github.com/lersek/edk2/commits/drop_efi_acpi_s3_save_proto

Cc: Yao Jiewen <***@intel.com>
Cc: Jeff Fan <***@intel.com>
Cc: Jordan Justen <***@intel.com>
Cc: Ard Biesheuvel <***@linaro.org>
Cc: David Wei <***@intel.com>
Cc: Tim He <***@intel.com>

Thanks
Laszlo

Ard Biesheuvel (1):
IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call

Laszlo Ersek (8):
IntelFrameworkModulePkg: AcpiS3SaveDxe: prepare for End-of-Dxe
callback
IntelFrameworkModulePkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
OvmfPkg: AcpiS3SaveDxe: prepare for End-of-Dxe callback
OvmfPkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
Vlv2TbltDevicePkg: replace AcpiS3Save->S3Save() with End-of-Dxe signal
OvmfPkg: reorganize (but don't reorder) S3 save state sequence
IntelFrameworkModulePkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
OvmfPkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL

21 files changed, 228 insertions(+), 283 deletions(-)
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c | 103 ++++++-------
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 5 +-
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 151 ++++++--------------
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 9 +-
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c | 86 +++++++++++
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h | 4 +
OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 4 +
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c | 11 --
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c | 53 +++++--
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 2 +-
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
Vlv2TbltDevicePkg/PlatformPkg.dec | 1 -

[Laszlo Ersek]

We are preparing for detaching the S3Ready() functionality from the
EFI_ACPI_S3_SAVE_PROTOCOL.S3Save() protocol member function. Instead, we
will hook the same logic to the End-of-Dxe event group.

The EFI_ACPI_S3_SAVE_PROTOCOL has another member: GetLegacyMemorySize().
According to the documenation,

This function returns the size of the legacy memory (meaning below 1 MB)
that is required during an S3 resume. Before the Framework-based
firmware transfers control to the OS, it has to transition from flat
mode into real mode in case the OS supplies only a real-mode waking
vector. This transition requires a certain amount of legacy memory.
After getting the size of legacy memory below, the caller is responsible
for allocating the legacy memory below 1 MB according to the size that
is returned. The specific implementation of allocating the legacy memory
is out of the scope of this specification.

When EFI_ACPI_S3_SAVE_PROTOCOL.S3Save() is called, the address of the
legacy memory allocated above must be passed to it, in the
LegacyMemoryAddress parameter.

In practice however:

- The S3Ready() function ignores the LegacyMemoryAddress completely.

- No code in the edk2 tree calls
EFI_ACPI_S3_SAVE_PROTOCOL.GetLegacyMemorySize(), ever.

- All callers of EFI_ACPI_S3_SAVE_PROTOCOL.S3Save() in the edk2 tree pass
a NULL LegacyMemoryAddress:

BdsLibBootViaBootOption()
[IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c]

OnReadyToBoot()
[Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c]

InstallReadyToLock()
[Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c]

BdsLibBootViaBootOption()
[Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c]

For this reason, ASSERT() explicitly that LegacyGetS3MemorySize() is never
called, and that the LegacyMemoryAddress parameter is always NULL.

This fact is important to capture in the code, because in the End-of-Dxe
callback, no LegacyMemoryAddress parameter can be taken. So let's make it
clear that we actually don't even have any use for that parameter.

Cc: Yao Jiewen <***@intel.com>
Cc: Jeff Fan <***@intel.com>
Cc: David Wei <***@intel.com>
Cc: Tim He <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
---
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
index 6de1871..ac3e947 100644
--- a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
+++ b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
@@ -2,6 +2,7 @@
This is an implementation of the ACPI S3 Save protocol. This is defined in
S3 boot path specification 0.9.

+Copyright (c) 2015, Red Hat, Inc.<BR>
Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>

This program and the accompanying materials
@@ -422,6 +423,8 @@ LegacyGetS3MemorySize (
OUT UINTN *Size
)
{
+ ASSERT (FALSE);
+
if (Size == NULL) {
return EFI_INVALID_PARAMETER;
}
@@ -469,6 +472,8 @@ S3Ready (
}
AlreadyEntered = TRUE;

+ ASSERT (LegacyMemoryAddress == NULL);
+
AcpiS3Context = AllocateMemoryBelow4G (EfiReservedMemoryType, sizeof(*AcpiS3Context));
ASSERT (AcpiS3Context != NULL);
AcpiS3ContextBuffer = (EFI_PHYSICAL_ADDRESS)(UINTN)AcpiS3Context;

[Laszlo Ersek]

We are preparing for detaching the S3Ready() functionality from the
EFI_ACPI_S3_SAVE_PROTOCOL.S3Save() protocol member function. Instead, we
will hook the same logic to the End-of-Dxe event group.

The EFI_ACPI_S3_SAVE_PROTOCOL has another member: GetLegacyMemorySize().
According to the documenation,

This function returns the size of the legacy memory (meaning below 1 MB)
that is required during an S3 resume. Before the Framework-based
firmware transfers control to the OS, it has to transition from flat
mode into real mode in case the OS supplies only a real-mode waking
vector. This transition requires a certain amount of legacy memory.
After getting the size of legacy memory below, the caller is responsible
for allocating the legacy memory below 1 MB according to the size that
is returned. The specific implementation of allocating the legacy memory
is out of the scope of this specification.

When EFI_ACPI_S3_SAVE_PROTOCOL.S3Save() is called, the address of the
legacy memory allocated above must be passed to it, in the
LegacyMemoryAddress parameter.

In practice however:

- The S3Ready() function ignores the LegacyMemoryAddress completely.

- No code in the edk2 tree calls
EFI_ACPI_S3_SAVE_PROTOCOL.GetLegacyMemorySize(), ever.

- All callers of this specific implementation of
EFI_ACPI_S3_SAVE_PROTOCOL.S3Save() in the edk2 tree pass a NULL
LegacyMemoryAddress:

BdsLibBootViaBootOption()
[IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c]

For this reason, ASSERT() explicitly that LegacyGetS3MemorySize() is never
called, and that the LegacyMemoryAddress parameter is always NULL.

This fact is important to capture in the code, because in the End-of-Dxe
callback, no LegacyMemoryAddress parameter can be taken. So let's make it
clear that we actually don't even have any use for that parameter.

This patch ports the identical change from IntelFrameworkModulePkg to
OvmfPkg.

Cc: Jordan Justen <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
---
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
index f322981..f05764a 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
+++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
@@ -2,6 +2,7 @@
This is an implementation of the ACPI S3 Save protocol. This is defined in
S3 boot path specification 0.9.

+Copyright (c) 2014-2015, Red Hat, Inc.<BR>
Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>

This program and the accompanying materials
@@ -402,6 +403,8 @@ LegacyGetS3MemorySize (
OUT UINTN *Size
)
{
+ ASSERT (FALSE);
+
if (Size == NULL) {
return EFI_INVALID_PARAMETER;
}
@@ -491,6 +494,8 @@ S3Ready (
}
AlreadyEntered = TRUE;

+ ASSERT (LegacyMemoryAddress == NULL);
+
AcpiS3Context = AllocateMemoryBelow4G (EfiReservedMemoryType, sizeof(*AcpiS3Context));
ASSERT (AcpiS3Context != NULL);
AcpiS3ContextBuffer = (EFI_PHYSICAL_ADDRESS)(UINTN)AcpiS3Context;

[Laszlo Ersek]

Call S3Ready() whenever the first of the following occurs:
- a driver signals End-of-Dxe,
- a driver calls EFI_ACPI_S3_SAVE_PROTOCOL.S3Save().

S3Ready() already contains a static, function scope "latch" that causes it
to exit early when called for the second time or later.

(At the moment, the only platform in the edk2 tree that includes this
driver is Vlv2TbltDevicePkg. That platform does not signal End-of-Dxe
(yet).)

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146
Suggested-by: Yao Jiewen <***@intel.com>
Cc: Yao Jiewen <***@intel.com>
Cc: Jeff Fan <***@intel.com>
Cc: David Wei <***@intel.com>
Cc: Tim He <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
---
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 1 +
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c | 46 ++++++++++++++++++++
2 files changed, 47 insertions(+)

diff --git a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
index c5dec05..e5fb92e 100644
--- a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
+++ b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
@@ -60,6 +60,7 @@ [Guids]
## SOMETIMES_CONSUMES ## Variable:L"AcpiGlobalVariable"
## SOMETIMES_PRODUCES ## Variable:L"AcpiGlobalVariable"
gEfiAcpiVariableCompatiblityGuid
+ gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event

[Protocols]
gEfiAcpiS3SaveProtocolGuid ## PRODUCES
diff --git a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
index ac3e947..e0fa866 100644
--- a/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
+++ b/IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c
@@ -28,6 +28,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Guid/AcpiVariableCompatibility.h>
#include <Guid/AcpiS3Context.h>
#include <Guid/Acpi.h>
+#include <Guid/EventGroup.h>
#include <Protocol/AcpiS3Save.h>
#include <IndustryStandard/Acpi.h>

@@ -550,6 +551,40 @@ S3Ready (
}

/**
+ Callback function executed when the EndOfDxe event group is signaled.
+
+ @param[in] Event Event whose notification function is being invoked.
+ @param[in] Context The pointer to the notification function's context, which
+ is implementation-dependent.
+**/
+VOID
+EFIAPI
+OnEndOfDxe (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_STATUS Status;
+
+ //
+ // Our S3Ready() function ignores both of its parameters, and always
+ // succeeds.
+ //
+ Status = S3Ready (
+ NULL, // This
+ NULL // LegacyMemoryAddress
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Close the event, deregistering the callback and freeing resources.
+ //
+ Status = gBS->CloseEvent (Event);
+ ASSERT_EFI_ERROR (Status);
+}
+
+
+/**
The Driver Entry Point.

The function is the driver Entry point which will produce AcpiS3SaveProtocol.
@@ -570,6 +605,7 @@ InstallAcpiS3Save (
)
{
EFI_STATUS Status;
+ EFI_EVENT EndOfDxeEvent;

if (!FeaturePcdGet(PcdPlatformCsmSupport)) {
//
@@ -591,5 +627,15 @@ InstallAcpiS3Save (
&mS3Save
);
ASSERT_EFI_ERROR (Status);
+
+ Status = gBS->CreateEventEx (
+ EVT_NOTIFY_SIGNAL,
+ TPL_CALLBACK,
+ OnEndOfDxe,
+ NULL, /* NotifyContext */
+ &gEfiEndOfDxeEventGroupGuid,
+ &EndOfDxeEvent
+ );
+ ASSERT_EFI_ERROR (Status);
return Status;
}

[Laszlo Ersek]

Call S3Ready() whenever the first of the following occurs:
- a driver signals End-of-Dxe,
- a driver calls EFI_ACPI_S3_SAVE_PROTOCOL.S3Save().

S3Ready() already contains a static, function scope "latch" that causes it
to exit early when called for the second time or later.

(At the moment, the only platform in the edk2 tree that includes this
driver is OvmfPkg. That platform does not signal End-of-Dxe (yet).)

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146
Suggested-by: Yao Jiewen <***@intel.com>
Cc: Jordan Justen <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
---
OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 1 +
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 46 ++++++++++++++++++++
2 files changed, 47 insertions(+)

diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
index ae56a20..81da2fb 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
+++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
@@ -58,6 +58,7 @@ [Guids]
gEfiAcpiS3ContextGuid # ALWAYS_CONSUMED
gEfiAcpi20TableGuid # ALWAYS_CONSUMED System Table
gEfiAcpi10TableGuid # ALWAYS_CONSUMED System Table
+ gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event

[Protocols]
gEfiAcpiS3SaveProtocolGuid # PROTOCOL ALWAYS_PRODUCED
diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
index f05764a..5eb33e0 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
+++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
@@ -29,6 +29,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Guid/AcpiVariableCompatibility.h>
#include <Guid/AcpiS3Context.h>
#include <Guid/Acpi.h>
+#include <Guid/EventGroup.h>
#include <Protocol/AcpiS3Save.h>
#include <Protocol/S3SaveState.h>
#include <Protocol/DxeSmmReadyToLock.h>
@@ -571,6 +572,40 @@ S3Ready (
}

/**
+ Callback function executed when the EndOfDxe event group is signaled.
+
+ @param[in] Event Event whose notification function is being invoked.
+ @param[in] Context The pointer to the notification function's context, which
+ is implementation-dependent.
+**/
+VOID
+EFIAPI
+OnEndOfDxe (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_STATUS Status;
+
+ //
+ // Our S3Ready() function ignores both of its parameters, and always
+ // succeeds.
+ //
+ Status = S3Ready (
+ NULL, // This
+ NULL // LegacyMemoryAddress
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Close the event, deregistering the callback and freeing resources.
+ //
+ Status = gBS->CloseEvent (Event);
+ ASSERT_EFI_ERROR (Status);
+}
+
+
+/**
The Driver Entry Point.

The function is the driver Entry point which will produce AcpiS3SaveProtocol.
@@ -591,6 +626,7 @@ InstallAcpiS3Save (
)
{
EFI_STATUS Status;
+ EFI_EVENT EndOfDxeEvent;

if (!QemuFwCfgS3Enabled()) {
return EFI_LOAD_ERROR;
@@ -612,5 +648,15 @@ InstallAcpiS3Save (
NULL
);
ASSERT_EFI_ERROR (Status);
+
+ Status = gBS->CreateEventEx (
+ EVT_NOTIFY_SIGNAL,
+ TPL_CALLBACK,
+ OnEndOfDxe,
+ NULL, /* NotifyContext */
+ &gEfiEndOfDxeEventGroupGuid,
+ &EndOfDxeEvent
+ );
+ ASSERT_EFI_ERROR (Status);
return Status;
}

[Laszlo Ersek]

Rather than calling AcpiS3Save->S3Save() (in three different places no
less), signal the End-of-Dxe event group in PlatformBdsInit(). With the
modified AcpiS3SaveDxe driver in IntelFrameworkModulePkg, this should have
the same effect.

The event is signaled in PlatformBdsInit() because this way:

- it occurs before InstallReadyToLock() is called from
PlatformBdsPolicyBehavior(), and installs DxeSmmReadyToLock,

- we satisfy the requirement of the PI spec, volume 2, "5.1.2.1 End of DXE
Event":

Prior to invoking any UEFI drivers, applications, or connecting
consoles, the platform should signal the event
EFI_END_OF_DXE_EVENT_GUID.

Of the three preexistent AcpiS3Save->S3Save() calls, at least two must
have been ineffective anyway:

- AcpiPlatform called AcpiS3Save->S3Save() in a ready-to-boot callback,
which was bound to run after PlatformBdsPolicyBehavior() called
InstallReadyToLock(). InstallReadyToLock() would call
AcpiS3Save->S3Save() first.

- Override/IntelFrameworkModulePkg/Library/GenericBdsLib called
AcpiS3Save->S3Save() in BdsLibBootViaBootOption(), which occurred
similarly after the call made by InstallReadyToLock().

Cc: Yao Jiewen <***@intel.com>
Cc: David Wei <***@intel.com>
Cc: Tim He <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
---

Notes:
This patch is completely untested; I can't even compile
Vlv2TbltDevicePkg.

Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 2 +-
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h | 1 -
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c | 11 ----
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c | 53 ++++++++++++++------
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 ----
Vlv2TbltDevicePkg/PlatformPkg.dec | 1 -
9 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf b/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf
index 24fa913..82fdde0 100644
--- a/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf
+++ b/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf
@@ -70,7 +70,6 @@ [Protocols]
gEnhancedSpeedstepProtocolGuid
gEfiPlatformCpuProtocolGuid
gEfiAcpiSupportProtocolGuid
- gEfiAcpiS3SaveProtocolGuid
gEfiCpuIoProtocolGuid
gEfiPs2PolicyProtocolGuid
gEfiFirmwareVolume2ProtocolGuid
diff --git a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf
index 45578e8..363978c 100644
--- a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf
+++ b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf
@@ -82,7 +82,6 @@ [Protocols]
gEfiDxeSmmReadyToLockProtocolGuid
gEfiUserManagerProtocolGuid
gEfiDeferredImageLoadProtocolGuid
- gEfiAcpiS3SaveProtocolGuid
gEfiSpiProtocolGuid ## PROTOCOL CONSUMES
gExitPmAuthProtocolGuid
gEfiTdtOperationProtocolGuid
@@ -97,6 +96,7 @@ [Guids]
gEfiGlobalVariableGuid
gEfiNormalSetupGuid
gEfiPartTypeSystemPartGuid
+ gEfiEndOfDxeEventGroupGuid

[Pcd]
gPlatformModuleTokenSpaceGuid.PcdFlashFvRecovery2Base
diff --git a/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf b/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
index 33ca298..e9240f4 100644
--- a/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
+++ b/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
@@ -120,7 +120,6 @@ [Protocols]
gEfiLegacyBiosProtocolGuid ## SOMETIMES_CONSUMES
gEfiCpuArchProtocolGuid ## CONSUMES
gEfiDevicePathProtocolGuid ## CONSUMES
- gEfiAcpiS3SaveProtocolGuid ## SOMETIMES_CONSUMES
gEfiGraphicsOutputProtocolGuid ## SOMETIMES_CONSUMES
gEfiUgaDrawProtocolGuid |gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## SOMETIMES_CONSUMES
gEfiOEMBadgingProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h
index d757243..572c71d 100644
--- a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h
+++ b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h
@@ -38,7 +38,6 @@ Abstract:
#include <Protocol/DxeSmmReadyToLock.h>
#include <Protocol/UserManager.h>
#include <Protocol/DeferredImageLoad.h>
-#include <Protocol/AcpiS3Save.h>
#include <Protocol/ExitPmAuth.h>
#include <Protocol/MmioDevice.h>
#include <Protocol/I2cBusMcg.h>
diff --git a/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h b/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
index c32579b..7201d8a 100644
--- a/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
+++ b/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
@@ -33,7 +33,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Protocol/SimpleNetwork.h>
#include <Protocol/FirmwareVolume2.h>
#include <Protocol/PciIo.h>
-#include <Protocol/AcpiS3Save.h>
#include <Protocol/OEMBadging.h>
#include <Protocol/GraphicsOutput.h>
#include <Protocol/UgaDraw.h>
diff --git a/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c b/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c
index c39c36d..05104fe 100644
--- a/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c
+++ b/Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c
@@ -49,7 +49,6 @@ Abstract:
#include <Protocol/CpuIo.h>
#include <Guid/BoardFeatures.h>
#include <Protocol/AcpiSupport.h>
-#include <Protocol/AcpiS3Save.h>
#include <Protocol/Ps2Policy.h>
#include <Library/CpuIA32.h>
#include <SetupMode.h>
@@ -642,7 +641,6 @@ OnReadyToBoot (
EFI_STATUS Status;
EFI_ACPI_TABLE_VERSION TableVersion;
EFI_ACPI_SUPPORT_PROTOCOL *AcpiSupport;
- EFI_ACPI_S3_SAVE_PROTOCOL *AcpiS3Save;
SYSTEM_CONFIGURATION SetupVarBuffer;
UINTN VariableSize;
EFI_PLATFORM_CPU_INFO *PlatformCpuInfoPtr = NULL;
@@ -726,15 +724,6 @@ OnReadyToBoot (
TableVersion
);
ASSERT_EFI_ERROR (Status);
-
- //
- // S3 script save.
- //
- Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL, (VOID **) &AcpiS3Save);
- if (!EFI_ERROR (Status)) {
- AcpiS3Save->S3Save (AcpiS3Save, NULL);
- }
-
}

VOID
diff --git a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c
index 195d734..8652d4f 100644
--- a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c
+++ b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c
@@ -1,5 +1,7 @@
/** @file

+ Copyright (c) 2015, Red Hat, Inc.<BR>
+ Copyright (c) 2015, Linaro Ltd.<BR>
Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.<BR>

This program and the accompanying materials are licensed and made available under
@@ -26,6 +28,7 @@ Abstract:
#include "BdsPlatform.h"
#include "SetupMode.h"
#include <Guid/SetupVariable.h>
+#include <Guid/EventGroup.h>
#include <Library/TcgPhysicalPresenceLib.h>
#include <Library/TrEEPhysicalPresenceLib.h>
#include <Protocol/I2cMasterMcg.h>
@@ -148,7 +151,6 @@ InstallReadyToLock (
EFI_STATUS Status;
EFI_HANDLE Handle;
EFI_SMM_ACCESS2_PROTOCOL *SmmAccess;
- EFI_ACPI_S3_SAVE_PROTOCOL *AcpiS3Save;

//
// Install DxeSmmReadyToLock protocol prior to the processing of boot options
@@ -159,19 +161,6 @@ InstallReadyToLock (
(VOID **) &SmmAccess
);
if (!EFI_ERROR (Status)) {
-
- //
- // Prepare S3 information, this MUST be done before DxeSmmReadyToLock
- //
- Status = gBS->LocateProtocol (
- &gEfiAcpiS3SaveProtocolGuid,
- NULL,
- (VOID **)&AcpiS3Save
- );
- if (!EFI_ERROR (Status)) {
- AcpiS3Save->S3Save (AcpiS3Save, NULL);
- }
-
Handle = NULL;
Status = gBS->InstallProtocolInterface (
&Handle,
@@ -205,6 +194,25 @@ ShellImageCallback (
DEBUG ((EFI_D_INFO, "BdsEntry ShellImageCallback \n"));
}

+/**
+ Empty callback function executed when the EndOfDxe event group is signaled.
+
+ We only need this function because we'd like to signal EndOfDxe, and for that
+ we need to create an event, with a callback function.
+
+ @param[in] Event Event whose notification function is being invoked.
+ @param[in] Context The pointer to the notification function's context, which
+ is implementation-dependent.
+**/
+VOID
+EFIAPI
+OnEndOfDxe (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+}
+
//
// BDS Platform Functions
//
@@ -224,6 +232,7 @@ PlatformBdsInit (
)
{
EFI_STATUS Status;
+ EFI_EVENT EndOfDxeEvent;
EFI_EVENT ShellImageEvent;
EFI_GUID ShellEnvProtocol = SHELL_ENVIRONMENT_INTERFACE_PROTOCOL;

@@ -235,6 +244,22 @@ PlatformBdsInit (
BdsLibSaveMemoryTypeInformation ();

//
+ // Signal the EndOfDxe event group.
+ //
+ Status = gBS->CreateEventEx (
+ EVT_NOTIFY_SIGNAL,
+ TPL_CALLBACK,
+ OnEndOfDxe,
+ NULL, /* NotifyContext */
+ &gEfiEndOfDxeEventGroupGuid,
+ &EndOfDxeEvent
+ );
+ if (!EFI_ERROR (Status)) {
+ gBS->SignalEvent (EndOfDxeEvent);
+ gBS->CloseEvent (EndOfDxeEvent);
+ }
+
+ //
// Before user authentication, the user identification devices need be connected
// from the platform customized device paths
//
diff --git a/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c b/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
index 6ada862..0d3e077 100644
--- a/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
+++ b/Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
@@ -2235,7 +2235,6 @@ BdsLibBootViaBootOption (
EFI_DEVICE_PATH_PROTOCOL *FilePath;
EFI_LOADED_IMAGE_PROTOCOL *ImageInfo;
EFI_DEVICE_PATH_PROTOCOL *WorkingDevicePath;
- EFI_ACPI_S3_SAVE_PROTOCOL *AcpiS3Save;
LIST_ENTRY TempBootLists;
EFI_BOOT_LOGO_PROTOCOL *BootLogo;

@@ -2243,15 +2242,6 @@ BdsLibBootViaBootOption (
*ExitData = NULL;

//
- // Notes: this code can be remove after the s3 script table
- // hook on the event EVT_SIGNAL_READY_TO_BOOT or
- // EVT_SIGNAL_LEGACY_BOOT
- //
- Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL, (VOID **) &AcpiS3Save);
- if (!EFI_ERROR (Status)) {
- AcpiS3Save->S3Save (AcpiS3Save, NULL);
- }
- //
// If it's Device Path that starts with a hard drive path, append it with the front part to compose a
// full device path
//
diff --git a/Vlv2TbltDevicePkg/PlatformPkg.dec b/Vlv2TbltDevicePkg/PlatformPkg.dec
index 378cb53..b220239 100644
--- a/Vlv2TbltDevicePkg/PlatformPkg.dec
+++ b/Vlv2TbltDevicePkg/PlatformPkg.dec
@@ -84,7 +84,6 @@ [Protocols]
gEfiPciPlatformProtocolGuid = { 0x07d75280, 0x27d4, 0x4d69, { 0x90, 0xd0, 0x56, 0x43, 0xe2, 0x38, 0xb3, 0x41 } }
gEnhancedSpeedstepProtocolGuid = { 0x91a1ddcf, 0x5374, 0x4939, { 0x89, 0x51, 0xd7, 0x29, 0x3f, 0x1a, 0x78, 0x6f } }
gEfiAcpiSupportProtocolGuid = { 0xdbff9d55, 0x89b7, 0x46da, { 0xbd, 0xdf, 0x67, 0x7d, 0x3d, 0xc0, 0x24, 0x1d } }
- gEfiAcpiS3SaveProtocolGuid = { 0x125f2de1, 0xfb85, 0x440c, { 0xa5, 0x4c, 0x4d, 0x99, 0x35, 0x8a, 0x8d, 0x38 } }
gEfiCpuIoProtocolGuid = { 0xB0732526, 0x38C8, 0x4b40, { 0x88, 0x77, 0x61, 0xC7, 0xB0, 0x6A, 0xAC, 0x45 } }
gPlatformGOPPolicyGuid = { 0xec2e931b, 0x3281, 0x48a5, { 0x81, 0x07, 0xdf, 0x8a, 0x8b, 0xed, 0x3c, 0x5d } }
gEfiGopDisplayBrightnessProtocolGuid = { 0x6ff23f1d, 0x877c, 0x4b1b, { 0x93, 0xfc, 0xf1, 0x42, 0xb2, 0xee, 0xa6, 0xa7 } }

[Laszlo Ersek]

Currently we have the following call chain in OVMF:

BdsLibBootViaBootOption()
[IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c]
//
// calls, via AcpiS3Save->S3Save():
//
S3Ready() [OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c]
//
// 1. saves S3 state, then calls:
//
SaveS3BootScript() [OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c]
//
// 2. saves INFO opcode in S3 boot script
// 3. installs DxeSmmReadyToLockProtocol
//

This call chain was introduced in git commit 5a217a06 (SVN r15305,
"OvmfPkg: S3 Suspend: save boot script after ACPI context"). That patch
was necessary because there was no other way, due to GenericBdsLib calling
S3Save() from BdsLibBootViaBootOption(), to perform the necessary steps in
the right order:
- save S3 system information,
- save a final (well, only) boot script opcode,
- signal DxeSmmReadyToLock, closing the boot script, and locking down
LockBox and SMM.

This GenericBdsLib bug is going to be fixed in the next patch -- the call
in BdsLibBootViaBootOption() will be eliminated. Also, following Yao
Jiewen's idea, AcpiS3SaveDxe now saves S3 system state on End-of-Dxe as
well (which OVMF's BDS doesn't signal yet).

Therefore, reorganize the call tree as follows:

PlatformBdsPolicyBehavior()
[OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c]
//
// signals End-of-Dxe
//
OnEndOfDxe() [OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c]
S3Ready() [OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c]
//
// 1. saves S3 state
//

SaveS3BootScript() [OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c]
//
// 2. saves INFO opcode in S3 boot script
// 3. installs DxeSmmReadyToLockProtocol
//

The installation of DxeSmmReadyToLockProtocol belongs with Platform BDS,
not AcpiS3SaveDxe, and we can now undo the hack in SVN r15305, without
upsetting the relative order of the steps.

Plus, after the patch OVMF's BDS signals End-of-Dxe (in the right place),
which has additional benefits: variable reclaim, and the splitting of the
memory regions that is part of the recently added UEFI 2.5 properties
table feature.

Note that after the patch, both the new and the old calls to S3Ready()
occur. (The old call will be removed in the next patch.) Luckily, that
function has always been protected against second and later calls with a
static variable.

This patch has been regression tested for S3 suspend and resume, with
Fedora and Windows Server 2012 R2 guests.

Cc: Jordan Justen <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
---
OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 4 +-
OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 4 +
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h | 4 +
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 49 -----------
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c | 86 ++++++++++++++++++++
5 files changed, 95 insertions(+), 52 deletions(-)

diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
index 81da2fb..e657bbe 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
+++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
@@ -66,8 +66,6 @@ [Protocols]
gEfiLegacyBiosProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEfiLegacyRegion2ProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
gFrameworkEfiMpServiceProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
- gEfiS3SaveStateProtocolGuid # PROTOCOL ALWAYS_CONSUMED
- gEfiDxeSmmReadyToLockProtocolGuid # PROTOCOL ALWAYS_PRODUCED

[FeaturePcd]
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPlatformCsmSupport ## CONSUMES
@@ -79,4 +77,4 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable

[Depex]
- gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid AND gEfiS3SaveStateProtocolGuid
+ gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
diff --git a/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf b/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
index 5a28d78..f66f51c 100644
--- a/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
+++ b/OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf
@@ -64,4 +64,8 @@ [Pcd.IA32, Pcd.X64]

[Protocols]
gEfiDecompressProtocolGuid
+ gEfiS3SaveStateProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
+ gEfiDxeSmmReadyToLockProtocolGuid # PROTOCOL SOMETIMES_PRODUCED

+[Guids]
+ gEfiEndOfDxeEventGroupGuid
diff --git a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h
index 7006fb3..f451e7e 100644
--- a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h
@@ -47,17 +47,21 @@ Abstract:
#include <Library/DevicePathLib.h>
#include <Library/IoLib.h>
#include <Library/NvVarsFileLib.h>
+#include <Library/QemuFwCfgLib.h>

#include <Protocol/Decompress.h>
#include <Protocol/PciIo.h>
#include <Protocol/FirmwareVolume2.h>
#include <Protocol/SimpleFileSystem.h>
+#include <Protocol/S3SaveState.h>
+#include <Protocol/DxeSmmReadyToLock.h>

#include <Guid/Acpi.h>
#include <Guid/SmBios.h>
#include <Guid/Mps.h>
#include <Guid/HobList.h>
#include <Guid/GlobalVariable.h>
+#include <Guid/EventGroup.h>

#include <OvmfPlatforms.h>

diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
index 5eb33e0..8372db8 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
+++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
@@ -31,8 +31,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Guid/Acpi.h>
#include <Guid/EventGroup.h>
#include <Protocol/AcpiS3Save.h>
-#include <Protocol/S3SaveState.h>
-#include <Protocol/DxeSmmReadyToLock.h>
#include <Protocol/LockBox.h>
#include <IndustryStandard/Acpi.h>

@@ -415,48 +413,6 @@ LegacyGetS3MemorySize (
}

/**
- Save the S3 boot script.
-
- Note that we trigger DxeSmmReadyToLock here -- otherwise the script wouldn't
- be saved actually. Triggering this protocol installation event in turn locks
- down SMM, so no further changes to LockBoxes or SMRAM are possible
- afterwards.
-**/
-STATIC
-VOID
-EFIAPI
-SaveS3BootScript (
- VOID
- )
-{
- EFI_STATUS Status;
- EFI_S3_SAVE_STATE_PROTOCOL *BootScript;
- EFI_HANDLE Handle;
- STATIC CONST UINT8 Info[] = { 0xDE, 0xAD, 0xBE, 0xEF };
-
- Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, NULL,
- (VOID **) &BootScript);
- ASSERT_EFI_ERROR (Status);
-
- //
- // Despite the opcode documentation in the PI spec, the protocol
- // implementation embeds a deep copy of the info in the boot script, rather
- // than storing just a pointer to runtime or NVS storage.
- //
- Status = BootScript->Write(BootScript, EFI_BOOT_SCRIPT_INFORMATION_OPCODE,
- (UINT32) sizeof Info,
- (EFI_PHYSICAL_ADDRESS)(UINTN) &Info);
- ASSERT_EFI_ERROR (Status);
-
- Handle = NULL;
- Status = gBS->InstallProtocolInterface (&Handle,
- &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE,
- NULL);
- ASSERT_EFI_ERROR (Status);
-}
-
-
-/**
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
@@ -563,11 +519,6 @@ S3Ready (
Status = SetLockBoxAttributes (&gEfiAcpiS3ContextGuid, LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE);
ASSERT_EFI_ERROR (Status);

- //
- // Save the boot script too. Note that this requires/includes emitting the
- // DxeSmmReadyToLock event, which in turn locks down SMM.
- //
- SaveS3BootScript ();
return EFI_SUCCESS;
}

diff --git a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
index 1eb2a8b..7440380 100644
--- a/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
@@ -1152,6 +1152,68 @@ Returns:
}


+/**
+ Empty callback function executed when the EndOfDxe event group is signaled.
+
+ We only need this function because we'd like to signal EndOfDxe, and for that
+ we need to create an event, with a callback function.
+
+ @param[in] Event Event whose notification function is being invoked.
+ @param[in] Context The pointer to the notification function's context, which
+ is implementation-dependent.
+**/
+STATIC
+VOID
+EFIAPI
+OnEndOfDxe (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+}
+
+
+/**
+ Save the S3 boot script.
+
+ Note that we trigger DxeSmmReadyToLock here -- otherwise the script wouldn't
+ be saved actually. Triggering this protocol installation event in turn locks
+ down SMM, so no further changes to LockBoxes or SMRAM are possible
+ afterwards.
+**/
+STATIC
+VOID
+SaveS3BootScript (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_S3_SAVE_STATE_PROTOCOL *BootScript;
+ EFI_HANDLE Handle;
+ STATIC CONST UINT8 Info[] = { 0xDE, 0xAD, 0xBE, 0xEF };
+
+ Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, NULL,
+ (VOID **) &BootScript);
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Despite the opcode documentation in the PI spec, the protocol
+ // implementation embeds a deep copy of the info in the boot script, rather
+ // than storing just a pointer to runtime or NVS storage.
+ //
+ Status = BootScript->Write(BootScript, EFI_BOOT_SCRIPT_INFORMATION_OPCODE,
+ (UINT32) sizeof Info,
+ (EFI_PHYSICAL_ADDRESS)(UINTN) &Info);
+ ASSERT_EFI_ERROR (Status);
+
+ Handle = NULL;
+ Status = gBS->InstallProtocolInterface (&Handle,
+ &gEfiDxeSmmReadyToLockProtocolGuid, EFI_NATIVE_INTERFACE,
+ NULL);
+ ASSERT_EFI_ERROR (Status);
+}
+
+
VOID
EFIAPI
PlatformBdsPolicyBehavior (
@@ -1186,11 +1248,35 @@ Returns:
{
EFI_STATUS Status;
EFI_BOOT_MODE BootMode;
+ EFI_EVENT EndOfDxeEvent;

DEBUG ((EFI_D_INFO, "PlatformBdsPolicyBehavior\n"));

ConnectRootBridge ();

+ //
+ // We can't signal End-of-Dxe earlier than this. Namely, End-of-Dxe triggers
+ // the preparation of S3 system information. That logic has a hard dependency
+ // on the presence of the FACS ACPI table. Since our ACPI tables are only
+ // installed after PCI enumeration completes, we must not trigger the S3 save
+ // earlier, hence we can't signal End-of-Dxe earlier.
+ //
+ Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, OnEndOfDxe,
+ NULL /* NotifyContext */, &gEfiEndOfDxeEventGroupGuid,
+ &EndOfDxeEvent);
+ if (!EFI_ERROR (Status)) {
+ gBS->SignalEvent (EndOfDxeEvent);
+ gBS->CloseEvent (EndOfDxeEvent);
+ }
+
+ if (QemuFwCfgS3Enabled ()) {
+ //
+ // Save the boot script too. Note that this requires/includes emitting the
+ // DxeSmmReadyToLock event, which in turn locks down SMM.
+ //
+ SaveS3BootScript ();
+ }
+
if (PcdGetBool (PcdOvmfFlashVariablesEnable)) {
DEBUG ((EFI_D_INFO, "PlatformBdsPolicyBehavior: not restoring NvVars "
"from disk since flash variables appear to be supported.\n"));

[Laszlo Ersek]

From: Ard Biesheuvel <***@linaro.org>

The AcpiS3->S3Save() call needs to occur before the end-of-DXE event
is signalled. The end-of-DXE event needs to be signalled prior to
invoking any UEFI drivers, applications, or connecting consoles.

This means the call to S3Save() that occurs in BdsLibBootViaBootOption()
violates the ordering constraints, and should be removed. Since it is
the responsibility of the platform BDS to signal the end-of-DXE event,
it should also perform the AcpiS3->S3Save() call at an appropriate time.

Commit message update from Laszlo Ersek <***@redhat.com>:

Following the idea of Jiewen Yao
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146>,
the S3 system info will now be collected directly in response to
End-of-Dxe, and AcpiS3->S3Save() calls will be completely eliminated.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
Reviewed-by: Laszlo Ersek <***@redhat.com>
Reviewed-by: Yao, Jiewen <***@intel.com>
[***@redhat.com: updated commit message]
Cc: Ard Biesheuvel <***@linaro.org>
Cc: Yao, Jiewen <***@intel.com>
Signed-off-by: Laszlo Ersek <***@redhat.com>
---
IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 ----------
3 files changed, 12 deletions(-)

diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
index 5381e33..5a138a9 100644
--- a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
+++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
@@ -119,7 +119,6 @@ [Protocols]
gEfiLegacyBiosProtocolGuid ## SOMETIMES_CONSUMES
gEfiCpuArchProtocolGuid ## CONSUMES
gEfiDevicePathProtocolGuid ## CONSUMES
- gEfiAcpiS3SaveProtocolGuid ## SOMETIMES_CONSUMES
gEfiGraphicsOutputProtocolGuid ## SOMETIMES_CONSUMES
gEfiUgaDrawProtocolGuid |gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## SOMETIMES_CONSUMES
gEfiOEMBadgingProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
index c32579b..7201d8a 100644
--- a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
+++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
@@ -33,7 +33,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Protocol/SimpleNetwork.h>
#include <Protocol/FirmwareVolume2.h>
#include <Protocol/PciIo.h>
-#include <Protocol/AcpiS3Save.h>
#include <Protocol/OEMBadging.h>
#include <Protocol/GraphicsOutput.h>
#include <Protocol/UgaDraw.h>
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
index e02a710..4b7eca7 100644
--- a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
+++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
@@ -2233,7 +2233,6 @@ BdsLibBootViaBootOption (
EFI_DEVICE_PATH_PROTOCOL *FilePath;
EFI_LOADED_IMAGE_PROTOCOL *ImageInfo;
EFI_DEVICE_PATH_PROTOCOL *WorkingDevicePath;
- EFI_ACPI_S3_SAVE_PROTOCOL *AcpiS3Save;
LIST_ENTRY TempBootLists;
EFI_BOOT_LOGO_PROTOCOL *BootLogo;

@@ -2241,15 +2240,6 @@ BdsLibBootViaBootOption (
*ExitData = NULL;

//
- // Notes: this code can be remove after the s3 script table
- // hook on the event EVT_SIGNAL_READY_TO_BOOT or
- // EVT_SIGNAL_LEGACY_BOOT
- //
- Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL, (VOID **) &AcpiS3Save);
- if (!EFI_ERROR (Status)) {
- AcpiS3Save->S3Save (AcpiS3Save, NULL);
- }
- //
// If it's Device Path that starts with a hard drive path, append it with the front part to compose a
// full device path
//

[Laszlo Ersek]

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 <***@intel.com>
Cc: Jeff Fan <***@intel.com>
Cc: David Wei <***@intel.com>
Cc: Tim He <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@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,

[Yao, Jiewen]

Reviewed by: Yao, Jiewen <***@intel.com>

-----Original Message-----
From: Laszlo Ersek [mailto:***@redhat.com]
Sent: Saturday, June 27, 2015 9:08 AM
To: edk2-***@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 <***@intel.com>
Cc: Jeff Fan <***@intel.com>
Cc: David Wei <***@intel.com>
Cc: Tim He <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@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

[Tian, Hot]

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.

Thanks,
Hot

-----Original Message-----
From: Yao, Jiewen [mailto:***@intel.com]
Sent: Saturday, June 27, 2015 19:56
To: Laszlo Ersek; edk2-***@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 <***@intel.com>

-----Original Message-----
From: Laszlo Ersek [mailto:***@redhat.com]
Sent: Saturday, June 27, 2015 9:08 AM
To: edk2-***@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 <***@intel.com>
Cc: Jeff Fan <***@intel.com>
Cc: David Wei <***@intel.com>
Cc: Tim He <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@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-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[Laszlo Ersek]

Post by Tian, Hot
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.

Post by Tian, Hot
Thanks,
Hot
-----Original Message-----
Sent: Saturday, June 27, 2015 19:56
Cc: He, Tim; Fan, Jeff
Subject: Re: [edk2] [PATCH 8/9] IntelFrameworkModulePkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
-----Original Message-----
Sent: Saturday, June 27, 2015 9:08 AM
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.
Contributed-under: TianoCore Contribution Agreement 1.0
---
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 @@
-# AcpiS3Save module installs ACPI S3 Save protocol to prepare S3 boot data.
+# AcpiS3Save module installs EndOfDxe callback to prepare S3 boot data.
#
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.
-
-
-
-**/
-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
-
@retval EFI_SUCCESS All information was saved successfully.
-
**/
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 = {
/**
- 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.
-
-
-
-**/
-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
-
@retval EFI_SUCCESS All information was saved successfully.
-
**/
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[Laszlo Ersek]

At this point, nothing in the OVMF build calls EFI_ACPI_S3_SAVE_PROTOCOL
member functions; simplify the code by dropping this protocol interface.

Cc: Jordan Justen <***@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <***@redhat.com>
---
OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 4 +-
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h | 28 +-------
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 71 ++------------------
3 files changed, 6 insertions(+), 97 deletions(-)

diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
index e657bbe..a503327 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
+++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
@@ -1,5 +1,5 @@
## @file
-# Component description file for AcpiS3Save module.
+# AcpiS3Save module installs EndOfDxe callback to prepare S3 boot data.
#
# This is an implementation of the ACPI S3 Save protocol.
# Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>
@@ -61,14 +61,12 @@ [Guids]
gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event

[Protocols]
- gEfiAcpiS3SaveProtocolGuid # PROTOCOL ALWAYS_PRODUCED
gEfiLockBoxProtocolGuid # PROTOCOL ALWAYS_PRODUCED
gEfiLegacyBiosProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEfiLegacyRegion2ProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
gFrameworkEfiMpServiceProtocolGuid # PROTOCOL SOMETIMES_CONSUMED

[FeaturePcd]
- gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPlatformCsmSupport ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode

[Pcd]
diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h
index 65974a3..6d8a9bb 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h
+++ b/OvmfPkg/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/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
index 8372db8..659acae 100644
--- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
+++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
@@ -30,19 +30,11 @@ 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 <Protocol/LockBox.h>
#include <IndustryStandard/Acpi.h>

#include "AcpiS3Save.h"

-UINTN mLegacyRegionSize;
-
-EFI_ACPI_S3_SAVE_PROTOCOL mS3Save = {
- LegacyGetS3MemorySize,
- S3Ready,
-};
-
EFI_GUID mAcpiS3IdtrProfileGuid = {
0xdea652b0, 0xd587, 0x4c54, { 0xb5, 0xb4, 0xc6, 0x82, 0xe7, 0xa0, 0xaa, 0x3d }
};
@@ -385,52 +377,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;
@@ -442,17 +398,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;
@@ -539,13 +490,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);

//
@@ -583,18 +530,8 @@ InstallAcpiS3Save (
return EFI_LOAD_ERROR;
}

- if (!FeaturePcdGet(PcdPlatformCsmSupport)) {
- //
- // More memory for no CSM tip, because GDT need relocation
- //
- mLegacyRegionSize = 0x250;
- } else {
- mLegacyRegionSize = 0x100;
- }
-
Status = gBS->InstallMultipleProtocolInterfaces (
&ImageHandle,
- &gEfiAcpiS3SaveProtocolGuid, &mS3Save,
&gEfiLockBoxProtocolGuid, NULL,
NULL
);

[Jordan Justen]

Patches 1-4 & 7-9: Reviewed-by: Jordan Justen <***@intel.com>

For Patches 5 & 6, I would like to ask if the 'end-of-dxe' signal
could be added as separate patches. (Inserted before 5/6, I assume.)
Would that be possible, or would the S3 interdependencies prevent it?
It seems like that is important enough of a platform change that it
should stand on its own, if possible.

It would also be nice if MdePkg had a PiLib so we could add a EndOfDxe
functions, similar to EfiSignalEventReadyToBoot and
EfiCreateEventReadyToBootEx.

-Jordan

Post by Laszlo Ersek
In
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146>,

And in the future, I believe we could remove EFI_ACPI_S3_SAVE_PROTOCOL
protocol completely. (This is Framework 0.9 defined API, not PI
defined API)
1) AcpiS3Save does not install EFI_ACPI_S3_SAVE_PROTOCOL.
2) AcpiS3Save registers EndOfDxe callback function.
3) AcpiS3Save does S3Save() in EndOfDxe callback function.
So everything could be self-contained. There is no need to rely on
platform BDS.
Then this module eliminates IntelFrameworkPkg dependency and can be
moved to MdeModulePkg later.

This series implements the idea, and "on the side", it changes OVMF to
kick the EndOfDxe event group (which is where the entire discussion has
come from -- Ard started it because ArmVirtPkg lacked EndOfDxe).
The series is organized carefully. It is bisectable, plus it has
- Patches #1 and #2 make IntelFrameworkModulePkg/AcpiS3SaveDxe act upon
EndOfDxe *in addition to* the usual S3Save() protocol member call.
- Patches #3 and #4 do the same to OvmfPkg's clone of the driver.
(Before anyone asks: yes, that clone needs to remain separate.)
- Patch #5 modifies Vlv2TbltDevicePkg. That package is the only one
in the edk2 tree that uses IntelFrameworkModulePkg/AcpiS3SaveDxe.
Because I'm implementing Jiewen's idea in
IntelFrameworkModulePkg/AcpiS3SaveDxe, I must migrate the driver's
clients, and Vlv2TbltDevicePkg is one client. (The only one.)
So patch #5 flips Vlv2TbltDevicePkg from calling the S3Save() protocol
member to kicking the EndOfDxe event group. Unfortunately, I couldn't
even *compile* this package, let alone test it. Thus, I need help with
this patch.
Importantly, because Vlv2TbltDevicePkg has its own clone of
GenericBdsLib, Ard's patch (patch #7) below does *not* affect it.
Therefore the Vlv2TbltDevicePkg changes are completely isolated to
patch #5.
- Patch #6 switches over OvmfPkg from depending on the S3Save() protocol
member call (inside IntelFrameworkModulePkg/GenericBdsLib) to kicking
EndOfDxe explicitly.
- Patch #7 is the one from Ard. It eliminates the S3Save() protocol
member call in IntelFrameworkModulePkg/GenericBdsLib.
- After patch #5, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by
IntelFrameworkModulePkg/AcpiS3SaveDxe, so that interface is removed in
patch #8.
- After patches #6 and #7, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by OvmfPkg/AcpiS3SaveDxe.
Therefore that interface is removed in patch #9.
Now here comes the "orthogonal" dimension: the sub-series consisting of
patches #3, #4, #6, #7 and #9 implements Jiewen's idea for OvmfPkg
*only*. It does affect central code in one patch (in Ard's patch #7),
but that central code is actually not used by anything except OvmfPkg,
because Vlv2TbltDevicePkg has its own clone of GenericBdsLib.
So, if patches #1, #2, #5 and #8 look problematic, then I'm completely
fine dropping them. Then IntelFrameworkModulePkg/AcpiS3SaveDxe and
Vlv2TbltDevicePkg won't be touched at all, and Jiewen's idea (and the
kicking of EndOfDxe) will only be implemented for OvmfPkg.
I regression tested OVMF with these changes: S3 continues to work fine
with Fedora and Windows Server 2012 R2, and they also boot okay (and
reject S3 suspend attempts) when S3 is disabled in QEMU.
https://github.com/lersek/edk2/commits/drop_efi_acpi_s3_save_proto
Thanks
Laszlo
IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call
IntelFrameworkModulePkg: AcpiS3SaveDxe: prepare for End-of-Dxe
callback
IntelFrameworkModulePkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
OvmfPkg: AcpiS3SaveDxe: prepare for End-of-Dxe callback
OvmfPkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
Vlv2TbltDevicePkg: replace AcpiS3Save->S3Save() with End-of-Dxe signal
OvmfPkg: reorganize (but don't reorder) S3 save state sequence
IntelFrameworkModulePkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
OvmfPkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
21 files changed, 228 insertions(+), 283 deletions(-)
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c | 103 ++++++-------
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 5 +-
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 151 ++++++--------------
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 9 +-
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c | 86 +++++++++++
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h | 4 +
OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 4 +
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c | 11 --
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c | 53 +++++--
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 2 +-
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
Vlv2TbltDevicePkg/PlatformPkg.dec | 1 -
--
1.8.3.1

[Laszlo Ersek]

Thank you.

Post by Jordan Justen
For Patches 5 & 6, I would like to ask if the 'end-of-dxe' signal
could be added as separate patches. (Inserted before 5/6, I assume.)
Would that be possible, or would the S3 interdependencies prevent it?
It seems like that is important enough of a platform change that it
should stand on its own, if possible.

Good idea.

For OvmfPkg, I'll only have to split patch #6 in two: the first half
would only signal EndOfDxe, and the code movement from AcpiS3SaveDxe to
PlatformBdsLib would be the separate second part. Considering the order
of steps only, it doesn't matter if SaveS3BootScript() is called by
S3Ready() as a final step when PlatformBDS signals EndOfDxe, versus
PlatformBDS calls SaveS3BootScript() directly, after the EndOfDxe
handlers, including S3Ready(), complete.

I think I can split up patch #5 similarly:

- Signal EndOfDxe first. Three of the AcpiS3Save->S3Save() calls in
Vlv2TbltDevicePkg seem to be ineffective anyway. This first patch
would just make the one call in InstallReadyToLock() ineffective too.

- Remove the AcpiS3Save->S3Save() calls separately.

So I think both patches in question can be split up "purely textually",
ie. without having to introduce any logical detours.

Post by Jordan Justen
It would also be nice if MdePkg had a PiLib so we could add a EndOfDxe
functions, similar to EfiSignalEventReadyToBoot and
EfiCreateEventReadyToBootEx.

This is a good idea, and you must know in advance why I hate it
nonetheless. Writing this library, with one function in it, probably
takes (tens of) minutes. However, getting the naming right, and finding
time for Mike when he can review and accept such a patch, would delay
this thing for weeks again. Oh and let's not forget the time it would
take for everyone to agree on the interface of the library.

If you'd like to implement this library, please go ahead, but please do
it separately from this work. Once "signal EndOfDxe" is available as a
separate library function, I promise I'll rebase OvmfPkg, ArmVirtPkg,
and Vlv2TbltDevicePkg on top.

Thanks
Laszlo

Post by Jordan Justen
-Jordan

Post by Laszlo Ersek
In
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146>,

And in the future, I believe we could remove EFI_ACPI_S3_SAVE_PROTOCOL
protocol completely. (This is Framework 0.9 defined API, not PI
defined API)
1) AcpiS3Save does not install EFI_ACPI_S3_SAVE_PROTOCOL.
2) AcpiS3Save registers EndOfDxe callback function.
3) AcpiS3Save does S3Save() in EndOfDxe callback function.
So everything could be self-contained. There is no need to rely on
platform BDS.
Then this module eliminates IntelFrameworkPkg dependency and can be
moved to MdeModulePkg later.

This series implements the idea, and "on the side", it changes OVMF to
kick the EndOfDxe event group (which is where the entire discussion has
come from -- Ard started it because ArmVirtPkg lacked EndOfDxe).
The series is organized carefully. It is bisectable, plus it has
- Patches #1 and #2 make IntelFrameworkModulePkg/AcpiS3SaveDxe act upon
EndOfDxe *in addition to* the usual S3Save() protocol member call.
- Patches #3 and #4 do the same to OvmfPkg's clone of the driver.
(Before anyone asks: yes, that clone needs to remain separate.)
- Patch #5 modifies Vlv2TbltDevicePkg. That package is the only one
in the edk2 tree that uses IntelFrameworkModulePkg/AcpiS3SaveDxe.
Because I'm implementing Jiewen's idea in
IntelFrameworkModulePkg/AcpiS3SaveDxe, I must migrate the driver's
clients, and Vlv2TbltDevicePkg is one client. (The only one.)
So patch #5 flips Vlv2TbltDevicePkg from calling the S3Save() protocol
member to kicking the EndOfDxe event group. Unfortunately, I couldn't
even *compile* this package, let alone test it. Thus, I need help with
this patch.
Importantly, because Vlv2TbltDevicePkg has its own clone of
GenericBdsLib, Ard's patch (patch #7) below does *not* affect it.
Therefore the Vlv2TbltDevicePkg changes are completely isolated to
patch #5.
- Patch #6 switches over OvmfPkg from depending on the S3Save() protocol
member call (inside IntelFrameworkModulePkg/GenericBdsLib) to kicking
EndOfDxe explicitly.
- Patch #7 is the one from Ard. It eliminates the S3Save() protocol
member call in IntelFrameworkModulePkg/GenericBdsLib.
- After patch #5, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by
IntelFrameworkModulePkg/AcpiS3SaveDxe, so that interface is removed in
patch #8.
- After patches #6 and #7, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by OvmfPkg/AcpiS3SaveDxe.
Therefore that interface is removed in patch #9.
Now here comes the "orthogonal" dimension: the sub-series consisting of
patches #3, #4, #6, #7 and #9 implements Jiewen's idea for OvmfPkg
*only*. It does affect central code in one patch (in Ard's patch #7),
but that central code is actually not used by anything except OvmfPkg,
because Vlv2TbltDevicePkg has its own clone of GenericBdsLib.
So, if patches #1, #2, #5 and #8 look problematic, then I'm completely
fine dropping them. Then IntelFrameworkModulePkg/AcpiS3SaveDxe and
Vlv2TbltDevicePkg won't be touched at all, and Jiewen's idea (and the
kicking of EndOfDxe) will only be implemented for OvmfPkg.
I regression tested OVMF with these changes: S3 continues to work fine
with Fedora and Windows Server 2012 R2, and they also boot okay (and
reject S3 suspend attempts) when S3 is disabled in QEMU.
https://github.com/lersek/edk2/commits/drop_efi_acpi_s3_save_proto
Thanks
Laszlo
IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call
IntelFrameworkModulePkg: AcpiS3SaveDxe: prepare for End-of-Dxe
callback
IntelFrameworkModulePkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
OvmfPkg: AcpiS3SaveDxe: prepare for End-of-Dxe callback
OvmfPkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
Vlv2TbltDevicePkg: replace AcpiS3Save->S3Save() with End-of-Dxe signal
OvmfPkg: reorganize (but don't reorder) S3 save state sequence
IntelFrameworkModulePkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
OvmfPkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
21 files changed, 228 insertions(+), 283 deletions(-)
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c | 103 ++++++-------
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 5 +-
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 151 ++++++--------------
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 9 +-
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c | 86 +++++++++++
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h | 4 +
OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 4 +
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c | 11 --
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c | 53 +++++--
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 2 +-
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
Vlv2TbltDevicePkg/PlatformPkg.dec | 1 -
--
1.8.3.1

[Laszlo Ersek]

All,

Post by Laszlo Ersek
In
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16146>,

And in the future, I believe we could remove EFI_ACPI_S3_SAVE_PROTOCOL
protocol completely. (This is Framework 0.9 defined API, not PI
defined API)
1) AcpiS3Save does not install EFI_ACPI_S3_SAVE_PROTOCOL.
2) AcpiS3Save registers EndOfDxe callback function.
3) AcpiS3Save does S3Save() in EndOfDxe callback function.
So everything could be self-contained. There is no need to rely on
platform BDS.
Then this module eliminates IntelFrameworkPkg dependency and can be
moved to MdeModulePkg later.

This series implements the idea, and "on the side", it changes OVMF to
kick the EndOfDxe event group (which is where the entire discussion has
come from -- Ard started it because ArmVirtPkg lacked EndOfDxe).
The series is organized carefully. It is bisectable, plus it has
- Patches #1 and #2 make IntelFrameworkModulePkg/AcpiS3SaveDxe act upon
EndOfDxe *in addition to* the usual S3Save() protocol member call.
- Patches #3 and #4 do the same to OvmfPkg's clone of the driver.
(Before anyone asks: yes, that clone needs to remain separate.)
- Patch #5 modifies Vlv2TbltDevicePkg. That package is the only one
in the edk2 tree that uses IntelFrameworkModulePkg/AcpiS3SaveDxe.
Because I'm implementing Jiewen's idea in
IntelFrameworkModulePkg/AcpiS3SaveDxe, I must migrate the driver's
clients, and Vlv2TbltDevicePkg is one client. (The only one.)
So patch #5 flips Vlv2TbltDevicePkg from calling the S3Save() protocol
member to kicking the EndOfDxe event group. Unfortunately, I couldn't
even *compile* this package, let alone test it. Thus, I need help with
this patch.
Importantly, because Vlv2TbltDevicePkg has its own clone of
GenericBdsLib, Ard's patch (patch #7) below does *not* affect it.
Therefore the Vlv2TbltDevicePkg changes are completely isolated to
patch #5.
- Patch #6 switches over OvmfPkg from depending on the S3Save() protocol
member call (inside IntelFrameworkModulePkg/GenericBdsLib) to kicking
EndOfDxe explicitly.
- Patch #7 is the one from Ard. It eliminates the S3Save() protocol
member call in IntelFrameworkModulePkg/GenericBdsLib.
- After patch #5, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by
IntelFrameworkModulePkg/AcpiS3SaveDxe, so that interface is removed in
patch #8.
- After patches #6 and #7, nothing in the edk2 tree depends on the
EFI_ACPI_S3_SAVE_PROTOCOL interface exported by OvmfPkg/AcpiS3SaveDxe.
Therefore that interface is removed in patch #9.
Now here comes the "orthogonal" dimension: the sub-series consisting of
patches #3, #4, #6, #7 and #9 implements Jiewen's idea for OvmfPkg
*only*. It does affect central code in one patch (in Ard's patch #7),
but that central code is actually not used by anything except OvmfPkg,
because Vlv2TbltDevicePkg has its own clone of GenericBdsLib.
So, if patches #1, #2, #5 and #8 look problematic, then I'm completely
fine dropping them. Then IntelFrameworkModulePkg/AcpiS3SaveDxe and
Vlv2TbltDevicePkg won't be touched at all, and Jiewen's idea (and the
kicking of EndOfDxe) will only be implemented for OvmfPkg.

I have not received any kind of feedback for the Vlv2TbltDevicePkg
patches (in nine days).

Since I would not commit Vlv2TbltDevicePkg patches without maintainer
R-b tags, I'm going to drop all patches from the v2 series that affect
Vlv2TbltDevicePkg. (This is technically feasible: please see my earlier
description above.) The change will be limited to OvmfPkg and
IntelFrameworkModulePkg/Library/GenericBdsLib exclusively. (And even the
latter does not affect Vlv2TbltDevicePkg.)

If you disagree (ie. you'd like me to keep the Vlv2TbltDevicePkg
patches, modified or unmodified), then please speak up soon.

Personally I'm completely fine dropping those patches, hence my plan for v2.

Thanks
Laszlo

Post by Laszlo Ersek
I regression tested OVMF with these changes: S3 continues to work fine
with Fedora and Windows Server 2012 R2, and they also boot okay (and
reject S3 suspend attempts) when S3 is disabled in QEMU.
https://github.com/lersek/edk2/commits/drop_efi_acpi_s3_save_proto
Thanks
Laszlo
IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call
IntelFrameworkModulePkg: AcpiS3SaveDxe: prepare for End-of-Dxe
callback
IntelFrameworkModulePkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
OvmfPkg: AcpiS3SaveDxe: prepare for End-of-Dxe callback
OvmfPkg: AcpiS3SaveDxe: call S3Ready() at End-of-Dxe
Vlv2TbltDevicePkg: replace AcpiS3Save->S3Save() with End-of-Dxe signal
OvmfPkg: reorganize (but don't reorder) S3 save state sequence
IntelFrameworkModulePkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
OvmfPkg: AcpiS3SaveDxe: drop EFI_ACPI_S3_SAVE_PROTOCOL
21 files changed, 228 insertions(+), 283 deletions(-)
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c | 103 ++++++-------
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 5 +-
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c | 151 ++++++--------------
OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.h | 28 +---
OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf | 9 +-
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c | 86 +++++++++++
OvmfPkg/Library/PlatformBdsLib/BdsPlatform.h | 4 +
OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 4 +
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c | 11 --
Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.inf | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c | 53 +++++--
Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h | 1 -
Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf | 2 +-
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 --
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf | 1 -
Vlv2TbltDevicePkg/Override/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
Vlv2TbltDevicePkg/PlatformPkg.dec | 1 -