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

["Sami Mujawar" <sami.mujawar@arm.com>]

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