On 06/09/15 22:42, Ard Biesheuvel wrote:
> On 9 June 2015 at 18:33, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 05/29/15 14:33, Ard Biesheuvel wrote:
>>> Clone ArmGicArchLib into a SEC phase specific ArmGicArchSecLib
>>> so that we can modify the former in a subsequent patch to cache
>>> the GIC revision in a global variable.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> ---
>>> ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/AArch64/ArmGicArchLib.c
>>> | 0
>>> ArmPkg/Library/{ArmGicArchLib => ArmGicArchSecLib}/Arm/ArmGicArchLib.c
>>> | 0
>>> ArmPkg/Library/{ArmGicArchLib/ArmGicArchLib.inf =>
>>> ArmGicArchSecLib/ArmGicArchSecLib.inf} | 6 +++---
>>> ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
>>> | 1 +
>>> ArmPlatformPkg/ArmPlatformPkg.dsc
>>> | 2 ++
>>> ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
>>> | 2 ++
>>> ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>>> | 2 ++
>>> 7 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>>> b/ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
>>> similarity index 100%
>>> copy from ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c
>>> copy to ArmPkg/Library/ArmGicArchSecLib/AArch64/ArmGicArchLib.c
>>> diff --git a/ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>>> b/ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
>>> similarity index 100%
>>> copy from ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c
>>> copy to ArmPkg/Library/ArmGicArchSecLib/Arm/ArmGicArchLib.c
>>> diff --git a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
>>> b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>> similarity index 79%
>>> copy from ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
>>> copy to ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>> index d71b2adc3027..a2fb623a8537 100644
>>> --- a/ArmPkg/Library/ArmGicArchLib/ArmGicArchLib.inf
>>> +++ b/ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>> @@ -13,11 +13,11 @@
>>>
>>> [Defines]
>>> INF_VERSION = 0x00010005
>>> - BASE_NAME = ArmGicArchLib
>>> - FILE_GUID = cd67f41a-26e9-4482-90c9-a9aff803382a
>>> + BASE_NAME = ArmGicArchSecLib
>>> + FILE_GUID = c1dd9745-9459-4e9a-9f5b-99cbd233c27d
>>> MODULE_TYPE = BASE
>>> VERSION_STRING = 1.0
>>> - LIBRARY_CLASS = ArmGicArchLib
>>> + LIBRARY_CLASS = ArmGicArchLib|SEC
>>
>> * Okay, so this patch does the clone for SEC modules and points the SEC
>> library class resolutions in the DSC files to the clone, overriding the
>> default resolutions in them.
>>
>> ArmPkg/ArmPkg.dsc and ArmVirtPkg/ArmVirt.dsc.inc are not updated. For
>> ArmVirtPkg, the default resolution stays intact in this patch, which
>> makes sense, because patch #7 handles it separately.
>>
>> But, is there any particular reason to leave ArmPkg/ArmPkg.dsc unchanged
>> in this patch?
>
> Actually, that is an oversight. I should probably add it there for
> completeness, although the exact purpose of these package .DSCs
> escapes me, to be honest.
Me too. :)
> Anyway, ArmPkg.dsc still builds ok with these changes, so I assume
> there is no SEC module being built that [transitively] relies on
> ArmGicArchLib
Right.
>
>> Hm... I guess it should be safe, because patch #5 is
>> explicit about DXE_DRIVER, UEFI_DRIVER, UEFI_APPLICATION; so if there's
>> a problem later, it will show up as a build time failure. Okay.
>>
>> * Anyway, my main concern was PEIMs. Especially because in patch #3 you
>> wrote,
>>
>>> this statelessness is only needed for SEC type modules
>>
>> This is in general not true, because PEIMs also may execute-in-place
>> from flash. However, from patch #5 I can see that the caching version
>> will be available only to DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION,
>> leaving PEIMs uncovered -- which is safe, and also good enough since
>> (apparently) no PEIMs depend on this library class.
>>
>
> i have you on record (and I can dig up the email, if you like) saying
> that you prefer the LIBRARY_CLASS phase modifiers to be minimal.
Do you also have me on record for never contradicting myself? ;)
So, don't bother.
Cheers
Laszlo
>> If you have to respin the series for any reason, please consider
>> mentioning PEIMs in the commit message of patch #3 and patch #4, and
>> making ArmGicArchSecLib available to PEIMs too. However, as I said
>> above, that would be quite speculative, and this could be addressed any
>> time later, if a new PEIM actually needed the library. So feel free to
>> ignore this note.
>>
>> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>>
>
> Thanks,
> Ard,
>
>
>>>
>>> [Sources.ARM]
>>> Arm/ArmGicArchLib.c
>>> diff --git a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
>>> b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
>>> index 0859bc31ff00..6e6687c8a735 100644
>>> --- a/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
>>> +++ b/ArmPlatformPkg/ArmPlatformPkg-2ndstage.dsc
>>> @@ -138,6 +138,7 @@
>>>
>>> PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
>>>
>>> PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
>>> PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
>>> + ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>>
>>> [LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
>>> MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
>>> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc
>>> b/ArmPlatformPkg/ArmPlatformPkg.dsc
>>> index be31025d9bd0..14d82f61ec76 100644
>>> --- a/ArmPlatformPkg/ArmPlatformPkg.dsc
>>> +++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
>>> @@ -134,6 +134,8 @@
>>>
>>> DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
>>>
>>> DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
>>>
>>> + ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>> +
>>> [LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
>>> MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
>>>
>>> diff --git a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
>>> b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
>>> index 19de3816ae5c..4b4867f9926a 100644
>>> --- a/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
>>> +++ b/ArmPlatformPkg/ArmRealViewEbPkg/ArmRealViewEb.dsc.inc
>>> @@ -128,6 +128,8 @@
>>>
>>> PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
>>> !endif
>>>
>>> + ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>> +
>>> [LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
>>> MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
>>>
>>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>>> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>>> index 84e2a9987f7d..8f7b5f1644de 100644
>>> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>>> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>>> @@ -144,6 +144,8 @@
>>> # Trustzone Support
>>>
>>> ArmTrustedMonitorLib|ArmPlatformPkg/Library/ArmTrustedMonitorLibNull/ArmTrustedMonitorLibNull.inf
>>>
>>> + ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
>>> +
>>> [LibraryClasses.common.PEI_CORE]
>>> HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>>> PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>>>
>>
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
