From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [edk2][PATCH V3 1/1] ArmPlatformPkg/PrePeiCore: Explicitly invoke constructor for SEC phase To: Sami Mujawar ,devel@edk2.groups.io From: "Rohit Mathew" X-Originating-Location: Cambridge, England, GB (217.140.99.251) X-Originating-Platform: Windows Chrome 103 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Fri, 22 Jul 2022 10:08:17 -0700 References: In-Reply-To: Message-ID: <3602.1658509697810873618@groups.io> Content-Type: multipart/alternative; boundary="k5EzFDbkn8ylDjoyCwCr" --k5EzFDbkn8ylDjoyCwCr Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Sami, Thank you for the review. Please find my response inline - On Thu, Jul 21, 2022 at 04:55 PM, Sami Mujawar wrote: >=20 >=20 >=20 > Hi Rohit, >=20 >=20 >=20 > Please find my response inline marked [SAMI]. >=20 >=20 >=20 > Regards, >=20 >=20 >=20 > Sami Mujawar >=20 > On 06/07/2022 02:42 pm, Rohit Mathew wrote: >=20 >> 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. =20 >=20 > [SAMI] Can you rephrase the commit message to be clearer, please? [Rohit] Sure. Done. >=20 >=20 >> This change is essential >> to initialize uart for SEC phase. >>=20 >> Signed-off-by: >> Rohit Mathew ( rohit.mathew@arm.com ) >> --- >>=20 >> ArmPlatformPkg/PrePeiCore/PrePeiCore.h | 11 ++++++++++- >>=20 >> ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 6 +++++- >> 2 files changed, 15 >> insertions(+), 2 deletions(-) >>=20 >> Changes since V1: >> - Rebased on top of >> latest master branch. >> - Addressed comments from Ard. >>=20 >> Changes since V2: >> - >> Rebased on top of latest master branch. >>=20 >> Link to github branch for the >> patch - >> https://github.com/rohit-arm/edk2/tree/sec_constructor_issue >>=20 >> 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 >> =20 >> Main file supporting the transition to PEI Core in Normal World for >> Versatile Express >> =20 >> - Copyright (c) 2011, ARM Limited. All rights >> reserved. >> + Copyright (c) 2011 - 2022, ARM Limited. All rights reserved. >>=20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >> =20 >> @@ -73,4 +73,13 @@ >> PeiCommonExceptionEntry ( >> IN UINTN LR >> ); >> =20 >> +/* >> + * Constructor for >> SEC phase >=20 >=20 >=20 > [SAMI] The above description does not appear to be correct. >=20 >=20 >=20 > Would the following test be more appropriate? >=20 > Autogenerated function that calls the library constructors for all of the > module's > dependent libraries. > [/SAMI] >=20 >=20 [Rohit]. Thanks for correcting this. Done. >=20 >=20 >=20 >> + */ >> +VOID >> +EFIAPI >> +ProcessLibraryConstructorList ( >> + VOID >> + ); >> + >>=20 >> #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 >> =20 >> Main file supporting the transition to PEI Core in Normal World for >> Versatile Express >> =20 >> - Copyright (c) 2011-2014, ARM Limited. All rights >> reserved. >> + Copyright (c) 2011 - 2022, ARM Limited. All rights reserved. >>=20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >> =20 >> @@ -86,6 +86,10 @@ >> CEntryPoint ( >> ArmEnableVFP (); >> } >> =20 >> + // Explicitly invoke the >> library constructor to resolve any library >> + // dependency. >> +=20 >> ProcessLibraryConstructorList(); >=20 > [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-TARG= ET 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=C2=A0 Total time Elapsed: 0= :00:04 PROGRESS - --->Test Success: Compiler Plugin DEBUG " [/Rohit] >=20 >=20 >> + >> // Note: The MMU will be enabled by MemoryPeim. Only the primary core >> will have the MMU on. >> =20 >>=20 >=20 >=20 Regards, Rohit >=20 >=20 >> // If not primary Jump to Secondary Main >>=20 >=20 > --k5EzFDbkn8ylDjoyCwCr Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable
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:<= /div> 
Invoke the constructor in the SEC phase to cal=
l 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>
---
 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_co=
nstructor_issue

diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.h b/ArmPlatformPkg/PrePei=
Core/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 Vers=
atile Express
=20
-  Copyright (c) 2011, ARM Limited. All rights reserved.
+  Copyright (c) 2011 - 2022, ARM Limited. All rights reserved.
=20
   SPDX-License-Identifier: BSD-2-Clause-Patent
=20
@@ -73,4 +73,13 @@ PeiCommonExceptionEntry (
   IN UINTN   LR
   );
=20
+/*
+ * Constructor for SEC phase

[SAMI] The above description does not appear to be correct.

Would the following test be more appropriate?

  Autogenerated function that cal= ls 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/PrePei=
Core/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 Vers=
atile Express
=20
-  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+  Copyright (c) 2011 - 2022, ARM Limited. All rights reserved.
=20
   SPDX-License-Identifier: BSD-2-Clause-Patent
=20
@@ -86,6 +86,10 @@ CEntryPoint (
     ArmEnableVFP ();
   }
=20
+  // 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-TARGE= T

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

PROGRESS - --->Test Success: Host Unit Test Dsc Complete Check Test N= O-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 wi=
ll have the MMU on.
=20

Regards,

Rohit

 

   // If not primary Jump to Secondary Main
--k5EzFDbkn8ylDjoyCwCr--