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

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

[rohit.mathew]

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
+  );
+
 #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.
+  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
   if (ArmPlatformIsPrimaryCore (MpId)) {
-- 
2.25.1

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

[Ard Biesheuvel]

(+ 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?

>  #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.

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

Please leave this comment as before.

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

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

[Rohit Mathew]

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