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

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

On 28 March 2017 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/28/17 18:16, Ard Biesheuvel wrote:
>> (use correct address for Marcin -- please take into account when replying)
>>
>> On 28 March 2017 at 17:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> As a follow up to the changes proposed by Laszlo to make ACPI and DT
>>> mutually exclusive on ArmVirtQemu, this patch proposed a DT platform
>
> new typo relative to v1: s/proposed/proposes/
>
>>> 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.
>>>
>>> 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>
>>> ---
>>> v2: fixup various errors and rough edges pointed out by Laszlo
>>>
>>>  EmbeddedPkg/EmbeddedPkg.dec                         |   6 +
>>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf |  65 ++++++
>>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h   |  31 +++
>>>  EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h |  23 +++
>>>  EmbeddedPkg/Include/Guid/DtPlatformFormSet.h        |  23 +++
>>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr |  51 +++++
>>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c   | 216 ++++++++++++++++++++
>>>  EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.uni |  27 +++
>>>  8 files changed, 442 insertions(+)
>>>
>>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
>>> index 29736fb4a71d..871fc5ff4016 100644
>>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>>> @@ -62,6 +62,12 @@ [Guids.common]
>>>    ## Include/Guid/PlatformHasDeviceTree.h
>>>    gEdkiiPlatformHasDeviceTreeGuid = { 0x7ebb920d, 0x1aaf, 0x46d9, { 0xb2, 0xaf, 0x54, 0x1e, 0x1d, 0xce, 0x14, 0x8b } }
>>>
>>> +  # 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 } }
>>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>>> new file mode 100644
>>> index 000000000000..aa1d3abb4012
>>> --- /dev/null
>>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
>>> @@ -0,0 +1,65 @@
>>> +## @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]
>>> +  gDtPlatformFormSetGuid
>>> +  gEdkiiPlatformHasAcpiGuid
>>> +  gDtPlatformDefaultDtbFileGuid
>>> +  gFdtTableGuid
>>> +
>>> +[Protocols]
>>> +  gEfiHiiConfigAccessProtocolGuid                ## PRODUCES
>
> I think this should have been dropped as well -- you can remove it when
> you commit the patch, if the removal works. (I think it should.)
>
>>> +
>>> +[Depex]
>>> +  gEfiHiiDatabaseProtocolGuid         AND
>
> IMO this should have been dropped too. You are using UefiHiiServicesLib
> (only one instance available:
> "MdeModulePkg/Library/UefiHiiServicesLib/UefiHiiServicesLib.inf"), and
> that instance gives you a dependency on gEfiHiiDatabaseProtocolGuid
> automatically.
>
> You never use the protocol manually, just through the following chain:
>
> * HiiAddPackages() is from HiiLib (only one instance:
> MdeModulePkg/Library/UefiHiiLib/UefiHiiLib.inf), which relies on
> "gHiiDatabase"
>
> * "gHiiDatabase" is defined / initialized in UefiHiiServicesLib
> (UefiHiiServicesLibConstructor()), dependent on
> gEfiHiiDatabaseProtocolGuid, and the library instance also spells out
> the DEPEX for that.
>
> So I think, if you can drop this from the depex while keeping things
> functional, you should. No need to resubmit just because of this, again.
>
>
> Some more things which look useless for this driver:
> - PcdLib (class in INF, hdr inclusion in source)
> - ... OK I guess that's all.
>
> (In my v1 review I wrote "Basically, please make this INF as minimal as
> possible" -- apologies for not spelling it out in more detail.)
>
>>> +  gEfiVariableArchProtocolGuid        AND
>>> +  gEfiVariableWriteArchProtocolGuid
>>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h
>>> new file mode 100644
>>> index 000000000000..2369367277b0
>>> --- /dev/null
>>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.h
>>> @@ -0,0 +1,31 @@
>>> +/** @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
>>> +
>>> +#define DT_ACPI_VARIABLE_NAME   L"DtAcpiPref"
>>> +
>>> +typedef struct {
>>> +  UINT8         Pref;
>>> +  UINT8         Reserved[3];
>>> +} DT_ACPI_VARSTORE_DATA;
>>> +
>>> +#endif
>>> diff --git a/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h b/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h
>>> new file mode 100644
>>> index 000000000000..c44b4d9522fe
>>> --- /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  \
>>> +  { 0x25462cda, 0x221f, 0x47df, { 0xac, 0x1d, 0x25, 0x9c, 0xfa, 0xa4, 0xe3, 0x26 } }
>>> +
>>> +extern EFI_GUID gDtPlatformDefaultDtbFileGuid;
>>> +
>>> +#endif
>>> 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
>>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr
>>> new file mode 100644
>>> index 000000000000..3516746c4d84
>>> --- /dev/null
>>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.vfr
>>> @@ -0,0 +1,51 @@
>>> +/** @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,
>>> +
>>> +  efivarstore DT_ACPI_VARSTORE_DATA,
>>> +    attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_NON_VOLATILE,  // EFI variable attributes
>>> +    name  = DtAcpiPref,
>>> +    guid  = DT_PLATFORM_FORMSET_GUID;
>>> +
>>> +  form formid = 0x1000,
>>> +    title  = STRING_TOKEN(STR_MAIN_FORM_TITLE);
>>> +
>>> +    oneof varid = DtAcpiPref.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;
>>> +
>>> +    subtitle text = STRING_TOKEN(STR_NULL_STRING);
>>> +
>>> +  endform;
>>> +
>>> +endformset;
>>> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>>> new file mode 100644
>>> index 000000000000..f714551213c2
>>> --- /dev/null
>>> +++ b/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c
>>> @@ -0,0 +1,216 @@
>>> +/** @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)
>>> +    }
>>> +  }
>>> +};
>>> +
>>> +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);
>>> +
>>> +  if (HiiHandle == NULL) {
>>> +    gBS->UninstallMultipleProtocolInterfaces (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;
>>> +  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 DT/ACPI preference from the DtAcpiPref variable.
>>> +    //
>>> +    BufferSize = sizeof (DtAcpiPref);
>>> +    Status = gRT->GetVariable(DT_ACPI_VARIABLE_NAME, &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: invalid value for DtAcpiPref, defaulting to DT\n",
>>> +      __FUNCTION__));
>
> Can you replace this instance of "... DtAcpiPref ..." with "... %a ..."
> and DT_ACPI_VARIABLE_NAME? No need to resubmit.
>

That should be %s, I suppose?

> With those warts removed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks!