From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 701B921BC6A23 for ; Tue, 28 Mar 2017 03:43:26 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BFE9461B84; Tue, 28 Mar 2017 10:43:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BFE9461B84 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com BFE9461B84 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-89.phx2.redhat.com [10.3.116.89]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1654194D3E; Tue, 28 Mar 2017 10:43:23 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org, leif.lindholm@linaro.org, marcin.wojtas@semihalf.com References: <20170327100754.32387-1-ard.biesheuvel@linaro.org> Cc: agraf@suse.de From: Laszlo Ersek Message-ID: <2420a783-d72d-e9f2-bd92-54e4e4866cc3@redhat.com> Date: Tue, 28 Mar 2017 12:43:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170327100754.32387-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 28 Mar 2017 10:43:26 +0000 (UTC) Subject: Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Mar 2017 10:43:26 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > --- > > 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.
> +# > +# 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 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 > +#include > + > +#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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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