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

[Laszlo Ersek <lersek@redhat.com>]

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?)

> 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.

> 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.

> 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?

> +  } 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
Laszlo