* [PATCH 01/10] Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 02/10] ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf" Laszlo Ersek
` (11 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
In a later patch, we'll modify the depex of
"ArmPkg/Drivers/CpuDxe/CpuDxe.inf" (currently "AFTER gArmGicDxeFileGuid")
to "gHardwareInterruptProtocolGuid OR gHardwareInterrupt2ProtocolGuid".
Considering platforms that include "ArmPkg/Drivers/CpuDxe/CpuDxe.inf",
there are two classes:
(1) The platform gets its gHardwareInterruptProtocolGuid or
gHardwareInterrupt2ProtocolGuid instance from
"ArmPkg/Drivers/ArmGic/ArmGicDxe.inf". For such platforms, the
upcoming CpuDxe change is not a problem, because commit 61a7b0ec634f
made ArmGicDxe wait for the CPU Arch Protocol with a protocol notify.
(2) The platform gets its hardware interrupt protocol(s) from a different
driver that has a hard depex on the CPU Arch Protocol. The upcoming
CpuDxe change would lead to a loop in the DXE dispatch order.
In the edk2 tree, only "BeagleBoardPkg/BeagleBoardPkg.dsc" falls in class
(2), and the driver in question is "Omap35xxPkg/InterruptDxe". Port (most
of) commit 61a7b0ec634f to it.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
build-tested only, as part of BeagleBoardPkg
Omap35xxPkg/InterruptDxe/InterruptDxe.inf | 6 +-
Omap35xxPkg/InterruptDxe/HardwareInterrupt.c | 81 +++++++++++++++-----
2 files changed, 66 insertions(+), 21 deletions(-)
diff --git a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
index 6deb8c3f675c..61ad89af2758 100644
--- a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
+++ b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
@@ -41,14 +41,14 @@ [LibraryClasses]
PrintLib
UefiDriverEntryPoint
IoLib
ArmLib
[Protocols]
- gHardwareInterruptProtocolGuid
- gEfiCpuArchProtocolGuid
+ gHardwareInterruptProtocolGuid ## PRODUCES
+ gEfiCpuArchProtocolGuid ## CONSUMES ## NOTIFY
[FixedPcd.common]
gEmbeddedTokenSpaceGuid.PcdInterruptBaseAddress
[Depex]
- gEfiCpuArchProtocolGuid
+ TRUE
diff --git a/Omap35xxPkg/InterruptDxe/HardwareInterrupt.c b/Omap35xxPkg/InterruptDxe/HardwareInterrupt.c
index 09e22b5921b0..2ddc7c0566d0 100644
--- a/Omap35xxPkg/InterruptDxe/HardwareInterrupt.c
+++ b/Omap35xxPkg/InterruptDxe/HardwareInterrupt.c
@@ -293,12 +293,60 @@ EFI_HARDWARE_INTERRUPT_PROTOCOL gHardwareInterruptProtocol = {
EnableInterruptSource,
DisableInterruptSource,
GetInterruptSourceState,
EndOfInterrupt
};
+STATIC VOID *mCpuArchProtocolNotifyEventRegistration;
+
+STATIC
+VOID
+EFIAPI
+CpuArchEventProtocolNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_CPU_ARCH_PROTOCOL *Cpu;
+ EFI_STATUS Status;
+
+ //
+ // Get the CPU protocol that this driver requires.
+ //
+ Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: gBS->LocateProtocol() - %r\n", __FUNCTION__,
+ Status));
+ ASSERT (FALSE);
+ return;
+ }
+
+ //
+ // Unregister the default exception handler.
+ //
+ Status = Cpu->RegisterInterruptHandler (Cpu, EXCEPT_ARM_IRQ, NULL);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: Cpu->RegisterInterruptHandler() - %r\n",
+ __FUNCTION__, Status));
+ ASSERT (FALSE);
+ return;
+ }
+
+ //
+ // Register to receive interrupts
+ //
+ Status = Cpu->RegisterInterruptHandler (Cpu, EXCEPT_ARM_IRQ,
+ IrqInterruptHandler);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: Cpu->RegisterInterruptHandler() - %r\n",
+ __FUNCTION__, Status));
+ ASSERT (FALSE);
+ return;
+ }
+}
+
/**
Initialize the state information for the CPU Architectural Protocol
@param ImageHandle of the loaded driver
@param SystemTable Pointer to the System Table
@@ -311,13 +359,13 @@ EFI_STATUS
InterruptDxeInitialize (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
EFI_STATUS Status;
- EFI_CPU_ARCH_PROTOCOL *Cpu;
+ EFI_EVENT CpuArchEvent;
// Make sure the Interrupt Controller Protocol is not already installed in the system.
ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
// Make sure all interrupts are disabled by default.
MmioWrite32 (INTCPS_MIR(0), 0xFFFFFFFF);
@@ -328,30 +376,27 @@ InterruptDxeInitialize (
Status = gBS->InstallMultipleProtocolInterfaces(&gHardwareInterruptHandle,
&gHardwareInterruptProtocolGuid, &gHardwareInterruptProtocol,
NULL);
ASSERT_EFI_ERROR(Status);
//
- // Get the CPU protocol that this driver requires.
- //
- Status = gBS->LocateProtocol(&gEfiCpuArchProtocolGuid, NULL, (VOID **)&Cpu);
- ASSERT_EFI_ERROR(Status);
-
- //
- // Unregister the default exception handler.
- //
- Status = Cpu->RegisterInterruptHandler(Cpu, EXCEPT_ARM_IRQ, NULL);
- ASSERT_EFI_ERROR(Status);
-
- //
- // Register to receive interrupts
- //
- Status = Cpu->RegisterInterruptHandler(Cpu, EXCEPT_ARM_IRQ, IrqInterruptHandler);
- ASSERT_EFI_ERROR(Status);
+ // Install the interrupt handler as soon as the CPU arch protocol appears.
+ //
+ CpuArchEvent = EfiCreateProtocolNotifyEvent (
+ &gEfiCpuArchProtocolGuid,
+ TPL_CALLBACK,
+ CpuArchEventProtocolNotify,
+ NULL,
+ &mCpuArchProtocolNotifyEventRegistration
+ );
+ ASSERT (CpuArchEvent != NULL);
// Register for an ExitBootServicesEvent
Status = gBS->CreateEvent(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY, ExitBootServicesEvent, NULL, &EfiExitBootServicesEvent);
- ASSERT_EFI_ERROR(Status);
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ gBS->CloseEvent (CpuArchEvent);
+ }
return Status;
}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 02/10] ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
2018-04-12 0:55 ` [PATCH 01/10] Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 03/10] ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex Laszlo Ersek
` (10 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
"ArmGicDxe.inf" currently does not document how the protocols in the
[Protocols] section are used. Such comments help us analyze behavior, so
let's add them now.
- gHardwareInterruptProtocolGuid and gHardwareInterrupt2ProtocolGuid are
always produced on the InterruptDxeInitialize() -> (GicV2DxeInitialize()
| GicV3DxeInitialize()) -> InstallAndRegisterInterruptService() call
path.
- gEfiCpuArchProtocolGuid is consumed in the CpuArchEventProtocolNotify()
protocol notify callback. (Technically this is "conditional"; however
the firmware cannot work without architectural protocols, so we can call
it unconditional.)
While at it, drop the gArmGicDxeFileGuid comment from FILE_GUID; we're
going to make that GUID uninteresting soon.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
index 24b02ef30e83..9c85b1e069dc 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+++ b/ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
@@ -13,13 +13,13 @@
#
#**/
[Defines]
INF_VERSION = 0x00010005
BASE_NAME = ArmGicDxe
- FILE_GUID = DE371F7C-DEC4-4D21-ADF1-593ABCC15882 # gArmGicDxeFileGuid
+ FILE_GUID = DE371F7C-DEC4-4D21-ADF1-593ABCC15882
MODULE_TYPE = DXE_DRIVER
VERSION_STRING = 1.0
ENTRY_POINT = InterruptDxeInitialize
[Sources.common]
@@ -45,15 +45,15 @@ [LibraryClasses]
UefiDriverEntryPoint
IoLib
PcdLib
UefiLib
[Protocols]
- gHardwareInterruptProtocolGuid
- gHardwareInterrupt2ProtocolGuid
- gEfiCpuArchProtocolGuid
+ gHardwareInterruptProtocolGuid ## PRODUCES
+ gHardwareInterrupt2ProtocolGuid ## PRODUCES
+ gEfiCpuArchProtocolGuid ## CONSUMES ## NOTIFY
[Pcd.common]
gArmTokenSpaceGuid.PcdGicDistributorBase
gArmTokenSpaceGuid.PcdGicRedistributorsBase
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase
gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 03/10] ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
2018-04-12 0:55 ` [PATCH 01/10] Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify Laszlo Ersek
2018-04-12 0:55 ` [PATCH 02/10] ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf" Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 04/10] EmbeddedPkg: introduce NvVarStoreFormattedLib Laszlo Ersek
` (9 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
Commit 61a7b0ec634f ("ArmPkg/Gic: force GIC driver to run before CPU arch
protocol driver", 2018-02-06) explains why CpuDxe should be dispatched
after ArmGicDxe.
To implement the ordering, we should use a regular protocol depex rather
than the less flexible AFTER opcode. ArmGicDxe installs
gHardwareInterruptProtocolGuid and gHardwareInterrupt2ProtocolGuid as one
of the last actions on its entry point stack; either of those is OK for
CpuDxe to wait for.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPkg/ArmPkg.dec | 2 --
ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index a55b6268ff85..5dbd019e2966 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -45,14 +45,12 @@ [Guids.common]
gArmTokenSpaceGuid = { 0xBB11ECFE, 0x820F, 0x4968, { 0xBB, 0xA6, 0xF7, 0x6A, 0xFE, 0x30, 0x25, 0x96 } }
## ARM MPCore table
# Include/Guid/ArmMpCoreInfo.h
gArmMpCoreInfoGuid = { 0xa4ee0728, 0xe5d7, 0x4ac5, {0xb2, 0x1e, 0x65, 0x8e, 0xd8, 0x57, 0xe8, 0x34} }
- gArmGicDxeFileGuid = { 0xde371f7c, 0xdec4, 0x4d21, { 0xad, 0xf1, 0x59, 0x3a, 0xbc, 0xc1, 0x58, 0x82 } }
-
[Ppis]
## Include/Ppi/ArmMpCoreInfo.h
gArmMpCoreInfoPpiGuid = { 0x6847cc74, 0xe9ec, 0x4f8f, {0xa2, 0x9d, 0xab, 0x44, 0xe7, 0x54, 0xa8, 0xfc} }
[PcdsFeatureFlag.common]
gArmTokenSpaceGuid.PcdCpuDxeProduceDebugSupport|FALSE|BOOLEAN|0x00000001
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
index b748f0344537..c32d2cb9c7d4 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
@@ -71,7 +71,7 @@ [Pcd.common]
gArmTokenSpaceGuid.PcdVFPEnabled
[FeaturePcd.common]
gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
[Depex]
- AFTER gArmGicDxeFileGuid
+ gHardwareInterruptProtocolGuid OR gHardwareInterrupt2ProtocolGuid
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 04/10] EmbeddedPkg: introduce NvVarStoreFormattedLib
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (2 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 03/10] ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 05/10] ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly Laszlo Ersek
` (8 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
Some platforms don't format a variable store template at build time;
instead they format the non-volatile varstore flash chip during boot,
dynamically. Introduce NvVarStoreFormattedLib to enable such platforms to
delay the "variable read" service drivers until the platform specific
module(s) report that the variable store has been formatted.
The platform-specific module that performs the formatting during startup
is usually an FVB or MM FVB driver. Under the proposed scheme, it becomes
responsible for installing gEdkiiNvVarStoreFormattedGuid with a NULL
interface in the protocol database. In turn, the platform DSC will hook
NvVarStoreFormattedLib into the variable service driver, to make the
latter wait for the FVB driver. Platforms that need not delay the variable
service driver like this may still use the same FVB driver;
gEdkiiNvVarStoreFormattedGuid will simply be ignored.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
EmbeddedPkg/EmbeddedPkg.dec | 3 ++
EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf | 52 ++++++++++++++++++++
EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h | 39 +++++++++++++++
EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c | 41 +++++++++++++++
4 files changed, 135 insertions(+)
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index e48ce2e95bf7..c7b134dd34e6 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -64,12 +64,15 @@ [Guids.common]
# File GUID for default DTB image embedded in the firmware volume
gDtPlatformDefaultDtbFileGuid = { 0x25462cda, 0x221f, 0x47df, { 0xac, 0x1d, 0x25, 0x9c, 0xfa, 0xa4, 0xe3, 0x26 } }
# HII form set GUID for ConsolePrefDxe driver
gConsolePrefFormSetGuid = { 0x2d2358b4, 0xe96c, 0x484d, { 0xb2, 0xdd, 0x7c, 0x2e, 0xdf, 0xc7, 0xd5, 0x6f } }
+ ## Include/Guid/NvVarStoreFormatted.h
+ gEdkiiNvVarStoreFormattedGuid = { 0xd1a86e3f, 0x0707, 0x4c35, { 0x83, 0xcd, 0xdc, 0x2c, 0x29, 0xc8, 0x91, 0xa3 } }
+
[Protocols.common]
gHardwareInterruptProtocolGuid = { 0x2890B3EA, 0x053D, 0x1643, { 0xAD, 0x0C, 0xD6, 0x48, 0x08, 0xDA, 0x3F, 0xF1 } }
gHardwareInterrupt2ProtocolGuid = { 0x32898322, 0x2da1, 0x474a, { 0xba, 0xaa, 0xf3, 0xf7, 0xcf, 0x56, 0x94, 0x70 } }
gEmbeddedDeviceGuid = { 0xbf4b9d10, 0x13ec, 0x43dd, { 0x88, 0x80, 0xe9, 0xb, 0x71, 0x8f, 0x27, 0xde } }
gEmbeddedExternalDeviceProtocolGuid = { 0x735F8C64, 0xD696, 0x44D0, { 0xBD, 0xF2, 0x44, 0x7F, 0xD0, 0x5A, 0x54, 0x06 }}
gEmbeddedGpioProtocolGuid = { 0x17a0a3d7, 0xc0a5, 0x4635, { 0xbb, 0xd5, 0x07, 0x21, 0x87, 0xdf, 0xe2, 0xee }}
diff --git a/EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf b/EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
new file mode 100644
index 000000000000..b02fc7810948
--- /dev/null
+++ b/EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
@@ -0,0 +1,52 @@
+## @file
+# A hook-in library for:
+# - MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+# - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+# - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+#
+# Plugging this library instance into one of the above modules makes that
+# variable service backend wait for another platform module to dynamically
+# initialize or verify EFI_FIRMWARE_VOLUME_HEADER and VARIABLE_STORE_HEADER in
+# the non-volatile variable store FVB device. The initialization / verification
+# is signaled by installing gEdkiiNvVarStoreFormattedGuid into the
+# phase-matching PPI or protocol database, with a NULL interface. (Note that
+# installing gEdkiiNvVarStoreFormattedGuid into either the DXE or the MM
+# protocol database will unblock VariableSmm -- refer to EFI_SECTION_MM_DEPEX
+# in the PI spec.)
+#
+# Copyright (C) 2018, Red Hat, Inc.
+#
+# 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 = 1.27
+ BASE_NAME = NvVarStoreFormattedLib
+ FILE_GUID = 78f76ae8-ae62-4455-8148-c3a7ebaaa3f3
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = NvVarStoreFormattedLib|PEIM DXE_RUNTIME_DRIVER DXE_SMM_DRIVER
+ CONSTRUCTOR = NvVarStoreFormattedInitialize
+
+[Sources]
+ NvVarStoreFormattedLib.c
+
+[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+
+#
+# The matching DEPEX section below will generate the EFI_SECTION_PEI_DEPEX,
+# EFI_SECTION_DXE_DEPEX or EFI_SECTION_MM_DEPEX leaf section for the PEIM
+# (EFI_FV_FILETYPE_PEIM), DXE_RUNTIME_DRIVER (EFI_FV_FILETYPE_DRIVER), or
+# DXE_SMM_DRIVER (EFI_FV_FILETYPE_MM) module, respectively.
+#
+[Depex.common.PEIM, Depex.common.DXE_RUNTIME_DRIVER, Depex.common.DXE_SMM_DRIVER]
+ gEdkiiNvVarStoreFormattedGuid
diff --git a/EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h b/EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h
new file mode 100644
index 000000000000..4e057e4d894d
--- /dev/null
+++ b/EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h
@@ -0,0 +1,39 @@
+/** @file
+ EDKII NvVarStore Formatted GUID
+
+ A NULL protocol instance with this GUID in the DXE and/or MM protocol
+ databases, and/or a NULL PPI with this GUID in the PPI database, implies that
+ a DXE or MM driver, or a PEIM, has verified (or dynamically ensured) that the
+ non-volatile variable store has valid and consistent headers
+ (EFI_FIRMWARE_VOLUME_HEADER and VARIABLE_STORE_HEADER).
+
+ Said predicate is required by the read-only variable PEIM, and the read side
+ of the runtime variable DXE and MM drivers, immediately after they are
+ dispatched. This GUID presents platforms with one way to coordinate between
+ their module(s) that format the variable store FVB device and the variable
+ service drivers.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License that 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 __EDKII_NV_VAR_STORE_FORMATTED_H__
+#define __EDKII_NV_VAR_STORE_FORMATTED_H__
+
+#define EDKII_NV_VAR_STORE_FORMATTED_GUID \
+ { \
+ 0xd1a86e3f, 0x0707, 0x4c35, \
+ { 0x83, 0xcd, 0xdc, 0x2c, 0x29, 0xc8, 0x91, 0xa3 } \
+ }
+
+extern EFI_GUID gEdkiiNvVarStoreFormattedGuid;
+
+#endif
diff --git a/EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c b/EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c
new file mode 100644
index 000000000000..001e47cfe87c
--- /dev/null
+++ b/EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c
@@ -0,0 +1,41 @@
+/** @file
+ A hook-in library for:
+ - MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+ - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+ - MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+
+ Plugging this library instance into one of the above modules makes that
+ variable service backend wait for another platform module to dynamically
+ initialize or verify EFI_FIRMWARE_VOLUME_HEADER and VARIABLE_STORE_HEADER in
+ the non-volatile variable store FVB device. The initialization / verification
+ is signaled by installing gEdkiiNvVarStoreFormattedGuid into the
+ phase-matching PPI or protocol database, with a NULL interface. (Note that
+ installing gEdkiiNvVarStoreFormattedGuid into either the DXE or the MM
+ protocol database will unblock VariableSmm -- refer to EFI_SECTION_MM_DEPEX
+ in the PI spec.)
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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 <Base.h>
+
+RETURN_STATUS
+EFIAPI
+NvVarStoreFormattedInitialize (
+ VOID
+ )
+{
+ //
+ // Do nothing, just imbue VariablePei / VariableRuntimeDxe / VariableSmm with
+ // a PPI or protocol dependency on EDKII_NV_VAR_STORE_FORMATTED_GUID.
+ //
+ return RETURN_SUCCESS;
+}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 05/10] ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (3 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 04/10] EmbeddedPkg: introduce NvVarStoreFormattedLib Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 06/10] ArmPlatformPkg/NorFlashDxe: cue the variable driver with NvVarStoreFormatted Laszlo Ersek
` (7 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
The lazy initialization of the varstore FVB makes no longer sense at this
point:
- "mNorFlashInstanceTemplate.Initialize" is NULL;
- in NorFlashCreateInstance(), we only set Instance->Initialize to
non-NULL -- namely NorFlashFvbInitialize() -- if the FVB stands for the
variable store (see "ContainVariableStorage" / "SupportFvb");
- we call Instance->Initialize() from three places:
- from NorFlashWriteSingleBlock(), which is too late for the variable
read service ("variable write" depends on "variable read");
- from InitializeFvAndVariableStoreHeaders(), but that is only reachable
from NorFlashFvbInitialize(), i.e. recursively from
Instance->Initialize() itself;
- and from FvbRead(), which is never called by the variable driver, only
by the FTW driver. However, the variable driver may read (not write)
the memory-mapped varstore flash chip before the FTW driver is
dispatched.
Therefore the lazy initialization is both superfluous and insufficient.
Initialize the varstore headers eagerly, before we install the FVB
protocol interface.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 6 ------
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 13 +------------
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 9 ---------
3 files changed, 1 insertion(+), 27 deletions(-)
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
index c24680098f62..5c07694fbfaa 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
@@ -119,36 +119,30 @@
#define INSTANCE_FROM_FVB_THIS(a) CR(a, NOR_FLASH_INSTANCE, FvbProtocol, NOR_FLASH_SIGNATURE)
#define INSTANCE_FROM_BLKIO_THIS(a) CR(a, NOR_FLASH_INSTANCE, BlockIoProtocol, NOR_FLASH_SIGNATURE)
#define INSTANCE_FROM_DISKIO_THIS(a) CR(a, NOR_FLASH_INSTANCE, DiskIoProtocol, NOR_FLASH_SIGNATURE)
typedef struct _NOR_FLASH_INSTANCE NOR_FLASH_INSTANCE;
-typedef EFI_STATUS (*NOR_FLASH_INITIALIZE) (NOR_FLASH_INSTANCE* Instance);
-
typedef struct {
VENDOR_DEVICE_PATH Vendor;
EFI_DEVICE_PATH_PROTOCOL End;
} NOR_FLASH_DEVICE_PATH;
struct _NOR_FLASH_INSTANCE {
UINT32 Signature;
EFI_HANDLE Handle;
- BOOLEAN Initialized;
- NOR_FLASH_INITIALIZE Initialize;
-
UINTN DeviceBaseAddress;
UINTN RegionBaseAddress;
UINTN Size;
EFI_LBA StartLba;
EFI_BLOCK_IO_PROTOCOL BlockIoProtocol;
EFI_BLOCK_IO_MEDIA Media;
EFI_DISK_IO_PROTOCOL DiskIoProtocol;
- BOOLEAN SupportFvb;
EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL FvbProtocol;
VOID* ShadowBuffer;
NOR_FLASH_DEVICE_PATH DevicePath;
};
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index 1098d9501cc7..46e815beb343 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -29,15 +29,12 @@ NOR_FLASH_INSTANCE **mNorFlashInstances;
UINT32 mNorFlashDeviceCount;
NOR_FLASH_INSTANCE mNorFlashInstanceTemplate = {
NOR_FLASH_SIGNATURE, // Signature
NULL, // Handle ... NEED TO BE FILLED
- FALSE, // Initialized
- NULL, // Initialize
-
0, // DeviceBaseAddress ... NEED TO BE FILLED
0, // RegionBaseAddress ... NEED TO BE FILLED
0, // Size ... NEED TO BE FILLED
0, // StartLba
{
@@ -66,13 +63,12 @@ NOR_FLASH_INSTANCE mNorFlashInstanceTemplate = {
{
EFI_DISK_IO_PROTOCOL_REVISION, // Revision
NorFlashDiskIoReadDisk, // ReadDisk
NorFlashDiskIoWriteDisk // WriteDisk
},
- FALSE, // SupportFvb ... NEED TO BE FILLED
{
FvbGetAttributes, // GetAttributes
FvbSetAttributes, // SetAttributes
FvbGetPhysicalAddress, // GetPhysicalAddress
FvbGetBlockSize, // GetBlockSize
FvbRead, // Read
@@ -134,14 +130,13 @@ NorFlashCreateInstance (
Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
if (Instance->ShadowBuffer == NULL) {
return EFI_OUT_OF_RESOURCES;
}
if (SupportFvb) {
- Instance->SupportFvb = TRUE;
- Instance->Initialize = NorFlashFvbInitialize;
+ NorFlashFvbInitialize (Instance);
Status = gBS->InstallMultipleProtocolInterfaces (
&Instance->Handle,
&gEfiDevicePathProtocolGuid, &Instance->DevicePath,
&gEfiBlockIoProtocolGuid, &Instance->BlockIoProtocol,
&gEfiFirmwareVolumeBlockProtocolGuid, &Instance->FvbProtocol,
@@ -149,14 +144,12 @@ NorFlashCreateInstance (
);
if (EFI_ERROR(Status)) {
FreePool (Instance);
return Status;
}
} else {
- Instance->Initialized = TRUE;
-
Status = gBS->InstallMultipleProtocolInterfaces (
&Instance->Handle,
&gEfiDevicePathProtocolGuid, &Instance->DevicePath,
&gEfiBlockIoProtocolGuid, &Instance->BlockIoProtocol,
&gEfiDiskIoProtocolGuid, &Instance->DiskIoProtocol,
NULL
@@ -921,16 +914,12 @@ NorFlashWriteSingleBlock (
UINTN BlockSize;
UINTN BlockAddress;
UINTN PrevBlockAddress;
PrevBlockAddress = 0;
- if (!Instance->Initialized && Instance->Initialize) {
- Instance->Initialize(Instance);
- }
-
DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=%ld, Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes, Buffer));
// Detect WriteDisabled state
if (Instance->Media.ReadOnly == TRUE) {
DEBUG ((EFI_D_ERROR, "NorFlashWriteSingleBlock: ERROR - Can not write: Device is in WriteDisabled state.\n"));
// It is in WriteDisabled state, return an error right away
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
index 3ea858f46ffb..c3e6489f398f 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
@@ -56,16 +56,12 @@ InitializeFvAndVariableStoreHeaders (
EFI_STATUS Status;
VOID* Headers;
UINTN HeadersLength;
EFI_FIRMWARE_VOLUME_HEADER *FirmwareVolumeHeader;
VARIABLE_STORE_HEADER *VariableStoreHeader;
- if (!Instance->Initialized && Instance->Initialize) {
- Instance->Initialize (Instance);
- }
-
HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + sizeof(EFI_FV_BLOCK_MAP_ENTRY) + sizeof(VARIABLE_STORE_HEADER);
Headers = AllocateZeroPool(HeadersLength);
// FirmwareVolumeHeader->FvLength is declared to have the Variable area AND the FTW working area AND the FTW Spare contiguous.
ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase));
ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase));
@@ -428,16 +424,12 @@ FvbRead (
NOR_FLASH_INSTANCE *Instance;
Instance = INSTANCE_FROM_FVB_THIS(This);
DEBUG ((DEBUG_BLKIO, "FvbRead(Parameters: Lba=%ld, Offset=0x%x, *NumBytes=0x%x, Buffer @ 0x%08x)\n", Instance->StartLba + Lba, Offset, *NumBytes, Buffer));
- if (!Instance->Initialized && Instance->Initialize) {
- Instance->Initialize(Instance);
- }
-
TempStatus = EFI_SUCCESS;
// Cache the block size to avoid de-referencing pointers all the time
BlockSize = Instance->Media.BlockSize;
DEBUG ((DEBUG_BLKIO, "FvbRead: Check if (Offset=0x%x + NumBytes=0x%x) <= BlockSize=0x%x\n", Offset, *NumBytes, BlockSize ));
@@ -746,13 +738,12 @@ NorFlashFvbInitialize (
Status = gDS->SetMemorySpaceAttributes (
Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
ASSERT_EFI_ERROR (Status);
- Instance->Initialized = TRUE;
mFlashNvStorageVariableBase = FixedPcdGet32 (PcdFlashNvStorageVariableBase);
// Set the index of the first LBA for the FVB
Instance->StartLba = (PcdGet32 (PcdFlashNvStorageVariableBase) - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
BootMode = GetBootModeHob ();
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 06/10] ArmPlatformPkg/NorFlashDxe: cue the variable driver with NvVarStoreFormatted
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (4 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 05/10] ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 07/10] ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid Laszlo Ersek
` (6 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
The BEFORE depex opcode that we currently use to force ourselves in front
of the variable driver cannot be combined with other depex opcodes.
Replace the depex with TRUE, and signal NvVarStoreFormattedLib through the
installation of "gEdkiiNvVarStoreFormattedGuid".
Platforms that rely on NorFlashDxe to format the variable store (as
opposed to formatting a variable store template through an FDF file, as
part of the build) should hook NvVarStoreFormattedLib into the variable
drivers they use, so that the latter await our cue.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPlatformPkg/ArmPlatformPkg.dec | 4 ----
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf | 7 +++----
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 13 +++++++++++++
3 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
index 7cec775abeee..8f32a5e2c14d 100644
--- a/ArmPlatformPkg/ArmPlatformPkg.dec
+++ b/ArmPlatformPkg/ArmPlatformPkg.dec
@@ -37,16 +37,12 @@ [LibraryClasses]
LcdPlatformLib|Include/Library/LcdPlatformLib.h
NorFlashPlatformLib|Include/Library/NorFlashPlatformLib.h
PL011UartLib|Include/Library/PL011UartLib.h
[Guids.common]
gArmPlatformTokenSpaceGuid = { 0x9c0aaed4, 0x74c5, 0x4043, { 0xb4, 0x17, 0xa3, 0x22, 0x38, 0x14, 0xce, 0x76 } }
- #
- # Following Guid must match FILE_GUID in MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
- #
- gVariableRuntimeDxeFileGuid = { 0xcbd2e4d5, 0x7068, 0x4ff5, { 0xb4, 0x62, 0x98, 0x22, 0xb4, 0xad, 0x8d, 0x60 } }
[PcdsFeatureFlag.common]
gArmPlatformTokenSpaceGuid.PcdSendSgiToBringUpSecondaryCores|FALSE|BOOLEAN|0x00000004
gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked|FALSE|BOOLEAN|0x0000003C
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index 812dafd065b2..c40ac27a6599 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -28,12 +28,13 @@ [Sources.common]
NorFlashBlockIoDxe.c
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
ArmPlatformPkg/ArmPlatformPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
[LibraryClasses]
IoLib
BaseLib
DebugLib
HobLib
@@ -46,12 +47,13 @@ [LibraryClasses]
[Guids]
gEfiSystemNvDataFvGuid
gEfiVariableGuid
gEfiAuthenticatedVariableGuid
gEfiEventVirtualAddressChangeGuid
+ gEdkiiNvVarStoreFormattedGuid ## PRODUCES ## PROTOCOL
[Protocols]
gEfiBlockIoProtocolGuid
gEfiDevicePathProtocolGuid
gEfiFirmwareVolumeBlockProtocolGuid
gEfiDiskIoProtocolGuid
@@ -64,10 +66,7 @@ [Pcd.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked
[Depex]
- #
- # NorFlashDxe must be loaded before VariableRuntimeDxe in case empty flash needs populating with default values
- #
- BEFORE gVariableRuntimeDxeFileGuid
+ TRUE
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
index c3e6489f398f..e62ffbb433d0 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
@@ -22,12 +22,13 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/DxeServicesTableLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Guid/VariableFormat.h>
#include <Guid/SystemNvDataGuid.h>
+#include <Guid/NvVarStoreFormatted.h>
#include "NorFlashDxe.h"
STATIC EFI_EVENT mFvbVirtualAddrChangeEvent;
STATIC UINTN mFlashNvStorageVariableBase;
@@ -773,12 +774,24 @@ NorFlashFvbInitialize (
Status = InitializeFvAndVariableStoreHeaders (Instance);
if (EFI_ERROR(Status)) {
return Status;
}
}
+ //
+ // The driver implementing the variable read service can now be dispatched;
+ // the varstore headers are in place.
+ //
+ Status = gBS->InstallProtocolInterface (
+ &gImageHandle,
+ &gEdkiiNvVarStoreFormattedGuid,
+ EFI_NATIVE_INTERFACE,
+ NULL
+ );
+ ASSERT_EFI_ERROR (Status);
+
//
// Register for the virtual address change event
//
Status = gBS->CreateEventEx (
EVT_NOTIFY_SIGNAL,
TPL_NOTIFY,
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 07/10] ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (5 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 06/10] ArmPlatformPkg/NorFlashDxe: cue the variable driver with NvVarStoreFormatted Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 08/10] ArmPlatformPkg/PL031RealTimeClockLib: " Laszlo Ersek
` (5 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
NorFlashFvbInitialize() calls gDS->SetMemorySpaceAttributes() to mark the
varstore flash region as uncached. This DXE service depends on the CPU
Architectural protocol, and the DXE core is allowed to return
EFI_NOT_AVAILABLE_YET if it hasn't dispatched ArmPkg/Drivers/CpuDxe
earlier. Make the dependency explicit.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index c40ac27a6599..a59a21a03e0a 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -66,7 +66,7 @@ [Pcd.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked
[Depex]
- TRUE
+ gEfiCpuArchProtocolGuid
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 08/10] ArmPlatformPkg/PL031RealTimeClockLib: depend on gEfiCpuArchProtocolGuid
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (6 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 07/10] ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 0:55 ` [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid Laszlo Ersek
` (4 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
The RealTimeClockLib class is declared under EmbeddedPkg, so that
platforms can provide the internals for the
EmbeddedPkg/RealTimeClockRuntimeDxe driver. In turn the driver produces
the Real Time Clock Arch Protocol, without which UEFI drivers cannot be
dispatched.
The PL031RealTimeClockLib instance calls gDS->SetMemorySpaceAttributes()
in the LibRtcInitialize() public function. This DXE service depends on the
CPU Arch Protocol. Add it to the depex.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
index a3e4f73e7d05..3d62aeabd670 100644
--- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
+++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
@@ -16,13 +16,13 @@
[Defines]
INF_VERSION = 0x00010005
BASE_NAME = PL031RealTimeClockLib
FILE_GUID = 470DFB96-E205-4515-A75E-2E60F853E79D
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = RealTimeClockLib
+ LIBRARY_CLASS = RealTimeClockLib|DXE_RUNTIME_DRIVER
[Sources.common]
PL031RealTimeClockLib.c
[Packages]
MdePkg/MdePkg.dec
@@ -41,6 +41,9 @@ [LibraryClasses]
[Guids]
gEfiEventVirtualAddressChangeGuid
[Pcd]
gArmPlatformTokenSpaceGuid.PcdPL031RtcBase
gArmPlatformTokenSpaceGuid.PcdPL031RtcPpmAccuracy
+
+[Depex.common.DXE_RUNTIME_DRIVER]
+ gEfiCpuArchProtocolGuid
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (7 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 08/10] ArmPlatformPkg/PL031RealTimeClockLib: " Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 6:28 ` Ard Biesheuvel
2018-04-12 0:55 ` [PATCH 10/10] ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into VariableRuntimeDxe Laszlo Ersek
` (3 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
PlatformHasAcpiDtDxe consumes the DynamicHii PCD called
"gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call
terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe
driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX
so that we not attempt the call before the PCD driver can successfully
read non-volatile variables.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
index 5351e741aa41..8346f7f24b32 100644
--- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
+++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
@@ -44,7 +44,7 @@ [Guids]
gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
[Pcd]
gArmVirtTokenSpaceGuid.PcdForceNoAcpi
[Depex]
- TRUE
+ gEfiVariableArchProtocolGuid
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
2018-04-12 0:55 ` [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid Laszlo Ersek
@ 2018-04-12 6:28 ` Ard Biesheuvel
2018-04-12 9:05 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Ard Biesheuvel @ 2018-04-12 6:28 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm, Steve Capper,
Supreeth Venkatesh
On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
> PlatformHasAcpiDtDxe consumes the DynamicHii PCD called
> "gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call
> terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe
> driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX
> so that we not attempt the call before the PCD driver can successfully
> read non-volatile variables.
>
OK, so does this mean that UefiRuntimeServicesTableLib deliberately
does not depex on gEfiVariableArchProtocolGuid, and it will return
EFI_NOT_AVAILABLE_YET when called too early? If that is the case, I
would assume it is the responsibility of the PcdLib implementation to
deal with that, not the PcdLib clients.
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Steve Capper <steve.capper@linaro.org>
> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> index 5351e741aa41..8346f7f24b32 100644
> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> @@ -44,7 +44,7 @@ [Guids]
> gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
>
> [Pcd]
> gArmVirtTokenSpaceGuid.PcdForceNoAcpi
>
> [Depex]
> - TRUE
> + gEfiVariableArchProtocolGuid
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
2018-04-12 6:28 ` Ard Biesheuvel
@ 2018-04-12 9:05 ` Laszlo Ersek
2018-04-12 10:06 ` Ard Biesheuvel
2018-04-12 15:16 ` Gao, Liming
0 siblings, 2 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 9:05 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Steve Capper,
Supreeth Venkatesh
On 04/12/18 08:28, Ard Biesheuvel wrote:
> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>> PlatformHasAcpiDtDxe consumes the DynamicHii PCD called
>> "gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call
>> terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe
>> driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX
>> so that we not attempt the call before the PCD driver can successfully
>> read non-volatile variables.
>>
>
> OK, so does this mean that UefiRuntimeServicesTableLib deliberately
> does not depex on gEfiVariableArchProtocolGuid,
Yes, I do think so; the runtime services are expected to become
available gradually. For example, "MdePkg/Include/Protocol/Variable.h"
has a great long comment on this.
> and it will return
> EFI_NOT_AVAILABLE_YET when called too early? If that is the case, I
> would assume it is the responsibility of the PcdLib implementation to
> deal with that, not the PcdLib clients.
Yes, this idea occurred to me as well, but I think it is not correct.
The PcdLib instance for the DXE phase is responsible for talking to the
PCD protocol, and indeed it has a depex on that protocol
(MdePkg/Library/DxePcdLib/DxePcdLib.inf). However, if a module uses
dynamic PCDs, but none of those dynamic PCDs are HII (i.e., backed by
non-volatile UEFI variables), then IMO neither the PcdLib instance, nor
the PCD DXE driver, should depend on the variable read service
(gEfiVariableArchProtocolGuid).
(Well the PCD DXE driver is shared / used by all of the DXE modules, so
it couldn't depend on gEfiVariableArchProtocolGuid in any case at all.)
Ultimately, in the optimal case, the PcdLib instance for DXE should
"know" if any PCDs used by the main driver module are DynamicHII, and in
that case, the PcdLib instance should create a dependency on the
variable read (and possibly write) services. I think edk2 simply doesn't
support this use case at this moment (it is ultimately a DSC decision
what type we give to a PCD, choosing from the possibilities listed in
the DEC), so we have to take care of it ourselves. I do see that this
limits the reusability of individual drivers across different DSC files
-- if another DSC makes the same PCD fixed, for example, then the
gEfiVariableArchProtocolGuid dependency becomes useless / needlessly
restrictive in the driver.
We could factor out just the gEfiVariableArchProtocolGuid depex into
another NULL class lib, and hook that into PlatformHasAcpiDtDxe in the
DSC file, because in the DSC we do know that the PCD is DynamicHII. If
you feel strongly about it, it's doable, but I certainly think it's
overkill. PlatformHasAcpiDtDxe got "Platform" in its name; it's pretty
unlikely to be reused in any other DSC. (Even ArmVirtXen has a different
implementation, which does not use the PCD.) I feel the proposed patch
should be fine -- what's your take?
Thanks!
Laszlo
>
>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Steve Capper <steve.capper@linaro.org>
>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>> index 5351e741aa41..8346f7f24b32 100644
>> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>> @@ -44,7 +44,7 @@ [Guids]
>> gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
>>
>> [Pcd]
>> gArmVirtTokenSpaceGuid.PcdForceNoAcpi
>>
>> [Depex]
>> - TRUE
>> + gEfiVariableArchProtocolGuid
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
2018-04-12 9:05 ` Laszlo Ersek
@ 2018-04-12 10:06 ` Ard Biesheuvel
2018-04-12 15:16 ` Gao, Liming
1 sibling, 0 replies; 27+ messages in thread
From: Ard Biesheuvel @ 2018-04-12 10:06 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm, Steve Capper,
Supreeth Venkatesh
On 12 April 2018 at 11:05, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/12/18 08:28, Ard Biesheuvel wrote:
>> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>> PlatformHasAcpiDtDxe consumes the DynamicHii PCD called
>>> "gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call
>>> terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe
>>> driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX
>>> so that we not attempt the call before the PCD driver can successfully
>>> read non-volatile variables.
>>>
>>
>> OK, so does this mean that UefiRuntimeServicesTableLib deliberately
>> does not depex on gEfiVariableArchProtocolGuid,
>
> Yes, I do think so; the runtime services are expected to become
> available gradually. For example, "MdePkg/Include/Protocol/Variable.h"
> has a great long comment on this.
>
>> and it will return
>> EFI_NOT_AVAILABLE_YET when called too early? If that is the case, I
>> would assume it is the responsibility of the PcdLib implementation to
>> deal with that, not the PcdLib clients.
>
> Yes, this idea occurred to me as well, but I think it is not correct.
> The PcdLib instance for the DXE phase is responsible for talking to the
> PCD protocol, and indeed it has a depex on that protocol
> (MdePkg/Library/DxePcdLib/DxePcdLib.inf). However, if a module uses
> dynamic PCDs, but none of those dynamic PCDs are HII (i.e., backed by
> non-volatile UEFI variables), then IMO neither the PcdLib instance, nor
> the PCD DXE driver, should depend on the variable read service
> (gEfiVariableArchProtocolGuid).
>
> (Well the PCD DXE driver is shared / used by all of the DXE modules, so
> it couldn't depend on gEfiVariableArchProtocolGuid in any case at all.)
>
> Ultimately, in the optimal case, the PcdLib instance for DXE should
> "know" if any PCDs used by the main driver module are DynamicHII, and in
> that case, the PcdLib instance should create a dependency on the
> variable read (and possibly write) services. I think edk2 simply doesn't
> support this use case at this moment (it is ultimately a DSC decision
> what type we give to a PCD, choosing from the possibilities listed in
> the DEC), so we have to take care of it ourselves. I do see that this
> limits the reusability of individual drivers across different DSC files
> -- if another DSC makes the same PCD fixed, for example, then the
> gEfiVariableArchProtocolGuid dependency becomes useless / needlessly
> restrictive in the driver.
>
> We could factor out just the gEfiVariableArchProtocolGuid depex into
> another NULL class lib, and hook that into PlatformHasAcpiDtDxe in the
> DSC file, because in the DSC we do know that the PCD is DynamicHII. If
> you feel strongly about it, it's doable, but I certainly think it's
> overkill. PlatformHasAcpiDtDxe got "Platform" in its name; it's pretty
> unlikely to be reused in any other DSC. (Even ArmVirtXen has a different
> implementation, which does not use the PCD.) I feel the proposed patch
> should be fine -- what's your take?
>
I missed the bit where gRT->GetVariable() is only called for HII style
dynamic PCDs, so I agree that it makes sense to deal with things as
you are doing here.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
2018-04-12 9:05 ` Laszlo Ersek
2018-04-12 10:06 ` Ard Biesheuvel
@ 2018-04-12 15:16 ` Gao, Liming
2018-04-12 16:53 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Gao, Liming @ 2018-04-12 15:16 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm
Laszlo:
We also have the similar idea that auto appends Variable read or Variable write protocol. Build tool can know which PCD is configured as DynamicHii. But, it doesn't know whether this PCD is consumed or produced in the driver entry point. If PCD is used in driver entry point, Build tool needs to append Variable arch protocol depex. Otherwise, BaseTool doesn't need do it. Further, I think BaseTools can use PCD usage described in Pcd section. CONSUMES means PCD read in the driver entry point, PRODUCES means PCD write in the driver entry point. Seemly, this solution can work well. But, it depends on the correct PCD usage in module INF. If PCD usage is not described or wrongly described, it may cause system boot failure. Consider its impact, we have not added this support.
[Pcd]
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## SOMETIMES_CONSUMES
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## PRODUCES
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## SOMETIMES_PRODUCES
Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, April 12, 2018 5:06 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2] [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
>
> On 04/12/18 08:28, Ard Biesheuvel wrote:
> > On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
> >> PlatformHasAcpiDtDxe consumes the DynamicHii PCD called
> >> "gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call
> >> terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe
> >> driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX
> >> so that we not attempt the call before the PCD driver can successfully
> >> read non-volatile variables.
> >>
> >
> > OK, so does this mean that UefiRuntimeServicesTableLib deliberately
> > does not depex on gEfiVariableArchProtocolGuid,
>
> Yes, I do think so; the runtime services are expected to become
> available gradually. For example, "MdePkg/Include/Protocol/Variable.h"
> has a great long comment on this.
>
> > and it will return
> > EFI_NOT_AVAILABLE_YET when called too early? If that is the case, I
> > would assume it is the responsibility of the PcdLib implementation to
> > deal with that, not the PcdLib clients.
>
> Yes, this idea occurred to me as well, but I think it is not correct.
> The PcdLib instance for the DXE phase is responsible for talking to the
> PCD protocol, and indeed it has a depex on that protocol
> (MdePkg/Library/DxePcdLib/DxePcdLib.inf). However, if a module uses
> dynamic PCDs, but none of those dynamic PCDs are HII (i.e., backed by
> non-volatile UEFI variables), then IMO neither the PcdLib instance, nor
> the PCD DXE driver, should depend on the variable read service
> (gEfiVariableArchProtocolGuid).
>
> (Well the PCD DXE driver is shared / used by all of the DXE modules, so
> it couldn't depend on gEfiVariableArchProtocolGuid in any case at all.)
>
> Ultimately, in the optimal case, the PcdLib instance for DXE should
> "know" if any PCDs used by the main driver module are DynamicHII, and in
> that case, the PcdLib instance should create a dependency on the
> variable read (and possibly write) services. I think edk2 simply doesn't
> support this use case at this moment (it is ultimately a DSC decision
> what type we give to a PCD, choosing from the possibilities listed in
> the DEC), so we have to take care of it ourselves. I do see that this
> limits the reusability of individual drivers across different DSC files
> -- if another DSC makes the same PCD fixed, for example, then the
> gEfiVariableArchProtocolGuid dependency becomes useless / needlessly
> restrictive in the driver.
>
> We could factor out just the gEfiVariableArchProtocolGuid depex into
> another NULL class lib, and hook that into PlatformHasAcpiDtDxe in the
> DSC file, because in the DSC we do know that the PCD is DynamicHII. If
> you feel strongly about it, it's doable, but I certainly think it's
> overkill. PlatformHasAcpiDtDxe got "Platform" in its name; it's pretty
> unlikely to be reused in any other DSC. (Even ArmVirtXen has a different
> implementation, which does not use the PCD.) I feel the proposed patch
> should be fine -- what's your take?
>
> Thanks!
> Laszlo
>
> >
> >
> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >> Cc: Steve Capper <steve.capper@linaro.org>
> >> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >> ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> >> index 5351e741aa41..8346f7f24b32 100644
> >> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> >> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> >> @@ -44,7 +44,7 @@ [Guids]
> >> gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
> >>
> >> [Pcd]
> >> gArmVirtTokenSpaceGuid.PcdForceNoAcpi
> >>
> >> [Depex]
> >> - TRUE
> >> + gEfiVariableArchProtocolGuid
> >> --
> >> 2.14.1.3.gb7cf6e02401b
> >>
> >>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
2018-04-12 15:16 ` Gao, Liming
@ 2018-04-12 16:53 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 16:53 UTC (permalink / raw)
To: Gao, Liming, Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Leif Lindholm
On 04/12/18 17:16, Gao, Liming wrote:
> Laszlo:
> We also have the similar idea that auto appends Variable read or Variable write protocol. Build tool can know which PCD is configured as DynamicHii. But, it doesn't know whether this PCD is consumed or produced in the driver entry point. If PCD is used in driver entry point, Build tool needs to append Variable arch protocol depex. Otherwise, BaseTool doesn't need do it. Further, I think BaseTools can use PCD usage described in Pcd section. CONSUMES means PCD read in the driver entry point, PRODUCES means PCD write in the driver entry point. Seemly, this solution can work well. But, it depends on the correct PCD usage in module INF. If PCD usage is not described or wrongly described, it may cause system boot failure. Consider its impact, we have not added this support.
>
> [Pcd]
> gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES
> gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## SOMETIMES_CONSUMES
> gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## PRODUCES
> gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## SOMETIMES_PRODUCES
Right, I agree with everything you said. The usage comments are really
helpful (and not just for PCDs but also for protocols, guids, ...) but
in their current form they are indeed a bit too lax for basing
build-time decisions on them. If we ever want to support this in
BaseTools, then I think it will take a first-class extension to the
current INF file format; i.e., with syntax and semantic checking, error
messages, build failures etc. I think we can manage the current
situation, we just have to be aware of it.
Thank you!
Laszlo
>
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>> Sent: Thursday, April 12, 2018 5:06 PM
>> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
>> Subject: Re: [edk2] [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid
>>
>> On 04/12/18 08:28, Ard Biesheuvel wrote:
>>> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> PlatformHasAcpiDtDxe consumes the DynamicHii PCD called
>>>> "gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call
>>>> terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe
>>>> driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX
>>>> so that we not attempt the call before the PCD driver can successfully
>>>> read non-volatile variables.
>>>>
>>>
>>> OK, so does this mean that UefiRuntimeServicesTableLib deliberately
>>> does not depex on gEfiVariableArchProtocolGuid,
>>
>> Yes, I do think so; the runtime services are expected to become
>> available gradually. For example, "MdePkg/Include/Protocol/Variable.h"
>> has a great long comment on this.
>>
>>> and it will return
>>> EFI_NOT_AVAILABLE_YET when called too early? If that is the case, I
>>> would assume it is the responsibility of the PcdLib implementation to
>>> deal with that, not the PcdLib clients.
>>
>> Yes, this idea occurred to me as well, but I think it is not correct.
>> The PcdLib instance for the DXE phase is responsible for talking to the
>> PCD protocol, and indeed it has a depex on that protocol
>> (MdePkg/Library/DxePcdLib/DxePcdLib.inf). However, if a module uses
>> dynamic PCDs, but none of those dynamic PCDs are HII (i.e., backed by
>> non-volatile UEFI variables), then IMO neither the PcdLib instance, nor
>> the PCD DXE driver, should depend on the variable read service
>> (gEfiVariableArchProtocolGuid).
>>
>> (Well the PCD DXE driver is shared / used by all of the DXE modules, so
>> it couldn't depend on gEfiVariableArchProtocolGuid in any case at all.)
>>
>> Ultimately, in the optimal case, the PcdLib instance for DXE should
>> "know" if any PCDs used by the main driver module are DynamicHII, and in
>> that case, the PcdLib instance should create a dependency on the
>> variable read (and possibly write) services. I think edk2 simply doesn't
>> support this use case at this moment (it is ultimately a DSC decision
>> what type we give to a PCD, choosing from the possibilities listed in
>> the DEC), so we have to take care of it ourselves. I do see that this
>> limits the reusability of individual drivers across different DSC files
>> -- if another DSC makes the same PCD fixed, for example, then the
>> gEfiVariableArchProtocolGuid dependency becomes useless / needlessly
>> restrictive in the driver.
>>
>> We could factor out just the gEfiVariableArchProtocolGuid depex into
>> another NULL class lib, and hook that into PlatformHasAcpiDtDxe in the
>> DSC file, because in the DSC we do know that the PCD is DynamicHII. If
>> you feel strongly about it, it's doable, but I certainly think it's
>> overkill. PlatformHasAcpiDtDxe got "Platform" in its name; it's pretty
>> unlikely to be reused in any other DSC. (Even ArmVirtXen has a different
>> implementation, which does not use the PCD.) I feel the proposed patch
>> should be fine -- what's your take?
>>
>> Thanks!
>> Laszlo
>>
>>>
>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>>> Cc: Steve Capper <steve.capper@linaro.org>
>>>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>> ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>> b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>>>> index 5351e741aa41..8346f7f24b32 100644
>>>> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>>>> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>>>> @@ -44,7 +44,7 @@ [Guids]
>>>> gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
>>>>
>>>> [Pcd]
>>>> gArmVirtTokenSpaceGuid.PcdForceNoAcpi
>>>>
>>>> [Depex]
>>>> - TRUE
>>>> + gEfiVariableArchProtocolGuid
>>>> --
>>>> 2.14.1.3.gb7cf6e02401b
>>>>
>>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 10/10] ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into VariableRuntimeDxe
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (8 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid Laszlo Ersek
@ 2018-04-12 0:55 ` Laszlo Ersek
2018-04-12 10:09 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Ard Biesheuvel
` (2 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 0:55 UTC (permalink / raw)
To: edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper, Supreeth Venkatesh
In spite of both ArmVirtQemu and ArmVirtQemuKernel formatting the variable
store template at build time, link NvVarStoreFormattedLib into
VariableRuntimeDxe via NULL class resolution on both platforms. This lets
us test the depexes implemented in the previous patches.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
2 files changed, 2 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 76a83b99a303..824070edc2a9 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -254,12 +254,13 @@ [Components.common]
#
ArmPkg/Drivers/CpuDxe/CpuDxe.inf
MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
+ NULL|EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
# don't use unaligned CopyMem () on the UEFI varstore NOR flash region
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
}
!if $(SECURE_BOOT_ENABLE) == TRUE
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
<LibraryClasses>
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 7d9a9196d640..2368ba283bff 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -243,12 +243,13 @@ [Components.common]
#
ArmPkg/Drivers/CpuDxe/CpuDxe.inf
MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
+ NULL|EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
# don't use unaligned CopyMem () on the UEFI varstore NOR flash region
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
}
!if $(SECURE_BOOT_ENABLE) == TRUE
MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
<LibraryClasses>
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (9 preceding siblings ...)
2018-04-12 0:55 ` [PATCH 10/10] ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into VariableRuntimeDxe Laszlo Ersek
@ 2018-04-12 10:09 ` Ard Biesheuvel
2018-04-12 13:39 ` Steve Capper
` (2 more replies)
2018-04-12 16:51 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Supreeth Venkatesh
2018-04-12 19:29 ` Laszlo Ersek
12 siblings, 3 replies; 27+ messages in thread
From: Ard Biesheuvel @ 2018-04-12 10:09 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm, Steve Capper,
Supreeth Venkatesh
On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: depex_fixes
>
> ArmVirtQemu boots again, it just took a few more patches than I expected
> :)
>
> Some of these patches will have to be ported to edk2-platforms, I think.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Steve Capper <steve.capper@linaro.org>
> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (10):
> Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
> ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
> ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
> EmbeddedPkg: introduce NvVarStoreFormattedLib
> ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
> ArmPlatformPkg/NorFlashDxe: cue the variable driver with
> NvVarStoreFormatted
> ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
> ArmPlatformPkg/PL031RealTimeClockLib: depend on
> gEfiCpuArchProtocolGuid
> ArmVirtPkg/PlatformHasAcpiDtDxe: depend on
> gEfiVariableArchProtocolGuid
> ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into
> VariableRuntimeDxe
>
Laszlo,
Thanks a lot for taking care of this. I am glad we finally got rid of
the BEFORE depex in the NOR flash driver, which has been problematic
for years.
For the series,
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
but please allow some time for Leif to chime in as well.
Thanks,
Ard.
> ArmPkg/ArmPkg.dec | 2 -
> ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 8 +-
> ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +-
> ArmPlatformPkg/ArmPlatformPkg.dec | 4 -
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 13 +---
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 6 --
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf | 7 +-
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 22 +++---
> ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 5 +-
> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
> ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
> EmbeddedPkg/EmbeddedPkg.dec | 3 +
> EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h | 39 ++++++++++
> EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c | 41 ++++++++++
> EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf | 52 +++++++++++++
> Omap35xxPkg/InterruptDxe/HardwareInterrupt.c | 81 +++++++++++++++-----
> Omap35xxPkg/InterruptDxe/InterruptDxe.inf | 6 +-
> 18 files changed, 230 insertions(+), 65 deletions(-)
> create mode 100644 EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
> create mode 100644 EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h
> create mode 100644 EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c
>
> --
> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 10:09 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Ard Biesheuvel
@ 2018-04-12 13:39 ` Steve Capper
2018-04-12 16:49 ` Laszlo Ersek
2018-04-12 16:44 ` Laszlo Ersek
2018-04-12 17:23 ` Leif Lindholm
2 siblings, 1 reply; 27+ messages in thread
From: Steve Capper @ 2018-04-12 13:39 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Laszlo Ersek, edk2-devel@lists.01.org, Leif Lindholm,
Supreeth Venkatesh
On 12 April 2018 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>> Repo: https://github.com/lersek/edk2.git
>> Branch: depex_fixes
>>
>> ArmVirtQemu boots again, it just took a few more patches than I expected
>> :)
>>
>> Some of these patches will have to be ported to edk2-platforms, I think.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Steve Capper <steve.capper@linaro.org>
>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (10):
>> Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
>> ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
>> ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
>> EmbeddedPkg: introduce NvVarStoreFormattedLib
>> ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
>> ArmPlatformPkg/NorFlashDxe: cue the variable driver with
>> NvVarStoreFormatted
>> ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
>> ArmPlatformPkg/PL031RealTimeClockLib: depend on
>> gEfiCpuArchProtocolGuid
>> ArmVirtPkg/PlatformHasAcpiDtDxe: depend on
>> gEfiVariableArchProtocolGuid
>> ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into
>> VariableRuntimeDxe
>>
>
> Laszlo,
>
> Thanks a lot for taking care of this. I am glad we finally got rid of
> the BEFORE depex in the NOR flash driver, which has been problematic
> for years.
>
> For the series,
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> but please allow some time for Leif to chime in as well.
>
> Thanks,
> Ard.
>
>
>
Hi Laszlo,
Thanks for this!
I've booted with both DEBUG and RELEASE configurations on an FVP model
using this series.
I built this on Debian Stretch (using the AArch64 cross compiler).
In order to test on FVP (which is defined in edk2-platforms), I made a
simple modification to:
edk2-platforms/Platform/ARM/VExpressPkg/ArmVExpress-FVP-AArch64.dsc
(Basically mirroring what you did in patch #10)
FWIW, feel free to add:
Tested-by: Steve Capper <steve.capper@linaro.org>
Cheers,
--
Steve
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 13:39 ` Steve Capper
@ 2018-04-12 16:49 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 16:49 UTC (permalink / raw)
To: Steve Capper, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Supreeth Venkatesh
On 04/12/18 15:39, Steve Capper wrote:
> On 12 April 2018 at 11:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Repo: https://github.com/lersek/edk2.git
>>> Branch: depex_fixes
>>>
>>> ArmVirtQemu boots again, it just took a few more patches than I expected
>>> :)
>>>
>>> Some of these patches will have to be ported to edk2-platforms, I think.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Cc: Steve Capper <steve.capper@linaro.org>
>>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> Laszlo Ersek (10):
>>> Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
>>> ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
>>> ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
>>> EmbeddedPkg: introduce NvVarStoreFormattedLib
>>> ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
>>> ArmPlatformPkg/NorFlashDxe: cue the variable driver with
>>> NvVarStoreFormatted
>>> ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
>>> ArmPlatformPkg/PL031RealTimeClockLib: depend on
>>> gEfiCpuArchProtocolGuid
>>> ArmVirtPkg/PlatformHasAcpiDtDxe: depend on
>>> gEfiVariableArchProtocolGuid
>>> ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into
>>> VariableRuntimeDxe
>>>
>>
>> Laszlo,
>>
>> Thanks a lot for taking care of this. I am glad we finally got rid of
>> the BEFORE depex in the NOR flash driver, which has been problematic
>> for years.
>>
>> For the series,
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> but please allow some time for Leif to chime in as well.
>>
>> Thanks,
>> Ard.
>>
>>
>>
>
> Hi Laszlo,
> Thanks for this!
>
> I've booted with both DEBUG and RELEASE configurations on an FVP model
> using this series.
> I built this on Debian Stretch (using the AArch64 cross compiler).
>
> In order to test on FVP (which is defined in edk2-platforms), I made a
> simple modification to:
> edk2-platforms/Platform/ARM/VExpressPkg/ArmVExpress-FVP-AArch64.dsc
> (Basically mirroring what you did in patch #10)
That's great; I'm relieved it wasn't more complex than that. :)
> FWIW, feel free to add:
> Tested-by: Steve Capper <steve.capper@linaro.org>
Thank you -- I'll add your T-b to the following patches: #2, #3, #4, #5,
#6, #7, #8. As those patches affect modules that seem to be used by
"edk2-platforms/Platform/ARM/VExpressPkg/ArmVExpress-FVP-AArch64.dsc".
Cheers!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 10:09 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Ard Biesheuvel
2018-04-12 13:39 ` Steve Capper
@ 2018-04-12 16:44 ` Laszlo Ersek
2018-04-12 17:23 ` Leif Lindholm
2 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 16:44 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Leif Lindholm, Steve Capper,
Supreeth Venkatesh
On 04/12/18 12:09, Ard Biesheuvel wrote:
> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>> Repo: https://github.com/lersek/edk2.git
>> Branch: depex_fixes
>>
>> ArmVirtQemu boots again, it just took a few more patches than I expected
>> :)
>>
>> Some of these patches will have to be ported to edk2-platforms, I think.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Steve Capper <steve.capper@linaro.org>
>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (10):
>> Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
>> ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
>> ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
>> EmbeddedPkg: introduce NvVarStoreFormattedLib
>> ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
>> ArmPlatformPkg/NorFlashDxe: cue the variable driver with
>> NvVarStoreFormatted
>> ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
>> ArmPlatformPkg/PL031RealTimeClockLib: depend on
>> gEfiCpuArchProtocolGuid
>> ArmVirtPkg/PlatformHasAcpiDtDxe: depend on
>> gEfiVariableArchProtocolGuid
>> ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into
>> VariableRuntimeDxe
>>
>
> Laszlo,
>
> Thanks a lot for taking care of this. I am glad we finally got rid of
> the BEFORE depex in the NOR flash driver, which has been problematic
> for years.
>
> For the series,
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Thank you!
>
> but please allow some time for Leif to chime in as well.
Will certainly do!
Cheers!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 10:09 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Ard Biesheuvel
2018-04-12 13:39 ` Steve Capper
2018-04-12 16:44 ` Laszlo Ersek
@ 2018-04-12 17:23 ` Leif Lindholm
2018-04-12 17:45 ` Laszlo Ersek
2 siblings, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2018-04-12 17:23 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Steve Capper, Supreeth Venkatesh
On Thu, Apr 12, 2018 at 12:09:46PM +0200, Ard Biesheuvel wrote:
> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
> > Repo: https://github.com/lersek/edk2.git
> > Branch: depex_fixes
> >
> > ArmVirtQemu boots again, it just took a few more patches than I expected
> > :)
> >
> > Some of these patches will have to be ported to edk2-platforms, I think.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Steve Capper <steve.capper@linaro.org>
> > Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
> >
> > Thanks,
> > Laszlo
> >
> > Laszlo Ersek (10):
> > Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
> > ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
> > ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
> > EmbeddedPkg: introduce NvVarStoreFormattedLib
> > ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
> > ArmPlatformPkg/NorFlashDxe: cue the variable driver with
> > NvVarStoreFormatted
> > ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
> > ArmPlatformPkg/PL031RealTimeClockLib: depend on
> > gEfiCpuArchProtocolGuid
> > ArmVirtPkg/PlatformHasAcpiDtDxe: depend on
> > gEfiVariableArchProtocolGuid
> > ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into
> > VariableRuntimeDxe
> >
>
> Laszlo,
>
> Thanks a lot for taking care of this. I am glad we finally got rid of
> the BEFORE depex in the NOR flash driver, which has been problematic
> for years.
>
> For the series,
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> but please allow some time for Leif to chime in as well.
Well, there are a couple of places where I could nitpick on
alphabetical sorting of includes, but none in files that weren't
already disorderly. And like Ard, I am very grateful for this quite
invasive (yet clean) bugfix.
So, for 1-9/10:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> Thanks,
> Ard.
>
>
>
> > ArmPkg/ArmPkg.dec | 2 -
> > ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 8 +-
> > ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +-
> > ArmPlatformPkg/ArmPlatformPkg.dec | 4 -
> > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 13 +---
> > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 6 --
> > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf | 7 +-
> > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 22 +++---
> > ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 5 +-
> > ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> > ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
> > ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
> > EmbeddedPkg/EmbeddedPkg.dec | 3 +
> > EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h | 39 ++++++++++
> > EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c | 41 ++++++++++
> > EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf | 52 +++++++++++++
> > Omap35xxPkg/InterruptDxe/HardwareInterrupt.c | 81 +++++++++++++++-----
> > Omap35xxPkg/InterruptDxe/InterruptDxe.inf | 6 +-
> > 18 files changed, 230 insertions(+), 65 deletions(-)
> > create mode 100644 EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
> > create mode 100644 EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h
> > create mode 100644 EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c
> >
> > --
> > 2.14.1.3.gb7cf6e02401b
> >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 17:23 ` Leif Lindholm
@ 2018-04-12 17:45 ` Laszlo Ersek
2018-04-12 18:13 ` derailing into patch style discussion Leif Lindholm
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 17:45 UTC (permalink / raw)
To: Leif Lindholm, Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Steve Capper, Supreeth Venkatesh
On 04/12/18 19:23, Leif Lindholm wrote:
> On Thu, Apr 12, 2018 at 12:09:46PM +0200, Ard Biesheuvel wrote:
>> On 12 April 2018 at 02:55, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Repo: https://github.com/lersek/edk2.git
>>> Branch: depex_fixes
>>>
>>> ArmVirtQemu boots again, it just took a few more patches than I expected
>>> :)
>>>
>>> Some of these patches will have to be ported to edk2-platforms, I think.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Cc: Steve Capper <steve.capper@linaro.org>
>>> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> Laszlo Ersek (10):
>>> Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
>>> ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
>>> ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
>>> EmbeddedPkg: introduce NvVarStoreFormattedLib
>>> ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
>>> ArmPlatformPkg/NorFlashDxe: cue the variable driver with
>>> NvVarStoreFormatted
>>> ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
>>> ArmPlatformPkg/PL031RealTimeClockLib: depend on
>>> gEfiCpuArchProtocolGuid
>>> ArmVirtPkg/PlatformHasAcpiDtDxe: depend on
>>> gEfiVariableArchProtocolGuid
>>> ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into
>>> VariableRuntimeDxe
>>>
>>
>> Laszlo,
>>
>> Thanks a lot for taking care of this. I am glad we finally got rid of
>> the BEFORE depex in the NOR flash driver, which has been problematic
>> for years.
>>
>> For the series,
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> but please allow some time for Leif to chime in as well.
>
> Well, there are a couple of places where I could nitpick on
> alphabetical sorting of includes,
And, believe me, you would have my total agreement :), but in such
cases, there's always a fork in the road: (a) add a separate patch that
first sorts the includes and [LibraryClasses], without functional
changes, or (b) just stick with the existing disorder, and get in and
out as surgically as possible. My personal preference is (a), but it has
drawn disagreement -- even accusations of pedantry :) :) --, and/or
suggestions to squash such patches with functional changes, in the past,
so I trod more lightly now. Rest assured, I didn't *miss* those places,
I just elected to close my eyes! ;)
> but none in files that weren't
> already disorderly. And like Ard, I am very grateful for this quite
> invasive (yet clean) bugfix.
>
> So, for 1-9/10:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Thank you! I shall get my TODO list (or, should I say, TODO "hash
table", considering its cleanliness) in order, and then I'll get to
pushing stuff.
I *very* much appreciate the quick feedback on this!
Cheers,
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* derailing into patch style discussion
2018-04-12 17:45 ` Laszlo Ersek
@ 2018-04-12 18:13 ` Leif Lindholm
2018-04-12 18:48 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2018-04-12 18:13 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org
Since you already have my r-b on the set, I'll pick up the style
topic, partly because I'm not sure if I've ever explained my
thinking publicly in words that anyone other than Ard understands.
On Thu, Apr 12, 2018 at 07:45:19PM +0200, Laszlo Ersek wrote:
> > Well, there are a couple of places where I could nitpick on
> > alphabetical sorting of includes,
>
> And, believe me, you would have my total agreement :), but in such
> cases, there's always a fork in the road: (a) add a separate patch that
> first sorts the includes and [LibraryClasses], without functional
> changes, or (b) just stick with the existing disorder, and get in and
> out as surgically as possible. My personal preference is (a), but it has
> drawn disagreement -- even accusations of pedantry :) :) --, and/or
> suggestions to squash such patches with functional changes, in the past,
> so I trod more lightly now. Rest assured, I didn't *miss* those places,
> I just elected to close my eyes! ;)
Right :)
So, yes - I do strongly agree with the idea of keeping functionality
and style changes separate (ask Evan), so that wasn't exactly what I
was referring to.
In general my internal "optimal" situation is one where the purely
functional diff leaves the code in a (quite subjectively, since it's
still not conformant) better state on average than it was.
My subjective mark of optimal is that which would minimise any
subsequent patch that was _only_ a style cleanup.
To pick an example from this set (1/10):
diff --git a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
index 6deb8c3f675c..61ad89af2758 100644
--- a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
+++ b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
@@ -41,14 +41,14 @@ [LibraryClasses]
PrintLib
UefiDriverEntryPoint
IoLib
ArmLib
[Protocols]
- gHardwareInterruptProtocolGuid
- gEfiCpuArchProtocolGuid
+ gHardwareInterruptProtocolGuid ## PRODUCES
+ gEfiCpuArchProtocolGuid ## CONSUMES ## NOTIFY
---
This one is pretty straightforward - without touching any non-modified
lines, these _could_ be reordered alphabetically.
(Yes, that change may have functional side-effects, but only on
undefined behaviour, so no different from rebasing to a version with
newer BaseTools can give you.)
Where the minimum diff logic comes in is in situations like (6/10):
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index 812dafd065b2..c40ac27a6599 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -28,12 +28,13 @@ [Sources.common]
NorFlashBlockIoDxe.c
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
ArmPlatformPkg/ArmPlatformPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
---
Applying my "optimal" rule here would have meant inserting the new
line before MdePkg/MdeModulePkg .decs instead. That way a cleanup
patch would end up doing
[Packages]
+ ArmPlatformPkg/ArmPlatformPkg.dec
EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
- ArmPlatformPkg/ArmPlatformPkg.dec
instead of
[Packages]
- MdePkg/MdePkg.dec
- MdeModulePkg/MdeModulePkg.dec
ArmPlatformPkg/ArmPlatformPkg.dec
EmbeddedPkg/EmbeddedPkg.dec
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
Anyway, this is all an aside - I just thought I would give you an
insight into the mind of Leif.
/
Leif
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: derailing into patch style discussion
2018-04-12 18:13 ` derailing into patch style discussion Leif Lindholm
@ 2018-04-12 18:48 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 18:48 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org
On 04/12/18 20:13, Leif Lindholm wrote:
> Since you already have my r-b on the set, I'll pick up the style
> topic, partly because I'm not sure if I've ever explained my
> thinking publicly in words that anyone other than Ard understands.
>
> On Thu, Apr 12, 2018 at 07:45:19PM +0200, Laszlo Ersek wrote:
>>> Well, there are a couple of places where I could nitpick on
>>> alphabetical sorting of includes,
>>
>> And, believe me, you would have my total agreement :), but in such
>> cases, there's always a fork in the road: (a) add a separate patch that
>> first sorts the includes and [LibraryClasses], without functional
>> changes, or (b) just stick with the existing disorder, and get in and
>> out as surgically as possible. My personal preference is (a), but it has
>> drawn disagreement -- even accusations of pedantry :) :) --, and/or
>> suggestions to squash such patches with functional changes, in the past,
>> so I trod more lightly now. Rest assured, I didn't *miss* those places,
>> I just elected to close my eyes! ;)
>
> Right :)
>
> So, yes - I do strongly agree with the idea of keeping functionality
> and style changes separate (ask Evan), so that wasn't exactly what I
> was referring to.
>
> In general my internal "optimal" situation is one where the purely
> functional diff leaves the code in a (quite subjectively, since it's
> still not conformant) better state on average than it was.
>
> My subjective mark of optimal is that which would minimise any
> subsequent patch that was _only_ a style cleanup.
>
> To pick an example from this set (1/10):
> diff --git a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
> b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
> index 6deb8c3f675c..61ad89af2758 100644
> --- a/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
> +++ b/Omap35xxPkg/InterruptDxe/InterruptDxe.inf
> @@ -41,14 +41,14 @@ [LibraryClasses]
> PrintLib
> UefiDriverEntryPoint
> IoLib
> ArmLib
>
> [Protocols]
> - gHardwareInterruptProtocolGuid
> - gEfiCpuArchProtocolGuid
> + gHardwareInterruptProtocolGuid ## PRODUCES
> + gEfiCpuArchProtocolGuid ## CONSUMES ## NOTIFY
>
> ---
> This one is pretty straightforward - without touching any non-modified
> lines, these _could_ be reordered alphabetically.
>
> (Yes, that change may have functional side-effects, but only on
> undefined behaviour, so no different from rebasing to a version with
> newer BaseTools can give you.)
>
> Where the minimum diff logic comes in is in situations like (6/10):
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index 812dafd065b2..c40ac27a6599 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -28,12 +28,13 @@ [Sources.common]
> NorFlashBlockIoDxe.c
>
> [Packages]
> MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
> ArmPlatformPkg/ArmPlatformPkg.dec
> + EmbeddedPkg/EmbeddedPkg.dec
>
> ---
> Applying my "optimal" rule here would have meant inserting the new
> line before MdePkg/MdeModulePkg .decs instead. That way a cleanup
> patch would end up doing
>
> [Packages]
> + ArmPlatformPkg/ArmPlatformPkg.dec
> EmbeddedPkg/EmbeddedPkg.dec
> MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
> - ArmPlatformPkg/ArmPlatformPkg.dec
>
> instead of
>
> [Packages]
> - MdePkg/MdePkg.dec
> - MdeModulePkg/MdeModulePkg.dec
> ArmPlatformPkg/ArmPlatformPkg.dec
> EmbeddedPkg/EmbeddedPkg.dec
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
>
>
> Anyway, this is all an aside - I just thought I would give you an
> insight into the mind of Leif.
I do see the point -- basically, even if we don't imlement separate
cleanups, and just add the functional changes on top of whatever we
already have, if there are multiple ways to keep the functional changes
purely functional and focused, we had better select the one that at
least doesn't make the style worse. Using your [Packages] example, it's
possible *not* to increase the disorder, without actually sorting those
lines. I fully admit this eluded me -- I considered "messy" all-or-nothing.
I'll try to remember this next time. (It's honestly a bit difficult for
me, because if I take the time & effort to be considerate like this, I'd
mostly (though perhaps not always) just write the patch that cleans up
first.)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (10 preceding siblings ...)
2018-04-12 10:09 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Ard Biesheuvel
@ 2018-04-12 16:51 ` Supreeth Venkatesh
2018-04-12 17:46 ` Laszlo Ersek
2018-04-12 19:29 ` Laszlo Ersek
12 siblings, 1 reply; 27+ messages in thread
From: Supreeth Venkatesh @ 2018-04-12 16:51 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper
Thanks Laszlo.
It works for me too.
Supreeth
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, April 11, 2018 7:56 PM
To: edk2-devel@lists.01.org
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm <leif.lindholm@linaro.org>; Steve Capper <steve.capper@linaro.org>; Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Subject: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
Repo: https://github.com/lersek/edk2.git
Branch: depex_fixes
ArmVirtQemu boots again, it just took a few more patches than I expected
:)
Some of these patches will have to be ported to edk2-platforms, I think.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Thanks,
Laszlo
Laszlo Ersek (10):
Omap35xxPkg/InterruptDxe: replace CPU Arch Protocol depex with notify
ArmPkg/ArmGicDxe: annotate protocol usage in "ArmGicDxe.inf"
ArmPkg/CpuDxe: order CpuDxe after ArmGicDxe via protocol depex
EmbeddedPkg: introduce NvVarStoreFormattedLib
ArmPlatformPkg/NorFlashDxe: initialize varstore headers eagerly
ArmPlatformPkg/NorFlashDxe: cue the variable driver with
NvVarStoreFormatted
ArmPlatformPkg/NorFlashDxe: depend on gEfiCpuArchProtocolGuid
ArmPlatformPkg/PL031RealTimeClockLib: depend on
gEfiCpuArchProtocolGuid
ArmVirtPkg/PlatformHasAcpiDtDxe: depend on
gEfiVariableArchProtocolGuid
ArmVirtPkg/ArmVirtQemu: hook NvVarStoreFormattedLib into
VariableRuntimeDxe
ArmPkg/ArmPkg.dec | 2 -
ArmPkg/Drivers/ArmGic/ArmGicDxe.inf | 8 +-
ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +-
ArmPlatformPkg/ArmPlatformPkg.dec | 4 -
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 13 +---
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 6 --
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf | 7 +-
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 22 +++---
ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf | 5 +-
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +-
EmbeddedPkg/EmbeddedPkg.dec | 3 +
EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h | 39 ++++++++++
EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c | 41 ++++++++++
EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf | 52 +++++++++++++
Omap35xxPkg/InterruptDxe/HardwareInterrupt.c | 81 +++++++++++++++-----
Omap35xxPkg/InterruptDxe/InterruptDxe.inf | 6 +-
18 files changed, 230 insertions(+), 65 deletions(-) create mode 100644 EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
create mode 100644 EmbeddedPkg/Include/Guid/NvVarStoreFormatted.h
create mode 100644 EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.c
--
2.14.1.3.gb7cf6e02401b
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 16:51 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Supreeth Venkatesh
@ 2018-04-12 17:46 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 17:46 UTC (permalink / raw)
To: Supreeth Venkatesh, edk2-devel@lists.01.org
Cc: Ard Biesheuvel, Leif Lindholm, Steve Capper
On 04/12/18 18:51, Supreeth Venkatesh wrote:
> Thanks Laszlo.
> It works for me too.
Appreciate the testing!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes
2018-04-12 0:55 [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Laszlo Ersek
` (11 preceding siblings ...)
2018-04-12 16:51 ` [PATCH 00/10] ArmPkg, ArmPlatformPkg, ArmVirtPkg, EmbeddedPkg, Omap35xxPkg: depex fixes Supreeth Venkatesh
@ 2018-04-12 19:29 ` Laszlo Ersek
12 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-04-12 19:29 UTC (permalink / raw)
To: edk2-devel; +Cc: Leif Lindholm, Ard Biesheuvel
On 04/12/18 02:55, Laszlo Ersek wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: depex_fixes
>
> ArmVirtQemu boots again, it just took a few more patches than I expected
> :)
>
> Some of these patches will have to be ported to edk2-platforms, I think.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Steve Capper <steve.capper@linaro.org>
> Cc: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Pushed as commit range 153f5c7a93be..bf453d581ecf.
Thank you all!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread