Re: [edk2] [PATCH] ArmPlatformPkg: signal EndOfDxe event in PlatformBsdInit

2015-09-03 Thread Leif Lindholm 
On Tue, Sep 01, 2015 at 04:26:07PM +0200, Ard Biesheuvel wrote:
> Like the ArmVirtPkg platforms up until SVN r17713, the ArmPlatformPkg
> platforms built with the Intel BDS fail to signal the end-of-DXE event
> 'gEfiEndOfDxeEventGroupGuid' when entering the BDS phase, which results
> in some loss of functionality, i.e., variable reclaim in the VariableDxe
> drivers, and the splitting of the memory regions that is part of the recently
> added UEFI 2.5 properties table feature.
> 
> As discussed on the edk2-devel mailing list here:
> 
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109
> 
> it is up to the platform BDS to signal that event, since there may be
> platform specific ordering constraints with respect to the signalling
> of the event that are difficult to honor at the generic level.
> 
> So add the SignalEvent () call to PlatformBdsInit () of ArmPlatformPkg's
> PlatformBdsLib implementation for the Intel BDS.

Naïve, and perhaps silly question/comment:
ArmVirtPkg, and this patch, both call this from PlatformBdsInit(),
whereas OvmfPkg does it from PlatformBdsPolicyBehavior().

The difference is the following segment in
IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
which gets executed before signalling EndOfDxeEvent in OvmfPkg, but
not for Arm*:
---
  //
  // Set up the device list based on EFI 1.1 variables
  // process Driver and Load the driver's in the
  // driver option list
  //
  BdsLibBuildOptionFromVar (, L"DriverOrder");
  if (!IsListEmpty ()) {
BdsLibLoadDrivers ();
  }
  //
  // Check if we have the boot next option
  //
  mBootNext = BdsLibGetVariableAndSize (
L"BootNext",
,

);
---

and the code at the start of Ovmf's PlatformBdsPolicyBehavior():
---
  VisitAllInstancesOfProtocol (,
ConnectRootBridge, NULL);

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

Does the above not apply for any ArmPlatformPkg platforms (or others
importing ArmPlatformPkg's PlatformIntelBdsLib)?
If so, is this also not applicable to virtio-pci in ArmVirtPkg?

/
Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c  | 36 
> 
>  ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h  |  1 +
>  ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +
>  3 files changed, 38 insertions(+)
> 
> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c 
> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> index 98d5b277a636..e27e6d66df91 100644
> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> @@ -31,6 +31,24 @@ PlatformIntelBdsConstructor (
>return EFI_SUCCESS;
>  }
>  
> +/**
> +  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
> +EmptyCallbackFunction (
> +  IN EFI_EVENTEvent,
> +  IN VOID *Context
> +  )
> +{
> +}
> +
>  //
>  // BDS Platform Functions
>  //
> @@ -45,6 +63,24 @@ PlatformBdsInit (
>VOID
>)
>  {
> +  EFI_EVENT   EndOfDxeEvent;
> +  EFI_STATUS  Status;
> +
> +  //
> +  // Signal EndOfDxe PI Event
> +  //
> +  Status = gBS->CreateEventEx (
> +  EVT_NOTIFY_SIGNAL,
> +  TPL_CALLBACK,
> +  EmptyCallbackFunction,
> +  NULL,
> +  ,
> +  
> +  );
> +  if (!EFI_ERROR (Status)) {
> +gBS->SignalEvent (EndOfDxeEvent);
> +gBS->CloseEvent (EndOfDxeEvent);
> +  }
>  }
>  
>  STATIC
> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h 
> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
> index 7122d58be7d7..da428288fb9f 100644
> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
> @@ -30,5 +30,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #include 
>  
>  #include 
> +#include 
>  
>  #endif // _INTEL_BDS_PLATFORM_H
> diff --git 
> a/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf 
>

Re: [edk2] [PATCH] ArmPlatformPkg: signal EndOfDxe event in PlatformBsdInit

2015-09-01 Thread Ard Biesheuvel 
On 1 September 2015 at 17:11, Laszlo Ersek  wrote:
> On 09/01/15 16:26, Ard Biesheuvel wrote:
>> Like the ArmVirtPkg platforms up until SVN r17713, the ArmPlatformPkg
>> platforms built with the Intel BDS fail to signal the end-of-DXE event
>> 'gEfiEndOfDxeEventGroupGuid' when entering the BDS phase, which results
>> in some loss of functionality, i.e., variable reclaim in the VariableDxe
>> drivers, and the splitting of the memory regions that is part of the recently
>> added UEFI 2.5 properties table feature.
>>
>> As discussed on the edk2-devel mailing list here:
>>
>> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109
>>
>> it is up to the platform BDS to signal that event, since there may be
>> platform specific ordering constraints with respect to the signalling
>> of the event that are difficult to honor at the generic level.
>>
>> So add the SignalEvent () call to PlatformBdsInit () of ArmPlatformPkg's
>> PlatformBdsLib implementation for the Intel BDS.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c  | 36 
>> 
>>  ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h  |  1 +
>>  ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c 
>> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> index 98d5b277a636..e27e6d66df91 100644
>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> @@ -31,6 +31,24 @@ PlatformIntelBdsConstructor (
>>return EFI_SUCCESS;
>>  }
>>
>> +/**
>> +  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
>> +EmptyCallbackFunction (
>> +  IN EFI_EVENTEvent,
>> +  IN VOID *Context
>> +  )
>> +{
>> +}
>> +
>>  //
>>  // BDS Platform Functions
>>  //
>> @@ -45,6 +63,24 @@ PlatformBdsInit (
>>VOID
>>)
>>  {
>> +  EFI_EVENT   EndOfDxeEvent;
>> +  EFI_STATUS  Status;
>> +
>> +  //
>> +  // Signal EndOfDxe PI Event
>> +  //
>> +  Status = gBS->CreateEventEx (
>> +  EVT_NOTIFY_SIGNAL,
>> +  TPL_CALLBACK,
>> +  EmptyCallbackFunction,
>> +  NULL,
>> +  ,
>> +  
>> +  );
>> +  if (!EFI_ERROR (Status)) {
>> +gBS->SignalEvent (EndOfDxeEvent);
>> +gBS->CloseEvent (EndOfDxeEvent);
>> +  }
>>  }
>>
>>  STATIC
>> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h 
>> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
>> index 7122d58be7d7..da428288fb9f 100644
>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
>> @@ -30,5 +30,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
>> EXPRESS OR IMPLIED.
>>  #include 
>>
>>  #include 
>> +#include 
>>
>>  #endif // _INTEL_BDS_PLATFORM_H
>> diff --git 
>> a/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf 
>> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> index a18c5ea71f70..39df113288d2 100644
>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> @@ -53,6 +53,7 @@ [LibraryClasses]
>>
>>  [Guids]
>>gArmGlobalVariableGuid
>> +  gEfiEndOfDxeEventGroupGuid
>>
>>  [Pcd]
>>gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths
>>
>
> As far as I can see, the only platform affected by this change (ie. the
> only client of the library instance) is "ArmVirtPkg/ArmVirtXen.dsc". I
> think it should be okay to signal End-of-Dxe on that platform, as early
> as in PlatformBdsInit(). (We do the same in ArmVirtQemu.)
>

I am adding support for the Intel BDS to ArmVExpress-FVP-AArch64 as
well, and noticed that the memory protection feature did not work in
that case.

> Reviewed-by: Laszlo Ersek 

Thanks,
Ard.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] ArmPlatformPkg: signal EndOfDxe event in PlatformBsdInit

2015-09-01 Thread Ard Biesheuvel 
Like the ArmVirtPkg platforms up until SVN r17713, the ArmPlatformPkg
platforms built with the Intel BDS fail to signal the end-of-DXE event
'gEfiEndOfDxeEventGroupGuid' when entering the BDS phase, which results
in some loss of functionality, i.e., variable reclaim in the VariableDxe
drivers, and the splitting of the memory regions that is part of the recently
added UEFI 2.5 properties table feature.

As discussed on the edk2-devel mailing list here:

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/16088/focus=16109

it is up to the platform BDS to signal that event, since there may be
platform specific ordering constraints with respect to the signalling
of the event that are difficult to honor at the generic level.

So add the SignalEvent () call to PlatformBdsInit () of ArmPlatformPkg's
PlatformBdsLib implementation for the Intel BDS.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c  | 36 

 ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h  |  1 +
 ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf |  1 +
 3 files changed, 38 insertions(+)

diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c 
b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index 98d5b277a636..e27e6d66df91 100644
--- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -31,6 +31,24 @@ PlatformIntelBdsConstructor (
   return EFI_SUCCESS;
 }
 
+/**
+  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
+EmptyCallbackFunction (
+  IN EFI_EVENTEvent,
+  IN VOID *Context
+  )
+{
+}
+
 //
 // BDS Platform Functions
 //
@@ -45,6 +63,24 @@ PlatformBdsInit (
   VOID
   )
 {
+  EFI_EVENT   EndOfDxeEvent;
+  EFI_STATUS  Status;
+
+  //
+  // Signal EndOfDxe PI Event
+  //
+  Status = gBS->CreateEventEx (
+  EVT_NOTIFY_SIGNAL,
+  TPL_CALLBACK,
+  EmptyCallbackFunction,
+  NULL,
+  ,
+  
+  );
+  if (!EFI_ERROR (Status)) {
+gBS->SignalEvent (EndOfDxeEvent);
+gBS->CloseEvent (EndOfDxeEvent);
+  }
 }
 
 STATIC
diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h 
b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
index 7122d58be7d7..da428288fb9f 100644
--- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
+++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.h
@@ -30,5 +30,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 
 #include 
+#include 
 
 #endif // _INTEL_BDS_PLATFORM_H
diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf 
b/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index a18c5ea71f70..39df113288d2 100644
--- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -53,6 +53,7 @@ [LibraryClasses]
 
 [Guids]
   gArmGlobalVariableGuid
+  gEfiEndOfDxeEventGroupGuid
 
 [Pcd]
   gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths
-- 
1.9.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel