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

[Laszlo Ersek <lersek@redhat.com>]

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