Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI

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

On 28 March 2017 at 11:43, Laszlo Ersek <lersek@redhat.com> wrote:
> Ard,
>
> On 03/27/17 12:07, Ard Biesheuvel wrote:
>> As a follow up to the changes proposed by Laszlo to make ACPI and DT
>> mutually exclusive on ArmVirtQemu, this patch proposes a DT platform
>> DXE driver that either installs the NULL protocol PlatformHasAcpiGuid,
>> or installs the FV embedded DTB binary as a configuration table under
>> the appropriate GUID, depending on a preference setting recorded as
>> a UEFI variable, and configurable via a HII screen. This is intended
>> for bare metal platforms.
>>
>> The DTB binary can be embedded in the firmware image by adding the
>> following to the platform .fdf file:
>>
>>   FILE FREEFORM = 25462CDA-221F-47DF-AC1D-259CFAA4E326 {
>>     SECTION RAW = SomePkg/path/to/foo.dtb
>>   }
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> This depends on patch 4/12 of Laszlo's series
>> https://lists.01.org/pipermail/edk2-devel/2017-March/009004.html
>>
>> Note to Marcin: the setup page was only tested with the generic BDS, not
>> the Intel BDS. This shouldn't matter in practice, since they should both
>> implement the prerequisite protocols, but moving to the generic BDS is
>> probably a good idea regardless.
>>
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 211 ++++++++++++++++++++
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h   |  29 +++
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  70 +++++++
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.uni |  27 +++
>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr |  57 ++++++
>>  EmbeddedPkg/EmbeddedPkg.dec                         |   6 +
>>  EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h |  23 +++
>>  EmbeddedPkg/Include/Guid/DtPlatformFormSet.h        |  23 +++
>>  8 files changed, 446 insertions(+)
>>
>
> Please consider adopting
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10
>
> and setting the "diff.orderFile" knob in your edk2 tree accordingly -- it is a lot easier to review a patch if the declarative changes (headers, VFRs, INF / DEC / DSC files) come first, and the imperative changes (C code) comes second. I think I'll reorder files in your patch for this response.
>
> (BTW can you double-check that all new files in this patch have CRLF line terminators?)
>

Ah, apologies, I switched to my arm64 development box, but apparently
failed to carry over those .dotfiles

>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
>> index 2c2cf41103c2..c0f24422dc12 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>> @@ -56,6 +56,12 @@ [Guids.common]
>>    gFdtHobGuid   = { 0x16958446, 0x19B7, 0x480B, { 0xB0, 0x47, 0x74, 0x85, 0xAD, 0x3F, 0x71, 0x6D } }
>>    gFdtVariableGuid = { 0x25a4fd4a, 0x9703, 0x4ba9, { 0xa1, 0x90, 0xb7, 0xc8, 0x4e, 0xfb, 0x3e, 0x57 } }
>>
>> +  # HII form set GUID for DtPlatformDxe driver
>> +  gDtPlatformFormSetGuid = { 0x2b7a240d, 0xd5ad, 0x4fd6, { 0xbe, 0x1c, 0xdf, 0xa4, 0x41, 0x5f, 0x55, 0x26 } }
>> +
>> +  # File GUID for default DTB image embedded in the firmware volume
>> +  gDtPlatformDefaultDtbFileGuid = { 0x25462cda, 0x221f, 0x47df, { 0xac, 0x1d, 0x25, 0x9c, 0xfa, 0xa4, 0xe3, 0x26 } }
>> +
>>  [Protocols.common]
>>    gHardwareInterruptProtocolGuid =  { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
>>    gEfiDebugSupportPeriodicCallbackProtocolGuid = { 0x9546e07c, 0x2cbb, 0x4c88, { 0x98, 0x6c, 0xcd, 0x34, 0x10, 0x86, 0xf0, 0x44 } }
>
> This hunk looks okay to me.
>
>> diff --git a/EmbeddedPkg/Include/Guid/DtPlatformFormSet.h b/EmbeddedPkg/Include/Guid/DtPlatformFormSet.h
>> new file mode 100644
>> index 000000000000..71e3e7ebf7f3
>> --- /dev/null
>> +++ b/EmbeddedPkg/Include/Guid/DtPlatformFormSet.h
>> @@ -0,0 +1,23 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017, Linaro Limited. All rights reserved.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of the BSD License
>> +*  which accompanies this distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#ifndef __DT_PLATFORM_FORMSET_H__
>> +#define __DT_PLATFORM_FORMSET_H__
>> +
>> +#define DT_PLATFORM_FORMSET_GUID  \
>> +  { 0x2b7a240d, 0xd5ad, 0x4fd6, { 0xbe, 0x1c, 0xdf, 0xa4, 0x41, 0x5f, 0x55, 0x26 } }
>> +
>> +extern EFI_GUID gDtPlatformFormSetGuid;
>> +
>> +#endif
>>
>
> Also okay.
>
>
>> diff --git a/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h b/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h
>> new file mode 100644
>> index 000000000000..98099ae68d6d
>> --- /dev/null
>> +++ b/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h
>> @@ -0,0 +1,23 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017, Linaro Limited. All rights reserved.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of the BSD License
>> +*  which accompanies this distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#ifndef __DT_PLATFORM_DEFAULT_DTB_FILE_H__
>> +#define __DT_PLATFORM_DEFAULT_DTB_FILE_H__
>> +
>> +#define DT_PLATFORM_DEFAULT_DTB_FILE_GUID  \
>> +  { 0x2b7a240d, 0xd5ad, 0x4fd6, { 0xbe, 0x1c, 0xdf, 0xa4, 0x41, 0x5f, 0x55, 0x26 } }
>> +
>> +extern EFI_GUID gDtPlatformDefaultDtbFileGuid;
>> +
>> +#endif
>
> This is incorrect, you forgot to update DT_PLATFORM_DEFAULT_DTB_FILE_GUID to the actual GUID value of gDtPlatformDefaultDtbFileGuid. You are still using gDtPlatformFormSetGuid here.
>

Oops. Thanks for spotting that.

@Marcin: this breaks the .fdf embedded DTB file so I should fix this
before attempting to run this code on your board.

>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>> new file mode 100644
>> index 000000000000..d0a99c4ecd55
>> --- /dev/null
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>> @@ -0,0 +1,70 @@
>> +## @file
>> +#
>> +#  Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
>> +#
>> +#  This program and the accompanying materials are licensed and made
>> +#  available under the terms and conditions of the BSD License which
>> +#  accompanies this distribution.  The full text of the license may be
>> +#  found at http://opensource.org/licenses/bsd-license.php
>> +#
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
>> +#  IMPLIED.
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION               = 0x00010019
>> +  BASE_NAME                 = DtPlatformDxe
>> +  FILE_GUID                 = FC097B3C-2EBD-4A75-A3DA-121DCAB365CC
>> +  MODULE_TYPE               = DXE_DRIVER
>> +  VERSION_STRING            = 1.0
>> +  ENTRY_POINT               = DtPlatformDxeEntryPoint
>> +
>> +#
>> +# The following information is for reference only and not required by the build tools.
>> +#
>> +#  VALID_ARCHITECTURES      = IA32 X64 ARM AARCH64
>> +#
>> +
>> +[Sources]
>> +  DtPlatformDxe.c
>> +  DtPlatformHii.vfr
>> +  DtPlatformHii.uni
>> +
>> +[Packages]
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  DebugLib
>> +  UefiLib
>> +  UefiDriverEntryPoint
>> +  UefiBootServicesTableLib
>> +  UefiRuntimeServicesTableLib
>> +  UefiHiiServicesLib
>> +  MemoryAllocationLib
>> +  HiiLib
>> +  PcdLib
>> +  DxeServicesLib
>> +
>> +[Guids]
>> +  gEfiIfrTianoGuid                               ## PRODUCES            ## GUID  # HII opcode
>> +  ## PRODUCES                ## HII
>> +  ## CONSUMES                ## HII
>> +  gDtPlatformFormSetGuid
>> +  gEfiVirtualDiskGuid                            ## SOMETIMES_CONSUMES  ## GUID
>> +  gEfiFileInfoGuid                               ## SOMETIMES_CONSUMES  ## GUID  # Indicate the information type
>> +  gEdkiiPlatformHasAcpiGuid
>> +  gDtPlatformDefaultDtbFileGuid
>> +  gFdtTableGuid
>> +
>> +[Protocols]
>> +  gEfiHiiConfigAccessProtocolGuid                ## PRODUCES
>> +
>> +[Depex]
>> +  gEfiHiiDatabaseProtocolGuid         AND
>> +  gEfiVariableArchProtocolGuid        AND
>> +  gEfiVariableWriteArchProtocolGuid
>
> Can you please trim the sections in this INF file?
>
> For example, gEfiIfrTianoGuid is never used. Nor gEfiVirtualDiskGuid, gEfiFileInfoGuid. Etc.
>
> The ## remarks to the right look funny as well. Basically, please make this INF as minimal as possible.
>

Ack

>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.uni b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.uni
>> new file mode 100644
>> index 000000000000..f84069261486
>> --- /dev/null
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.uni
>> @@ -0,0 +1,27 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of the BSD License
>> +*  which accompanies this distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#langdef en-US  "English"
>> +
>> +#string STR_FORM_SET_TITLE             #language en-US "O/S Hardware Description Selection"
>> +#string STR_FORM_SET_TITLE_HELP        #language en-US "Press <Enter> to choose between ACPI and DT hardware descriptions."
>> +
>> +#string STR_MAIN_FORM_TITLE            #language en-US "O/S Hardware Description Selection"
>> +#string STR_RAM_DISK_NULL_STRING       #language en-US ""
>
> "ram disk"? :)
>
>> +
>> +#string STR_DT_ACPI_SELECT_PROMPT      #language en-US "O/S Hardware Description"
>> +#string STR_DT_ACPI_SELECT_HELP        #language en-US "Select the hardware description that will be exposed to the O/S."
>> +
>> +#string STR_DT_ACPI_SELECT_DT          #language en-US "Device Tree"
>> +#string STR_DT_ACPI_SELECT_ACPI        #language en-US "ACPI"
>
> Hmm okay. I wonder how you are handling the "variable missing" case, but I guess I'll see it soon below.
>
>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h
>> new file mode 100644
>> index 000000000000..2fb05a0336b6
>> --- /dev/null
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h
>> @@ -0,0 +1,29 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017, Linaro Limited. All rights reserved.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of the BSD License
>> +*  which accompanies this distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#ifndef __DT_PLATFORM_DXE_H__
>> +#define __DT_PLATFORM_DXE_H__
>> +
>> +#include <Guid/HiiPlatformSetupFormset.h>
>> +#include <Guid/DtPlatformFormSet.h>
>> +
>> +#define DT_ACPI_SELECT_DT       0x0
>> +#define DT_ACPI_SELECT_ACPI     0x1
>> +
>> +typedef struct {
>> +  UINT8         Pref;
>> +  UINT8         Reserved[3];
>> +} DT_ACPI_VARSTORE_DATA;
>> +
>> +#endif
>
> OK.
>
>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr
>> new file mode 100644
>> index 000000000000..d0c5463dd8f0
>> --- /dev/null
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr
>> @@ -0,0 +1,57 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of the BSD License
>> +*  which accompanies this distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#include "DtPlatformDxe.h"
>> +
>> +//
>> +// EFI Variable attributes
>> +//
>> +#define EFI_VARIABLE_NON_VOLATILE       0x00000001
>> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
>> +#define EFI_VARIABLE_RUNTIME_ACCESS     0x00000004
>> +#define EFI_VARIABLE_READ_ONLY          0x00000008
>> +
>> +formset
>> +  guid      = DT_PLATFORM_FORMSET_GUID,
>> +  title     = STRING_TOKEN(STR_FORM_SET_TITLE),
>> +  help      = STRING_TOKEN(STR_FORM_SET_TITLE_HELP),
>> +  classguid = EFI_HII_PLATFORM_SETUP_FORMSET_GUID,
>
> Not sure what classguid is really good for -- more precisely, I didn't use it in OvmfPkg/PlatformDxe. It probably doesn't hurt though.
>
>> +
>> +  efivarstore DT_ACPI_VARSTORE_DATA,
>> +    attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_NON_VOLATILE,  // EFI variable attributes
>> +    name  = AcpiDtPref,
>> +    guid  = DT_PLATFORM_FORMSET_GUID;
>
> I guess we can use the formset GUID for the variable namespace GUID too.
>
>> +
>> +  form formid = 0x1000,
>> +    title  = STRING_TOKEN(STR_MAIN_FORM_TITLE);
>> +
>> +    oneof varid = AcpiDtPref.Pref,
>> +        prompt      = STRING_TOKEN(STR_DT_ACPI_SELECT_PROMPT),
>> +        help        = STRING_TOKEN(STR_DT_ACPI_SELECT_HELP),
>> +        flags       = NUMERIC_SIZE_1 | INTERACTIVE | RESET_REQUIRED,
>> +        option text = STRING_TOKEN(STR_DT_ACPI_SELECT_DT), value = DT_ACPI_SELECT_DT, flags = DEFAULT;
>> +        option text = STRING_TOKEN(STR_DT_ACPI_SELECT_ACPI), value = DT_ACPI_SELECT_ACPI, flags = 0;
>> +    endoneof;
>
> Looks reasonable.
>
>> +
>> +    subtitle text = STRING_TOKEN(STR_RAM_DISK_NULL_STRING);
>
> "ram disk"
>
>> +
>> +    guidop
>> +      guid = DT_PLATFORM_FORMSET_GUID,
>> +      datatype = DT_ACPI_VARSTORE_DATA,
>> +        data.Pref  = 0x0,
>> +    endguidop;
>
> What is this good for? What happens if you drop it?
>
>> +
>> +  endform;
>> +
>> +endformset;
>
> So, at this point I think I should already remark that the names are somewhat inconsistent; you use both acpi-dt order and dt-acpi as well:
>
> - AcpiDtPref
> - DT_ACPI_*
>
> Can you make these consistent? (Rename whatever's easiest.)
>
>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> new file mode 100644
>> index 000000000000..d37a01e51d7d
>> --- /dev/null
>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>> @@ -0,0 +1,211 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +*
>> +*  This program and the accompanying materials
>> +*  are licensed and made available under the terms and conditions of the BSD License
>> +*  which accompanies this distribution.  The full text of the license may be found at
>> +*  http://opensource.org/licenses/bsd-license.php
>> +*
>> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +*
>> +**/
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/UefiDriverEntryPoint.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
>> +#include <Library/UefiHiiServicesLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/HiiLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/DxeServicesLib.h>
>> +
>> +#include "DtPlatformDxe.h"
>> +
>> +extern  UINT8                     DtPlatformHiiBin[];
>> +extern  UINT8                     DtPlatformDxeStrings[];
>> +
>> +typedef struct {
>> +  VENDOR_DEVICE_PATH              VendorDevicePath;
>> +  EFI_DEVICE_PATH_PROTOCOL        End;
>> +} HII_VENDOR_DEVICE_PATH;
>> +
>> +STATIC HII_VENDOR_DEVICE_PATH     mDtPlatformDxeVendorDevicePath = {
>> +  {
>> +    {
>> +      HARDWARE_DEVICE_PATH,
>> +      HW_VENDOR_DP,
>> +      {
>> +        (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
>> +        (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
>> +      }
>> +    },
>> +    DT_PLATFORM_FORMSET_GUID
>> +  },
>> +  {
>> +    END_DEVICE_PATH_TYPE,
>> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> +    {
>> +      (UINT8) (END_DEVICE_PATH_LENGTH),
>> +      (UINT8) ((END_DEVICE_PATH_LENGTH) >> 8)
>> +    }
>> +  }
>> +};
>
> Right, this matches my memories.
>
>> +
>> +STATIC
>> +EFI_STATUS
>> +InstallHiiPages (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUS                      Status;
>> +  EFI_HII_HANDLE                  HiiHandle;
>> +  EFI_HANDLE                      DriverHandle;
>> +
>> +  DriverHandle = NULL;
>> +  Status = gBS->InstallMultipleProtocolInterfaces (&DriverHandle,
>> +                  &gEfiDevicePathProtocolGuid,
>> +                  &mDtPlatformDxeVendorDevicePath,
>> +                  NULL);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  HiiHandle = HiiAddPackages (&gDtPlatformFormSetGuid,
>> +                              DriverHandle,
>> +                              DtPlatformDxeStrings,
>> +                              DtPlatformHiiBin,
>> +                              NULL);
>
> Looks OK thus far.
>
>> +
>> +  if (HiiHandle == NULL) {
>> +    gBS->UninstallMultipleProtocolInterfaces (&DriverHandle,
>
> Typo: this should be "DriverHandle" only, not "&DriverHandle".
>
>> +                  &gEfiDevicePathProtocolGuid,
>> +                  &mDtPlatformDxeVendorDevicePath,
>> +                  NULL);
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  The entry point for DtPlatformDxe driver.
>> +
>> +  @param[in] ImageHandle     The image handle of the driver.
>> +  @param[in] SystemTable     The system table.
>> +
>> +  @retval EFI_ALREADY_STARTED     The driver already exists in system.
>> +  @retval EFI_OUT_OF_RESOURCES    Fail to execute entry point due to lack of
>> +                                  resources.
>> +  @retval EFI_SUCCES              All the related protocols are installed on
>> +                                  the driver.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +DtPlatformDxeEntryPoint (
>> +  IN EFI_HANDLE                   ImageHandle,
>> +  IN EFI_SYSTEM_TABLE             *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS                      Status;
>> +  EFI_HANDLE                      DriverHandle;
>> +  DT_ACPI_VARSTORE_DATA           DtAcpiPref;
>> +  UINTN                           BufferSize;
>> +  VOID                            *Dtb;
>> +  UINTN                           DtbSize;
>> +  VOID                            *DtbCopy;
>> +
>> +  //
>> +  // Check whether a DTB blob is included in the firmware image.
>> +  //
>> +  Dtb = NULL;
>> +  Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
>> +             EFI_SECTION_RAW, 0, &Dtb, &DtbSize);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_WARN, "%a: no DTB blob found, defaulting to ACPI\n",
>> +      __FUNCTION__));
>> +    DtAcpiPref.Pref = DT_ACPI_SELECT_ACPI;
>> +  } else {
>> +    //
>> +    // Get the current ACPI/DT preference from the AcpiDtPref variable.
>> +    //
>
> The comment says "AcpiDtPref variable" (which corresponds to the name under which the VFR also refers to the variable), but below the variable services actually get a "DtAcpiPref" argument as variable name.
>
> Please fix this inconsistency together with the other instances.
>
>> +    BufferSize = sizeof (DtAcpiPref);
>> +    Status = gRT->GetVariable(L"DtAcpiPref", &gDtPlatformFormSetGuid, NULL,
>> +                    &BufferSize, &DtAcpiPref);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_WARN, "%a: no DT/ACPI preference found, defaulting to DT\n",
>> +        __FUNCTION__));
>> +      DtAcpiPref.Pref = DT_ACPI_SELECT_DT;
>> +    }
>> +  }
>> +
>> +  if (!EFI_ERROR (Status) &&
>> +      DtAcpiPref.Pref != DT_ACPI_SELECT_ACPI &&
>> +      DtAcpiPref.Pref != DT_ACPI_SELECT_DT) {
>> +    DEBUG ((DEBUG_WARN, "%a: illegal value for DtAcpiPref, defaulting to DT\n",
>> +      __FUNCTION__));
>
> Personal request: please replace "illegal" with "invalid".
>
>> +    DtAcpiPref.Pref = DT_ACPI_SELECT_DT;
>> +    Status = EFI_INVALID_PARAMETER; // trigger setvar below
>> +  }
>> +
>> +  //
>> +  // Write the newly selected default value back to the variable store.
>> +  //
>> +  if (EFI_ERROR (Status)) {
>> +    Status = gRT->SetVariable(L"DtAcpiPref", &gDtPlatformFormSetGuid,
>> +                    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
>> +                    sizeof (DtAcpiPref), &DtAcpiPref);
>
> I suggest to use a macro for the variable name...
>
>> +    if (EFI_ERROR (Status)) {
>> +      return Status;
>> +    }
>> +  }
>> +
>> +  if (DtAcpiPref.Pref == DT_ACPI_SELECT_ACPI) {
>> +    //
>> +    // ACPI was selected: install the gEdkiiPlatformHasAcpiGuid GUID as a
>> +    // NULL protocol to unlock dispatch of ACPI related drivers.
>> +    //
>> +    DriverHandle = NULL;
>> +    Status = gBS->InstallMultipleProtocolInterfaces (&DriverHandle,
>> +                    gEdkiiPlatformHasAcpiGuid, NULL, NULL);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR,
>> +        "%a: failed to install gEdkiiPlatformHasAcpiGuid as a protocol\n",
>> +        __FUNCTION__));
>> +      return Status;
>> +    }
>
> I think you don't need to create a new controller handle for gEdkiiPlatformHasAcpiGuid; using ImageHandle should be fine.
>
> (Same for gEfiDevicePathProtocolGuid in InstallHiiPages().)
>
> Not critical though, just simpler perhaps.
>
>> +  } else if (DtAcpiPref.Pref == DT_ACPI_SELECT_DT) {
>> +    //
>> +    // DT was selected: copy the blob into newly allocated memory and install
>> +    // a reference to it as the FDT configuration table.
>> +    //
>> +    DtbCopy = AllocateCopyPool (DtbSize, Dtb);
>> +    if (DtbCopy == NULL) {
>> +      return EFI_OUT_OF_RESOURCES;
>> +    }
>> +    Status = gBS->InstallConfigurationTable (&gFdtTableGuid, DtbCopy);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration table\n",
>> +        __FUNCTION__));
>> +      FreePool (DtbCopy);
>> +      return Status;
>> +    }
>
> Looks good. The original DTB (found in the FV section) lives in flash, generally speaking, right?
>

Yes.

>> +  } else {
>> +    ASSERT (FALSE);
>> +  }
>> +
>> +  //
>> +  // No point in installing the HII pages if ACPI is the only description
>> +  // we have
>> +  //
>> +  if (Dtb == NULL) {
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  return InstallHiiPages ();
>> +}
>

Thanks for the review! I will respin this as soon as we know where
PlatformHasAcpiGuid has landed :-)