Re: [PATCH v2 01/12] Revert "ArmVirtPkg/FdtClientDxe: make DT table installation !ACPI dependent"

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 17 March 2017 at 20:47, Laszlo Ersek <lersek@redhat.com> wrote:
> This reverts commit 78c41ff519b187d8979cda7074f007a6323f9acd.
>
> We realized that DXE drivers that are independent of AcpiPlatformDxe (that
> is, independent of QEMU's ACPI generation), such as RamDiskDxe and
> BootGraphicsResourceTableDxe, may produce and/or manipulate ACPI tables,
> at driver dispatch or even at Ready To Boot.
>
> This makes it unsafe for us to check for ACPI presence in the UEFI system
> config table in a Ready To Boot callback, in order to decide about
> exposing the DT.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmVirtPkg/ArmVirtPkg.dec                | 10 ++++++++++
>  ArmVirtPkg/ArmVirtQemu.dsc               |  5 +++++
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf |  5 +++--
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c   | 16 +++++-----------
>  4 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index efe83a383d55..a5ec42166445 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -58,3 +58,13 @@ [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 0bbbe4a7aa4a..d6b3c0db5530 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -34,6 +34,7 @@ [Defines]
>    # -D FLAG=VALUE
>    #
>    DEFINE SECURE_BOOT_ENABLE      = FALSE
> +  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
>
>  !include ArmVirtPkg/ArmVirt.dsc.inc
>
> @@ -94,6 +95,10 @@ [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.inf b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> index 9861f41e968b..00017727c32c 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
> @@ -37,16 +37,17 @@ [LibraryClasses]
>    HobLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> -  UefiLib
>
>  [Protocols]
>    gFdtClientProtocolGuid                  ## PRODUCES
>
>  [Guids]
> -  gEfiAcpi20TableGuid
>    gEfiEventReadyToBootGuid
>    gFdtHobGuid
>    gFdtTableGuid
>
> +[FeaturePcd]
> +  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot
> +
>  [Depex]
>    TRUE
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 21c1074e331c..4cf79f70cb2a 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -17,11 +17,9 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiDriverEntryPoint.h>
>  #include <Library/UefiBootServicesTableLib.h>
> -#include <Library/UefiLib.h>
>  #include <Library/HobLib.h>
>  #include <libfdt.h>
>
> -#include <Guid/Acpi.h>
>  #include <Guid/EventGroup.h>
>  #include <Guid/Fdt.h>
>  #include <Guid/FdtHob.h>
> @@ -318,16 +316,12 @@ OnReadyToBoot (
>    )
>  {
>    EFI_STATUS      Status;
> -  VOID            *Table;
>
> -  //
> -  // 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) {
> +  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);
>    }
> --
> 2.9.3
>
>