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.biesheu...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
