Re: [edk2-devel] [edk2][PATCH V1 1/1] ArmPlatformPkg/PrePeiCore: Explicitly invoke constructor for SEC phase

["Rohit Mathew" <rohit.mathew@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 3508 bytes --]

Hi Ard,

Apologies, I missed your response.

On Tue, May 3, 2022 at 09:43 AM, Ard Biesheuvel wrote:

> 
> (+ Rebecca)
> 
> On Tue, 8 Mar 2022 at 12:55, Rohit Mathew <rohit.mathew@arm.com> wrote:
> 
>> PrePeiCore's CEntry function calls DebugLib library's print API before
>> the library is initialized. So, invoke the constructor in the SEC phase
>> to call into initialization functions associated with libraries linked
>> with this particular module. This change is essential to initialize
>> uart for SEC.
>> 
>> Signed-off-by: Rohit Mathew <rohit.mathew@arm.com>
>> ---
>> ArmPlatformPkg/PrePeiCore/PrePeiCore.h | 11 ++++++++++-
>> ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 9 +++++++--
>> 2 files changed, 17 insertions(+), 3 deletions(-)
>> 
>> Link to github branch with the patches in this series -
>> https://github.com/rohit-arm/edk2/tree/sec_constructor_issue
>> 
>> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>> b/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>> index 0345dd7bdd2a..d2491aa586ee 100644
>> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>> @@ -1,7 +1,7 @@
>> /** @file
>> Main file supporting the transition to PEI Core in Normal World for
>> Versatile Express
>> 
>> - Copyright (c) 2011, ARM Limited. All rights reserved.
>> + Copyright (c) 2011-2022, ARM Limited. All rights reserved.
>> 
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>> 
>> @@ -73,4 +73,13 @@ PeiCommonExceptionEntry (
>> IN UINTN LR
>> );
>> 
>> +/*
>> + * Constructor for SEC phase
>> + */
>> +VOID
>> +EFIAPI
>> +ProcessLibraryConstructorList (
>> + VOID
>> + );
>> +
> 
> Why do we need this? Can't we just include PiPei.h or something like that?

PiPei.h has already been included in ArmPlatformPkg/PrePeiCore/PrePeiCore.h. I tried including ArmPlatformPkg/PrePi/PrePi.h which had ProcessLibraryConstructorList prototype, but both PrePeicore.h and PrePi.h have there own versions for "VOID PrimaryMain(*)" which resulted in conflict.

> 
> 
>> #endif
>> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
>> b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
>> index 6dd9bcdea24f..b5f7d2f05b1e 100644
>> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
>> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
>> @@ -1,7 +1,7 @@
>> /** @file
>> Main file supporting the transition to PEI Core in Normal World for
>> Versatile Express
>> 
>> - Copyright (c) 2011-2014, ARM Limited. All rights reserved.
>> + Copyright (c) 2011-2022, ARM Limited. All rights reserved.
>> 
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>> 
>> @@ -86,7 +86,12 @@ CEntryPoint (
>> ArmEnableVFP ();
>> }
>> 
>> - // Note: The MMU will be enabled by MemoryPeim. Only the primary core
>> will have the MMU on.
>> + // In the Sec phase, explicitly invoke the library constructors. This
>> helps
>> + // the DebugPrint library to be initialized before it is used by
>> subsequent
>> + // code executed in the sec phase.
> 
> Please drop the reference to DebugLib - calling library constructors
> could be needed for any library dependency.

Sure.

> 
> 
>> + ProcessLibraryConstructorList();
>> +
>> + //Note: The MMU will be enabled by MemoryPeim. Only the primary core
>> will have the MMU on.
> 
> Please leave this comment as before.

Will do that.

> 
> 
>> // If not primary Jump to Secondary Main
>> if (ArmPlatformIsPrimaryCore (MpId)) {
>> --
>> 2.25.1
> 
> 

Regards,
Rohit

[-- Attachment #2: Type: text/html, Size: 3874 bytes --]