From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AFA1D21DFA7B5 for ; Tue, 28 Mar 2017 03:47:40 -0700 (PDT) Received: by mail-it0-x229.google.com with SMTP id y18so25937391itc.1 for ; Tue, 28 Mar 2017 03:47:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=H8sj9BtJvwHevyhFYddt9hDAgrTkPngujX2fPJQjEvo=; b=BalV52fMUF0JkCm0gb634ZGQzd/L+76mLaOjLfJzq0WDXDeAQlxG4+mfim7abjjmhd eFJU/RpRYlaS9d2uQie4GBGYSbGsFXyfg4EmJR8Wr7XadM5VFFmcVaGYdVeUEuz8yF79 OEaYEVqghkqa16fK7KrODvlUs14/MLXPo+41o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=H8sj9BtJvwHevyhFYddt9hDAgrTkPngujX2fPJQjEvo=; b=e77I0YqDFC7hTaLQZCPoKPUPK2oWmRuiI8fJPAHp7AP/9AGKQIEXaQWcu12AM/x62g DfvVtsWNMswqAn3zcoDQA9exjDWdq7VDbX13xR7JnSksrx4ch/EIHbFBKHy+xRF3Jv+P egd339hJ+Z4lVG+xKVsgwtywPQQw9VCPOFUgmbdCeTHpG/il6HCfT1fcRV/bSB+cCglO YOYOHRsylVnTDANnufm+/hSijCEftIARVrxEMYY6g1NBSrAYi12IIdtRwKQiNjrxGIfK 66HDYf2km3lHXtnetpPK4dT/jLMdtY3A9yjuq59ifaZ19Vjf2oKXuiKX7ejXnk0vIjN3 5TuQ== X-Gm-Message-State: AFeK/H0XtAB2o2GQUJUFOZR3/AZBt2oz0eIuE1lzEOA7KQPuUgqQ0GwzVRWmXC8DRsmnV8MT8zgG+Ewh0ZEV8Jm2 X-Received: by 10.36.207.212 with SMTP id y203mr13643559itf.63.1490698059433; Tue, 28 Mar 2017 03:47:39 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 28 Mar 2017 03:47:39 -0700 (PDT) In-Reply-To: <2420a783-d72d-e9f2-bd92-54e4e4866cc3@redhat.com> References: <20170327100754.32387-1-ard.biesheuvel@linaro.org> <2420a783-d72d-e9f2-bd92-54e4e4866cc3@redhat.com> From: Ard Biesheuvel Date: Tue, 28 Mar 2017 11:47:39 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Marcin Wojtas , Alexander Graf 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:47:41 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 28 March 2017 at 11:43, Laszlo Ersek 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 =3D 25462CDA-221F-47DF-AC1D-259CFAA4E326 { >> SECTION RAW =3D 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-gi= t-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, VFR= s, INF / DEC / DSC files) come first, and the imperative changes (C code) c= omes 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 =3D { 0x16958446, 0x19B7, 0x480B, { 0xB0, 0x47, 0x74, 0= x85, 0xAD, 0x3F, 0x71, 0x6D } } >> gFdtVariableGuid =3D { 0x25a4fd4a, 0x9703, 0x4ba9, { 0xa1, 0x90, 0xb7= , 0xc8, 0x4e, 0xfb, 0x3e, 0x57 } } >> >> + # HII form set GUID for DtPlatformDxe driver >> + gDtPlatformFormSetGuid =3D { 0x2b7a240d, 0xd5ad, 0x4fd6, { 0xbe, 0x1c= , 0xdf, 0xa4, 0x41, 0x5f, 0x55, 0x26 } } >> + >> + # File GUID for default DTB image embedded in the firmware volume >> + gDtPlatformDefaultDtbFileGuid =3D { 0x25462cda, 0x221f, 0x47df, { 0xa= c, 0x1d, 0x25, 0x9c, 0xfa, 0xa4, 0xe3, 0x26 } } >> + >> [Protocols.common] >> gHardwareInterruptProtocolGuid =3D { 0x2890B3EA, 0x053D, 0x1643, { 0= xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } } >> gEfiDebugSupportPeriodicCallbackProtocolGuid =3D { 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 th= e BSD License >> +* which accompanies this distribution. The full text of the license m= ay 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, 0= x55, 0x26 } } >> + >> +extern EFI_GUID gDtPlatformFormSetGuid; >> + >> +#endif >> > > Also okay. > > >> diff --git a/EmbeddedPkg/Include/Guid/DtPlatformDefaultDtbFile.h b/Embed= dedPkg/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 th= e BSD License >> +* which accompanies this distribution. The full text of the license m= ay 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, 0= x55, 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 u= sing 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/Embed= dedPkg/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 =3D 0x00010019 >> + BASE_NAME =3D DtPlatformDxe >> + FILE_GUID =3D FC097B3C-2EBD-4A75-A3DA-121DCAB365CC >> + MODULE_TYPE =3D DXE_DRIVER >> + VERSION_STRING =3D 1.0 >> + ENTRY_POINT =3D DtPlatformDxeEntryPoint >> + >> +# >> +# The following information is for reference only and not required by t= he build tools. >> +# >> +# VALID_ARCHITECTURES =3D 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, gEf= iFileInfoGuid. Etc. > > The ## remarks to the right look funny as well. Basically, please make th= is INF as minimal as possible. > Ack >> diff --git a/EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformHii.uni b/Embed= dedPkg/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 th= e BSD License >> +* which accompanies this distribution. The full text of the license m= ay 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 De= scription Selection" >> +#string STR_FORM_SET_TITLE_HELP #language en-US "Press t= o choose between ACPI and DT hardware descriptions." >> + >> +#string STR_MAIN_FORM_TITLE #language en-US "O/S Hardware De= scription Selection" >> +#string STR_RAM_DISK_NULL_STRING #language en-US "" > > "ram disk"? :) > >> + >> +#string STR_DT_ACPI_SELECT_PROMPT #language en-US "O/S Hardware De= scription" >> +#string STR_DT_ACPI_SELECT_HELP #language en-US "Select the hard= ware 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/Embedde= dPkg/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 th= e BSD License >> +* which accompanies this distribution. The full text of the license m= ay 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/Embed= dedPkg/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 th= e BSD License >> +* which accompanies this distribution. The full text of the license m= ay 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 =3D DT_PLATFORM_FORMSET_GUID, >> + title =3D STRING_TOKEN(STR_FORM_SET_TITLE), >> + help =3D STRING_TOKEN(STR_FORM_SET_TITLE_HELP), >> + classguid =3D EFI_HII_PLATFORM_SETUP_FORMSET_GUID, > > Not sure what classguid is really good for -- more precisely, I didn't us= e it in OvmfPkg/PlatformDxe. It probably doesn't hurt though. > >> + >> + efivarstore DT_ACPI_VARSTORE_DATA, >> + attribute =3D EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_NON_VO= LATILE, // EFI variable attributes >> + name =3D AcpiDtPref, >> + guid =3D DT_PLATFORM_FORMSET_GUID; > > I guess we can use the formset GUID for the variable namespace GUID too. > >> + >> + form formid =3D 0x1000, >> + title =3D STRING_TOKEN(STR_MAIN_FORM_TITLE); >> + >> + oneof varid =3D AcpiDtPref.Pref, >> + prompt =3D STRING_TOKEN(STR_DT_ACPI_SELECT_PROMPT), >> + help =3D STRING_TOKEN(STR_DT_ACPI_SELECT_HELP), >> + flags =3D NUMERIC_SIZE_1 | INTERACTIVE | RESET_REQUIRED, >> + option text =3D STRING_TOKEN(STR_DT_ACPI_SELECT_DT), value =3D = DT_ACPI_SELECT_DT, flags =3D DEFAULT; >> + option text =3D STRING_TOKEN(STR_DT_ACPI_SELECT_ACPI), value = =3D DT_ACPI_SELECT_ACPI, flags =3D 0; >> + endoneof; > > Looks reasonable. > >> + >> + subtitle text =3D STRING_TOKEN(STR_RAM_DISK_NULL_STRING); > > "ram disk" > >> + >> + guidop >> + guid =3D DT_PLATFORM_FORMSET_GUID, >> + datatype =3D DT_ACPI_VARSTORE_DATA, >> + data.Pref =3D 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 some= what 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/Embedde= dPkg/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 th= e BSD License >> +* which accompanies this distribution. The full text of the license m= ay 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 =3D { >> + { >> + { >> + 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 =3D NULL; >> + Status =3D gBS->InstallMultipleProtocolInterfaces (&DriverHandle, >> + &gEfiDevicePathProtocolGuid, >> + &mDtPlatformDxeVendorDevicePath, >> + NULL); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + >> + HiiHandle =3D HiiAddPackages (&gDtPlatformFormSetGuid, >> + DriverHandle, >> + DtPlatformDxeStrings, >> + DtPlatformHiiBin, >> + NULL); > > Looks OK thus far. > >> + >> + if (HiiHandle =3D=3D 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 la= ck of >> + resources. >> + @retval EFI_SUCCES All the related protocols are install= ed 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 =3D NULL; >> + Status =3D 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 =3D 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 und= er which the VFR also refers to the variable), but below the variable servi= ces actually get a "DtAcpiPref" argument as variable name. > > Please fix this inconsistency together with the other instances. > >> + BufferSize =3D sizeof (DtAcpiPref); >> + Status =3D 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 =3D DT_ACPI_SELECT_DT; >> + } >> + } >> + >> + if (!EFI_ERROR (Status) && >> + DtAcpiPref.Pref !=3D DT_ACPI_SELECT_ACPI && >> + DtAcpiPref.Pref !=3D DT_ACPI_SELECT_DT) { >> + DEBUG ((DEBUG_WARN, "%a: illegal value for DtAcpiPref, defaulting t= o DT\n", >> + __FUNCTION__)); > > Personal request: please replace "illegal" with "invalid". > >> + DtAcpiPref.Pref =3D DT_ACPI_SELECT_DT; >> + Status =3D EFI_INVALID_PARAMETER; // trigger setvar below >> + } >> + >> + // >> + // Write the newly selected default value back to the variable store. >> + // >> + if (EFI_ERROR (Status)) { >> + Status =3D gRT->SetVariable(L"DtAcpiPref", &gDtPlatformFormSetGuid, >> + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVIC= E_ACCESS, >> + sizeof (DtAcpiPref), &DtAcpiPref); > > I suggest to use a macro for the variable name... > >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + } >> + >> + if (DtAcpiPref.Pref =3D=3D DT_ACPI_SELECT_ACPI) { >> + // >> + // ACPI was selected: install the gEdkiiPlatformHasAcpiGuid GUID as= a >> + // NULL protocol to unlock dispatch of ACPI related drivers. >> + // >> + DriverHandle =3D NULL; >> + Status =3D 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 gEdkiiPlatfo= rmHasAcpiGuid; using ImageHandle should be fine. > > (Same for gEfiDevicePathProtocolGuid in InstallHiiPages().) > > Not critical though, just simpler perhaps. > >> + } else if (DtAcpiPref.Pref =3D=3D DT_ACPI_SELECT_DT) { >> + // >> + // DT was selected: copy the blob into newly allocated memory and i= nstall >> + // a reference to it as the FDT configuration table. >> + // >> + DtbCopy =3D AllocateCopyPool (DtbSize, Dtb); >> + if (DtbCopy =3D=3D NULL) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + Status =3D gBS->InstallConfigurationTable (&gFdtTableGuid, DtbCopy)= ; >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: failed to install FDT configuration tab= le\n", >> + __FUNCTION__)); >> + FreePool (DtbCopy); >> + return Status; >> + } > > Looks good. The original DTB (found in the FV section) lives in flash, ge= nerally speaking, right? > Yes. >> + } else { >> + ASSERT (FALSE); >> + } >> + >> + // >> + // No point in installing the HII pages if ACPI is the only descripti= on >> + // we have >> + // >> + if (Dtb =3D=3D NULL) { >> + return EFI_SUCCESS; >> + } >> + >> + return InstallHiiPages (); >> +} > Thanks for the review! I will respin this as soon as we know where PlatformHasAcpiGuid has landed :-)