[edk2,RFC] IntelFrameworkModulePkg: signal end-of-DXE event upon platform BDS invocation

Message ID 1435175294-21718-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel June 24, 2015, 7:48 p.m.
According to paragraph 5.1.2.1 of the PI spec vol 2 version 1.4, the
EFI_END_OF_DXE_EVENT_GUID event must be signalled prior to invoking any
UEFI drivers, applications, or connecting consoles.

This implements the event signalling in a new GenericBdsLib function
BdsLibSignalEndOfDxeEvent (), and adds the invocation to BdsEntry () right
before the point where it hands over to PlatformBdsInit().

In order to retain the mandated ordering between saving the ACPI S3 script
and signalling the EFI_END_OF_DXE_EVENT_GUID event, saving of the S3 script
is moved to BdsLibSignalEndOfDxeEvent () as well, right before the event
is signalled.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

I am having trouble wrapping my head around all the interdependencies, but I do
think that this may be a feasible approach.

 IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h         | 14 +++
 IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c         | 70 ++++++++++--
 IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf |  1 +
 IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h  |  1 +
 IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c             |  3 +
 5 files changed, 79 insertions(+), 10 deletions(-)

Comments

Ard Biesheuvel June 25, 2015, 7:50 a.m. | #1
On 25 June 2015 at 01:31, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Hi Ard
> You are right that END_OF_DXE must be signal in Bds.
>
> My thought is that this event should be signal in "platform BDS" instead of "generic BDS".
>
> The reason is that only platform know when to exit platform manufacture auth state, and platform may have some special step before that.
>

That is true.

> Signing end of dxe in generic BDS may break existing platform.
>
> Would you mind to let me know which platform forget to signal this, and can we fix it in platform BDS?
>

There are two constraints for the moment where end-of-DXE is signalled:
- according to the PI spec

"""
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.
"""

which means PlatformBdsInit() would be a reasonable candidate
location, provided that it gets documented in the PlatformBdsLib API
that the even should be signalled then.

However, there is another constraint, according to
https://firmware.intel.com/sites/default/files/A_Tour_Beyond_BIOS_Implementing_S3_resume_with_EDKII.pdf

"""
PlatformBDS need 1) call AcpiS3->S3Save() to save S3 system
information, then 2) signal EndOfDxe to give DXE silicon driver last
chance to save boot script, and finally 3) signal 17 SmmReadyToLock to
close boot script and save boot script to LockBox. These calls must be
in this order, or the boot script might not be able to save correctly.
"""

The problem is (as Laszlo has pointed out) that the generic BDS calls
AcpiS3->S3Save () in BdsLibBootViaBootOption (), which is problematic
for two reasons:
- it is long after PlatformBdsInit(), so signalling end-of-DXE there
will occur before S3Save
- BdsLibBootViaBootOption () may never return control to the Platform
BDS, so there is no way the Platform BDS can ensure that end-of-DXE
will be signalled after the existing S3Save()

In summary, the S3Save() call needs to be moved if we want to adhere
to both constraints above.
Ard Biesheuvel June 25, 2015, 8:08 a.m. | #2
On 25 June 2015 at 09:58, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/25/15 01:31, Yao, Jiewen wrote:
>> Hi Ard
>> You are right that END_OF_DXE must be signal in Bds.
>>
>> My thought is that this event should be signal in "platform BDS"
>> instead of "generic BDS".
>>
>> The reason is that only platform know when to exit platform
>> manufacture auth state, and platform may have some special step
>> before that.
>>
>> Signing end of dxe in generic BDS may break existing platform.
>
> Ard, I think we have the justification we needed for going with
> platform-specific End-of-Dxe signalling. Please update your original
> patch with a link to Jiewen's message:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109
>
> And I'm going to ACK it like that for ArmVirtPkg.
>

OK, fair enough.

> (
> Note, this is actually option #2 from my email
> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16095>:
>
>> - Or, OVMF will have to signal End-of-Dxe within the
>>   SaveS3BootScript() function (see again the patch), right before
>>   installing "gEfiDxeSmmReadyToLockProtocolGuid". Only that would
>>   place the new step (2) between existent steps (1) and (3).
>>
>>   In parallel, ArmVirtPkg, which doesn't implement S3 or SMM, could
>>   signal End-of-Dxe wherever it wanted to. Meaning, your patch is
>>   correct (assuming it doesn't break anything in practice), *if* we
>>   prefer this option.
> )
>
> Jiewen,
>
>> Would you mind to let me know which platform forget to signal this,
>
> That's every single platform in the open source edk2 tree that uses
> Intel BDS.
>
>> and can we fix it in platform BDS?
>
> Yes, we can do that; Ard already posted a patch with such an approach
> for ArmVirtPkg, and I intend to do the same for OvmfPkg. Our main doubt
> was whether it was *right* (by design) to do this separately for each
> platform (in the respective PlatformBdsLib instance), as opposed to
> doing it centrally in the BDS driver.
>
> You just lifted that doubt, so we know the way forward.
>

Indeed. FYI, I noticed that S3Save () does the right thing if it is
invoked more than once.
That means you may be able to get away with calling it both in
PlatformBdsInit() (before end-of-DXE) *and* keep the original call
site in GenericBdsLib intact, and still end up with something that
works. It does rely on an implementation detail of AcpiS3Save though
...
Ard Biesheuvel June 25, 2015, 8:27 a.m. | #3
On 25 June 2015 at 10:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 25 June 2015 at 09:58, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/25/15 01:31, Yao, Jiewen wrote:
>>> Hi Ard
>>> You are right that END_OF_DXE must be signal in Bds.
>>>
>>> My thought is that this event should be signal in "platform BDS"
>>> instead of "generic BDS".
>>>
>>> The reason is that only platform know when to exit platform
>>> manufacture auth state, and platform may have some special step
>>> before that.
>>>
>>> Signing end of dxe in generic BDS may break existing platform.
>>
>> Ard, I think we have the justification we needed for going with
>> platform-specific End-of-Dxe signalling. Please update your original
>> patch with a link to Jiewen's message:
>>
>> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109
>>
>> And I'm going to ACK it like that for ArmVirtPkg.
>>
>
> OK, fair enough.
>
>> (
>> Note, this is actually option #2 from my email
>> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16095>:
>>
>>> - Or, OVMF will have to signal End-of-Dxe within the
>>>   SaveS3BootScript() function (see again the patch), right before
>>>   installing "gEfiDxeSmmReadyToLockProtocolGuid". Only that would
>>>   place the new step (2) between existent steps (1) and (3).
>>>
>>>   In parallel, ArmVirtPkg, which doesn't implement S3 or SMM, could
>>>   signal End-of-Dxe wherever it wanted to. Meaning, your patch is
>>>   correct (assuming it doesn't break anything in practice), *if* we
>>>   prefer this option.
>> )
>>
>> Jiewen,
>>
>>> Would you mind to let me know which platform forget to signal this,
>>
>> That's every single platform in the open source edk2 tree that uses
>> Intel BDS.
>>
>>> and can we fix it in platform BDS?
>>
>> Yes, we can do that; Ard already posted a patch with such an approach
>> for ArmVirtPkg, and I intend to do the same for OvmfPkg. Our main doubt
>> was whether it was *right* (by design) to do this separately for each
>> platform (in the respective PlatformBdsLib instance), as opposed to
>> doing it centrally in the BDS driver.
>>
>> You just lifted that doubt, so we know the way forward.
>>
>
> Indeed. FYI, I noticed that S3Save () does the right thing if it is
> invoked more than once.
> That means you may be able to get away with calling it both in
> PlatformBdsInit() (before end-of-DXE) *and* keep the original call
> site in GenericBdsLib intact, and still end up with something that
> works. It does rely on an implementation detail of AcpiS3Save though

... which you are probably doing already, I guess ... :-)


>>> -----Original Message-----
>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>> Sent: Thursday, June 25, 2015 3:48 AM
>>> To: edk2-devel@lists.sourceforge.net; lersek@redhat.com; olivier.martin@arm.com; Justen, Jordan L; Fan, Jeff
>>> Subject: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal end-of-DXE event upon platform BDS invocation
>>>
>>> According to paragraph 5.1.2.1 of the PI spec vol 2 version 1.4, the EFI_END_OF_DXE_EVENT_GUID event must be signalled prior to invoking any UEFI drivers, applications, or connecting consoles.
>>>
>>> This implements the event signalling in a new GenericBdsLib function BdsLibSignalEndOfDxeEvent (), and adds the invocation to BdsEntry () right before the point where it hands over to PlatformBdsInit().
>>>
>>> In order to retain the mandated ordering between saving the ACPI S3 script and signalling the EFI_END_OF_DXE_EVENT_GUID event, saving of the S3 script is moved to BdsLibSignalEndOfDxeEvent () as well, right before the event is signalled.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>
>>> I am having trouble wrapping my head around all the interdependencies, but I do think that this may be a feasible approach.
>>>
>>>  IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h         | 14 +++
>>>  IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c         | 70 ++++++++++--
>>>  IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf |  1 +  IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h  |  1 +
>>>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c             |  3 +
>>>  5 files changed, 79 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
>>> index ecd68a003bbb..86cda6615bf7 100644
>>> --- a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
>>> +++ b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
>>> @@ -1110,5 +1110,19 @@ DisableQuietBoot (
>>>    VOID
>>>    );
>>>
>>> +
>>> +/**
>>> +  Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI
>>> +spec
>>> +  vol 2 version 1.4 paragraph 5.1.2.1
>>> +
>>> +  @retval EFI_SUCCESS     The event was signalled successfully
>>> +
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +BdsLibSignalEndOfDxeEvent (
>>> +  VOID
>>> +  );
>>> +
>>>  #endif
>>>
>>> diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
>>> index e02a71015edc..d84b0c1a453b 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
>>>    //
>>> @@ -4363,3 +4353,63 @@ BdsLibUpdateFvFileDevicePath (
>>>    }
>>>    return EFI_NOT_FOUND;
>>>  }
>>> +
>>> +/**
>>> +  An empty function to pass error checking of CreateEventEx ().
>>> +
>>> +  @param  Event                 Event whose notification function is being invoked.
>>> +  @param  Context               Pointer to the notification function's context,
>>> +                                which is implementation-dependent.
>>> +
>>> +**/
>>> +STATIC VOID
>>> +EFIAPI
>>> +BdsEmptyCallbackFunction (
>>> +  IN EFI_EVENT                Event,
>>> +  IN VOID                     *Context
>>> +  )
>>> +{
>>> +}
>>> +
>>> +/**
>>> +  Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI
>>> +spec
>>> +  vol 2 version 1.4 paragraph 5.1.2.1
>>> +
>>> +  @retval EFI_SUCCESS     The event was signalled successfully
>>> +
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +BdsLibSignalEndOfDxeEvent (
>>> +  VOID
>>> +  )
>>> +{
>>> +  EFI_ACPI_S3_SAVE_PROTOCOL       *AcpiS3Save;
>>> +  EFI_EVENT                       EndOfDxeEvent;
>>> +  EFI_STATUS                      Status;
>>> +
>>> +  //
>>> +  // Save the S3 resume script before signalling the end-of-DXE event.
>>> +  //
>>> +  Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL,
>>> + (VOID **) &AcpiS3Save);  if (!EFI_ERROR (Status)) {
>>> +    AcpiS3Save->S3Save (AcpiS3Save, NULL);  }
>>> +
>>> +  //
>>> +  // Signal EndOfDxe PI Event
>>> +  //
>>> +  Status = gBS->CreateEventEx (
>>> +      EVT_NOTIFY_SIGNAL,
>>> +      TPL_NOTIFY,
>>> +      BdsEmptyCallbackFunction,
>>> +      NULL,
>>> +      &gEfiEndOfDxeEventGroupGuid,
>>> +      &EndOfDxeEvent
>>> +      );
>>> +  if (!EFI_ERROR (Status)) {
>>> +    gBS->SignalEvent (EndOfDxeEvent);
>>> +  }
>>> +
>>> +  return Status;
>>> +}
>>> diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
>>> index 5381e331ff8c..42ac1185f3ab 100644
>>> --- a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
>>> +++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
>>> @@ -104,6 +104,7 @@ [Guids]
>>>    ## SOMETIMES_CONSUMES ## Variable:L"LegacyDevOrder"
>>>    gEfiLegacyDevOrderVariableGuid
>>>    gEdkiiStatusCodeDataTypeVariableGuid          ## SOMETIMES_CONSUMES ## GUID
>>> +  gEfiEndOfDxeEventGroupGuid
>>>
>>>  [Protocols]
>>>    gEfiSimpleFileSystemProtocolGuid              ## SOMETIMES_CONSUMES
>>> diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
>>> index c32579bfc577..658097ec22ea 100644
>>> --- a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
>>> +++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
>>> @@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>>  #include <Guid/LastEnumLang.h>
>>>  #include <Guid/LegacyDevOrder.h>
>>>  #include <Guid/StatusCodeDataTypeVariable.h>
>>> +#include <Guid/EventGroup.h>
>>>
>>>  #include <Library/PrintLib.h>
>>>  #include <Library/DebugLib.h>
>>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>>> index ae7ad2153c51..0413d34fce02 100644
>>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>>> @@ -623,6 +623,9 @@ BdsEntry (
>>>    InitializeLanguage (TRUE);
>>>    InitializeFrontPage (TRUE);
>>>
>>> +  Status = BdsLibSignalEndOfDxeEvent ();  ASSERT_EFI_ERROR (Status);
>>> +
>>>    //
>>>    // Do the platform init, can be customized by OEM/IBV
>>>    //
>>> --
>>> 1.9.1
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Monitor 25 network devices or servers for free with OpManager!
>>> OpManager is web-based network management software that monitors
>>> network devices and physical & virtual servers, alerts via email & sms
>>> for fault. Monitor 25 devices for free with no restriction. Download now
>>> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>>>
>>

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
Ard Biesheuvel June 25, 2015, 8:40 a.m. | #4
On 25 June 2015 at 10:35, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> 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)
>
> The better design might be:
> 1) AcpiS3Save does not install EFI_ACPI_S3_SAVE_PROTOCOL.
> 2) AcpiS3Save registers EndOfDxe callback function.

Is there a guaranteed dispatch order for events? If not, this may be
problematic, since the silicon driver must receive the end-of-DXE
event before AcpiS3Save does, otherwise S3Save () may be called too
late

> 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.
>
> Thank you
> Yao Jiewen
>
> -----Original Message-----
> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
> Sent: Thursday, June 25, 2015 4:24 PM
> To: edk2-devel@lists.sourceforge.net
> Cc: Fan, Jeff
> Subject: Re: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal end-of-DXE event upon platform BDS invocation
>
> HI Ard
> You are very careful and definitely right.
>
> The code in BdsBoot.c seems be added in more than 10 years ago, when we did EDK-I. At that time, we do not have such thorough design review.
> In old days, we just need find a place to call S3Save() then someone added it, and leave it till now.
> It still works till now, because S3Save() function only required to be entered once. Since most platforms already call S3Save() in platform BDS. It has no impact here. See code below:
> =======================
>   //
>   // 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.
>   //
>   if (AlreadyEntered) {
>     return EFI_SUCCESS;
>   }
>   AlreadyEntered = TRUE;
> =======================
>
> Also, if a platform fail to call S3Save() early and let BDS trigger S3Save() here, I think system will ASSERT, because SaveLockBox will fail.
> =======================
>   Status = SaveLockBox (
>              &mAcpiS3IdtrProfileGuid,
>              (VOID *)(UINTN)Idtr,
>              (UINTN)sizeof(IA32_DESCRIPTOR)
>              );
>   ASSERT_EFI_ERROR (Status);
> =======================
>
> As conclusion, I completely agree with you that this code in BdsBoot.c should be removed to eliminate confusing.
>
> Thank you
> Yao Jiewen
>
>
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, June 25, 2015 3:51 PM
> To: edk2-devel@lists.sourceforge.net
> Cc: Fan, Jeff
> Subject: Re: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal end-of-DXE event upon platform BDS invocation
>
> On 25 June 2015 at 01:31, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>> Hi Ard
>> You are right that END_OF_DXE must be signal in Bds.
>>
>> My thought is that this event should be signal in "platform BDS" instead of "generic BDS".
>>
>> The reason is that only platform know when to exit platform manufacture auth state, and platform may have some special step before that.
>>
>
> That is true.
>
>> Signing end of dxe in generic BDS may break existing platform.
>>
>> Would you mind to let me know which platform forget to signal this, and can we fix it in platform BDS?
>>
>
> There are two constraints for the moment where end-of-DXE is signalled:
> - according to the PI spec
>
> """
> 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.
> """
>
> which means PlatformBdsInit() would be a reasonable candidate location, provided that it gets documented in the PlatformBdsLib API that the even should be signalled then.
>
> However, there is another constraint, according to https://firmware.intel.com/sites/default/files/A_Tour_Beyond_BIOS_Implementing_S3_resume_with_EDKII.pdf
>
> """
> PlatformBDS need 1) call AcpiS3->S3Save() to save S3 system information, then 2) signal EndOfDxe to give DXE silicon driver last chance to save boot script, and finally 3) signal 17 SmmReadyToLock to close boot script and save boot script to LockBox. These calls must be in this order, or the boot script might not be able to save correctly.
> """
>
> The problem is (as Laszlo has pointed out) that the generic BDS calls
> AcpiS3->S3Save () in BdsLibBootViaBootOption (), which is problematic
> for two reasons:
> - it is long after PlatformBdsInit(), so signalling end-of-DXE there will occur before S3Save
> - BdsLibBootViaBootOption () may never return control to the Platform BDS, so there is no way the Platform BDS can ensure that end-of-DXE will be signalled after the existing S3Save()
>
> In summary, the S3Save() call needs to be moved if we want to adhere to both constraints above.
>
> --
> Ard.
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Thursday, June 25, 2015 3:48 AM
>> To: edk2-devel@lists.sourceforge.net; lersek@redhat.com;
>> olivier.martin@arm.com; Justen, Jordan L; Fan, Jeff
>> Subject: [edk2] [RFC PATCH] IntelFrameworkModulePkg: signal end-of-DXE
>> event upon platform BDS invocation
>>
>> According to paragraph 5.1.2.1 of the PI spec vol 2 version 1.4, the EFI_END_OF_DXE_EVENT_GUID event must be signalled prior to invoking any UEFI drivers, applications, or connecting consoles.
>>
>> This implements the event signalling in a new GenericBdsLib function BdsLibSignalEndOfDxeEvent (), and adds the invocation to BdsEntry () right before the point where it hands over to PlatformBdsInit().
>>
>> In order to retain the mandated ordering between saving the ACPI S3 script and signalling the EFI_END_OF_DXE_EVENT_GUID event, saving of the S3 script is moved to BdsLibSignalEndOfDxeEvent () as well, right before the event is signalled.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> I am having trouble wrapping my head around all the interdependencies, but I do think that this may be a feasible approach.
>>
>>  IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h         | 14 +++
>>  IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c         | 70 ++++++++++--
>>  IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf |  1 +  IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h  |  1 +
>>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c             |  3 +
>>  5 files changed, 79 insertions(+), 10 deletions(-)
>>
>> diff --git a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
>> b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
>> index ecd68a003bbb..86cda6615bf7 100644
>> --- a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
>> +++ b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
>> @@ -1110,5 +1110,19 @@ DisableQuietBoot (
>>    VOID
>>    );
>>
>> +
>> +/**
>> +  Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI
>> +spec
>> +  vol 2 version 1.4 paragraph 5.1.2.1
>> +
>> +  @retval EFI_SUCCESS     The event was signalled successfully
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +BdsLibSignalEndOfDxeEvent (
>> +  VOID
>> +  );
>> +
>>  #endif
>>
>> diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
>> b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
>> index e02a71015edc..d84b0c1a453b 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
>>    //
>> @@ -4363,3 +4353,63 @@ BdsLibUpdateFvFileDevicePath (
>>    }
>>    return EFI_NOT_FOUND;
>>  }
>> +
>> +/**
>> +  An empty function to pass error checking of CreateEventEx ().
>> +
>> +  @param  Event                 Event whose notification function is being invoked.
>> +  @param  Context               Pointer to the notification function's context,
>> +                                which is implementation-dependent.
>> +
>> +**/
>> +STATIC VOID
>> +EFIAPI
>> +BdsEmptyCallbackFunction (
>> +  IN EFI_EVENT                Event,
>> +  IN VOID                     *Context
>> +  )
>> +{
>> +}
>> +
>> +/**
>> +  Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI
>> +spec
>> +  vol 2 version 1.4 paragraph 5.1.2.1
>> +
>> +  @retval EFI_SUCCESS     The event was signalled successfully
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +BdsLibSignalEndOfDxeEvent (
>> +  VOID
>> +  )
>> +{
>> +  EFI_ACPI_S3_SAVE_PROTOCOL       *AcpiS3Save;
>> +  EFI_EVENT                       EndOfDxeEvent;
>> +  EFI_STATUS                      Status;
>> +
>> +  //
>> +  // Save the S3 resume script before signalling the end-of-DXE event.
>> +  //
>> +  Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL,
>> + (VOID **) &AcpiS3Save);  if (!EFI_ERROR (Status)) {
>> +    AcpiS3Save->S3Save (AcpiS3Save, NULL);  }
>> +
>> +  //
>> +  // Signal EndOfDxe PI Event
>> +  //
>> +  Status = gBS->CreateEventEx (
>> +      EVT_NOTIFY_SIGNAL,
>> +      TPL_NOTIFY,
>> +      BdsEmptyCallbackFunction,
>> +      NULL,
>> +      &gEfiEndOfDxeEventGroupGuid,
>> +      &EndOfDxeEvent
>> +      );
>> +  if (!EFI_ERROR (Status)) {
>> +    gBS->SignalEvent (EndOfDxeEvent);  }
>> +
>> +  return Status;
>> +}
>> diff --git
>> a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
>> b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
>> index 5381e331ff8c..42ac1185f3ab 100644
>> --- a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
>> +++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
>> @@ -104,6 +104,7 @@ [Guids]
>>    ## SOMETIMES_CONSUMES ## Variable:L"LegacyDevOrder"
>>    gEfiLegacyDevOrderVariableGuid
>>    gEdkiiStatusCodeDataTypeVariableGuid          ## SOMETIMES_CONSUMES ## GUID
>> +  gEfiEndOfDxeEventGroupGuid
>>
>>  [Protocols]
>>    gEfiSimpleFileSystemProtocolGuid              ## SOMETIMES_CONSUMES
>> diff --git
>> a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
>> b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
>> index c32579bfc577..658097ec22ea 100644
>> --- a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
>> +++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
>> @@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>  #include <Guid/LastEnumLang.h>
>>  #include <Guid/LegacyDevOrder.h>
>>  #include <Guid/StatusCodeDataTypeVariable.h>
>> +#include <Guid/EventGroup.h>
>>
>>  #include <Library/PrintLib.h>
>>  #include <Library/DebugLib.h>
>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> index ae7ad2153c51..0413d34fce02 100644
>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -623,6 +623,9 @@ BdsEntry (
>>    InitializeLanguage (TRUE);
>>    InitializeFrontPage (TRUE);
>>
>> +  Status = BdsLibSignalEndOfDxeEvent ();  ASSERT_EFI_ERROR (Status);
>> +
>>    //
>>    // Do the platform init, can be customized by OEM/IBV
>>    //
>> --
>> 1.9.1
>>
>>
>> ----------------------------------------------------------------------
>> -------- Monitor 25 network devices or servers for free with
>> OpManager!
>> OpManager is web-based network management software that monitors
>> network devices and physical & virtual servers, alerts via email & sms
>> for fault. Monitor 25 devices for free with no restriction. Download
>> now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>>
>> ----------------------------------------------------------------------
>> -------- Monitor 25 network devices or servers for free with
>> OpManager!
>> OpManager is web-based network management software that monitors
>> network devices and physical & virtual servers, alerts via email & sms
>> for fault. Monitor 25 devices for free with no restriction. Download
>> now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors
> network devices and physical & virtual servers, alerts via email & sms
> for fault. Monitor 25 devices for free with no restriction. Download now
> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
Ard Biesheuvel June 25, 2015, 9:02 a.m. | #5
On 25 June 2015 at 10:51, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> The dispatch order can be determined by TPL. Event with high TPL runs first.
> But, there is no guaranteed dispatch order if 2 events have same TPL.
>
> Would you please let me know what problem you find?
>
> I cannot see any interaction between S3Save and silicon driver.
> Also, I cannot see the problem if S3Save just find ACPI and save LockBox(), and LockBox service is closed at SmmReadyToLock.
>

OK, let me try to explain:

You are suggesting to trigger S3Save by the end-of-DXE event. So how
do you plan to guarantee that the ordering constraint below is
honored?

"""
PlatformBDS need 1) call AcpiS3->S3Save() to save S3 system
information, then 2) signal EndOfDxe to give DXE silicon driver last
chance to save boot script, and finally 3) signal SmmReadyToLock to
close boot script and save boot script to LockBox. These calls must be
in this order, or the boot script might not be able to save correctly.
"""

You could give the S3Save() callback for end-of-DXE the highest
priority, but if the DXE silicon driver uses the highest priority as
well, it may still be invoked before S3Save() instead of after.
Ard Biesheuvel June 25, 2015, 10:24 a.m. | #6
On 25 June 2015 at 12:23, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/25/15 10:27, Ard Biesheuvel wrote:
>> On 25 June 2015 at 10:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 25 June 2015 at 09:58, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 06/25/15 01:31, Yao, Jiewen wrote:
>>>>> Hi Ard
>>>>> You are right that END_OF_DXE must be signal in Bds.
>>>>>
>>>>> My thought is that this event should be signal in "platform BDS"
>>>>> instead of "generic BDS".
>>>>>
>>>>> The reason is that only platform know when to exit platform
>>>>> manufacture auth state, and platform may have some special step
>>>>> before that.
>>>>>
>>>>> Signing end of dxe in generic BDS may break existing platform.
>>>>
>>>> Ard, I think we have the justification we needed for going with
>>>> platform-specific End-of-Dxe signalling. Please update your original
>>>> patch with a link to Jiewen's message:
>>>>
>>>> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109
>>>>
>>>> And I'm going to ACK it like that for ArmVirtPkg.
>>>>
>>>
>>> OK, fair enough.
>>>
>>>> (
>>>> Note, this is actually option #2 from my email
>>>> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16095>:
>>>>
>>>>> - Or, OVMF will have to signal End-of-Dxe within the
>>>>>   SaveS3BootScript() function (see again the patch), right before
>>>>>   installing "gEfiDxeSmmReadyToLockProtocolGuid". Only that would
>>>>>   place the new step (2) between existent steps (1) and (3).
>>>>>
>>>>>   In parallel, ArmVirtPkg, which doesn't implement S3 or SMM, could
>>>>>   signal End-of-Dxe wherever it wanted to. Meaning, your patch is
>>>>>   correct (assuming it doesn't break anything in practice), *if* we
>>>>>   prefer this option.
>>>> )
>>>>
>>>> Jiewen,
>>>>
>>>>> Would you mind to let me know which platform forget to signal this,
>>>>
>>>> That's every single platform in the open source edk2 tree that uses
>>>> Intel BDS.
>>>>
>>>>> and can we fix it in platform BDS?
>>>>
>>>> Yes, we can do that; Ard already posted a patch with such an approach
>>>> for ArmVirtPkg, and I intend to do the same for OvmfPkg. Our main doubt
>>>> was whether it was *right* (by design) to do this separately for each
>>>> platform (in the respective PlatformBdsLib instance), as opposed to
>>>> doing it centrally in the BDS driver.
>>>>
>>>> You just lifted that doubt, so we know the way forward.
>>>>
>>>
>>> Indeed. FYI, I noticed that S3Save () does the right thing if it is
>>> invoked more than once.
>>> That means you may be able to get away with calling it both in
>>> PlatformBdsInit() (before end-of-DXE) *and* keep the original call
>>> site in GenericBdsLib intact, and still end up with something that
>>> works. It does rely on an implementation detail of AcpiS3Save though
>>
>> ... which you are probably doing already, I guess ... :-)
>
> Not sure if I understand right, but if you mean that OVMF's platform BDS
> already calls S3Save directly -- that's not the case. We currently rely
> on the call inside the BDS driver.
>

OK, guess I was too confused to pick up on that :-)

Anyway, I guess we should add such a call in the correct place before
removing it from BdsBoot.c then

Patch hide | download patch | download mbox

diff --git a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
index ecd68a003bbb..86cda6615bf7 100644
--- a/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
+++ b/IntelFrameworkModulePkg/Include/Library/GenericBdsLib.h
@@ -1110,5 +1110,19 @@  DisableQuietBoot (
   VOID
   );
 
+
+/**
+  Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI spec
+  vol 2 version 1.4 paragraph 5.1.2.1
+
+  @retval EFI_SUCCESS     The event was signalled successfully
+
+**/
+EFI_STATUS
+EFIAPI
+BdsLibSignalEndOfDxeEvent (
+  VOID
+  );
+
 #endif
 
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c
index e02a71015edc..d84b0c1a453b 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
   //
@@ -4363,3 +4353,63 @@  BdsLibUpdateFvFileDevicePath (
   }
   return EFI_NOT_FOUND;
 }
+
+/**
+  An empty function to pass error checking of CreateEventEx ().
+
+  @param  Event                 Event whose notification function is being invoked.
+  @param  Context               Pointer to the notification function's context,
+                                which is implementation-dependent.
+
+**/
+STATIC VOID
+EFIAPI
+BdsEmptyCallbackFunction (
+  IN EFI_EVENT                Event,
+  IN VOID                     *Context
+  )
+{
+}
+
+/**
+  Signal the event EFI_END_OF_DXE_EVENT_GUID as specified by the PI spec
+  vol 2 version 1.4 paragraph 5.1.2.1
+
+  @retval EFI_SUCCESS     The event was signalled successfully
+
+**/
+EFI_STATUS
+EFIAPI
+BdsLibSignalEndOfDxeEvent (
+  VOID
+  )
+{
+  EFI_ACPI_S3_SAVE_PROTOCOL       *AcpiS3Save;
+  EFI_EVENT                       EndOfDxeEvent;
+  EFI_STATUS                      Status;
+
+  //
+  // Save the S3 resume script before signalling the end-of-DXE event.
+  //
+  Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL, (VOID **) &AcpiS3Save);
+  if (!EFI_ERROR (Status)) {
+    AcpiS3Save->S3Save (AcpiS3Save, NULL);
+  }
+
+  //
+  // Signal EndOfDxe PI Event
+  //
+  Status = gBS->CreateEventEx (
+      EVT_NOTIFY_SIGNAL,
+      TPL_NOTIFY,
+      BdsEmptyCallbackFunction,
+      NULL,
+      &gEfiEndOfDxeEventGroupGuid,
+      &EndOfDxeEvent
+      );
+  if (!EFI_ERROR (Status)) {
+    gBS->SignalEvent (EndOfDxeEvent);
+  }
+
+  return Status;
+}
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
index 5381e331ff8c..42ac1185f3ab 100644
--- a/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
+++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/GenericBdsLib.inf
@@ -104,6 +104,7 @@  [Guids]
   ## SOMETIMES_CONSUMES ## Variable:L"LegacyDevOrder"
   gEfiLegacyDevOrderVariableGuid
   gEdkiiStatusCodeDataTypeVariableGuid          ## SOMETIMES_CONSUMES ## GUID
+  gEfiEndOfDxeEventGroupGuid
 
 [Protocols]
   gEfiSimpleFileSystemProtocolGuid              ## SOMETIMES_CONSUMES
diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
index c32579bfc577..658097ec22ea 100644
--- a/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
+++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/InternalBdsLib.h
@@ -52,6 +52,7 @@  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Guid/LastEnumLang.h>
 #include <Guid/LegacyDevOrder.h>
 #include <Guid/StatusCodeDataTypeVariable.h>
+#include <Guid/EventGroup.h>
 
 #include <Library/PrintLib.h>
 #include <Library/DebugLib.h>
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
index ae7ad2153c51..0413d34fce02 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -623,6 +623,9 @@  BdsEntry (
   InitializeLanguage (TRUE);
   InitializeFrontPage (TRUE);
 
+  Status = BdsLibSignalEndOfDxeEvent ();
+  ASSERT_EFI_ERROR (Status);
+
   //
   // Do the platform init, can be customized by OEM/IBV
   //