[PATCH 0/3] ArmVirtQemu: make DT vs ACPI support mutually exclusive

[PATCH 0/3] ArmVirtQemu: make DT vs ACPI support mutually exclusive

[Ard Biesheuvel]

Instead of supplying both ACPI and DT hw descriptions, and allow the latter
to be inihibited by setting a compile time define, make DT table installation
dependent on the absence of a ACPI 2.0 table when the ReadyToBoot even fires.

Ard Biesheuvel (3):
  ArmVirtPkg/ArmVirtPL031FdtClientLib: unconditionally disable DT node
  ArmVirtPkg/FdtClientDxe: install DT configuration table at ReadyToBoot
  ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

 ArmVirtPkg/ArmVirtPkg.dec                                                | 10 -----
 ArmVirtPkg/ArmVirtQemu.dsc                                               |  5 ---
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                                   | 46 ++++++++++++++++----
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf                                 |  6 +--
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c   | 22 +++++-----
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf |  3 --
 6 files changed, 51 insertions(+), 41 deletions(-)

-- 
2.7.4

[PATCH 1/3] ArmVirtPkg/ArmVirtPL031FdtClientLib: unconditionally disable DT node

[Ard Biesheuvel]

Disable the PL031 RTC DT node unconditionally rather than only when
the DT will be exposed to the OS. This allows us to defer the decision
whether to expose it to the OS to a later time without creating an
additional dependency on the FDT client code by the RTC driver.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c   | 22 +++++++++-----------
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf |  3 ---
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
index 82de7a51b32e..d168424a52f5 100644
--- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
+++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
@@ -66,18 +66,16 @@ ArmVirtPL031FdtClientLibConstructor (
 
   DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
 
-  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
-    //
-    // UEFI takes ownership of the RTC hardware, and exposes its functionality
-    // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
-    // need to disable it in the device tree to prevent the OS from attaching
-    // its device driver as well.
-    //
-    Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
-                          "disabled", sizeof ("disabled"));
-    if (EFI_ERROR (Status)) {
-        DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
-    }
+  //
+  // UEFI takes ownership of the RTC hardware, and exposes its functionality
+  // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
+  // need to disable it in the device tree to prevent the OS from attaching
+  // its device driver as well.
+  //
+  Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
+                        "disabled", sizeof ("disabled"));
+  if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
   }
 
   return EFI_SUCCESS;
diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
index 32dbff6f0852..342193651a86 100644
--- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
+++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
@@ -42,8 +42,5 @@ [Protocols]
 [Pcd]
   gArmPlatformTokenSpaceGuid.PcdPL031RtcBase
 
-[FeaturePcd]
-  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
-
 [Depex]
   gFdtClientProtocolGuid
-- 
2.7.4

Re: [PATCH 1/3] ArmVirtPkg/ArmVirtPL031FdtClientLib: unconditionally disable DT node

[Leif Lindholm]

On Thu, Mar 09, 2017 at 05:03:59PM +0100, Ard Biesheuvel wrote:
> Disable the PL031 RTC DT node unconditionally rather than only when
> the DT will be exposed to the OS. This allows us to defer the decision
> whether to expose it to the OS to a later time without creating an
> additional dependency on the FDT client code by the RTC driver.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

This seems like an improvement even without the rest of the series.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c   | 22 +++++++++-----------
>  ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf |  3 ---
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> index 82de7a51b32e..d168424a52f5 100644
> --- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> @@ -66,18 +66,16 @@ ArmVirtPL031FdtClientLibConstructor (
>  
>    DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
>  
> -  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
> -    //
> -    // UEFI takes ownership of the RTC hardware, and exposes its functionality
> -    // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
> -    // need to disable it in the device tree to prevent the OS from attaching
> -    // its device driver as well.
> -    //
> -    Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
> -                          "disabled", sizeof ("disabled"));
> -    if (EFI_ERROR (Status)) {
> -        DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
> -    }
> +  //
> +  // UEFI takes ownership of the RTC hardware, and exposes its functionality
> +  // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
> +  // need to disable it in the device tree to prevent the OS from attaching
> +  // its device driver as well.
> +  //
> +  Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
> +                        "disabled", sizeof ("disabled"));
> +  if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
>    }
>  
>    return EFI_SUCCESS;
> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> index 32dbff6f0852..342193651a86 100644
> --- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> @@ -42,8 +42,5 @@ [Protocols]
>  [Pcd]
>    gArmPlatformTokenSpaceGuid.PcdPL031RtcBase
>  
> -[FeaturePcd]
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
> -
>  [Depex]
>    gFdtClientProtocolGuid
> -- 
> 2.7.4
> 

Re: [PATCH 1/3] ArmVirtPkg/ArmVirtPL031FdtClientLib: unconditionally disable DT node

[Laszlo Ersek]

On 03/09/17 17:03, Ard Biesheuvel wrote:
> Disable the PL031 RTC DT node unconditionally rather than only when
> the DT will be exposed to the OS. This allows us to defer the decision
> whether to expose it to the OS to a later time without creating an
> additional dependency on the FDT client code by the RTC driver.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c   | 22 +++++++++-----------
>  ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf |  3 ---
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> index 82de7a51b32e..d168424a52f5 100644
> --- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> @@ -66,18 +66,16 @@ ArmVirtPL031FdtClientLibConstructor (
>  
>    DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
>  
> -  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
> -    //
> -    // UEFI takes ownership of the RTC hardware, and exposes its functionality
> -    // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
> -    // need to disable it in the device tree to prevent the OS from attaching
> -    // its device driver as well.
> -    //
> -    Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
> -                          "disabled", sizeof ("disabled"));
> -    if (EFI_ERROR (Status)) {
> -        DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
> -    }
> +  //
> +  // UEFI takes ownership of the RTC hardware, and exposes its functionality
> +  // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
> +  // need to disable it in the device tree to prevent the OS from attaching
> +  // its device driver as well.
> +  //
> +  Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
> +                        "disabled", sizeof ("disabled"));
> +  if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
>    }
>  
>    return EFI_SUCCESS;
> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> index 32dbff6f0852..342193651a86 100644
> --- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
> @@ -42,8 +42,5 @@ [Protocols]
>  [Pcd]
>    gArmPlatformTokenSpaceGuid.PcdPL031RtcBase
>  
> -[FeaturePcd]
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
> -
>  [Depex]
>    gFdtClientProtocolGuid
> 

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

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

[Ard Biesheuvel]

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

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

[Leif Lindholm]

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
> 

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

[Laszlo Ersek]

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

[PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel]

Instead of having a build time switch to prevent the FDT configuration
table from being installed, make this behavior dependent on whether we
are passing ACPI tables to the OS. This is done by looking for the
ACPI 2.0 configuration table, and only installing the FDT one if the
ACPI one cannot be found.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
 ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
 4 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index a5ec42166445..efe83a383d55 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   # EFI_VT_100_GUID.
   #
   gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
-
-[PcdsFeatureFlag]
-  #
-  # Pure ACPI boot
-  #
-  # Inhibit installation of the FDT as a configuration table if this feature
-  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
-  # description of the platform, and it is up to the OS to choose.
-  #
-  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 477dfdcfc764..7b266b98b949 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -34,7 +34,6 @@ [Defines]
   # -D FLAG=VALUE
   #
   DEFINE SECURE_BOOT_ENABLE      = FALSE
-  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
 
 !include ArmVirtPkg/ArmVirt.dsc.inc
 
@@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
 
-!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
-  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
-!endif
-
 [PcdsFixedAtBuild.common]
   gArmPlatformTokenSpaceGuid.PcdCoreCount|1
 !if $(ARCH) == AARCH64
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 0327af5739f2..2981977f3d20 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -17,6 +17,7 @@
 #include <Library/DebugLib.h>
 #include <Library/UefiDriverEntryPoint.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
 #include <Library/HobLib.h>
 #include <libfdt.h>
 
@@ -312,12 +313,16 @@ OnReadyToBoot (
   )
 {
   EFI_STATUS      Status;
+  VOID            *Table;
 
-  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.
-    //
+  //
+  // Only install the FDT as a configuration table if we are not exposing
+  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
+  // no meaning on ARM since we need at least ACPI 5.0 support, and the
+  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
+  //
+  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
+  if (EFI_ERROR (Status) || Table == NULL) {
     Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
     ASSERT_EFI_ERROR (Status);
   }
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
index 00017727c32c..9861f41e968b 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
@@ -37,17 +37,16 @@ [LibraryClasses]
   HobLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
+  UefiLib
 
 [Protocols]
   gFdtClientProtocolGuid                  ## PRODUCES
 
 [Guids]
+  gEfiAcpi20TableGuid
   gEfiEventReadyToBootGuid
   gFdtHobGuid
   gFdtTableGuid
 
-[FeaturePcd]
-  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
-
 [Depex]
   TRUE
-- 
2.7.4

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Leif Lindholm]

On Thu, Mar 09, 2017 at 05:04:01PM +0100, Ard Biesheuvel wrote:
> Instead of having a build time switch to prevent the FDT configuration
> table from being installed, make this behavior dependent on whether we
> are passing ACPI tables to the OS. This is done by looking for the
> ACPI 2.0 configuration table, and only installing the FDT one if the
> ACPI one cannot be found.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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

> ---
>  ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>  ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>  4 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index a5ec42166445..efe83a383d55 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # EFI_VT_100_GUID.
>    #
>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
> -
> -[PcdsFeatureFlag]
> -  #
> -  # Pure ACPI boot
> -  #
> -  # Inhibit installation of the FDT as a configuration table if this feature
> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
> -  # description of the platform, and it is up to the OS to choose.
> -  #
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 477dfdcfc764..7b266b98b949 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -34,7 +34,6 @@ [Defines]
>    # -D FLAG=VALUE
>    #
>    DEFINE SECURE_BOOT_ENABLE      = FALSE
> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>  
>  !include ArmVirtPkg/ArmVirt.dsc.inc
>  
> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>  
> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
> -!endif
> -
>  [PcdsFixedAtBuild.common]
>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>  !if $(ARCH) == AARCH64
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 0327af5739f2..2981977f3d20 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -17,6 +17,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiDriverEntryPoint.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
>  #include <Library/HobLib.h>
>  #include <libfdt.h>
>  
> @@ -312,12 +313,16 @@ OnReadyToBoot (
>    )
>  {
>    EFI_STATUS      Status;
> +  VOID            *Table;
>  
> -  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.
> -    //
> +  //
> +  // Only install the FDT as a configuration table if we are not exposing
> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
> +  //
> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
> +  if (EFI_ERROR (Status) || Table == NULL) {
>      Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>      ASSERT_EFI_ERROR (Status);
>    }
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> index 00017727c32c..9861f41e968b 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> @@ -37,17 +37,16 @@ [LibraryClasses]
>    HobLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  UefiLib
>  
>  [Protocols]
>    gFdtClientProtocolGuid                  ## PRODUCES
>  
>  [Guids]
> +  gEfiAcpi20TableGuid
>    gEfiEventReadyToBootGuid
>    gFdtHobGuid
>    gFdtTableGuid
>  
> -[FeaturePcd]
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
> -
>  [Depex]
>    TRUE
> -- 
> 2.7.4
> 

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/09/17 17:04, Ard Biesheuvel wrote:
> Instead of having a build time switch to prevent the FDT configuration
> table from being installed, make this behavior dependent on whether we
> are passing ACPI tables to the OS. This is done by looking for the
> ACPI 2.0 configuration table, and only installing the FDT one if the
> ACPI one cannot be found.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>  ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>  4 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index a5ec42166445..efe83a383d55 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # EFI_VT_100_GUID.
>    #
>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
> -
> -[PcdsFeatureFlag]
> -  #
> -  # Pure ACPI boot
> -  #
> -  # Inhibit installation of the FDT as a configuration table if this feature
> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
> -  # description of the platform, and it is up to the OS to choose.
> -  #
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 477dfdcfc764..7b266b98b949 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -34,7 +34,6 @@ [Defines]
>    # -D FLAG=VALUE
>    #
>    DEFINE SECURE_BOOT_ENABLE      = FALSE
> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>  
>  !include ArmVirtPkg/ArmVirt.dsc.inc
>  
> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>  
> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
> -!endif
> -
>  [PcdsFixedAtBuild.common]
>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>  !if $(ARCH) == AARCH64
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 0327af5739f2..2981977f3d20 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -17,6 +17,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiDriverEntryPoint.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
>  #include <Library/HobLib.h>
>  #include <libfdt.h>

Can you also #include <Guid/Acpi.h> for completeness? (For
gEfiAcpi20TableGuid's sake.)

>  
> @@ -312,12 +313,16 @@ OnReadyToBoot (
>    )
>  {
>    EFI_STATUS      Status;
> +  VOID            *Table;
>  
> -  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.
> -    //
> +  //
> +  // Only install the FDT as a configuration table if we are not exposing
> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
> +  //
> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
> +  if (EFI_ERROR (Status) || Table == NULL) {
>      Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>      ASSERT_EFI_ERROR (Status);
>    }

This change will affect Xen too (unlike -D PURE_ACPI_BOOT_ENABLE, which
does not affect Xen -- my patch set also didn't affect Xen, on purpose,
because I have no idea what Xen people want).

Xen installs its ACPI tables in "ArmVirtPkg/XenAcpiPlatformDxe".

I guess it's okay to strive for uniformity here, but the commit message
should mention Xen is included in the change. (The blurb subject is
currently "ArmVirtQemu: make DT vs ACPI support mutually exclusive".)

> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> index 00017727c32c..9861f41e968b 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> @@ -37,17 +37,16 @@ [LibraryClasses]
>    HobLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  UefiLib
>  
>  [Protocols]
>    gFdtClientProtocolGuid                  ## PRODUCES
>  
>  [Guids]
> +  gEfiAcpi20TableGuid
>    gEfiEventReadyToBootGuid
>    gFdtHobGuid
>    gFdtTableGuid
>  
> -[FeaturePcd]
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
> -
>  [Depex]
>    TRUE
> 

Looks good to me generally. Two more comments:

- Can you include the first patch from my series, that adds the missing
EFIAPI specifiers to the protocol members in FdtClientDxe? Since we're
already touching the code.

- I agree this is simpler than my approach, but I think it's also less
robust. If it breaks (due to unspecified ordering between ReadyToBoot
callbacks), you'll get to sort it out ;)

Thanks!
Laszlo

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/09/17 17:04, Ard Biesheuvel wrote:
> Instead of having a build time switch to prevent the FDT configuration
> table from being installed, make this behavior dependent on whether we
> are passing ACPI tables to the OS. This is done by looking for the
> ACPI 2.0 configuration table, and only installing the FDT one if the
> ACPI one cannot be found.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>  ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>  4 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index a5ec42166445..efe83a383d55 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # EFI_VT_100_GUID.
>    #
>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
> -
> -[PcdsFeatureFlag]
> -  #
> -  # Pure ACPI boot
> -  #
> -  # Inhibit installation of the FDT as a configuration table if this feature
> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
> -  # description of the platform, and it is up to the OS to choose.
> -  #
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 477dfdcfc764..7b266b98b949 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -34,7 +34,6 @@ [Defines]
>    # -D FLAG=VALUE
>    #
>    DEFINE SECURE_BOOT_ENABLE      = FALSE
> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>  
>  !include ArmVirtPkg/ArmVirt.dsc.inc
>  
> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>  
> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
> -!endif
> -
>  [PcdsFixedAtBuild.common]
>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>  !if $(ARCH) == AARCH64
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 0327af5739f2..2981977f3d20 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -17,6 +17,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiDriverEntryPoint.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
>  #include <Library/HobLib.h>
>  #include <libfdt.h>
>  
> @@ -312,12 +313,16 @@ OnReadyToBoot (
>    )
>  {
>    EFI_STATUS      Status;
> +  VOID            *Table;
>  
> -  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.
> -    //
> +  //
> +  // Only install the FDT as a configuration table if we are not exposing
> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
> +  //
> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
> +  if (EFI_ERROR (Status) || Table == NULL) {
>      Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>      ASSERT_EFI_ERROR (Status);
>    }

This code breaks the DT-only ("-no-acpi") boot with boot graphics
(virtio-gpu) enabled.

Namely, we recently included BootGraphicsResourceTableDxe in the build.
That driver calls InstallAcpiTable() for BGRT, which in turn causes
AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().

We all missed that just because QEMU doesn't produce ACPI payload (and
we consequently don't install it), other drivers in edk2 may
unconditionally install "auxiliary" ACPI tables, which minimally trigger
the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
Such a crippled set of ACPI tables isn't sufficient for booting however.

We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
and ScanTableInRSDT() in
"MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
think the above check should be reworked to look for the FADT
(EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
from these helper functions. No driver outside of
QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
always be part of QEMU's ACPI payload, if it generates one.

Thanks
Laszlo

> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> index 00017727c32c..9861f41e968b 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> @@ -37,17 +37,16 @@ [LibraryClasses]
>    HobLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  UefiLib
>  
>  [Protocols]
>    gFdtClientProtocolGuid                  ## PRODUCES
>  
>  [Guids]
> +  gEfiAcpi20TableGuid
>    gEfiEventReadyToBootGuid
>    gFdtHobGuid
>    gFdtTableGuid
>  
> -[FeaturePcd]
> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
> -
>  [Depex]
>    TRUE
> 

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel]

On 11 March 2017 at 06:55, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/09/17 17:04, Ard Biesheuvel wrote:
>> Instead of having a build time switch to prevent the FDT configuration
>> table from being installed, make this behavior dependent on whether we
>> are passing ACPI tables to the OS. This is done by looking for the
>> ACPI 2.0 configuration table, and only installing the FDT one if the
>> ACPI one cannot be found.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>>  ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>>  4 files changed, 12 insertions(+), 23 deletions(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>> index a5ec42166445..efe83a383d55 100644
>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>    # EFI_VT_100_GUID.
>>    #
>>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>> -
>> -[PcdsFeatureFlag]
>> -  #
>> -  # Pure ACPI boot
>> -  #
>> -  # Inhibit installation of the FDT as a configuration table if this feature
>> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
>> -  # description of the platform, and it is up to the OS to choose.
>> -  #
>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>> index 477dfdcfc764..7b266b98b949 100644
>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>> @@ -34,7 +34,6 @@ [Defines]
>>    # -D FLAG=VALUE
>>    #
>>    DEFINE SECURE_BOOT_ENABLE      = FALSE
>> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>>
>>  !include ArmVirtPkg/ArmVirt.dsc.inc
>>
>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>>
>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
>> -!endif
>> -
>>  [PcdsFixedAtBuild.common]
>>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>>  !if $(ARCH) == AARCH64
>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> index 0327af5739f2..2981977f3d20 100644
>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> @@ -17,6 +17,7 @@
>>  #include <Library/DebugLib.h>
>>  #include <Library/UefiDriverEntryPoint.h>
>>  #include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>>  #include <Library/HobLib.h>
>>  #include <libfdt.h>
>>
>> @@ -312,12 +313,16 @@ OnReadyToBoot (
>>    )
>>  {
>>    EFI_STATUS      Status;
>> +  VOID            *Table;
>>
>> -  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.
>> -    //
>> +  //
>> +  // Only install the FDT as a configuration table if we are not exposing
>> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
>> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
>> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
>> +  //
>> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
>> +  if (EFI_ERROR (Status) || Table == NULL) {
>>      Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>>      ASSERT_EFI_ERROR (Status);
>>    }
>
> This code breaks the DT-only ("-no-acpi") boot with boot graphics
> (virtio-gpu) enabled.
>
> Namely, we recently included BootGraphicsResourceTableDxe in the build.
> That driver calls InstallAcpiTable() for BGRT, which in turn causes
> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().
>
> We all missed that just because QEMU doesn't produce ACPI payload (and
> we consequently don't install it), other drivers in edk2 may
> unconditionally install "auxiliary" ACPI tables, which minimally trigger
> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
> Such a crippled set of ACPI tables isn't sufficient for booting however.
>
> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
> and ScanTableInRSDT() in
> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
> think the above check should be reworked to look for the FADT
> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
> from these helper functions. No driver outside of
> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
> always be part of QEMU's ACPI payload, if it generates one.
>

OK, that would get things working again, I suppose. But do we want
neutered ACPI tables to be exposed at all, even if there is a DT in
that case to boot from?

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/11/17 07:57, Ard Biesheuvel wrote:
> On 11 March 2017 at 06:55, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/09/17 17:04, Ard Biesheuvel wrote:
>>> Instead of having a build time switch to prevent the FDT configuration
>>> table from being installed, make this behavior dependent on whether we
>>> are passing ACPI tables to the OS. This is done by looking for the
>>> ACPI 2.0 configuration table, and only installing the FDT one if the
>>> ACPI one cannot be found.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>>>  ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>>>  4 files changed, 12 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>> index a5ec42166445..efe83a383d55 100644
>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>    # EFI_VT_100_GUID.
>>>    #
>>>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>>> -
>>> -[PcdsFeatureFlag]
>>> -  #
>>> -  # Pure ACPI boot
>>> -  #
>>> -  # Inhibit installation of the FDT as a configuration table if this feature
>>> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
>>> -  # description of the platform, and it is up to the OS to choose.
>>> -  #
>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>> index 477dfdcfc764..7b266b98b949 100644
>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>> @@ -34,7 +34,6 @@ [Defines]
>>>    # -D FLAG=VALUE
>>>    #
>>>    DEFINE SECURE_BOOT_ENABLE      = FALSE
>>> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>>>
>>>  !include ArmVirtPkg/ArmVirt.dsc.inc
>>>
>>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>>>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>>>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>>>
>>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
>>> -!endif
>>> -
>>>  [PcdsFixedAtBuild.common]
>>>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>>>  !if $(ARCH) == AARCH64
>>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>> index 0327af5739f2..2981977f3d20 100644
>>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>> @@ -17,6 +17,7 @@
>>>  #include <Library/DebugLib.h>
>>>  #include <Library/UefiDriverEntryPoint.h>
>>>  #include <Library/UefiBootServicesTableLib.h>
>>> +#include <Library/UefiLib.h>
>>>  #include <Library/HobLib.h>
>>>  #include <libfdt.h>
>>>
>>> @@ -312,12 +313,16 @@ OnReadyToBoot (
>>>    )
>>>  {
>>>    EFI_STATUS      Status;
>>> +  VOID            *Table;
>>>
>>> -  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.
>>> -    //
>>> +  //
>>> +  // Only install the FDT as a configuration table if we are not exposing
>>> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
>>> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
>>> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
>>> +  //
>>> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
>>> +  if (EFI_ERROR (Status) || Table == NULL) {
>>>      Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>>>      ASSERT_EFI_ERROR (Status);
>>>    }
>>
>> This code breaks the DT-only ("-no-acpi") boot with boot graphics
>> (virtio-gpu) enabled.
>>
>> Namely, we recently included BootGraphicsResourceTableDxe in the build.
>> That driver calls InstallAcpiTable() for BGRT, which in turn causes
>> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().
>>
>> We all missed that just because QEMU doesn't produce ACPI payload (and
>> we consequently don't install it), other drivers in edk2 may
>> unconditionally install "auxiliary" ACPI tables, which minimally trigger
>> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
>> Such a crippled set of ACPI tables isn't sufficient for booting however.
>>
>> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
>> and ScanTableInRSDT() in
>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>> think the above check should be reworked to look for the FADT
>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>> from these helper functions. No driver outside of
>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>> always be part of QEMU's ACPI payload, if it generates one.
>>
> 
> OK, that would get things working again, I suppose. But do we want
> neutered ACPI tables to be exposed at all, even if there is a DT in
> that case to boot from?

I think the neutered ACPI tables (on -no-acpi) should be fine. The
upstream Linux guest will prefer DT if it is present; it won't look at
the crippled ACPI tables. And if we only provide ACPI, then the tables
won't be crippled. And for Linux guests that favor ACPI over DT -- well,
just don't boot them with -no-acpi.

(

It would be hard to eliminate any and all ACPI tables on "-no-acpi" --
it could be necessary to modify individual / independent drivers that
install tables.

Excluding AcpiTableDxe (the one that produces EFI_ACPI_TABLE_PROTOCOL)
dynamically, or causing it to exit early, would be messy. For some
drivers, it could have the intended effect (if the driver handles the
protocol's absence gracefully), while some other drivers (that have a
depex on the protocol) wouldn't even load (possibly causing further
problems).

I think this just goes on to show how broken an ACPI-less UEFI setup is,
in the first place.

)

Laszlo

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Leif Lindholm]

On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
> >> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
> >> think the above check should be reworked to look for the FADT
> >> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
> >> from these helper functions. No driver outside of
> >> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
> >> always be part of QEMU's ACPI payload, if it generates one.
> > 
> > OK, that would get things working again, I suppose. But do we want
> > neutered ACPI tables to be exposed at all, even if there is a DT in
> > that case to boot from?
> 
> I think the neutered ACPI tables (on -no-acpi) should be fine. The
> upstream Linux guest will prefer DT if it is present;

Yes, but we've already determined that this situation is suboptimal,
which was what triggered this changeset to begin with.

/
    Leif

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel]

On 11 March 2017 at 10:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
>> >> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>> >> think the above check should be reworked to look for the FADT
>> >> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>> >> from these helper functions. No driver outside of
>> >> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>> >> always be part of QEMU's ACPI payload, if it generates one.
>> >
>> > OK, that would get things working again, I suppose. But do we want
>> > neutered ACPI tables to be exposed at all, even if there is a DT in
>> > that case to boot from?
>>
>> I think the neutered ACPI tables (on -no-acpi) should be fine. The
>> upstream Linux guest will prefer DT if it is present;
>
> Yes, but we've already determined that this situation is suboptimal,
> which was what triggered this changeset to begin with.
>

Indeed. Having an ACPI entry point config table installed, but not
having the essential tables that allow you to boot is a situation that
we should try very hard to avoid IMO

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/13/17 11:23, Ard Biesheuvel wrote:
> On 11 March 2017 at 10:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
>>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>>>> think the above check should be reworked to look for the FADT
>>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>>>> from these helper functions. No driver outside of
>>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>>>> always be part of QEMU's ACPI payload, if it generates one.
>>>>
>>>> OK, that would get things working again, I suppose. But do we want
>>>> neutered ACPI tables to be exposed at all, even if there is a DT in
>>>> that case to boot from?
>>>
>>> I think the neutered ACPI tables (on -no-acpi) should be fine. The
>>> upstream Linux guest will prefer DT if it is present;
>>
>> Yes, but we've already determined that this situation is suboptimal,
>> which was what triggered this changeset to begin with.
>>
> 
> Indeed. Having an ACPI entry point config table installed, but not
> having the essential tables that allow you to boot is a situation that
> we should try very hard to avoid IMO
> 

That requires dynamic disabling of AcpiTableDxe -- so that EFI_ACPI_TABLE_PROTOCOL is not produced, despite the driver being loaded --, and equipping all clients of EFI_ACPI_TABLE_PROTOCOL to fail gracefully when the protocol is not found. Alternatively, let AcpiTableDxe install the protocol, but prevent all clients from calling it.

We've done something like in the past with PcdAcpiS3Enable. It required IntelFrameworkModulePkg, MdeModulePkg and UefiCpuPkg changes.

$ git grep -l -w InstallAcpiTable

ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/AcpiTables.c
ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
EdkCompatibilityPkg/Foundation/Efi/Protocol/AcpiTable/AcpiTable.h
EmbeddedPkg/Library/AcpiLib/AcpiLib.c
IntelFrameworkModulePkg/Universal/Acpi/AcpiSupportDxe/AcpiSupportAcpiSupportProtocol.c
MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatform.c
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c
MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c
MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
MdePkg/Include/Protocol/AcpiTable.h
NetworkPkg/IScsiDxe/IScsiIbft.c
OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
OvmfPkg/AcpiPlatformDxe/Qemu.c
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
OvmfPkg/AcpiPlatformDxe/Xen.c
QuarkPlatformPkg/Acpi/Dxe/AcpiPlatform/AcpiPlatform.c
QuarkPlatformPkg/Acpi/DxeSmm/SmmPowerManagement/Ppm.c
SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
SecurityPkg/Tcg/TcgDxe/TcgDxe.c
SecurityPkg/Tcg/TcgSmm/TcgSmm.c
SecurityPkg/Tcg/TrEEDxe/TrEEDxe.c
SecurityPkg/Tcg/TrEESmm/TrEESmm.c
UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.c

(Some of these hits don't belong to EFI_ACPI_TABLE_PROTOCOL clients, of course.)

Thanks
Laszlo

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel]

On 13 March 2017 at 14:53, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/13/17 11:23, Ard Biesheuvel wrote:
>> On 11 March 2017 at 10:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
>>>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>>>>> think the above check should be reworked to look for the FADT
>>>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>>>>> from these helper functions. No driver outside of
>>>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>>>>> always be part of QEMU's ACPI payload, if it generates one.
>>>>>
>>>>> OK, that would get things working again, I suppose. But do we want
>>>>> neutered ACPI tables to be exposed at all, even if there is a DT in
>>>>> that case to boot from?
>>>>
>>>> I think the neutered ACPI tables (on -no-acpi) should be fine. The
>>>> upstream Linux guest will prefer DT if it is present;
>>>
>>> Yes, but we've already determined that this situation is suboptimal,
>>> which was what triggered this changeset to begin with.
>>>
>>
>> Indeed. Having an ACPI entry point config table installed, but not
>> having the essential tables that allow you to boot is a situation that
>> we should try very hard to avoid IMO
>>
>
> That requires dynamic disabling of AcpiTableDxe -- so that EFI_ACPI_TABLE_PROTOCOL is not produced, despite the driver being loaded --, and equipping all clients of EFI_ACPI_TABLE_PROTOCOL to fail gracefully when the protocol is not found. Alternatively, let AcpiTableDxe install the protocol, but prevent all clients from calling it.
>

Not really. We only care about what the OS gets to see, not what the
firmware ends up doing internally. So, say, at ReadyToBoot, we could
check that the ACPI entry point points to what we need minimally to
boot successfully, otherwise we rip it out. This will trigger the
recently added change to install the DT if no ACPI entry point was
found.

Not pretty, I know, but in this case, the handover state when invoking
the OS is much more important than clean handling in the firmware
itself.


> We've done something like in the past with PcdAcpiS3Enable. It required IntelFrameworkModulePkg, MdeModulePkg and UefiCpuPkg changes.
>
> $ git grep -l -w InstallAcpiTable
>
> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/AcpiTables.c
> ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
> EdkCompatibilityPkg/Foundation/Efi/Protocol/AcpiTable/AcpiTable.h
> EmbeddedPkg/Library/AcpiLib/AcpiLib.c
> IntelFrameworkModulePkg/Universal/Acpi/AcpiSupportDxe/AcpiSupportAcpiSupportProtocol.c
> MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatform.c
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
> MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c
> MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c
> MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
> MdePkg/Include/Protocol/AcpiTable.h
> NetworkPkg/IScsiDxe/IScsiIbft.c
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> OvmfPkg/AcpiPlatformDxe/Qemu.c
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> OvmfPkg/AcpiPlatformDxe/Xen.c
> QuarkPlatformPkg/Acpi/Dxe/AcpiPlatform/AcpiPlatform.c
> QuarkPlatformPkg/Acpi/DxeSmm/SmmPowerManagement/Ppm.c
> SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
> SecurityPkg/Tcg/TcgDxe/TcgDxe.c
> SecurityPkg/Tcg/TcgSmm/TcgSmm.c
> SecurityPkg/Tcg/TrEEDxe/TrEEDxe.c
> SecurityPkg/Tcg/TrEESmm/TrEESmm.c
> UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.c
>
> (Some of these hits don't belong to EFI_ACPI_TABLE_PROTOCOL clients, of course.)
>
> Thanks
> Laszlo

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/13/17 15:59, Ard Biesheuvel wrote:
> On 13 March 2017 at 14:53, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/13/17 11:23, Ard Biesheuvel wrote:
>>> On 11 March 2017 at 10:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
>>>>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>>>>>> think the above check should be reworked to look for the FADT
>>>>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>>>>>> from these helper functions. No driver outside of
>>>>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>>>>>> always be part of QEMU's ACPI payload, if it generates one.
>>>>>>
>>>>>> OK, that would get things working again, I suppose. But do we want
>>>>>> neutered ACPI tables to be exposed at all, even if there is a DT in
>>>>>> that case to boot from?
>>>>>
>>>>> I think the neutered ACPI tables (on -no-acpi) should be fine. The
>>>>> upstream Linux guest will prefer DT if it is present;
>>>>
>>>> Yes, but we've already determined that this situation is suboptimal,
>>>> which was what triggered this changeset to begin with.
>>>>
>>>
>>> Indeed. Having an ACPI entry point config table installed, but not
>>> having the essential tables that allow you to boot is a situation that
>>> we should try very hard to avoid IMO
>>>
>>
>> That requires dynamic disabling of AcpiTableDxe -- so that EFI_ACPI_TABLE_PROTOCOL is not produced, despite the driver being loaded --, and equipping all clients of EFI_ACPI_TABLE_PROTOCOL to fail gracefully when the protocol is not found. Alternatively, let AcpiTableDxe install the protocol, but prevent all clients from calling it.
>>
> 
> Not really. We only care about what the OS gets to see, not what the
> firmware ends up doing internally. So, say, at ReadyToBoot, we could
> check that the ACPI entry point points to what we need minimally to
> boot successfully, otherwise we rip it out. This will trigger the
> recently added change to install the DT if no ACPI entry point was
> found.
> 
> Not pretty, I know,

That's an understatement.

> but in this case, the handover state when invoking
> the OS is much more important than clean handling in the firmware
> itself.

The goal of the firmware is to boot the OS. Therefore the above argument
could be used to justify anything and everything at all in the firmware.

Anyway, here's a technical counter-argument too (i.e., against ripping
out ACPI altogether at ReadyToBoot): the ArmVirtPkg platforms include

  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf

In this driver, in function RamDiskDxeEntryPoint()
[MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDriver.c], we have a call
to EfiCreateEventReadyToBootEx(), which registers the RamDiskAcpiCheck()
callback function.

The leading comment on the RamDiskAcpiCheck() function (rewrapped here
for readability) is:

  Check whether EFI_ACPI_TABLE_PROTOCOL and EFI_ACPI_SDT_PROTOCOL are
  produced. If both protocols are produced, publish all the reserved
  memory type RAM disks to the NVDIMM Firmware Interface Table (NFIT).

We do have both protocols available; see commit d36447418d32
("ArmVirtPkg: Add Ramdisk support to ArmVirtPkg platforms", 2016-08-19).

In turn, RamDiskPublishNfit()
[MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c]:
- removes the NFIT and installs an updated one, if an NFIT exists to
begin with,
- creates both an SSDT (with RamDiskPublishSsdt()) and an NFIT, otherwise.

If RamDiskAcpiCheck() --> RamDiskPublishNfit() run after our callback --
in which we'd null the ACPI root pointer in the sysconfig table, IIUC
--, then further use of the ACPI protocols
- might crash,
- might transparently undo our nulling,
- or might even do what we want (that is, "nothing").

To me the behavior looks unpredictable.

Based on RamDiskDxe's workings, perhaps it is safe to say that the edk2
drivers equate the presence of the ACPI protocols to saying "this is an
ACPI system". If that's indeed the case, perhaps we need to make the
ACPI protocols *not* appear, dynamically.

Here's an idea how to implement that, cleanly:

(1) Introduce two new (empty / NULL) protocols with GUIDs
gAcpiSystemProtocolGuid and gDtSystemProtocolGuid.

(2) Introduce a plugin library instance (== no explicit class) that has
an empty constructor function (returning RETURN_SUCCESS), a DEPEX on
gAcpiSystemProtocolGuid, and nothing else. Let's call this AcpiSystemLib.

(3) In "ArmVirt.dsc.inc", hook AcpiSystemLib into
"MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf", via NULL
class resolution. This will imbue AcpiTableDxe with a DEPEX on
gAcpiSystemProtocolGuid.

(4) In FdtClientDxe, register a protocol notify callback for
gDtSystemProtocolGuid. In the callback, install the DT as a sysconfig table.

(5) Introduce a new DXE driver, called AcpiDtSystemDxe, which only
depends on QemuFwCfgLib (cleanly, again), which in turn makes it depend
on gFdtClientProtocolGuid. In AcpiDtSystemDxe's entry point, look up the
"etc/table-loader" fw_cfg file.

If the file exists (don't select or download it! just check the
existence), install gAcpiSystemProtocolGuid, and exit. That will unblock
AcpiTableDxe, and everything that depends on the ACPI protocols (via
depex or protocol notify).

Otherwise, install gDtSystemProtocolGuid in the entry point, and exit.
That will cause FdtClientDxe to install the sysconfig table.


This design takes a few new modules, but:
- it operates only with valid edk2 tools and means, zero hacks,
- it depends on no ReadyToBoot callbacks,
- it needs no modifications for core drivers,
- it does not try to remove resources once published.

Note that step (5) will turn Xen guests into constantly DT-only systems.
I don't know if that's appropriate. I guess Xen can expose another knob
(different from fw_cfg) to make this switch work.

Alternatively, if you want to stick with the status quo for Xen (both
ACPI and DT), implement XenAcpiDtSystemDxe, which would install *both*
protocols unconditionally. Then you'll get both DT and ACPI in the Xen
guest, same as now, and XenAcpiDtSystemDxe would be subject for further
customization.

If you want me to, I can work on this (I have a downstream RHBZ for the
feature, so I can justify spending time on this, even in the current
phase/state of RHEL development). I think I would start with reverting
patches #3 and #2 in this series.

If you agree with the idea and prefer to work on it yourself, that's
even better.

What do you think?

Thanks
Laszlo

> 
> 
>> We've done something like in the past with PcdAcpiS3Enable. It required IntelFrameworkModulePkg, MdeModulePkg and UefiCpuPkg changes.
>>
>> $ git grep -l -w InstallAcpiTable
>>
>> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/AcpiTables.c
>> ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
>> EdkCompatibilityPkg/Foundation/Efi/Protocol/AcpiTable/AcpiTable.h
>> EmbeddedPkg/Library/AcpiLib/AcpiLib.c
>> IntelFrameworkModulePkg/Universal/Acpi/AcpiSupportDxe/AcpiSupportAcpiSupportProtocol.c
>> MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatform.c
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
>> MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
>> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c
>> MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c
>> MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
>> MdePkg/Include/Protocol/AcpiTable.h
>> NetworkPkg/IScsiDxe/IScsiIbft.c
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> OvmfPkg/AcpiPlatformDxe/Qemu.c
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> OvmfPkg/AcpiPlatformDxe/Xen.c
>> QuarkPlatformPkg/Acpi/Dxe/AcpiPlatform/AcpiPlatform.c
>> QuarkPlatformPkg/Acpi/DxeSmm/SmmPowerManagement/Ppm.c
>> SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
>> SecurityPkg/Tcg/TcgDxe/TcgDxe.c
>> SecurityPkg/Tcg/TcgSmm/TcgSmm.c
>> SecurityPkg/Tcg/TrEEDxe/TrEEDxe.c
>> SecurityPkg/Tcg/TrEESmm/TrEESmm.c
>> UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.c
>>
>> (Some of these hits don't belong to EFI_ACPI_TABLE_PROTOCOL clients, of course.)
>>
>> Thanks
>> Laszlo

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/13/17 21:39, Laszlo Ersek wrote:
> On 03/13/17 15:59, Ard Biesheuvel wrote:
>> On 13 March 2017 at 14:53, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 03/13/17 11:23, Ard Biesheuvel wrote:
>>>> On 11 March 2017 at 10:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
>>>>>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>>>>>>> think the above check should be reworked to look for the FADT
>>>>>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>>>>>>> from these helper functions. No driver outside of
>>>>>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>>>>>>> always be part of QEMU's ACPI payload, if it generates one.
>>>>>>>
>>>>>>> OK, that would get things working again, I suppose. But do we want
>>>>>>> neutered ACPI tables to be exposed at all, even if there is a DT in
>>>>>>> that case to boot from?
>>>>>>
>>>>>> I think the neutered ACPI tables (on -no-acpi) should be fine. The
>>>>>> upstream Linux guest will prefer DT if it is present;
>>>>>
>>>>> Yes, but we've already determined that this situation is suboptimal,
>>>>> which was what triggered this changeset to begin with.
>>>>>
>>>>
>>>> Indeed. Having an ACPI entry point config table installed, but not
>>>> having the essential tables that allow you to boot is a situation that
>>>> we should try very hard to avoid IMO
>>>>
>>>
>>> That requires dynamic disabling of AcpiTableDxe -- so that EFI_ACPI_TABLE_PROTOCOL is not produced, despite the driver being loaded --, and equipping all clients of EFI_ACPI_TABLE_PROTOCOL to fail gracefully when the protocol is not found. Alternatively, let AcpiTableDxe install the protocol, but prevent all clients from calling it.
>>>
>>
>> Not really. We only care about what the OS gets to see, not what the
>> firmware ends up doing internally. So, say, at ReadyToBoot, we could
>> check that the ACPI entry point points to what we need minimally to
>> boot successfully, otherwise we rip it out. This will trigger the
>> recently added change to install the DT if no ACPI entry point was
>> found.
>>
>> Not pretty, I know,
> 
> That's an understatement.
> 
>> but in this case, the handover state when invoking
>> the OS is much more important than clean handling in the firmware
>> itself.
> 
> The goal of the firmware is to boot the OS. Therefore the above argument
> could be used to justify anything and everything at all in the firmware.
> 
> Anyway, here's a technical counter-argument too (i.e., against ripping
> out ACPI altogether at ReadyToBoot): the ArmVirtPkg platforms include
> 
>   MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> 
> In this driver, in function RamDiskDxeEntryPoint()
> [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDriver.c], we have a call
> to EfiCreateEventReadyToBootEx(), which registers the RamDiskAcpiCheck()
> callback function.
> 
> The leading comment on the RamDiskAcpiCheck() function (rewrapped here
> for readability) is:
> 
>   Check whether EFI_ACPI_TABLE_PROTOCOL and EFI_ACPI_SDT_PROTOCOL are
>   produced. If both protocols are produced, publish all the reserved
>   memory type RAM disks to the NVDIMM Firmware Interface Table (NFIT).
> 
> We do have both protocols available; see commit d36447418d32
> ("ArmVirtPkg: Add Ramdisk support to ArmVirtPkg platforms", 2016-08-19).
> 
> In turn, RamDiskPublishNfit()
> [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c]:
> - removes the NFIT and installs an updated one, if an NFIT exists to
> begin with,
> - creates both an SSDT (with RamDiskPublishSsdt()) and an NFIT, otherwise.
> 
> If RamDiskAcpiCheck() --> RamDiskPublishNfit() run after our callback --
> in which we'd null the ACPI root pointer in the sysconfig table, IIUC
> --, then further use of the ACPI protocols
> - might crash,
> - might transparently undo our nulling,
> - or might even do what we want (that is, "nothing").
> 
> To me the behavior looks unpredictable.
> 
> Based on RamDiskDxe's workings, perhaps it is safe to say that the edk2
> drivers equate the presence of the ACPI protocols to saying "this is an
> ACPI system". If that's indeed the case, perhaps we need to make the
> ACPI protocols *not* appear, dynamically.
> 
> Here's an idea how to implement that, cleanly:
> 
> (1) Introduce two new (empty / NULL) protocols with GUIDs
> gAcpiSystemProtocolGuid and gDtSystemProtocolGuid.
> 
> (2) Introduce a plugin library instance (== no explicit class) that has
> an empty constructor function (returning RETURN_SUCCESS), a DEPEX on
> gAcpiSystemProtocolGuid, and nothing else. Let's call this AcpiSystemLib.
> 
> (3) In "ArmVirt.dsc.inc", hook AcpiSystemLib into
> "MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf", via NULL
> class resolution. This will imbue AcpiTableDxe with a DEPEX on
> gAcpiSystemProtocolGuid.
> 
> (4) In FdtClientDxe, register a protocol notify callback for
> gDtSystemProtocolGuid. In the callback, install the DT as a sysconfig table.
> 
> (5) Introduce a new DXE driver, called AcpiDtSystemDxe, which only
> depends on QemuFwCfgLib (cleanly, again), which in turn makes it depend
> on gFdtClientProtocolGuid. In AcpiDtSystemDxe's entry point, look up the
> "etc/table-loader" fw_cfg file.
> 
> If the file exists (don't select or download it! just check the
> existence), install gAcpiSystemProtocolGuid, and exit. That will unblock
> AcpiTableDxe, and everything that depends on the ACPI protocols (via
> depex or protocol notify).
> 
> Otherwise, install gDtSystemProtocolGuid in the entry point, and exit.
> That will cause FdtClientDxe to install the sysconfig table.
> 
> 
> This design takes a few new modules, but:
> - it operates only with valid edk2 tools and means, zero hacks,
> - it depends on no ReadyToBoot callbacks,
> - it needs no modifications for core drivers,
> - it does not try to remove resources once published.
> 
> Note that step (5) will turn Xen guests into constantly DT-only systems.
> I don't know if that's appropriate. I guess Xen can expose another knob
> (different from fw_cfg) to make this switch work.
> 
> Alternatively, if you want to stick with the status quo for Xen (both
> ACPI and DT), implement XenAcpiDtSystemDxe, which would install *both*
> protocols unconditionally. Then you'll get both DT and ACPI in the Xen
> guest, same as now, and XenAcpiDtSystemDxe would be subject for further
> customization.

NB, you could use the same approach in firmware for physical system; the
only component you'd have to replace would be AcpiDtSystemDxe. Namely,
it would not depend on fw_cfg, but on an HII setting. It would

(a) install an HII form with a checkbox, so that the knob (stored in a
non-volatile UEFI variable) could be toggled for the next boot, and

(b) in the entry point, fetch the variable, and install either
gAcpiSystemProtocolGuid or gDtSystemProtocolGuid, as appropriate.

With this -- that is, with physical systems / HII -- in mind, I think
that the following components should be introduced under ArmPkg, not
ArmVirtPkg:

- gAcpiSystemProtocolGuid and gDtSystemProtocolGuid
- the AcpiSystemLib plugin (with the DEPEX)

Hm, well, okay, FdtClientDxe is also platform specific, so whatever
driver the physical system uses to expose the DT to the OS would have to
be adapted to gDtSystemProtocolGuid, with a protocol notify.

This sounds good to me (if I may say so about my own idea, sorry). Thoughts?

Thanks,
Laszlo

> If you want me to, I can work on this (I have a downstream RHBZ for the
> feature, so I can justify spending time on this, even in the current
> phase/state of RHEL development). I think I would start with reverting
> patches #3 and #2 in this series.
> 
> If you agree with the idea and prefer to work on it yourself, that's
> even better.
> 
> What do you think?
> 
> Thanks
> Laszlo
> 
>>
>>
>>> We've done something like in the past with PcdAcpiS3Enable. It required IntelFrameworkModulePkg, MdeModulePkg and UefiCpuPkg changes.
>>>
>>> $ git grep -l -w InstallAcpiTable
>>>
>>> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/AcpiTables.c
>>> ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
>>> EdkCompatibilityPkg/Foundation/Efi/Protocol/AcpiTable/AcpiTable.h
>>> EmbeddedPkg/Library/AcpiLib/AcpiLib.c
>>> IntelFrameworkModulePkg/Universal/Acpi/AcpiSupportDxe/AcpiSupportAcpiSupportProtocol.c
>>> MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatform.c
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>> MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
>>> MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableDxe/FirmwarePerformanceDxe.c
>>> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c
>>> MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c
>>> MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
>>> MdePkg/Include/Protocol/AcpiTable.h
>>> NetworkPkg/IScsiDxe/IScsiIbft.c
>>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>>> OvmfPkg/AcpiPlatformDxe/Qemu.c
>>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>>> OvmfPkg/AcpiPlatformDxe/Xen.c
>>> QuarkPlatformPkg/Acpi/Dxe/AcpiPlatform/AcpiPlatform.c
>>> QuarkPlatformPkg/Acpi/DxeSmm/SmmPowerManagement/Ppm.c
>>> SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
>>> SecurityPkg/Tcg/TcgDxe/TcgDxe.c
>>> SecurityPkg/Tcg/TcgSmm/TcgSmm.c
>>> SecurityPkg/Tcg/TrEEDxe/TrEEDxe.c
>>> SecurityPkg/Tcg/TrEESmm/TrEESmm.c
>>> UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.c
>>>
>>> (Some of these hits don't belong to EFI_ACPI_TABLE_PROTOCOL clients, of course.)
>>>
>>> Thanks
>>> Laszlo
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel]

On 13 March 2017 at 20:39, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/13/17 15:59, Ard Biesheuvel wrote:
>> On 13 March 2017 at 14:53, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 03/13/17 11:23, Ard Biesheuvel wrote:
>>>> On 11 March 2017 at 10:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>> On Sat, Mar 11, 2017 at 08:38:19AM +0100, Laszlo Ersek wrote:
>>>>>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>>>>>>> think the above check should be reworked to look for the FADT
>>>>>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>>>>>>> from these helper functions. No driver outside of
>>>>>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>>>>>>> always be part of QEMU's ACPI payload, if it generates one.
>>>>>>>
>>>>>>> OK, that would get things working again, I suppose. But do we want
>>>>>>> neutered ACPI tables to be exposed at all, even if there is a DT in
>>>>>>> that case to boot from?
>>>>>>
>>>>>> I think the neutered ACPI tables (on -no-acpi) should be fine. The
>>>>>> upstream Linux guest will prefer DT if it is present;
>>>>>
>>>>> Yes, but we've already determined that this situation is suboptimal,
>>>>> which was what triggered this changeset to begin with.
>>>>>
>>>>
>>>> Indeed. Having an ACPI entry point config table installed, but not
>>>> having the essential tables that allow you to boot is a situation that
>>>> we should try very hard to avoid IMO
>>>>
>>>
>>> That requires dynamic disabling of AcpiTableDxe -- so that EFI_ACPI_TABLE_PROTOCOL is not produced, despite the driver being loaded --, and equipping all clients of EFI_ACPI_TABLE_PROTOCOL to fail gracefully when the protocol is not found. Alternatively, let AcpiTableDxe install the protocol, but prevent all clients from calling it.
>>>
>>
>> Not really. We only care about what the OS gets to see, not what the
>> firmware ends up doing internally. So, say, at ReadyToBoot, we could
>> check that the ACPI entry point points to what we need minimally to
>> boot successfully, otherwise we rip it out. This will trigger the
>> recently added change to install the DT if no ACPI entry point was
>> found.
>>
>> Not pretty, I know,
>
> That's an understatement.
>
>> but in this case, the handover state when invoking
>> the OS is much more important than clean handling in the firmware
>> itself.
>
> The goal of the firmware is to boot the OS. Therefore the above argument
> could be used to justify anything and everything at all in the firmware.
>
> Anyway, here's a technical counter-argument too (i.e., against ripping
> out ACPI altogether at ReadyToBoot): the ArmVirtPkg platforms include
>
>   MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
>
> In this driver, in function RamDiskDxeEntryPoint()
> [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDriver.c], we have a call
> to EfiCreateEventReadyToBootEx(), which registers the RamDiskAcpiCheck()
> callback function.
>
> The leading comment on the RamDiskAcpiCheck() function (rewrapped here
> for readability) is:
>
>   Check whether EFI_ACPI_TABLE_PROTOCOL and EFI_ACPI_SDT_PROTOCOL are
>   produced. If both protocols are produced, publish all the reserved
>   memory type RAM disks to the NVDIMM Firmware Interface Table (NFIT).
>
> We do have both protocols available; see commit d36447418d32
> ("ArmVirtPkg: Add Ramdisk support to ArmVirtPkg platforms", 2016-08-19).
>
> In turn, RamDiskPublishNfit()
> [MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c]:
> - removes the NFIT and installs an updated one, if an NFIT exists to
> begin with,
> - creates both an SSDT (with RamDiskPublishSsdt()) and an NFIT, otherwise.
>
> If RamDiskAcpiCheck() --> RamDiskPublishNfit() run after our callback --
> in which we'd null the ACPI root pointer in the sysconfig table, IIUC
> --, then further use of the ACPI protocols
> - might crash,
> - might transparently undo our nulling,
> - or might even do what we want (that is, "nothing").
>
> To me the behavior looks unpredictable.
>

Talk about understatements :-)

> Based on RamDiskDxe's workings, perhaps it is safe to say that the edk2
> drivers equate the presence of the ACPI protocols to saying "this is an
> ACPI system". If that's indeed the case, perhaps we need to make the
> ACPI protocols *not* appear, dynamically.
>
> Here's an idea how to implement that, cleanly:
>
> (1) Introduce two new (empty / NULL) protocols with GUIDs
> gAcpiSystemProtocolGuid and gDtSystemProtocolGuid.
>
> (2) Introduce a plugin library instance (== no explicit class) that has
> an empty constructor function (returning RETURN_SUCCESS), a DEPEX on
> gAcpiSystemProtocolGuid, and nothing else. Let's call this AcpiSystemLib.
>
> (3) In "ArmVirt.dsc.inc", hook AcpiSystemLib into
> "MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf", via NULL
> class resolution. This will imbue AcpiTableDxe with a DEPEX on
> gAcpiSystemProtocolGuid.
>
> (4) In FdtClientDxe, register a protocol notify callback for
> gDtSystemProtocolGuid. In the callback, install the DT as a sysconfig table.
>
> (5) Introduce a new DXE driver, called AcpiDtSystemDxe, which only
> depends on QemuFwCfgLib (cleanly, again), which in turn makes it depend
> on gFdtClientProtocolGuid. In AcpiDtSystemDxe's entry point, look up the
> "etc/table-loader" fw_cfg file.
>
> If the file exists (don't select or download it! just check the
> existence), install gAcpiSystemProtocolGuid, and exit. That will unblock
> AcpiTableDxe, and everything that depends on the ACPI protocols (via
> depex or protocol notify).
>
> Otherwise, install gDtSystemProtocolGuid in the entry point, and exit.
> That will cause FdtClientDxe to install the sysconfig table.
>
>
> This design takes a few new modules, but:
> - it operates only with valid edk2 tools and means, zero hacks,
> - it depends on no ReadyToBoot callbacks,
> - it needs no modifications for core drivers,
> - it does not try to remove resources once published.
>
> Note that step (5) will turn Xen guests into constantly DT-only systems.
> I don't know if that's appropriate. I guess Xen can expose another knob
> (different from fw_cfg) to make this switch work.
>

I think the desire is to support ACPI on Xen/arm64 hosts, but it does
not seem to me that the current code would do anything useful in that
regard.

> Alternatively, if you want to stick with the status quo for Xen (both
> ACPI and DT), implement XenAcpiDtSystemDxe, which would install *both*
> protocols unconditionally. Then you'll get both DT and ACPI in the Xen
> guest, same as now, and XenAcpiDtSystemDxe would be subject for further
> customization.
>
> If you want me to, I can work on this (I have a downstream RHBZ for the
> feature, so I can justify spending time on this, even in the current
> phase/state of RHEL development). I think I would start with reverting
> patches #3 and #2 in this series.
>
> If you agree with the idea and prefer to work on it yourself, that's
> even better.
>
> What do you think?
>

I like it! There is very little new logic or processing required at
runtime, and everything else is managed by DxeCore's protocol
dispatch.

I will look into this tomorrow.

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/11/17 06:55, Laszlo Ersek wrote:
> On 03/09/17 17:04, Ard Biesheuvel wrote:
>> Instead of having a build time switch to prevent the FDT configuration
>> table from being installed, make this behavior dependent on whether we
>> are passing ACPI tables to the OS. This is done by looking for the
>> ACPI 2.0 configuration table, and only installing the FDT one if the
>> ACPI one cannot be found.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>>  ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>>  4 files changed, 12 insertions(+), 23 deletions(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>> index a5ec42166445..efe83a383d55 100644
>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>    # EFI_VT_100_GUID.
>>    #
>>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>> -
>> -[PcdsFeatureFlag]
>> -  #
>> -  # Pure ACPI boot
>> -  #
>> -  # Inhibit installation of the FDT as a configuration table if this feature
>> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
>> -  # description of the platform, and it is up to the OS to choose.
>> -  #
>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>> index 477dfdcfc764..7b266b98b949 100644
>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>> @@ -34,7 +34,6 @@ [Defines]
>>    # -D FLAG=VALUE
>>    #
>>    DEFINE SECURE_BOOT_ENABLE      = FALSE
>> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>>  
>>  !include ArmVirtPkg/ArmVirt.dsc.inc
>>  
>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>>  
>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
>> -!endif
>> -
>>  [PcdsFixedAtBuild.common]
>>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>>  !if $(ARCH) == AARCH64
>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> index 0327af5739f2..2981977f3d20 100644
>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> @@ -17,6 +17,7 @@
>>  #include <Library/DebugLib.h>
>>  #include <Library/UefiDriverEntryPoint.h>
>>  #include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>>  #include <Library/HobLib.h>
>>  #include <libfdt.h>
>>  
>> @@ -312,12 +313,16 @@ OnReadyToBoot (
>>    )
>>  {
>>    EFI_STATUS      Status;
>> +  VOID            *Table;
>>  
>> -  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.
>> -    //
>> +  //
>> +  // Only install the FDT as a configuration table if we are not exposing
>> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
>> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
>> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
>> +  //
>> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
>> +  if (EFI_ERROR (Status) || Table == NULL) {
>>      Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>>      ASSERT_EFI_ERROR (Status);
>>    }
> 
> This code breaks the DT-only ("-no-acpi") boot with boot graphics
> (virtio-gpu) enabled.
> 
> Namely, we recently included BootGraphicsResourceTableDxe in the build.
> That driver calls InstallAcpiTable() for BGRT, which in turn causes
> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().
> 
> We all missed that just because QEMU doesn't produce ACPI payload (and
> we consequently don't install it), other drivers in edk2 may
> unconditionally install "auxiliary" ACPI tables, which minimally trigger
> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
> Such a crippled set of ACPI tables isn't sufficient for booting however.
> 
> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
> and ScanTableInRSDT() in
> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
> think the above check should be reworked to look for the FADT
> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
> from these helper functions. No driver outside of
> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
> always be part of QEMU's ACPI payload, if it generates one.

... BTW this is exactly the kind of hard-to-predict unreliability of
ReadyToBoot callbacks that I was worried about. The patches that I
posted looked for the "etc/table-loader" fw_cfg file, which directly
corresponds to the absence of the "-no-acpi" switch. With a ReadyToBoot
callback, we depend on a larger, and fuzzier, pile of state.

I'm not suggesting that we return to my series -- I think this one can
be fixed up by looking for the FADT in particular --, I'd just like if
we all grew a healthy aversion and distrust to ReadyToBoot callbacks.
Their perceived simplicity is deceptive.

Thanks
Laszlo

> 
>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>> index 00017727c32c..9861f41e968b 100644
>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>> @@ -37,17 +37,16 @@ [LibraryClasses]
>>    HobLib
>>    UefiBootServicesTableLib
>>    UefiDriverEntryPoint
>> +  UefiLib
>>  
>>  [Protocols]
>>    gFdtClientProtocolGuid                  ## PRODUCES
>>  
>>  [Guids]
>> +  gEfiAcpi20TableGuid
>>    gEfiEventReadyToBootGuid
>>    gFdtHobGuid
>>    gFdtTableGuid
>>  
>> -[FeaturePcd]
>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
>> -
>>  [Depex]
>>    TRUE
>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel]

> On 11 Mar 2017, at 08:21, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 03/11/17 06:55, Laszlo Ersek wrote:
>>> On 03/09/17 17:04, Ard Biesheuvel wrote:
>>> Instead of having a build time switch to prevent the FDT configuration
>>> table from being installed, make this behavior dependent on whether we
>>> are passing ACPI tables to the OS. This is done by looking for the
>>> ACPI 2.0 configuration table, and only installing the FDT one if the
>>> ACPI one cannot be found.
>>> 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>>> ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>>> 4 files changed, 12 insertions(+), 23 deletions(-)
>>> 
>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>> index a5ec42166445..efe83a383d55 100644
>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>   # EFI_VT_100_GUID.
>>>   #
>>>   gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>>> -
>>> -[PcdsFeatureFlag]
>>> -  #
>>> -  # Pure ACPI boot
>>> -  #
>>> -  # Inhibit installation of the FDT as a configuration table if this feature
>>> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
>>> -  # description of the platform, and it is up to the OS to choose.
>>> -  #
>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>> index 477dfdcfc764..7b266b98b949 100644
>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>> @@ -34,7 +34,6 @@ [Defines]
>>>   # -D FLAG=VALUE
>>>   #
>>>   DEFINE SECURE_BOOT_ENABLE      = FALSE
>>> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>>> 
>>> !include ArmVirtPkg/ArmVirt.dsc.inc
>>> 
>>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>>> 
>>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
>>> -!endif
>>> -
>>> [PcdsFixedAtBuild.common]
>>>   gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>>> !if $(ARCH) == AARCH64
>>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>> index 0327af5739f2..2981977f3d20 100644
>>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>> @@ -17,6 +17,7 @@
>>> #include <Library/DebugLib.h>
>>> #include <Library/UefiDriverEntryPoint.h>
>>> #include <Library/UefiBootServicesTableLib.h>
>>> +#include <Library/UefiLib.h>
>>> #include <Library/HobLib.h>
>>> #include <libfdt.h>
>>> 
>>> @@ -312,12 +313,16 @@ OnReadyToBoot (
>>>   )
>>> {
>>>   EFI_STATUS      Status;
>>> +  VOID            *Table;
>>> 
>>> -  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.
>>> -    //
>>> +  //
>>> +  // Only install the FDT as a configuration table if we are not exposing
>>> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
>>> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
>>> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
>>> +  //
>>> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
>>> +  if (EFI_ERROR (Status) || Table == NULL) {
>>>     Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>>>     ASSERT_EFI_ERROR (Status);
>>>   }
>> 
>> This code breaks the DT-only ("-no-acpi") boot with boot graphics
>> (virtio-gpu) enabled.
>> 
>> Namely, we recently included BootGraphicsResourceTableDxe in the build.
>> That driver calls InstallAcpiTable() for BGRT, which in turn causes
>> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().
>> 
>> We all missed that just because QEMU doesn't produce ACPI payload (and
>> we consequently don't install it), other drivers in edk2 may
>> unconditionally install "auxiliary" ACPI tables, which minimally trigger
>> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
>> Such a crippled set of ACPI tables isn't sufficient for booting however.
>> 
>> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
>> and ScanTableInRSDT() in
>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>> think the above check should be reworked to look for the FADT
>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>> from these helper functions. No driver outside of
>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>> always be part of QEMU's ACPI payload, if it generates one.
> 
> ... BTW this is exactly the kind of hard-to-predict unreliability of
> ReadyToBoot callbacks that I was worried about. The patches that I
> posted looked for the "etc/table-loader" fw_cfg file, which directly
> corresponds to the absence of the "-no-acpi" switch. With a ReadyToBoot
> callback, we depend on a larger, and fuzzier, pile of state.
> 
> I'm not suggesting that we return to my series -- I think this one can
> be fixed up by looking for the FADT in particular --, I'd just like if
> we all grew a healthy aversion and distrust to ReadyToBoot callbacks.
> Their perceived simplicity is deceptive.
> 

Point taken. But couldn't we still check the existence of that at ReadyToBoot?

> 
>> 
>>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>>> index 00017727c32c..9861f41e968b 100644
>>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>>> @@ -37,17 +37,16 @@ [LibraryClasses]
>>>   HobLib
>>>   UefiBootServicesTableLib
>>>   UefiDriverEntryPoint
>>> +  UefiLib
>>> 
>>> [Protocols]
>>>   gFdtClientProtocolGuid                  ## PRODUCES
>>> 
>>> [Guids]
>>> +  gEfiAcpi20TableGuid
>>>   gEfiEventReadyToBootGuid
>>>   gFdtHobGuid
>>>   gFdtTableGuid
>>> 
>>> -[FeaturePcd]
>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
>>> -
>>> [Depex]
>>>   TRUE
>>> 
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>> 
> 

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/11/17 08:30, Ard Biesheuvel wrote:
> 
>> On 11 Mar 2017, at 08:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>> On 03/11/17 06:55, Laszlo Ersek wrote:
>>>> On 03/09/17 17:04, Ard Biesheuvel wrote:
>>>> Instead of having a build time switch to prevent the FDT configuration
>>>> table from being installed, make this behavior dependent on whether we
>>>> are passing ACPI tables to the OS. This is done by looking for the
>>>> ACPI 2.0 configuration table, and only installing the FDT one if the
>>>> ACPI one cannot be found.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>> ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>>>> ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>>>> 4 files changed, 12 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>>> index a5ec42166445..efe83a383d55 100644
>>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>>   # EFI_VT_100_GUID.
>>>>   #
>>>>   gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>>>> -
>>>> -[PcdsFeatureFlag]
>>>> -  #
>>>> -  # Pure ACPI boot
>>>> -  #
>>>> -  # Inhibit installation of the FDT as a configuration table if this feature
>>>> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
>>>> -  # description of the platform, and it is up to the OS to choose.
>>>> -  #
>>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
>>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>>> index 477dfdcfc764..7b266b98b949 100644
>>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>>> @@ -34,7 +34,6 @@ [Defines]
>>>>   # -D FLAG=VALUE
>>>>   #
>>>>   DEFINE SECURE_BOOT_ENABLE      = FALSE
>>>> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>>>>
>>>> !include ArmVirtPkg/ArmVirt.dsc.inc
>>>>
>>>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>>>>
>>>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
>>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
>>>> -!endif
>>>> -
>>>> [PcdsFixedAtBuild.common]
>>>>   gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>>>> !if $(ARCH) == AARCH64
>>>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>> index 0327af5739f2..2981977f3d20 100644
>>>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>> @@ -17,6 +17,7 @@
>>>> #include <Library/DebugLib.h>
>>>> #include <Library/UefiDriverEntryPoint.h>
>>>> #include <Library/UefiBootServicesTableLib.h>
>>>> +#include <Library/UefiLib.h>
>>>> #include <Library/HobLib.h>
>>>> #include <libfdt.h>
>>>>
>>>> @@ -312,12 +313,16 @@ OnReadyToBoot (
>>>>   )
>>>> {
>>>>   EFI_STATUS      Status;
>>>> +  VOID            *Table;
>>>>
>>>> -  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.
>>>> -    //
>>>> +  //
>>>> +  // Only install the FDT as a configuration table if we are not exposing
>>>> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
>>>> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
>>>> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
>>>> +  //
>>>> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
>>>> +  if (EFI_ERROR (Status) || Table == NULL) {
>>>>     Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>>>>     ASSERT_EFI_ERROR (Status);
>>>>   }
>>>
>>> This code breaks the DT-only ("-no-acpi") boot with boot graphics
>>> (virtio-gpu) enabled.
>>>
>>> Namely, we recently included BootGraphicsResourceTableDxe in the build.
>>> That driver calls InstallAcpiTable() for BGRT, which in turn causes
>>> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().
>>>
>>> We all missed that just because QEMU doesn't produce ACPI payload (and
>>> we consequently don't install it), other drivers in edk2 may
>>> unconditionally install "auxiliary" ACPI tables, which minimally trigger
>>> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
>>> Such a crippled set of ACPI tables isn't sufficient for booting however.
>>>
>>> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
>>> and ScanTableInRSDT() in
>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>> think the above check should be reworked to look for the FADT
>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>> from these helper functions. No driver outside of
>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>> always be part of QEMU's ACPI payload, if it generates one.
>>
>> ... BTW this is exactly the kind of hard-to-predict unreliability of
>> ReadyToBoot callbacks that I was worried about. The patches that I
>> posted looked for the "etc/table-loader" fw_cfg file, which directly
>> corresponds to the absence of the "-no-acpi" switch. With a ReadyToBoot
>> callback, we depend on a larger, and fuzzier, pile of state.
>>
>> I'm not suggesting that we return to my series -- I think this one can
>> be fixed up by looking for the FADT in particular --, I'd just like if
>> we all grew a healthy aversion and distrust to ReadyToBoot callbacks.
>> Their perceived simplicity is deceptive.
>>
> 
> Point taken. But couldn't we still check the existence of that at ReadyToBoot?

We could, but that would bring back the new INF file for QemuFwCfgLib,
and the explicit constructor call in FdtClientDxe (assuming we continue
to register the callback in FdtClientDxe -- I think it does belong there).

So that would be worst of both worlds.

Laszlo

> 
>>
>>>
>>>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>>>> index 00017727c32c..9861f41e968b 100644
>>>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>>>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
>>>> @@ -37,17 +37,16 @@ [LibraryClasses]
>>>>   HobLib
>>>>   UefiBootServicesTableLib
>>>>   UefiDriverEntryPoint
>>>> +  UefiLib
>>>>
>>>> [Protocols]
>>>>   gFdtClientProtocolGuid                  ## PRODUCES
>>>>
>>>> [Guids]
>>>> +  gEfiAcpi20TableGuid
>>>>   gEfiEventReadyToBootGuid
>>>>   gFdtHobGuid
>>>>   gFdtTableGuid
>>>>
>>>> -[FeaturePcd]
>>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
>>>> -
>>>> [Depex]
>>>>   TRUE
>>>>
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Ard Biesheuvel]

On 11 March 2017 at 08:41, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/11/17 08:30, Ard Biesheuvel wrote:
>>
>>> On 11 Mar 2017, at 08:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 03/11/17 06:55, Laszlo Ersek wrote:
>>>>> On 03/09/17 17:04, Ard Biesheuvel wrote:
>>>>> Instead of having a build time switch to prevent the FDT configuration
>>>>> table from being installed, make this behavior dependent on whether we
>>>>> are passing ACPI tables to the OS. This is done by looking for the
>>>>> ACPI 2.0 configuration table, and only installing the FDT one if the
>>>>> ACPI one cannot be found.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>> ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>>>>> ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>>>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>>>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>>>>> 4 files changed, 12 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>>>> index a5ec42166445..efe83a383d55 100644
>>>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>>>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>>>   # EFI_VT_100_GUID.
>>>>>   #
>>>>>   gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>>>>> -
>>>>> -[PcdsFeatureFlag]
>>>>> -  #
>>>>> -  # Pure ACPI boot
>>>>> -  #
>>>>> -  # Inhibit installation of the FDT as a configuration table if this feature
>>>>> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
>>>>> -  # description of the platform, and it is up to the OS to choose.
>>>>> -  #
>>>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
>>>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>>>> index 477dfdcfc764..7b266b98b949 100644
>>>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>>>> @@ -34,7 +34,6 @@ [Defines]
>>>>>   # -D FLAG=VALUE
>>>>>   #
>>>>>   DEFINE SECURE_BOOT_ENABLE      = FALSE
>>>>> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>>>>>
>>>>> !include ArmVirtPkg/ArmVirt.dsc.inc
>>>>>
>>>>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>>>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>>>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>>>>>
>>>>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
>>>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
>>>>> -!endif
>>>>> -
>>>>> [PcdsFixedAtBuild.common]
>>>>>   gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>>>>> !if $(ARCH) == AARCH64
>>>>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>>> index 0327af5739f2..2981977f3d20 100644
>>>>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>>> @@ -17,6 +17,7 @@
>>>>> #include <Library/DebugLib.h>
>>>>> #include <Library/UefiDriverEntryPoint.h>
>>>>> #include <Library/UefiBootServicesTableLib.h>
>>>>> +#include <Library/UefiLib.h>
>>>>> #include <Library/HobLib.h>
>>>>> #include <libfdt.h>
>>>>>
>>>>> @@ -312,12 +313,16 @@ OnReadyToBoot (
>>>>>   )
>>>>> {
>>>>>   EFI_STATUS      Status;
>>>>> +  VOID            *Table;
>>>>>
>>>>> -  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.
>>>>> -    //
>>>>> +  //
>>>>> +  // Only install the FDT as a configuration table if we are not exposing
>>>>> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
>>>>> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
>>>>> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
>>>>> +  //
>>>>> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
>>>>> +  if (EFI_ERROR (Status) || Table == NULL) {
>>>>>     Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>>>>>     ASSERT_EFI_ERROR (Status);
>>>>>   }
>>>>
>>>> This code breaks the DT-only ("-no-acpi") boot with boot graphics
>>>> (virtio-gpu) enabled.
>>>>
>>>> Namely, we recently included BootGraphicsResourceTableDxe in the build.
>>>> That driver calls InstallAcpiTable() for BGRT, which in turn causes
>>>> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().
>>>>
>>>> We all missed that just because QEMU doesn't produce ACPI payload (and
>>>> we consequently don't install it), other drivers in edk2 may
>>>> unconditionally install "auxiliary" ACPI tables, which minimally trigger
>>>> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
>>>> Such a crippled set of ACPI tables isn't sufficient for booting however.
>>>>
>>>> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
>>>> and ScanTableInRSDT() in
>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>>> think the above check should be reworked to look for the FADT
>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>>> from these helper functions. No driver outside of
>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>>> always be part of QEMU's ACPI payload, if it generates one.
>>>
>>> ... BTW this is exactly the kind of hard-to-predict unreliability of
>>> ReadyToBoot callbacks that I was worried about. The patches that I
>>> posted looked for the "etc/table-loader" fw_cfg file, which directly
>>> corresponds to the absence of the "-no-acpi" switch. With a ReadyToBoot
>>> callback, we depend on a larger, and fuzzier, pile of state.
>>>
>>> I'm not suggesting that we return to my series -- I think this one can
>>> be fixed up by looking for the FADT in particular --, I'd just like if
>>> we all grew a healthy aversion and distrust to ReadyToBoot callbacks.
>>> Their perceived simplicity is deceptive.
>>>
>>
>> Point taken. But couldn't we still check the existence of that at ReadyToBoot?
>
> We could, but that would bring back the new INF file for QemuFwCfgLib,
> and the explicit constructor call in FdtClientDxe (assuming we continue
> to register the callback in FdtClientDxe -- I think it does belong there).
>
> So that would be worst of both worlds.
>

Ah, of course. Silly me :-)

So my primary concern here, and I am glad we spotted it now, is that
the presence of neutered ACPI tables and no DT makes the system
unbootable. The existence of such firmware will, in turn, make it even
more difficult to convince the arm64 maintainers that ACPI should be
preferred over DT by default if both h/w descriptions are available.

So IIUC, firmwares the predate this series can never be affected in
such a way, right? (Unless you use a PURE_ACPI_BOOT build and pass
-no-acpi, in which case you will probably get the neutered tables but
you wouldn't have been able to boot in the first place)

So I will look into the FADT check on Monday, I agree that is the most
appropriate approach here.

Re: [PATCH 3/3] ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent

[Laszlo Ersek]

On 03/11/17 10:19, Ard Biesheuvel wrote:
> On 11 March 2017 at 08:41, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/11/17 08:30, Ard Biesheuvel wrote:
>>>
>>>> On 11 Mar 2017, at 08:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>
>>>>> On 03/11/17 06:55, Laszlo Ersek wrote:
>>>>>> On 03/09/17 17:04, Ard Biesheuvel wrote:
>>>>>> Instead of having a build time switch to prevent the FDT configuration
>>>>>> table from being installed, make this behavior dependent on whether we
>>>>>> are passing ACPI tables to the OS. This is done by looking for the
>>>>>> ACPI 2.0 configuration table, and only installing the FDT one if the
>>>>>> ACPI one cannot be found.
>>>>>>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>> ---
>>>>>> ArmVirtPkg/ArmVirtPkg.dec                | 10 ----------
>>>>>> ArmVirtPkg/ArmVirtQemu.dsc               |  5 -----
>>>>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 15 ++++++++++-----
>>>>>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 ++---
>>>>>> 4 files changed, 12 insertions(+), 23 deletions(-)
>>>>>>
>>>>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>>>>> index a5ec42166445..efe83a383d55 100644
>>>>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>>>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>>>>> @@ -58,13 +58,3 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>>>>   # EFI_VT_100_GUID.
>>>>>>   #
>>>>>>   gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>>>>>> -
>>>>>> -[PcdsFeatureFlag]
>>>>>> -  #
>>>>>> -  # Pure ACPI boot
>>>>>> -  #
>>>>>> -  # Inhibit installation of the FDT as a configuration table if this feature
>>>>>> -  # PCD is TRUE. Otherwise, the OS is presented with both a DT and an ACPI
>>>>>> -  # description of the platform, and it is up to the OS to choose.
>>>>>> -  #
>>>>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|FALSE|BOOLEAN|0x0000000a
>>>>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>>>>> index 477dfdcfc764..7b266b98b949 100644
>>>>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>>>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>>>>> @@ -34,7 +34,6 @@ [Defines]
>>>>>>   # -D FLAG=VALUE
>>>>>>   #
>>>>>>   DEFINE SECURE_BOOT_ENABLE      = FALSE
>>>>>> -  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>>>>>>
>>>>>> !include ArmVirtPkg/ArmVirt.dsc.inc
>>>>>>
>>>>>> @@ -94,10 +93,6 @@ [PcdsFeatureFlag.common]
>>>>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
>>>>>>   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>>>>>>
>>>>>> -!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
>>>>>> -  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
>>>>>> -!endif
>>>>>> -
>>>>>> [PcdsFixedAtBuild.common]
>>>>>>   gArmPlatformTokenSpaceGuid.PcdCoreCount|1
>>>>>> !if $(ARCH) == AARCH64
>>>>>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>>>> index 0327af5739f2..2981977f3d20 100644
>>>>>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>>>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>> #include <Library/DebugLib.h>
>>>>>> #include <Library/UefiDriverEntryPoint.h>
>>>>>> #include <Library/UefiBootServicesTableLib.h>
>>>>>> +#include <Library/UefiLib.h>
>>>>>> #include <Library/HobLib.h>
>>>>>> #include <libfdt.h>
>>>>>>
>>>>>> @@ -312,12 +313,16 @@ OnReadyToBoot (
>>>>>>   )
>>>>>> {
>>>>>>   EFI_STATUS      Status;
>>>>>> +  VOID            *Table;
>>>>>>
>>>>>> -  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.
>>>>>> -    //
>>>>>> +  //
>>>>>> +  // Only install the FDT as a configuration table if we are not exposing
>>>>>> +  // ACPI 2.0 (or later) tables. Note that the legacy ACPI table GUID has
>>>>>> +  // no meaning on ARM since we need at least ACPI 5.0 support, and the
>>>>>> +  // 64-bit ACPI 2.0 table GUID is mandatory in that case.
>>>>>> +  //
>>>>>> +  Status = EfiGetSystemConfigurationTable (&gEfiAcpi20TableGuid, &Table);
>>>>>> +  if (EFI_ERROR (Status) || Table == NULL) {
>>>>>>     Status = gBS->InstallConfigurationTable (&gFdtTableGuid, mDeviceTreeBase);
>>>>>>     ASSERT_EFI_ERROR (Status);
>>>>>>   }
>>>>>
>>>>> This code breaks the DT-only ("-no-acpi") boot with boot graphics
>>>>> (virtio-gpu) enabled.
>>>>>
>>>>> Namely, we recently included BootGraphicsResourceTableDxe in the build.
>>>>> That driver calls InstallAcpiTable() for BGRT, which in turn causes
>>>>> AcpiTableDxe to call InstallConfigurationTable(), via PublishTables().
>>>>>
>>>>> We all missed that just because QEMU doesn't produce ACPI payload (and
>>>>> we consequently don't install it), other drivers in edk2 may
>>>>> unconditionally install "auxiliary" ACPI tables, which minimally trigger
>>>>> the presence of the RSD PTR, RSDT and XSDT, and prevent DT installation.
>>>>> Such a crippled set of ACPI tables isn't sufficient for booting however.
>>>>>
>>>>> We have functions like FindAcpiFacsTableByAcpiGuid(), ScanTableInXSDT()
>>>>> and ScanTableInRSDT() in
>>>>> "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c". I
>>>>> think the above check should be reworked to look for the FADT
>>>>> (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) with code lifted
>>>>> from these helper functions. No driver outside of
>>>>> QemuFwCfgAcpiPlatformDxe will install the FADT. And, the FADT will
>>>>> always be part of QEMU's ACPI payload, if it generates one.
>>>>
>>>> ... BTW this is exactly the kind of hard-to-predict unreliability of
>>>> ReadyToBoot callbacks that I was worried about. The patches that I
>>>> posted looked for the "etc/table-loader" fw_cfg file, which directly
>>>> corresponds to the absence of the "-no-acpi" switch. With a ReadyToBoot
>>>> callback, we depend on a larger, and fuzzier, pile of state.
>>>>
>>>> I'm not suggesting that we return to my series -- I think this one can
>>>> be fixed up by looking for the FADT in particular --, I'd just like if
>>>> we all grew a healthy aversion and distrust to ReadyToBoot callbacks.
>>>> Their perceived simplicity is deceptive.
>>>>
>>>
>>> Point taken. But couldn't we still check the existence of that at ReadyToBoot?
>>
>> We could, but that would bring back the new INF file for QemuFwCfgLib,
>> and the explicit constructor call in FdtClientDxe (assuming we continue
>> to register the callback in FdtClientDxe -- I think it does belong there).
>>
>> So that would be worst of both worlds.
>>
> 
> Ah, of course. Silly me :-)
> 
> So my primary concern here, and I am glad we spotted it now, is that
> the presence of neutered ACPI tables and no DT makes the system
> unbootable. The existence of such firmware will, in turn, make it even
> more difficult to convince the arm64 maintainers that ACPI should be
> preferred over DT by default if both h/w descriptions are available.

Well, in the longer term, it shouldn't be necessary to convince the
arm64 kernel maintainers -- it's enough to convince the firmware
providers :) QEMU and libvirt produce ACPI by default, and ArmVirtQemu
will stop forwarding DT.

> 
> So IIUC, firmwares the predate this series can never be affected in
> such a way, right? (Unless you use a PURE_ACPI_BOOT build and pass
> -no-acpi, in which case you will probably get the neutered tables but
> you wouldn't have been able to boot in the first place)

Correct.

> 
> So I will look into the FADT check on Monday, I agree that is the most
> appropriate approach here.
> 

Thanks!
Laszlo