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

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

[Rohit Mathew]

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.  This change is essential
to initialize uart for SEC phase.

Signed-off-by: Rohit Mathew <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
+ */
+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();
+
   // Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.
 
   // If not primary Jump to Secondary Main
-- 
2.25.1

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

[Thomas Abraham]

On 06/07/2022 14:42, 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.  This change is essential
> to initialize uart for SEC phase.
> 
> Signed-off-by: Rohit Mathew <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
> + */
> +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();
> +
>     // Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.
>   
>     // If not primary Jump to Secondary Main

Reviewed-by: Thomas Abraham <thomas.abraham@arm.com>
Tested-by: Thomas Abraham <thomas.abraham@arm.com>

Thanks.

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

[Rohit Mathew]

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

Hi Team,

Gentle reminder for this patch.

Regards,
Rohit

On Wed, Jul 6, 2022 at 03:20 PM, Thomas Abraham wrote:

> 
> On 06/07/2022 14:42, 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. This change is essential
>> to initialize uart for SEC phase.
>> 
>> Signed-off-by: Rohit Mathew <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
>> + */
>> +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();
>> +
>> // Note: The MMU will be enabled by MemoryPeim. Only the primary core will
>> have the MMU on.
>> 
>> // If not primary Jump to Secondary Main
> 
> Reviewed-by: Thomas Abraham <thomas.abraham@arm.com>
> Tested-by: Thomas Abraham <thomas.abraham@arm.com>
> 
> Thanks.

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

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

[Sami Mujawar]

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

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?
> This change is essential
> to initialize uart for SEC phase.
>
> Signed-off-by: Rohit Mathew<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]
> + */
> +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?
> +
>     // Note: The MMU will be enabled by MemoryPeim. Only the primary core will have the MMU on.
>   
>     // If not primary Jump to Secondary Main

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

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

[Rohit Mathew]

[-- 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 --]

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

[Sami Mujawar]

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

Hi Rohit,

On Wed, Jul 6, 2022 at 06:42 AM, Rohit Mathew wrote:

> 
> =20
> @@ -86,6 +86,10 @@ CEntryPoint (
> ArmEnableVFP ();
> }
> =20
> + // Explicitly invoke the library constructor to resolve any library
> + // dependency.
> + ProcessLibraryConstructorList();
> +

I missed this in my previous feedback, but ProcessLibraryConstructorList() should only be called by the Primary core.

Regards,

Sami Mujawar

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

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

[Rohit Mathew]

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

Hi Sami,

On Thu, Jul 21, 2022 at 05:26 PM, Sami Mujawar wrote:

> 
> Hi Rohit,
> 
> On Wed, Jul 6, 2022 at 06:42 AM, Rohit Mathew wrote:
> 
>> =20
>> @@ -86,6 +86,10 @@ CEntryPoint (
>> ArmEnableVFP ();
>> }
>> =20
>> + // Explicitly invoke the library constructor to resolve any library
>> + // dependency.
>> + ProcessLibraryConstructorList();
>> +
> 
> I missed this in my previous feedback, but ProcessLibraryConstructorList()
> should only be called by the Primary core.
> 
> Regards,
> 
> Sami Mujawar

I have moved the call within -

if (ArmPlatformIsPrimaryCore (MpId)) {

This, and the other feedbacks have been addressed in V4.

Regards,
Rohit

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