On 26 June 2015 at 16:17, Laszlo Ersek <ler...@redhat.com> wrote:
> 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
[...]
>> @@ -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.)
>
I agree with the latter but not with the former. It was I who added
those redundant lines, and I am happy to propose a patch that goes on
top to remove them again.
Or you could add a patch that removes those two lines before this one,
if you need to do a V3 anyway.
This is a complicated series, and mixing cleanup work with actual
changes does not help those who need to look at this patch a while
from now, so squashing it into this one is the worst solution imo.
Regards,
Ard.
>>
>> 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
