[PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[Laszlo Ersek]

This series replaces the PURE_ACPI_BOOT_ENABLE build option with dynamic
behavior, matching QEMU's (inverse sense) "-no-acpi" switch. In
particular, DT and ACPI are no longer exposed to the guest at the same
time. (DT is exposed with "-no-acpi", or else ACPI is exposed without
"-no-acpi".)

Repo:   https://github.com/lersek/edk2.git
Branch: dynamic_pure_acpi
RHBZ:   https://bugzilla.redhat.com/show_bug.cgi?id=1430262

Tested with RHEL-7.3 for ARM and Fedora 24 guests (DT vs. ACPI).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks
Laszlo

Laszlo Ersek (6):
  ArmVirtPkg/FdtClientDxe: supplement missing EFIAPI calling conv
    specifiers
  ArmVirtPkg: introduce FDT_CLIENT_PROTOCOL.GetOsExposure() member
    function
  ArmVirtPkg/ArmVirtPL031FdtClientLib: get rid of PcdPureAcpiBoot
    dependency
  ArmVirtPkg/QemuFwCfgLib: add explicitly initialized instance
  ArmVirtPkg/FdtClientDxe: don't forward DT to OS if QEMU provides ACPI
  ArmVirtPkg: remove PURE_ACPI_BOOT_ENABLE and PcdPureAcpiBoot

 ArmVirtPkg/ArmVirtPkg.dec                                                          | 10 ---
 ArmVirtPkg/ArmVirtQemu.dsc                                                         | 10 +--
 ArmVirtPkg/ArmVirtQemuKernel.dsc                                                   |  5 +-
 ArmVirtPkg/ArmVirtXen.dsc                                                          |  5 +-
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                                             | 79 ++++++++++++++++++--
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf                                           |  5 +-
 ArmVirtPkg/Include/Protocol/FdtClient.h                                            | 26 +++++++
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c             |  6 +-
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf           |  3 -
 ArmVirtPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgLibExplicitInit.inf} | 15 ++--
 10 files changed, 124 insertions(+), 40 deletions(-)
 copy ArmVirtPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgLibExplicitInit.inf} (72%)

-- 
2.9.3

[PATCH 1/6] ArmVirtPkg/FdtClientDxe: supplement missing EFIAPI calling conv specifiers

[Laszlo Ersek]

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 7cc0c44ca12a..547a29fce62c 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -29,6 +29,7 @@ STATIC VOID  *mDeviceTreeBase;
 
 STATIC
 EFI_STATUS
+EFIAPI
 GetNodeProperty (
   IN  FDT_CLIENT_PROTOCOL     *This,
   IN  INT32                   Node,
@@ -55,6 +56,7 @@ GetNodeProperty (
 
 STATIC
 EFI_STATUS
+EFIAPI
 SetNodeProperty (
   IN  FDT_CLIENT_PROTOCOL     *This,
   IN  INT32                   Node,
@@ -267,6 +269,7 @@ FindMemoryNodeReg (
 
 STATIC
 EFI_STATUS
+EFIAPI
 GetOrInsertChosenNode (
   IN  FDT_CLIENT_PROTOCOL     *This,
   OUT INT32                   *Node
-- 
2.9.3

[PATCH 2/6] ArmVirtPkg: introduce FDT_CLIENT_PROTOCOL.GetOsExposure() member function

[Laszlo Ersek]

Introduce a protocol member function that allows consumers of the protocol
to determine whether the FDT is exposed to the guest OS.

The initial implementation simply reflects !PcdPureAcpiBoot.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Include/Protocol/FdtClient.h | 26 ++++++++++++++++++++
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c  | 11 +++++++++
 2 files changed, 37 insertions(+)

diff --git a/ArmVirtPkg/Include/Protocol/FdtClient.h b/ArmVirtPkg/Include/Protocol/FdtClient.h
index aad76db388be..4e189c2a3d2a 100644
--- a/ArmVirtPkg/Include/Protocol/FdtClient.h
+++ b/ArmVirtPkg/Include/Protocol/FdtClient.h
@@ -115,6 +115,30 @@ EFI_STATUS
   OUT INT32                   *Node
   );
 
+/*
+  @param[out] FdtExposedToOs  Whether the firmware exposes the FDT to the guest
+                              OS as an EFI system configuration table.
+
+  @retval EFI_NOT_STARTED  The protocol instance is in the process of
+                           determining whether it should expose the FDT to the
+                           guest OS as an EFI system configuration table.
+
+                           This status code is never returned to modules that
+                           depend on the protocol with a DEPEX, or wait for it
+                           with a TPL_CALLBACK protocol notify. Protocol
+                           notifies registered at higher task priority levels
+                           may see this return value (but such protocol
+                           notifies should not be used in the first place, in
+                           general).
+
+  @retval EFI_SUCCESS      FdtExposedToOs has been set.
+*/
+typedef
+EFI_STATUS
+(EFIAPI *FDT_CLIENT_GET_OS_EXPOSURE) (
+  OUT BOOLEAN *FdtExposedToOs
+  );
+
 struct _FDT_CLIENT_PROTOCOL {
   FDT_CLIENT_GET_NODE_PROPERTY             GetNodeProperty;
   FDT_CLIENT_SET_NODE_PROPERTY             SetNodeProperty;
@@ -128,6 +152,8 @@ struct _FDT_CLIENT_PROTOCOL {
   FDT_CLIENT_FIND_NEXT_MEMORY_NODE_REG     FindNextMemoryNodeReg;
 
   FDT_CLIENT_GET_OR_INSERT_CHOSEN_NODE     GetOrInsertChosenNode;
+
+  FDT_CLIENT_GET_OS_EXPOSURE               GetOsExposure;
 };
 
 extern EFI_GUID gFdtClientProtocolGuid;
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 547a29fce62c..6082b22d35c1 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -294,6 +294,16 @@ GetOrInsertChosenNode (
   return EFI_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+GetOsExposure (
+  OUT BOOLEAN *FdtExposedToOs
+  )
+{
+  *FdtExposedToOs = !FeaturePcdGet (PcdPureAcpiBoot);
+  return EFI_SUCCESS;
+}
+
 STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
   GetNodeProperty,
   SetNodeProperty,
@@ -304,6 +314,7 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
   FindMemoryNodeReg,
   FindNextMemoryNodeReg,
   GetOrInsertChosenNode,
+  GetOsExposure
 };
 
 EFI_STATUS
-- 
2.9.3

[PATCH 3/6] ArmVirtPkg/ArmVirtPL031FdtClientLib: get rid of PcdPureAcpiBoot dependency

[Laszlo Ersek]

Replace the FeaturePCD dependency with a call to the new FdtClientProtocol
member GetOsExposure(). ArmVirtPL031FdtClientLib depends on the protocol
with a DEPEX, hence the call will always succeed.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf | 3 ---
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c   | 6 +++++-
 2 files changed, 5 insertions(+), 4 deletions(-)

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
diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
index 82de7a51b32e..0de34df04308 100644
--- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
+++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
@@ -35,6 +35,7 @@ ArmVirtPL031FdtClientLibConstructor (
   UINT32                        RegSize;
   UINT64                        RegBase;
   RETURN_STATUS                 PcdStatus;
+  BOOLEAN                       FdtExposedToOs;
 
   Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
                   (VOID **)&FdtClient);
@@ -66,7 +67,10 @@ ArmVirtPL031FdtClientLibConstructor (
 
   DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
 
-  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
+  Status = FdtClient->GetOsExposure (&FdtExposedToOs);
+  ASSERT_EFI_ERROR (Status);
+
+  if (FdtExposedToOs) {
     //
     // UEFI takes ownership of the RTC hardware, and exposes its functionality
     // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
-- 
2.9.3

[PATCH 4/6] ArmVirtPkg/QemuFwCfgLib: add explicitly initialized instance

[Laszlo Ersek]

The QemuFwCfgLibExplicitInit instance differs from the normally used one
in that the client module has to call the QemuFwCfgInitialize() function
explicitly -- there is no library constructor --, and the client shall
also ensure that the dependency on FDT_CLIENT_PROTOCOL is satisfied --
there is no DepEx.

In particular this enables the module that produces FDT_CLIENT_PROTOCOL to
use the library, after the protocol is installed.

Note that neither QemuFwCfgLib instance calls
FDT_CLIENT_PROTOCOL.GetOsExposure(). In fact, the QemuFwCfgLibExplicitInit
instance will be utilized to implement
FDT_CLIENT_PROTOCOL.GetOsExposure().

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgLibExplicitInit.inf} | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
similarity index 72%
copy from ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
copy to ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
index eff4a2165062..007e5f1b2d54 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
@@ -1,8 +1,8 @@
 ## @file
 #
-#  Stateful, implicitly initialized fw_cfg library.
+#  Stateful, explicitly initialized fw_cfg library.
 #
-#  Copyright (C) 2013 - 2014, Red Hat, Inc.
+#  Copyright (C) 2013 - 2017, Red Hat, Inc.
 #  Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.<BR>
 #
 #  This program and the accompanying materials are licensed and made available
@@ -18,13 +18,11 @@
 
 [Defines]
   INF_VERSION                    = 0x00010005
-  BASE_NAME                      = QemuFwCfgLib
-  FILE_GUID                      = B271F41F-B841-48A9-BA8D-545B4BC2E2BF
+  BASE_NAME                      = QemuFwCfgLibExplicitInit
+  FILE_GUID                      = 7DF98175-3819-4966-A48C-E56056EA8F42
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = QemuFwCfgLib|DXE_DRIVER
-
-  CONSTRUCTOR                    = QemuFwCfgInitialize
+  LIBRARY_CLASS                  = QemuFwCfgLib|DXE_DRIVER UEFI_DRIVER
 
 #
 # The following information is for reference only and not required by the build
@@ -50,6 +48,3 @@ [LibraryClasses]
 
 [Protocols]
   gFdtClientProtocolGuid                                ## CONSUMES
-
-[Depex]
-  gFdtClientProtocolGuid
-- 
2.9.3

[PATCH 5/6] ArmVirtPkg/FdtClientDxe: don't forward DT to OS if QEMU provides ACPI

[Laszlo Ersek]

ArmVirtQemu can be built with -D PURE_ACPI_BOOT_ENABLE at the moment. We
should replace that build-time setting with a dynamic one: forward the DT
from QEMU to the guest kernel if and only if

- the guest architecture is arm32, or
- we run on Xen, or
- ACPI payload is not available from QEMU.

This will let QEMU's "-no-acpi" option exclusively expose DT vs. ACPI to
the guest. Showing both is never needed (it is actually detrimental to the
adoption of standards, such as SBSA / SBBR).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/ArmVirtQemu.dsc               |  5 +-
 ArmVirtPkg/ArmVirtQemuKernel.dsc         |  5 +-
 ArmVirtPkg/ArmVirtXen.dsc                |  5 +-
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 +-
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 67 ++++++++++++++++++--
 5 files changed, 75 insertions(+), 12 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 477dfdcfc764..863077f53edf 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -301,7 +301,10 @@ [Components.common]
   # Platform Driver
   #
   ArmVirtPkg/VirtioFdtDxe/VirtioFdtDxe.inf
-  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf {
+    <LibraryClasses>
+      QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
+  }
   ArmVirtPkg/HighMemDxe/HighMemDxe.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
   OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index fd39c2802a85..08a8eafd60d0 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -290,7 +290,10 @@ [Components.common]
   # Platform Driver
   #
   ArmVirtPkg/VirtioFdtDxe/VirtioFdtDxe.inf
-  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf {
+    <LibraryClasses>
+      QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
+  }
   ArmVirtPkg/HighMemDxe/HighMemDxe.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
   OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index 3422d1e5d996..4442329907ae 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -197,7 +197,10 @@ [Components.common]
   # Platform Driver
   #
   ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.inf
-  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf {
+    <LibraryClasses>
+      QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
+  }
 
   #
   # FAT filesystem + GPT/MBR partitioning
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
index 3a0cd37040eb..832f85bacd9b 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
@@ -29,12 +29,14 @@ [Packages]
   EmbeddedPkg/EmbeddedPkg.dec
   MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
   BaseLib
   DebugLib
   FdtLib
   HobLib
+  QemuFwCfgLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
 
@@ -45,8 +47,5 @@ [Guids]
   gFdtHobGuid
   gFdtTableGuid
 
-[FeaturePcd]
-  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
-
 [Depex]
   TRUE
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 6082b22d35c1..8def7662a271 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -18,6 +18,7 @@
 #include <Library/UefiDriverEntryPoint.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/HobLib.h>
+#include <Library/QemuFwCfgLib.h>
 #include <libfdt.h>
 
 #include <Guid/Fdt.h>
@@ -27,6 +28,15 @@
 
 STATIC VOID  *mDeviceTreeBase;
 
+typedef enum {
+  OsExposureUnavailable,
+  OsExposureEnabled,
+  OsExposureDisabled,
+  OsExposureMax
+} OS_EXPOSURE_STATE;
+
+STATIC OS_EXPOSURE_STATE mOsExposure;
+
 STATIC
 EFI_STATUS
 EFIAPI
@@ -300,7 +310,11 @@ GetOsExposure (
   OUT BOOLEAN *FdtExposedToOs
   )
 {
-  *FdtExposedToOs = !FeaturePcdGet (PcdPureAcpiBoot);
+  ASSERT (mOsExposure < OsExposureMax);
+  if (mOsExposure == OsExposureUnavailable) {
+    return EFI_NOT_STARTED;
+  }
+  *FdtExposedToOs = (BOOLEAN)(mOsExposure == OsExposureEnabled);
   return EFI_SUCCESS;
 }
 
@@ -317,6 +331,12 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
   GetOsExposure
 };
 
+RETURN_STATUS
+EFIAPI
+QemuFwCfgInitialize (
+  VOID
+  );
+
 EFI_STATUS
 EFIAPI
 InitializeFdtClientDxe (
@@ -327,6 +347,7 @@ InitializeFdtClientDxe (
   VOID              *Hob;
   VOID              *DeviceTreeBase;
   EFI_STATUS        Status;
+  EFI_TPL           OldTpl;
 
   Hob = GetFirstGuidHob (&gFdtHobGuid);
   if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
@@ -344,15 +365,49 @@ InitializeFdtClientDxe (
 
   DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, mDeviceTreeBase));
 
-  if (!FeaturePcdGet (PcdPureAcpiBoot)) {
+  //
+  // Install the protocol for our QemuFwCfgLibExplicitInit instance, but
+  // prevent any protocol notifies at TPL_CALLBACK from firing.
+  //
+  OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
+  Status = gBS->InstallProtocolInterface (&ImageHandle,
+                  &gFdtClientProtocolGuid, EFI_NATIVE_INTERFACE,
+                  &mFdtClientProtocol);
+  if (EFI_ERROR (Status)) {
+    goto RestoreTpl;
+  }
+
+  //
+  // Install the FDT as a configuration table only if:
+  // - we're running on 32-bit ARM, or
+  // - we're running on Xen, or
+  // - QEMU doesn't provide us with ACPI payload (e.g. due to the -no-acpi
+  //   option).
+  //
+  mOsExposure = OsExposureEnabled;
+  if (MAX_UINTN != MAX_UINT32) {
     //
-    // 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.
+    // Call the fw_cfg library constructor explicitly. It always succeeds;
+    // we'll check fw_cfg availability separately.
     //
+    QemuFwCfgInitialize ();
+    if (QemuFwCfgIsAvailable ()) {
+      FIRMWARE_CONFIG_ITEM FwCfgItem;
+      UINTN                FwCfgSize;
+
+      if (!RETURN_ERROR (QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem,
+                           &FwCfgSize))) {
+        mOsExposure = OsExposureDisabled;
+      }
+    }
+  }
+
+  if (mOsExposure == OsExposureEnabled) {
     Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DeviceTreeBase);
     ASSERT_EFI_ERROR (Status);
   }
 
-  return gBS->InstallProtocolInterface (&ImageHandle, &gFdtClientProtocolGuid,
-                EFI_NATIVE_INTERFACE, &mFdtClientProtocol);
+RestoreTpl:
+  gBS->RestoreTPL (OldTpl);
+  return Status;
 }
-- 
2.9.3

[PATCH 6/6] ArmVirtPkg: remove PURE_ACPI_BOOT_ENABLE and PcdPureAcpiBoot

[Laszlo Ersek]

The build flag and the FeaturePCD have no effect any longer, remove them.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/ArmVirtPkg.dec  | 10 ----------
 ArmVirtPkg/ArmVirtQemu.dsc |  5 -----
 2 files changed, 15 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 863077f53edf..d17aab971a86 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
-- 
2.9.3

Re: [PATCH 4/6] ArmVirtPkg/QemuFwCfgLib: add explicitly initialized instance

[Laszlo Ersek]

On 03/08/17 20:05, Laszlo Ersek wrote:
> The QemuFwCfgLibExplicitInit instance differs from the normally used one
> in that the client module has to call the QemuFwCfgInitialize() function
> explicitly -- there is no library constructor --, and the client shall
> also ensure that the dependency on FDT_CLIENT_PROTOCOL is satisfied --
> there is no DepEx.
> 
> In particular this enables the module that produces FDT_CLIENT_PROTOCOL to
> use the library, after the protocol is installed.
> 
> Note that neither QemuFwCfgLib instance calls
> FDT_CLIENT_PROTOCOL.GetOsExposure(). In fact, the QemuFwCfgLibExplicitInit
> instance will be utilized to implement
> FDT_CLIENT_PROTOCOL.GetOsExposure().
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  ArmVirtPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgLibExplicitInit.inf} | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
> similarity index 72%
> copy from ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> copy to ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
> index eff4a2165062..007e5f1b2d54 100644
> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLibExplicitInit.inf
> @@ -1,8 +1,8 @@
>  ## @file
>  #
> -#  Stateful, implicitly initialized fw_cfg library.
> +#  Stateful, explicitly initialized fw_cfg library.
>  #
> -#  Copyright (C) 2013 - 2014, Red Hat, Inc.
> +#  Copyright (C) 2013 - 2017, Red Hat, Inc.
>  #  Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.<BR>
>  #
>  #  This program and the accompanying materials are licensed and made available
> @@ -18,13 +18,11 @@
>  
>  [Defines]
>    INF_VERSION                    = 0x00010005
> -  BASE_NAME                      = QemuFwCfgLib
> -  FILE_GUID                      = B271F41F-B841-48A9-BA8D-545B4BC2E2BF
> +  BASE_NAME                      = QemuFwCfgLibExplicitInit
> +  FILE_GUID                      = 7DF98175-3819-4966-A48C-E56056EA8F42
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = QemuFwCfgLib|DXE_DRIVER
> -
> -  CONSTRUCTOR                    = QemuFwCfgInitialize
> +  LIBRARY_CLASS                  = QemuFwCfgLib|DXE_DRIVER UEFI_DRIVER

The LIBRARY_CLASS change was unintentional (rebase oversight); if no
other issue is found, I'll fix it up before I push the series.

Thanks & sorry
Laszlo

>  
>  #
>  # The following information is for reference only and not required by the build
> @@ -50,6 +48,3 @@ [LibraryClasses]
>  
>  [Protocols]
>    gFdtClientProtocolGuid                                ## CONSUMES
> -
> -[Depex]
> -  gFdtClientProtocolGuid
> 

Re: [PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[Ard Biesheuvel]

On 8 March 2017 at 20:05, Laszlo Ersek <lersek@redhat.com> wrote:
> This series replaces the PURE_ACPI_BOOT_ENABLE build option with dynamic
> behavior, matching QEMU's (inverse sense) "-no-acpi" switch. In
> particular, DT and ACPI are no longer exposed to the guest at the same
> time. (DT is exposed with "-no-acpi", or else ACPI is exposed without
> "-no-acpi".)
>
> Repo:   https://github.com/lersek/edk2.git
> Branch: dynamic_pure_acpi
> RHBZ:   https://bugzilla.redhat.com/show_bug.cgi?id=1430262
>
> Tested with RHEL-7.3 for ARM and Fedora 24 guests (DT vs. ACPI).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>

Hi Laszlo,

This looks complicated to me. Given that it is arguably a policy to
only expose on h/w description or the other, couldn't we simply remove
the FDT config table in BDS if an ACPI/ACPI2.0 config table is
present?

Re: [PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[Laszlo Ersek]

On 03/09/17 09:16, Ard Biesheuvel wrote:
> On 8 March 2017 at 20:05, Laszlo Ersek <lersek@redhat.com> wrote:
>> This series replaces the PURE_ACPI_BOOT_ENABLE build option with dynamic
>> behavior, matching QEMU's (inverse sense) "-no-acpi" switch. In
>> particular, DT and ACPI are no longer exposed to the guest at the same
>> time. (DT is exposed with "-no-acpi", or else ACPI is exposed without
>> "-no-acpi".)
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: dynamic_pure_acpi
>> RHBZ:   https://bugzilla.redhat.com/show_bug.cgi?id=1430262
>>
>> Tested with RHEL-7.3 for ARM and Fedora 24 guests (DT vs. ACPI).
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
> 
> Hi Laszlo,
> 
> This looks complicated to me. Given that it is arguably a policy to
> only expose on h/w description or the other, couldn't we simply remove
> the FDT config table in BDS if an ACPI/ACPI2.0 config table is
> present?

Technically we could do that, but I dislike it for two reasons:

- BDS is often the first victim found when looking for a driver to add
new code to that doesn't seem to fit very well elsewhere. That doesn't
make BDS any better a recipient, however. "For lack of a better driver"
is not a strong enough argument to dump code into BDS. If there's really
no better "topical" driver, then the code usually goes to PlatformDxe.

- Installing a sysconfig table (or any other system-wide resource) in
one driver, then undoing it in another driver, should be avoided as much
as possible, because it leads to non-trivial lifecycles and boggles our
minds over the longer term. If we can come to a decision that the table
shouldn't be installed in the first place, we should pursue that.

Another approach we could look into is: move the installation of the
sysconfig table from FdtClientDxe to AcpiPlatformDxe. Look for the ACPI
payload first, and fall back to installing DT (from within
AcpiPlatformDxe). However, DT should be installed even in builds (like
ARM32) that don't contain AcpiPlatformDxe at all.

This series indeed turned out a bit more complex than I had expected,
but it was the one I could post with a good conscience. Can you perhaps
identify the part(s) in more detail that seem overly complex to you?

Thanks,
Laszlo

Re: [PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[Ard Biesheuvel]

On 9 March 2017 at 12:01, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/09/17 09:16, Ard Biesheuvel wrote:
>> On 8 March 2017 at 20:05, Laszlo Ersek <lersek@redhat.com> wrote:
>>> This series replaces the PURE_ACPI_BOOT_ENABLE build option with dynamic
>>> behavior, matching QEMU's (inverse sense) "-no-acpi" switch. In
>>> particular, DT and ACPI are no longer exposed to the guest at the same
>>> time. (DT is exposed with "-no-acpi", or else ACPI is exposed without
>>> "-no-acpi".)
>>>
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: dynamic_pure_acpi
>>> RHBZ:   https://bugzilla.redhat.com/show_bug.cgi?id=1430262
>>>
>>> Tested with RHEL-7.3 for ARM and Fedora 24 guests (DT vs. ACPI).
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>
>> Hi Laszlo,
>>
>> This looks complicated to me. Given that it is arguably a policy to
>> only expose on h/w description or the other, couldn't we simply remove
>> the FDT config table in BDS if an ACPI/ACPI2.0 config table is
>> present?
>
> Technically we could do that, but I dislike it for two reasons:
>
> - BDS is often the first victim found when looking for a driver to add
> new code to that doesn't seem to fit very well elsewhere. That doesn't
> make BDS any better a recipient, however. "For lack of a better driver"
> is not a strong enough argument to dump code into BDS. If there's really
> no better "topical" driver, then the code usually goes to PlatformDxe.
>
> - Installing a sysconfig table (or any other system-wide resource) in
> one driver, then undoing it in another driver, should be avoided as much
> as possible, because it leads to non-trivial lifecycles and boggles our
> minds over the longer term. If we can come to a decision that the table
> shouldn't be installed in the first place, we should pursue that.
>
> Another approach we could look into is: move the installation of the
> sysconfig table from FdtClientDxe to AcpiPlatformDxe. Look for the ACPI
> payload first, and fall back to installing DT (from within
> AcpiPlatformDxe). However, DT should be installed even in builds (like
> ARM32) that don't contain AcpiPlatformDxe at all.
>

Or we could hook to the ReadyToBoot event in FdtClientDxe, and install
the DT config table if there is no ACPI/ACPI2.0 table registered.

> This series indeed turned out a bit more complex than I had expected,
> but it was the one I could post with a good conscience. Can you perhaps
> identify the part(s) in more detail that seem overly complex to you?
>

Building the same library in two different ways, having to call a
library constructor explicitly in some cases and muck about with TPL
levels to prevent a protocol notify from triggering are all things I
would really like to avoid tbh

Re: [PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[Laszlo Ersek]

On 03/09/17 13:26, Ard Biesheuvel wrote:
> On 9 March 2017 at 12:01, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/09/17 09:16, Ard Biesheuvel wrote:
>>> On 8 March 2017 at 20:05, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> This series replaces the PURE_ACPI_BOOT_ENABLE build option with dynamic
>>>> behavior, matching QEMU's (inverse sense) "-no-acpi" switch. In
>>>> particular, DT and ACPI are no longer exposed to the guest at the same
>>>> time. (DT is exposed with "-no-acpi", or else ACPI is exposed without
>>>> "-no-acpi".)
>>>>
>>>> Repo:   https://github.com/lersek/edk2.git
>>>> Branch: dynamic_pure_acpi
>>>> RHBZ:   https://bugzilla.redhat.com/show_bug.cgi?id=1430262
>>>>
>>>> Tested with RHEL-7.3 for ARM and Fedora 24 guests (DT vs. ACPI).
>>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>
>>>
>>> Hi Laszlo,
>>>
>>> This looks complicated to me. Given that it is arguably a policy to
>>> only expose on h/w description or the other, couldn't we simply remove
>>> the FDT config table in BDS if an ACPI/ACPI2.0 config table is
>>> present?
>>
>> Technically we could do that, but I dislike it for two reasons:
>>
>> - BDS is often the first victim found when looking for a driver to add
>> new code to that doesn't seem to fit very well elsewhere. That doesn't
>> make BDS any better a recipient, however. "For lack of a better driver"
>> is not a strong enough argument to dump code into BDS. If there's really
>> no better "topical" driver, then the code usually goes to PlatformDxe.
>>
>> - Installing a sysconfig table (or any other system-wide resource) in
>> one driver, then undoing it in another driver, should be avoided as much
>> as possible, because it leads to non-trivial lifecycles and boggles our
>> minds over the longer term. If we can come to a decision that the table
>> shouldn't be installed in the first place, we should pursue that.
>>
>> Another approach we could look into is: move the installation of the
>> sysconfig table from FdtClientDxe to AcpiPlatformDxe. Look for the ACPI
>> payload first, and fall back to installing DT (from within
>> AcpiPlatformDxe). However, DT should be installed even in builds (like
>> ARM32) that don't contain AcpiPlatformDxe at all.
>>
> 
> Or we could hook to the ReadyToBoot event in FdtClientDxe, and install
> the DT config table if there is no ACPI/ACPI2.0 table registered.

Yes, that's doable in our case, because we control the full platform.

Installing tables (any kinds of tables) in ReadyToBoot and similar event
handlers is generally a bad idea, because everyone thinks, "okay I'll
wait until the rest of the system is done setting up, and I'll just add
my stuff afterwards". Obviously, this results in much of the logic being
simply moved to such event callbacks, and the invocation order of
callbacks remains unspecified.

In more precise terms, if the ACPI tables too were installed in a
ReadyToBoot callback, your suggestion above would not work. And our ACPI
tables are not installed in a ReadyToBoot callback partly because I
ultimately introduced a separate event group for "PCI bridges have been
connected", and we signal it now explicitly from BDS (device connections
are BDS jurisdiction, and QEMU's ACPI generation depends on PCI state).

I just want to point out that we have a kind of "capital" here. By
carefully coding stuff we build capital, and by hooking stuff into
ReadyToBoot callbacks we spend (hopefully not "squander") that capital.

Originally, the APM Mustang firmware (open source, so I can talk about
it) would first install ACPI tables with constant, platform-tailored
contents (built from *.asl / *.aslc files), but reusing the stock
AcpiPlatformDxe C code without customization. Then it would install a
ReadyToBoot callback which looked up the right DSDT or SSDT, by walking
the table tree manually, and then it would poke data into the installed
table (DSDT or SSDT) in-place, using the ACPI SDT protocol.

Of course it was completely bogus and unreliable, and I changed the
constant table to contain external references, and I provided those
external references in a minimal, hand- and runtime- built SSDT, right
in AcpiPlatformDxe.

I'm not trying to carefully compose a strawman argument here, just
presenting why I'm nervous about ReadyToBoot callbacks that try to rely
on ordering between system-wide resources.


Also, please don't forget about the other (current) consumer of the
feature PCD, ArmVirtPL031FdtClientLib, which is plugged into
RealTimeClockRuntimeDxe. How do you suggest to rewrite the PCD test in
that driver under the ReadyToBoot callback scenario?

RealTimeClockRuntimeDxe's dispatch order is unspecified relative to the
installation of ACPI tables, so you couldn't look at the latter's
presence to see if the DTB needs an update. (In fact, because
AcpiPlatformDxe's main actions are cued in from BDS in practice, the
tables would be guaranteed not to exist when RealTimeClockRuntimeDxe looks.)

So ArmVirtPL031FdtClientLib would have to install another ReadyToBoot
callback for modifying the DTB. And this callback could be invoked
before or after the callback to FdtClientDxe. (I guess it would be okay,
but not very intuitive.)

> 
>> This series indeed turned out a bit more complex than I had expected,
>> but it was the one I could post with a good conscience. Can you perhaps
>> identify the part(s) in more detail that seem overly complex to you?
>>
> 
> Building the same library in two different ways, having to call a
> library constructor explicitly in some cases and muck about with TPL
> levels to prevent a protocol notify from triggering are all things I
> would really like to avoid tbh

Alright; can you please post the alternative patch set? (With the
ReadyToBoot callback(s), that is, not the BDS hack.)

Thanks!
Laszlo

Re: [PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[Leif Lindholm]

Hi Laszlo,

Apologies, I didn't ignore this set, I just missed it (and felt Ard's
set was a clean solution to this behaviour change).

A few comments below.

On Thu, Mar 09, 2017 at 04:30:19PM +0100, Laszlo Ersek wrote:
> On 03/09/17 13:26, Ard Biesheuvel wrote:
> >>> Hi Laszlo,
> >>>
> >>> This looks complicated to me. Given that it is arguably a policy to
> >>> only expose on h/w description or the other, couldn't we simply remove
> >>> the FDT config table in BDS if an ACPI/ACPI2.0 config table is
> >>> present?
> >>
> >> Technically we could do that, but I dislike it for two reasons:
> >>
> >> - BDS is often the first victim found when looking for a driver to add
> >> new code to that doesn't seem to fit very well elsewhere. That doesn't
> >> make BDS any better a recipient, however. "For lack of a better driver"
> >> is not a strong enough argument to dump code into BDS. If there's really
> >> no better "topical" driver, then the code usually goes to PlatformDxe.
> >>
> >> - Installing a sysconfig table (or any other system-wide resource) in
> >> one driver, then undoing it in another driver, should be avoided as much
> >> as possible, because it leads to non-trivial lifecycles and boggles our
> >> minds over the longer term. If we can come to a decision that the table
> >> shouldn't be installed in the first place, we should pursue that.
> >>
> >> Another approach we could look into is: move the installation of the
> >> sysconfig table from FdtClientDxe to AcpiPlatformDxe. Look for the ACPI
> >> payload first, and fall back to installing DT (from within
> >> AcpiPlatformDxe). However, DT should be installed even in builds (like
> >> ARM32) that don't contain AcpiPlatformDxe at all.
> >>
> > 
> > Or we could hook to the ReadyToBoot event in FdtClientDxe, and install
> > the DT config table if there is no ACPI/ACPI2.0 table registered.
> 
> Yes, that's doable in our case, because we control the full platform.
> 
> Installing tables (any kinds of tables) in ReadyToBoot and similar event
> handlers is generally a bad idea, because everyone thinks, "okay I'll
> wait until the rest of the system is done setting up, and I'll just add
> my stuff afterwards". Obviously, this results in much of the logic being
> simply moved to such event callbacks, and the invocation order of
> callbacks remains unspecified.
> 
> In more precise terms, if the ACPI tables too were installed in a
> ReadyToBoot callback, your suggestion above would not work. And our ACPI
> tables are not installed in a ReadyToBoot callback partly because I
> ultimately introduced a separate event group for "PCI bridges have been
> connected", and we signal it now explicitly from BDS (device connections
> are BDS jurisdiction, and QEMU's ACPI generation depends on PCI state).

All you say above is clearly correct.
But I am still not clear on why this is a problem.
This is a _very_ specific case, that applies only to virtual machines
(which we are in complete control of).

For hardware platforms wanting the ability to switch between different
hardware description types, as we discussed, we need a configuration
setting based on a dynamic Pcd or environment variable - so they won't
need to wait until the end.

> I just want to point out that we have a kind of "capital" here. By
> carefully coding stuff we build capital, and by hooking stuff into
> ReadyToBoot callbacks we spend (hopefully not "squander") that capital.

I fully agree with this as a strong default position. But I am
suggesting that in this case, the callback sort order genuinely does
not matter for this feature. At which point I prefer the simpler
solution of Ard's set.

That said, there are two maintainers of ArmVirtPkg, and I'm neither of
them :)

> Originally, the APM Mustang firmware (open source, so I can talk about
> it) would first install ACPI tables with constant, platform-tailored
> contents (built from *.asl / *.aslc files), but reusing the stock
> AcpiPlatformDxe C code without customization. Then it would install a
> ReadyToBoot callback which looked up the right DSDT or SSDT, by walking
> the table tree manually, and then it would poke data into the installed
> table (DSDT or SSDT) in-place, using the ACPI SDT protocol.
> 
> Of course it was completely bogus and unreliable, and I changed the
> constant table to contain external references, and I provided those
> external references in a minimal, hand- and runtime- built SSDT, right
> in AcpiPlatformDxe.
> 
> I'm not trying to carefully compose a strawman argument here, just
> presenting why I'm nervous about ReadyToBoot callbacks that try to rely
> on ordering between system-wide resources.
> 
> 
> Also, please don't forget about the other (current) consumer of the
> feature PCD, ArmVirtPL031FdtClientLib, which is plugged into
> RealTimeClockRuntimeDxe. How do you suggest to rewrite the PCD test in
> that driver under the ReadyToBoot callback scenario?
>
> RealTimeClockRuntimeDxe's dispatch order is unspecified relative to the
> installation of ACPI tables, so you couldn't look at the latter's
> presence to see if the DTB needs an update. (In fact, because
> AcpiPlatformDxe's main actions are cued in from BDS in practice, the
> tables would be guaranteed not to exist when RealTimeClockRuntimeDxe looks.)
> 
> So ArmVirtPL031FdtClientLib would have to install another ReadyToBoot
> callback for modifying the DTB. And this callback could be invoked
> before or after the callback to FdtClientDxe. (I guess it would be okay,
> but not very intuitive.)

(I think Ard's resolution of this, in 1/3, could be applied regardless
of method picked for the overall changeset.)

Regards,

Leif

> >> This series indeed turned out a bit more complex than I had expected,
> >> but it was the one I could post with a good conscience. Can you perhaps
> >> identify the part(s) in more detail that seem overly complex to you?
> >>
> > 
> > Building the same library in two different ways, having to call a
> > library constructor explicitly in some cases and muck about with TPL
> > levels to prevent a protocol notify from triggering are all things I
> > would really like to avoid tbh
> 
> Alright; can you please post the alternative patch set? (With the
> ReadyToBoot callback(s), that is, not the BDS hack.)
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/6] ArmVirtPkg: don't forward the DT to the OS if QEMU provides ACPI

[Laszlo Ersek]

On 03/09/17 18:00, Leif Lindholm wrote:
> Hi Laszlo,
> 
> Apologies, I didn't ignore this set, I just missed it

No wonder you missed it, I didn't CC you on it :) I wondered if I
should. In general I don't want to increase other people's email load,
and you aren't (yet?) listed as an ArmVirtPkg co-maintainer, so I didn't
CC you in the end.

> (and felt Ard's
> set was a clean solution to this behaviour change).

It is a simple solution, yes; I think its simplicity is deceptive
though, to an extent. To me, ReadyToBoot callbacks are a last resort.

If you grep the UEFI spec for EFI_EVENT_GROUP_READY_TO_BOOT, among other
locations, you find

  34.1.1 User Identify

  [...] When the UEFI Boot Manager signals the
  EFI_EVENT_GROUP_READY_TO_BOOT event group, the User Identity Manager
  publishes the current user profile information in the EFI System
  Configuration Table. [...]

The rest of the language is irrelevant here; my point is that there is
"prior art" for installing sysconfig tables at Ready To Boot. That's
entirely fine. What worries me is that the dependency we evaluate in the
callback is *generally* something that could be satisfied by *another*
Ready To Boot callback, and the ordering between those is unspecified.

Given the current platform, this is not a real issue in practice (we
don't install any of our ACPI tables at Ready To Boot), which is why I'm
open to Ard's solution. I just want us to be aware of this risk.

> 
> A few comments below.
> 
> On Thu, Mar 09, 2017 at 04:30:19PM +0100, Laszlo Ersek wrote:
>> On 03/09/17 13:26, Ard Biesheuvel wrote:
>>>>> Hi Laszlo,
>>>>>
>>>>> This looks complicated to me. Given that it is arguably a policy to
>>>>> only expose on h/w description or the other, couldn't we simply remove
>>>>> the FDT config table in BDS if an ACPI/ACPI2.0 config table is
>>>>> present?
>>>>
>>>> Technically we could do that, but I dislike it for two reasons:
>>>>
>>>> - BDS is often the first victim found when looking for a driver to add
>>>> new code to that doesn't seem to fit very well elsewhere. That doesn't
>>>> make BDS any better a recipient, however. "For lack of a better driver"
>>>> is not a strong enough argument to dump code into BDS. If there's really
>>>> no better "topical" driver, then the code usually goes to PlatformDxe.
>>>>
>>>> - Installing a sysconfig table (or any other system-wide resource) in
>>>> one driver, then undoing it in another driver, should be avoided as much
>>>> as possible, because it leads to non-trivial lifecycles and boggles our
>>>> minds over the longer term. If we can come to a decision that the table
>>>> shouldn't be installed in the first place, we should pursue that.
>>>>
>>>> Another approach we could look into is: move the installation of the
>>>> sysconfig table from FdtClientDxe to AcpiPlatformDxe. Look for the ACPI
>>>> payload first, and fall back to installing DT (from within
>>>> AcpiPlatformDxe). However, DT should be installed even in builds (like
>>>> ARM32) that don't contain AcpiPlatformDxe at all.
>>>>
>>>
>>> Or we could hook to the ReadyToBoot event in FdtClientDxe, and install
>>> the DT config table if there is no ACPI/ACPI2.0 table registered.
>>
>> Yes, that's doable in our case, because we control the full platform.
>>
>> Installing tables (any kinds of tables) in ReadyToBoot and similar event
>> handlers is generally a bad idea, because everyone thinks, "okay I'll
>> wait until the rest of the system is done setting up, and I'll just add
>> my stuff afterwards". Obviously, this results in much of the logic being
>> simply moved to such event callbacks, and the invocation order of
>> callbacks remains unspecified.
>>
>> In more precise terms, if the ACPI tables too were installed in a
>> ReadyToBoot callback, your suggestion above would not work. And our ACPI
>> tables are not installed in a ReadyToBoot callback partly because I
>> ultimately introduced a separate event group for "PCI bridges have been
>> connected", and we signal it now explicitly from BDS (device connections
>> are BDS jurisdiction, and QEMU's ACPI generation depends on PCI state).
> 
> All you say above is clearly correct.
> But I am still not clear on why this is a problem.
> This is a _very_ specific case, that applies only to virtual machines
> (which we are in complete control of).

Yes, the "complete control of the platform" argument is not lost on me.

> 
> For hardware platforms wanting the ability to switch between different
> hardware description types, as we discussed, we need a configuration
> setting based on a dynamic Pcd or environment variable - so they won't
> need to wait until the end.
> 
>> I just want to point out that we have a kind of "capital" here. By
>> carefully coding stuff we build capital, and by hooking stuff into
>> ReadyToBoot callbacks we spend (hopefully not "squander") that capital.
> 
> I fully agree with this as a strong default position. But I am
> suggesting that in this case, the callback sort order genuinely does
> not matter for this feature. At which point I prefer the simpler
> solution of Ard's set.

Yes, that's a valid argument.

> 
> That said, there are two maintainers of ArmVirtPkg, and I'm neither of
> them :)
> 
>> Originally, the APM Mustang firmware (open source, so I can talk about
>> it) would first install ACPI tables with constant, platform-tailored
>> contents (built from *.asl / *.aslc files), but reusing the stock
>> AcpiPlatformDxe C code without customization. Then it would install a
>> ReadyToBoot callback which looked up the right DSDT or SSDT, by walking
>> the table tree manually, and then it would poke data into the installed
>> table (DSDT or SSDT) in-place, using the ACPI SDT protocol.
>>
>> Of course it was completely bogus and unreliable, and I changed the
>> constant table to contain external references, and I provided those
>> external references in a minimal, hand- and runtime- built SSDT, right
>> in AcpiPlatformDxe.
>>
>> I'm not trying to carefully compose a strawman argument here, just
>> presenting why I'm nervous about ReadyToBoot callbacks that try to rely
>> on ordering between system-wide resources.
>>
>>
>> Also, please don't forget about the other (current) consumer of the
>> feature PCD, ArmVirtPL031FdtClientLib, which is plugged into
>> RealTimeClockRuntimeDxe. How do you suggest to rewrite the PCD test in
>> that driver under the ReadyToBoot callback scenario?
>>
>> RealTimeClockRuntimeDxe's dispatch order is unspecified relative to the
>> installation of ACPI tables, so you couldn't look at the latter's
>> presence to see if the DTB needs an update. (In fact, because
>> AcpiPlatformDxe's main actions are cued in from BDS in practice, the
>> tables would be guaranteed not to exist when RealTimeClockRuntimeDxe looks.)
>>
>> So ArmVirtPL031FdtClientLib would have to install another ReadyToBoot
>> callback for modifying the DTB. And this callback could be invoked
>> before or after the callback to FdtClientDxe. (I guess it would be okay,
>> but not very intuitive.)
> 
> (I think Ard's resolution of this, in 1/3, could be applied regardless
> of method picked for the overall changeset.)

I think I'm less opposed to Ard's solution than he is opposed to mine,
so I guess we should roll with Ard's upcoming v2 (as long as he agrees
to own any future fallout due to ReadyToBoot ordering) :)

Thanks
Laszlo

> 
> Regards,
> 
> Leif
> 
>>>> This series indeed turned out a bit more complex than I had expected,
>>>> but it was the one I could post with a good conscience. Can you perhaps
>>>> identify the part(s) in more detail that seem overly complex to you?
>>>>
>>>
>>> Building the same library in two different ways, having to call a
>>> library constructor explicitly in some cases and muck about with TPL
>>> levels to prevent a protocol notify from triggering are all things I
>>> would really like to avoid tbh
>>
>> Alright; can you please post the alternative patch set? (With the
>> ReadyToBoot callback(s), that is, not the BDS hack.)
>>
>> Thanks!
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel