comments below
On 06/26/15 10:37, Star Zeng wrote:
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.z...@intel.com>
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 30 ++++++++++--------------------
> ArmVirtPkg/ArmVirtQemu.fdf | 9 ++-------
> 2 files changed, 12 insertions(+), 27 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index b49389c..60f7a06 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -239,14 +239,7 @@
> ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> ArmPkg/Drivers/CpuPei/CpuPei.inf
>
> -!if $(SECURE_BOOT_ENABLE) == TRUE
> - SecurityPkg/VariableAuthenticated/Pei/VariablePei.inf {
> - <LibraryClasses>
> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
> - }
> -!else
> MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> -!endif
I'm a bit confused here. I assume that
"MdeModulePkg/Universal/Variable/Pei/VariablePei.inf", when it is built
with Secure Boot support, will *still* depend on BaseCryptLib. Maybe not
directly, but indirectly.
However, at the end of the patch set, I cannot find any BaseCryptLib
resolution (either in the dsc or in the dsc.inf file) that would point
to PeiCryptLib.inf.
So either I'm wrong about the merged "VariablePei.inf" driver needing
BaseCryptLib, or this is an error in the patch.
>
> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
> <LibraryClasses>
> @@ -267,22 +260,11 @@
> #
> ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
> -!if $(SECURE_BOOT_ENABLE) == TRUE
> - MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
> - <LibraryClasses>
> -
> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> - }
Okay, the patch preserves (re-adds) this lower down.
> - SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableRuntimeDxe.inf {
> - <LibraryClasses>
> - BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> - OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> - }
Yes, this should be removed. Good.
> -
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
Preserved lower down again. Good.
> -!else
> - MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
Preserved lower down as well (without the DxeImageVerificationLib hook),
good.
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
> <LibraryClasses>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> + OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
These should not be added. They are already resolved in
"ArmVirtPkg/ArmVirt.dsc.inc".
(Yes, they were superfluously resolved for
"SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableRuntimeDxe.inf" as
well.)
>
> TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
And I commented on these at the earlier ArmVirtPkg patch.
> !else
> @@ -290,6 +272,14 @@
>
> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
> !endif
> }
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> + MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
> + <LibraryClasses>
> +
> NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> + }
> +
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> +!else
> + MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
> !endif
Good.
> MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> index 73d088a..e822fdf 100644
> --- a/ArmVirtPkg/ArmVirtQemu.fdf
> +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> @@ -1,6 +1,7 @@
> #
> # Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> # Copyright (c) 2014, Linaro Limited. All rights reserved.
> +# Copyright (c) 2015, Intel Corporation. All rights reserved.
> #
> # This program and the accompanying materials
> # are licensed and made available under the terms and conditions of the BSD
> License
> @@ -116,11 +117,9 @@ READ_LOCK_STATUS = TRUE
> INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
> INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> + INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> !if $(SECURE_BOOT_ENABLE) == TRUE
> - INF SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableRuntimeDxe.inf
> INF
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> -!else
> - INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> !endif
> INF
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> INF EmbeddedPkg/ResetRuntimeDxe/ResetRuntimeDxe.inf
Good.
> @@ -264,11 +263,7 @@ READ_LOCK_STATUS = TRUE
> INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> INF ArmPkg/Drivers/CpuPei/CpuPei.inf
> INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> -!if $(SECURE_BOOT_ENABLE) == TRUE
> - INF SecurityPkg/VariableAuthenticated/Pei/VariablePei.inf
> -!else
> INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> -!endif
> INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
Makes sense.
>
> FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>
Summary:
(1) The
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
resolution seems to be missing for the merged
MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
PEIM in the secure boot case. Please add it to the
[LibraryClasses.common.PEIM]
section, conditionally, in the "ArmVirtPkg/ArmVirt.dsc.inc" file.
(2) No need to spell out
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
for the merged
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
driver; they are already handled by the dsc.inc file.
Hm... Actually, I just built the tree at the end of this series, with:
build -a AARCH64 -t GCC48 -p ArmVirtPkg/ArmVirtQemu.dsc \
--report-file=.../build.aa64virt.report -b DEBUG \
-D DEBUG_PRINT_ERROR_LEVEL=0x8040004F \
-D SECURE_BOOT_ENABLE -D INTEL_BDS
and then I checked the build report for any BaseCryptLib resolutions.
The merged "VariablePei.inf" does *not* depend on BaseCryptLib. So that
part of the patch is correct.
So, the only thing to fix in this patch is (2).
Thanks!
Laszlo
------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors
network devices and physical & virtual servers, alerts via email & sms
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
