On 06/29/15 10:14, Ard Biesheuvel wrote:
> 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.
I won't object.
Laszlo
>
> 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
