ArmVirtQemu: make DT vs ACPI support mutually exclusive

[edk2] [PATCH 2/3] ArmVirtPkg/FdtClientDxe: install DT configuration table at ReadyToBoot

Posted by Ard Biesheuvel 7 years, 7 months ago

Wait for ReadyToBoot to install the FDT configuration table. This allows
any driver to make modifications in the mean time, and will also allow us
to defer the decision of whether to install it in the first place to later
on in the boot.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 41 ++++++++++++++++----
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  1 +
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 7cc0c44ca12a..0327af5739f2 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -303,6 +303,30 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
   GetOrInsertChosenNode,
 };
 
+STATIC
+VOID
+EFIAPI
+OnReadyToBoot (
+  EFI_EVENT       Event,
+  VOID            *Context
+  )
+{
+  EFI_STATUS      Status;
+
+  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
+    //
+    // Only install the FDT as a configuration table if we want to leave it up
+    // to the OS to decide whether it prefers ACPI over DT.
+    //
+    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
+    ASSERT_EFI_ERROR (Status);
+  }
+
+  gBS->CloseEvent (Event);
+}
+
+STATIC EFI_EVENT         ReadyToBootEvent;
+
 EFI_STATUS
 EFIAPI
 InitializeFdtClientDxe (
@@ -330,14 +354,15 @@ InitializeFdtClientDxe (
 
   DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase));
 
-  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
-    //
-    // Only install the FDT as a configuration table if we want to leave it up
-    // to the OS to decide whether it prefers ACPI over DT.
-    //
-    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);
-    ASSERT_EFI_ERROR (Status);
-  }
+  Status = gBS->CreateEventEx (
+                  EVT_NOTIFY_SIGNAL,
+                  TPL_CALLBACK,
+                  OnReadyToBoot,
+                  NULL,
+                  &gEfiEventReadyToBootGuid,
+                  &ReadyToBootEvent
+                  );
+  ASSERT_EFI_ERROR (Status);
 
   return gBS->InstallProtocolInterface (&ImageHandle, &gFdtClientProtocolGuid,
                 EFI_NATIVE_INTERFACE, &mFdtClientProtocol);
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
index 3a0cd37040eb..00017727c32c 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
@@ -42,6 +42,7 @@ [Protocols]
   gFdtClientProtocolGuid                  ## PRODUCES
 
 [Guids]
+  gEfiEventReadyToBootGuid
   gFdtHobGuid
   gFdtTableGuid
 
-- 
2.7.4

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

Re: [edk2] [PATCH 2/3] ArmVirtPkg/FdtClientDxe: install DT configuration table at ReadyToBoot

Posted by Leif Lindholm 7 years, 7 months ago

On Thu, Mar 09, 2017 at 05:04:00PM +0100, Ard Biesheuvel wrote:
> Wait for ReadyToBoot to install the FDT configuration table. This allows

Semantic nitpicking: "Wait for" sounds a little like a delay.
"Defer FDT configuration table installation until ReadyToBoot is
signaled."?

Fold in if you agree that's an improvement.

> any driver to make modifications in the mean time, and will also allow us
> to defer the decision of whether to install it in the first place to later
> on in the boot.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 41 ++++++++++++++++----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  1 +
>  2 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 7cc0c44ca12a..0327af5739f2 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -303,6 +303,30 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
>    GetOrInsertChosenNode,
>  };
>  
> +STATIC
> +VOID
> +EFIAPI
> +OnReadyToBoot (
> +  EFI_EVENT       Event,
> +  VOID            *Context
> +  )
> +{
> +  EFI_STATUS      Status;
> +
> +  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
> +    //
> +    // Only install the FDT as a configuration table if we want to leave it up
> +    // to the OS to decide whether it prefers ACPI over DT.
> +    //

I was going to complain about how it felt pointless to introduce this
only to delete it in 2/3, but I really like how it improves the
clarity of 3/3.

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> +    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  gBS->CloseEvent (Event);
> +}
> +
> +STATIC EFI_EVENT         ReadyToBootEvent;
> +
>  EFI_STATUS
>  EFIAPI
>  InitializeFdtClientDxe (
> @@ -330,14 +354,15 @@ InitializeFdtClientDxe (
>  
>    DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase));
>  
> -  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
> -    //
> -    // Only install the FDT as a configuration table if we want to leave it up
> -    // to the OS to decide whether it prefers ACPI over DT.
> -    //
> -    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> +  Status = gBS->CreateEventEx (
> +                  EVT_NOTIFY_SIGNAL,
> +                  TPL_CALLBACK,
> +                  OnReadyToBoot,
> +                  NULL,
> +                  &gEfiEventReadyToBootGuid,
> +                  &ReadyToBootEvent
> +                  );
> +  ASSERT_EFI_ERROR (Status);
>  
>    return gBS->InstallProtocolInterface (&ImageHandle, &gFdtClientProtocolGuid,
>                  EFI_NATIVE_INTERFACE, &mFdtClientProtocol);
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> index 3a0cd37040eb..00017727c32c 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> @@ -42,6 +42,7 @@ [Protocols]
>    gFdtClientProtocolGuid                  ## PRODUCES
>  
>  [Guids]
> +  gEfiEventReadyToBootGuid
>    gFdtHobGuid
>    gFdtTableGuid
>  
> -- 
> 2.7.4
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/3] ArmVirtPkg/FdtClientDxe: install DT configuration table at ReadyToBoot

Posted by Laszlo Ersek 7 years, 7 months ago

On 03/09/17 17:04, Ard Biesheuvel wrote:
> Wait for ReadyToBoot to install the FDT configuration table. This allows
> any driver to make modifications in the mean time, and will also allow us
> to defer the decision of whether to install it in the first place to later
> on in the boot.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 41 ++++++++++++++++----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  1 +
>  2 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 7cc0c44ca12a..0327af5739f2 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -303,6 +303,30 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
>    GetOrInsertChosenNode,
>  };
>  
> +STATIC
> +VOID
> +EFIAPI
> +OnReadyToBoot (
> +  EFI_EVENT       Event,
> +  VOID            *Context
> +  )
> +{
> +  EFI_STATUS      Status;
> +
> +  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
> +    //
> +    // Only install the FDT as a configuration table if we want to leave it up
> +    // to the OS to decide whether it prefers ACPI over DT.
> +    //
> +    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  gBS->CloseEvent (Event);

OK, you didn't forget to close the event. :)

> +}
> +
> +STATIC EFI_EVENT         ReadyToBootEvent;

This should be called "mReadyToBootEvent".

> +
>  EFI_STATUS
>  EFIAPI
>  InitializeFdtClientDxe (
> @@ -330,14 +354,15 @@ InitializeFdtClientDxe (
>  
>    DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase));
>  
> -  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
> -    //
> -    // Only install the FDT as a configuration table if we want to leave it up
> -    // to the OS to decide whether it prefers ACPI over DT.
> -    //
> -    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> +  Status = gBS->CreateEventEx (
> +                  EVT_NOTIFY_SIGNAL,
> +                  TPL_CALLBACK,
> +                  OnReadyToBoot,
> +                  NULL,
> +                  &gEfiEventReadyToBootGuid,
> +                  &ReadyToBootEvent
> +                  );
> +  ASSERT_EFI_ERROR (Status);

I guess it's OK to insist that this succeed.

Also, this is done after the assignment to mDeviceTreeBase, so that's good.

>  
>    return gBS->InstallProtocolInterface (&ImageHandle, &gFdtClientProtocolGuid,
>                  EFI_NATIVE_INTERFACE, &mFdtClientProtocol);

However, if the protocol installation fails for any reason, we exit the
driver with an error code, and the driver will be unloaded. We'll have a
dangling callback pointer

In case of failure, the event should be closed (== CreateEventEx()
should be undone).

> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> index 3a0cd37040eb..00017727c32c 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> @@ -42,6 +42,7 @@ [Protocols]
>    gFdtClientProtocolGuid                  ## PRODUCES
>  
>  [Guids]
> +  gEfiEventReadyToBootGuid
>    gFdtHobGuid
>    gFdtTableGuid
>  
> 

For completeness, can you please also

#include <Guid/EventGroup.h>

for gEfiEventReadyToBootGuid's sake, in the C file? (It is already
pulled in by something else, but, again, for completenes...)

Looks okay to me otherwise.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 1/3] ArmVirtPkg/ArmVirtPL031FdtClientLib: unconditionally disable DT node
[edk2] [PATCH 2/3] ArmVirtPkg/FdtClientDxe: install DT configuration table at ReadyToBoot
[edk2] [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent