[edk2,05/10] StandaloneMmPkg/StandaloneMmCoreEntryPoint: drop explicit SerialPortLib call

Message ID 20190305133248.4828-6-ard.biesheuvel@linaro.org
State Accepted
Commit 4b771927c801a873c8b8169b07e77e64d7726198
Headers show
Series
  • StandaloneMmPkg, ArmPkg: cleanups and improvements
Related show

Commit Message

Ard Biesheuvel March 5, 2019, 1:32 p.m.
Sending DEBUG output to the serial port should only be done via
DebugLib calls, which is in charge of initializing the serial
port when appropriate. So drop the explicit SerialPortInitialize ()
invocation, and rely on normal constructor ordering to get the
serial port into the appropriate state at the right time.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 3 ---
 1 file changed, 3 deletions(-)

-- 
2.20.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Yao, Jiewen March 5, 2019, 1:52 p.m. | #1
Reviewed-by: Jiewen.yao@intel.com


> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Tuesday, March 5, 2019 5:33 AM

> To: edk2-devel@lists.01.org

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Achin Gupta

> <achin.gupta@arm.com>; Supreeth Venkatesh

> <supreeth.venkatesh@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>;

> Leif Lindholm <leif.lindholm@linaro.org>; Jagadeesh Ujja

> <jagadeesh.ujja@arm.com>

> Subject: [PATCH 05/10] StandaloneMmPkg/StandaloneMmCoreEntryPoint:

> drop explicit SerialPortLib call

> 

> Sending DEBUG output to the serial port should only be done via

> DebugLib calls, which is in charge of initializing the serial

> port when appropriate. So drop the explicit SerialPortInitialize ()

> invocation, and rely on normal constructor ordering to get the

> serial port into the appropriate state at the right time.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

> 

> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Standal

> oneMmCoreEntryPoint.c | 3 ---

>  1 file changed, 3 deletions(-)

> 

> diff --git

> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand

> aloneMmCoreEntryPoint.c

> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand

> aloneMmCoreEntryPoint.c

> index 5cca532456fd..c8e11a253d24 100644

> ---

> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand

> aloneMmCoreEntryPoint.c

> +++

> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand

> aloneMmCoreEntryPoint.c

> @@ -232,9 +232,6 @@ _ModuleEntryPoint (

>    VOID                                    *TeData;

>    UINTN                                   TeDataSize;

> 

> -  Status = SerialPortInitialize ();

> -  ASSERT_EFI_ERROR (Status);

> -

>    // Get Secure Partition Manager Version Information

>    Status = GetSpmVersion ();

>    if (EFI_ERROR (Status)) {

> --

> 2.20.1


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Achin Gupta March 6, 2019, 4:35 p.m. | #2
Hi Ard,

On Tue, Mar 05, 2019 at 02:32:43PM +0100, Ard Biesheuvel wrote:
> Sending DEBUG output to the serial port should only be done via

> DebugLib calls, which is in charge of initializing the serial

> port when appropriate. So drop the explicit SerialPortInitialize ()

> invocation, and rely on normal constructor ordering to get the

> serial port into the appropriate state at the right time.

>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 3 ---

>  1 file changed, 3 deletions(-)

>

> diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> index 5cca532456fd..c8e11a253d24 100644

> --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> @@ -232,9 +232,6 @@ _ModuleEntryPoint (

>    VOID                                    *TeData;

>    UINTN                                   TeDataSize;

>

> -  Status = SerialPortInitialize ();

> -  ASSERT_EFI_ERROR (Status);


This is done in the first few instructions after EL3 ERETs into S-EL0 to
initialise the StMM partition. The constructors will be called a bit later. I
agree with the change but does EDK2 provide a mechanism for early prints to the
console in case we need this in future.

cheers,
Achin

> -

>    // Get Secure Partition Manager Version Information

>    Status = GetSpmVersion ();

>    if (EFI_ERROR (Status)) {

> --

> 2.20.1

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 6, 2019, 4:41 p.m. | #3
On Wed, 6 Mar 2019 at 17:35, Achin Gupta <Achin.Gupta@arm.com> wrote:
>

> Hi Ard,

>

> On Tue, Mar 05, 2019 at 02:32:43PM +0100, Ard Biesheuvel wrote:

> > Sending DEBUG output to the serial port should only be done via

> > DebugLib calls, which is in charge of initializing the serial

> > port when appropriate. So drop the explicit SerialPortInitialize ()

> > invocation, and rely on normal constructor ordering to get the

> > serial port into the appropriate state at the right time.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 3 ---

> >  1 file changed, 3 deletions(-)

> >

> > diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> > index 5cca532456fd..c8e11a253d24 100644

> > --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> > +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> > @@ -232,9 +232,6 @@ _ModuleEntryPoint (

> >    VOID                                    *TeData;

> >    UINTN                                   TeDataSize;

> >

> > -  Status = SerialPortInitialize ();

> > -  ASSERT_EFI_ERROR (Status);

>

> This is done in the first few instructions after EL3 ERETs into S-EL0 to

> initialise the StMM partition. The constructors will be called a bit later. I

> agree with the change but does EDK2 provide a mechanism for early prints to the

> console in case we need this in future.

>


If so, the correct way to achieve this would be to call the DebugLib
constructor by hand, and that should call the SerialPortLib
constructor. Unfortunately, EDK2 is not put together like that, and
especially constructor ordering is slightly broken.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Achin Gupta March 6, 2019, 4:55 p.m. | #4
On Wed, Mar 06, 2019 at 05:41:30PM +0100, Ard Biesheuvel wrote:
> On Wed, 6 Mar 2019 at 17:35, Achin Gupta <Achin.Gupta@arm.com> wrote:

> >

> > Hi Ard,

> >

> > On Tue, Mar 05, 2019 at 02:32:43PM +0100, Ard Biesheuvel wrote:

> > > Sending DEBUG output to the serial port should only be done via

> > > DebugLib calls, which is in charge of initializing the serial

> > > port when appropriate. So drop the explicit SerialPortInitialize ()

> > > invocation, and rely on normal constructor ordering to get the

> > > serial port into the appropriate state at the right time.

> > >

> > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > > ---

> > >  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c | 3 ---

> > >  1 file changed, 3 deletions(-)

> > >

> > > diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> > > index 5cca532456fd..c8e11a253d24 100644

> > > --- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> > > +++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c

> > > @@ -232,9 +232,6 @@ _ModuleEntryPoint (

> > >    VOID                                    *TeData;

> > >    UINTN                                   TeDataSize;

> > >

> > > -  Status = SerialPortInitialize ();

> > > -  ASSERT_EFI_ERROR (Status);

> >

> > This is done in the first few instructions after EL3 ERETs into S-EL0 to

> > initialise the StMM partition. The constructors will be called a bit later. I

> > agree with the change but does EDK2 provide a mechanism for early prints to the

> > console in case we need this in future.

> >

>

> If so, the correct way to achieve this would be to call the DebugLib

> constructor by hand, and that should call the SerialPortLib

> constructor. Unfortunately, EDK2 is not put together like that, and

> especially constructor ordering is slightly broken.


Thanks!

Reviewed-by: achin.gupta@arm.com

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
index 5cca532456fd..c8e11a253d24 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c
@@ -232,9 +232,6 @@  _ModuleEntryPoint (
   VOID                                    *TeData;
   UINTN                                   TeDataSize;
 
-  Status = SerialPortInitialize ();
-  ASSERT_EFI_ERROR (Status);
-
   // Get Secure Partition Manager Version Information
   Status = GetSpmVersion ();
   if (EFI_ERROR (Status)) {