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

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

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

Hi Sami,

Thank you for the review. Please find my response inline -

On Thu, Jul 21, 2022 at 04:55 PM, Sami Mujawar wrote:

> 
> 
> 
> Hi Rohit,
> 
> 
> 
> Please find my response inline marked [SAMI].
> 
> 
> 
> Regards,
> 
> 
> 
> Sami Mujawar
> 
> On 06/07/2022 02:42 pm, Rohit Mathew wrote:
> 
>> Invoke the constructor in the SEC phase to call into initialization
>> functions associated with libraries linked with this particular module.
>> For instance, PrePeiCore's CEntryPoint function calls DebugLib library's
>> print API before the library is initialized.  
> 
> [SAMI] Can you rephrase the commit message to be clearer, please?

[Rohit] Sure. Done.

> 
> 
>> This change is essential
>> to initialize uart for SEC phase.
>> 
>> Signed-off-by:
>> Rohit Mathew <rohit.mathew@arm.com> ( rohit.mathew@arm.com )
>> ---
>> 
>> ArmPlatformPkg/PrePeiCore/PrePeiCore.h | 11 ++++++++++-
>> 
>> ArmPlatformPkg/PrePeiCore/PrePeiCore.c |  6 +++++-
>>  2 files changed, 15
>> insertions(+), 2 deletions(-)
>> 
>> Changes since V1:
>> - Rebased on top of
>> latest master branch.
>> - Addressed comments from Ard.
>> 
>> Changes since V2:
>> -
>> Rebased on top of latest master branch.
>> 
>> Link to github branch for the
>> patch -
>> https://github.com/rohit-arm/edk2/tree/sec_constructor_issue
>> 
>> diff --git
>> a/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>> b/ArmPlatformPkg/PrePeiCore/PrePeiCore.h
>> index 0345dd7bdd2a..b752c4b9e617
>> 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
> 
> 
> 
> [SAMI] The above description does not appear to be correct.
> 
> 
> 
> Would the following test be more appropriate?
> 
> Autogenerated function that calls the library constructors for all of the
> module's
> dependent libraries.
> [/SAMI]
> 
> 

[Rohit]. Thanks for correcting this. Done.

> 
> 
> 
>> + */
>> +VOID
>> +EFIAPI
>> +ProcessLibraryConstructorList (
>> +  VOID
>> +  );
>> +
>> 
>> #endif
>> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
>> b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
>> index 6dd9bcdea24f..9654866f0c13
>> 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,6 +86,10 @@
>> CEntryPoint (
>>      ArmEnableVFP ();
>>    }
>>  
>> +  // Explicitly invoke the
>> library constructor to resolve any library
>> +  // dependency.
>> + 
>> ProcessLibraryConstructorList();
> 
> [SAMI] I don't think this will pass the edk2 CI. Can you check, please?

[Rohit] Checked CI locally. Fixed spacing issue for parenthesis.

Snippet of CI run after fixing issues -

"

PROGRESS - --->Test Success: Uncrustify Coding Standard Test NO-TARGET

PROGRESS - --Running ArmPlatformPkg: Host Unit Test Dsc Complete Check Test NO-TARGET --

PROGRESS - --->Test Success: Host Unit Test Dsc Complete Check Test NO-TARGET

PROGRESS - --Running ArmPlatformPkg: Compiler Plugin DEBUG --

PROGRESS - Start time: 2022-07-22 17:53:34.737944

PROGRESS - Setting up the Environment

PROGRESS - Running Pre Build

PROGRESS - Running Build DEBUG

PROGRESS - Running Post Build

PROGRESS - End time: 2022-07-22 17:53:39.417824  Total time Elapsed: 0:00:04

PROGRESS - --->Test Success: Compiler Plugin DEBUG

"

[/Rohit]

> 
> 
>> +
>>    // Note: The MMU will be enabled by MemoryPeim. Only the primary core
>> will have the MMU on.
>>  
>> 
> 
> 

Regards,

Rohit

> 
> 
>>    // If not primary Jump to Secondary Main
>> 
> 
>

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