* [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI
@ 2017-03-27 10:07 Ard Biesheuvel
2017-03-28 10:43 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-03-27 10:07 UTC (permalink / raw)
To: edk2-devel, lersek, leif.lindholm, marcin.wojtas; +Cc: agraf, Ard Biesheuvel
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(+)
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)
+ }
+ }
+};
+
+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;
+ 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.
+ //
+ 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__));
+ 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);
+ 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;
+ }
+ } 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;
+ }
+ } 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 ();
+}
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
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
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 ""
+
+#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"
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,
+
+ efivarstore DT_ACPI_VARSTORE_DATA,
+ attribute = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_NON_VOLATILE, // EFI variable attributes
+ name = AcpiDtPref,
+ guid = DT_PLATFORM_FORMSET_GUID;
+
+ 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;
+
+ subtitle text = STRING_TOKEN(STR_RAM_DISK_NULL_STRING);
+
+ guidop
+ guid = DT_PLATFORM_FORMSET_GUID,
+ datatype = DT_ACPI_VARSTORE_DATA,
+ data.Pref = 0x0,
+ endguidop;
+
+ endform;
+
+endformset;
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 } }
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
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
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI
2017-03-27 10:07 [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI Ard Biesheuvel
@ 2017-03-28 10:43 ` Laszlo Ersek
2017-03-28 10:47 ` Ard Biesheuvel
2017-03-28 10:49 ` Laszlo Ersek
0 siblings, 2 replies; 6+ messages in thread
From: Laszlo Ersek @ 2017-03-28 10:43 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm, marcin.wojtas; +Cc: agraf
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI
2017-03-28 10:43 ` Laszlo Ersek
@ 2017-03-28 10:47 ` Ard Biesheuvel
2017-03-28 10:49 ` Laszlo Ersek
1 sibling, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-03-28 10:47 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm, Marcin Wojtas,
Alexander Graf
On 28 March 2017 at 11:43, Laszlo Ersek <lersek@redhat.com> 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 = 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?)
>
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 = { 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.
>
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/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.
>
Ack
>> 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?
>
Yes.
>> + } 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 for the review! I will respin this as soon as we know where
PlatformHasAcpiGuid has landed :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI
2017-03-28 10:43 ` Laszlo Ersek
2017-03-28 10:47 ` Ard Biesheuvel
@ 2017-03-28 10:49 ` Laszlo Ersek
2017-03-28 10:51 ` Ard Biesheuvel
1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-03-28 10:49 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm, marcin.wojtas; +Cc: agraf
On 03/28/17 12:43, Laszlo Ersek wrote:
> Ard,
>
> On 03/27/17 12:07, Ard Biesheuvel wrote:
[snip]
>> +/**
>> + 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 ();
>> +}
Consider uninstalling the FDT config table / the "has ACPI" protocol if
InstallHiiPages() fails and the driver exits with error (it gets
unloaded)? My inner pedant wants you to do this, although I do realize
that neither the FDT config table nor the NULL "has ACPI" protocol hook
back into this driver.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI
2017-03-28 10:49 ` Laszlo Ersek
@ 2017-03-28 10:51 ` Ard Biesheuvel
2017-03-28 10:58 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2017-03-28 10:51 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm, Marcin Wojtas,
Alexander Graf
On 28 March 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/28/17 12:43, Laszlo Ersek wrote:
>> Ard,
>>
>> On 03/27/17 12:07, Ard Biesheuvel wrote:
>
> [snip]
>
>>> +/**
>>> + 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 ();
>>> +}
>
> Consider uninstalling the FDT config table / the "has ACPI" protocol if
> InstallHiiPages() fails and the driver exits with error (it gets
> unloaded)? My inner pedant wants you to do this, although I do realize
> that neither the FDT config table nor the NULL "has ACPI" protocol hook
> back into this driver.
>
That was my reasoning, yes. I will add a comment to that effect, but I
am reluctant to break the boot entirely just because we failed to
register the HII pages.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI
2017-03-28 10:51 ` Ard Biesheuvel
@ 2017-03-28 10:58 ` Laszlo Ersek
0 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2017-03-28 10:58 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Marcin Wojtas,
Alexander Graf
On 03/28/17 12:51, Ard Biesheuvel wrote:
> On 28 March 2017 at 11:49, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/28/17 12:43, Laszlo Ersek wrote:
>>> Ard,
>>>
>>> On 03/27/17 12:07, Ard Biesheuvel wrote:
>>
>> [snip]
>>
>>>> +/**
>>>> + 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 ();
>>>> +}
>>
>> Consider uninstalling the FDT config table / the "has ACPI" protocol if
>> InstallHiiPages() fails and the driver exits with error (it gets
>> unloaded)? My inner pedant wants you to do this, although I do realize
>> that neither the FDT config table nor the NULL "has ACPI" protocol hook
>> back into this driver.
>>
>
> That was my reasoning, yes. I will add a comment to that effect, but I
> am reluctant to break the boot entirely just because we failed to
> register the HII pages.
>
Good point.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-28 10:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-27 10:07 [PATCH] EmbeddedPkg: add DT platform driver to select between DT and ACPI Ard Biesheuvel
2017-03-28 10:43 ` Laszlo Ersek
2017-03-28 10:47 ` Ard Biesheuvel
2017-03-28 10:49 ` Laszlo Ersek
2017-03-28 10:51 ` Ard Biesheuvel
2017-03-28 10:58 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox