On 06/25/15 12:34, Laszlo Ersek wrote:
> On 06/25/15 11:22, Ard Biesheuvel wrote:
>> 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 <ard.biesheu...@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>
>>
>
> 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 <ler...@redhat.com>
------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors
network devices and physical & virtual servers, alerts via email & sms
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
