[edk2] [PATCH] IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call

Discussion:

Ard Biesheuvel

2015-06-25 09:22:20 UTC

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.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 ----------
.../Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
3 files changed, 12 deletions(-)

diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
index e02a71015edc..4b7eca7bbd34 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
//
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
index 5381e331ff8c..5a138a9169b3 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 c32579bfc577..7201d8a33527 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>

--
1.9.1

Laszlo Ersek

2015-06-25 10:34:48 UTC

Permalink

Post by Ard Biesheuvel
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.
Contributed-under: TianoCore Contribution Agreement 1.0
---
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 ----------
.../Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
3 files changed, 12 deletions(-)
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
index e02a71015edc..4b7eca7bbd34 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
//
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
index 5381e331ff8c..5a138a9169b3 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 c32579bfc577..7201d8a33527 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>

I don't know enough to review this patch (beyond the suspicion that it
is correct, in theory). If it is accepted though, then it will break
OVMF, and we will have to fix OVMF then.

(If this patch is deemed correct by the IntelFrameworkModulePkg
maintainers, then adopting OVMF is the right thing to do.)

Thanks!
Laszlo

Laszlo Ersek

2015-06-25 10:37:01 UTC

Permalink

Post by Laszlo Ersek
(If this patch is deemed correct by the IntelFrameworkModulePkg
maintainers, then adopting OVMF is the right thing to do.)

Adapting, not adopting.

Laszlo Ersek

2015-06-25 10:43:14 UTC

Permalink

Post by Laszlo Ersek

I don't know enough to review this patch (beyond the suspicion that it
is correct, in theory). If it is accepted though, then it will break
OVMF, and we will have to fix OVMF then.
(If this patch is deemed correct by the IntelFrameworkModulePkg
maintainers, then adopting OVMF is the right thing to do.)

Sigh, let me try again. (Beyond the lame typo I corrected in the other
email.)

I don't know enough about all the client code (possibly proprietary
client code) that relies on this call being where it is. For OVMF, it
would cause breakage, but the code in OVMF that would break is there
anyway just for compatibility with this Intel BDS bug. So fixing up OVMF
in parallel, or before, or after this patch, would be the right thing to
do, if this patch were accepted.

So, as far as I'm concerned, and as far as I can see the impact of this
patch, it *does* look correct; after all it was me who suggested
something like this. And the commit message describes the ordering
accurately.

Reviewed-by: Laszlo Ersek <***@redhat.com>

Yao, Jiewen

2015-06-25 12:35:11 UTC

Permalink

Looks good. Thanks!

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

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Thursday, June 25, 2015 5:22 PM
To: edk2-***@lists.sourceforge.net; ***@redhat.com; Yao, Jiewen; Fan, Jeff
Cc: Ard Biesheuvel
Subject: [PATCH] IntelFrameworkModulePkg/GenericBdsLib: remove AcpiS3->S3Save() call

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.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c | 10 ----------
.../Library/GenericBdsLib/GenericBdsLib.inf | 1 -
IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h | 1 -
3 files changed, 12 deletions(-)

diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
index e02a71015edc..4b7eca7bbd34 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
//
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
index 5381e331ff8c..5a138a9169b3 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 c32579bfc577..7201d8a33527 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>
--
1.9.1

Laszlo Ersek

2015-06-26 21:52:44 UTC

Permalink

Jeff,

please do not commit this patch. As discussed earlier, it would break
OVMF's S3.

I'm working on a series, and I'll include this patch from Ard in it
(with the Reviewed-by tags that it has collected), in the right position.

Thanks!
Laszlo

Ard Biesheuvel

2015-06-26 22:26:00 UTC

Permalink

Post by Laszlo Ersek
Jeff,

please do not commit this patch. As discussed earlier, it would break
OVMF's S3.
I'm working on a series, and I'll include this patch from Ard in it
(with the Reviewed-by tags that it has collected), in the right position.

I agree. There is no urgency to get this patch merged, as far as I am
concerned, so I am totally fine with waiting for Laszlo to repost it
once OVMF is ready for it.

Thanks,
Ard.