[PATCH] IntelFsp2Pkg: PeiService pointer not reset to 0 in SecMain

[PATCH] IntelFsp2Pkg: PeiService pointer not reset to 0 in SecMain

[Chiu, Chasel]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2018

Current FSP SEC_IDT_TABLE structure is not natural aligned:
  typedef struct _SEC_IDT_TABLE {
    EFI_PEI_SERVICES  *PeiService;
    UINT64            IdtTable[];
  } SEC_IDT_TABLE;
Compiler will insert DWORD padding between 2 elements and
GetPeiServicesTablePointer() in early phase then always
returns padding data from stack, which was not rest to 0
in SecMain.

Solution is to align FSP SEC_IDT_TABLE structure to UefiCpuPkg
to have UINT64 as PeiService field and rest it to 0.

Test: Verified on internal platform and booting successfully
      with FSP API mode.

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
---
 IntelFsp2Pkg/FspSecCore/SecMain.c |  2 +-
 IntelFsp2Pkg/FspSecCore/SecMain.h | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c b/IntelFsp2Pkg/FspSecCore/SecMain.c
index a63d1336e4..7169afc6c7 100644
--- a/IntelFsp2Pkg/FspSecCore/SecMain.c
+++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
@@ -114,7 +114,7 @@ SecStartup (
   // |                   |
   // |                   |
   // |-------------------|---->  TempRamBase
-  IdtTableInStack.PeiService  = NULL;
+  IdtTableInStack.PeiService = 0;
   AsmReadIdtr (&IdtDescriptor);
   if (IdtDescriptor.Base == 0) {
     ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h b/IntelFsp2Pkg/FspSecCore/SecMain.h
index 6fb16febab..af7f387960 100644
--- a/IntelFsp2Pkg/FspSecCore/SecMain.h
+++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -29,7 +29,13 @@ typedef VOID (*PEI_CORE_ENTRY) ( \
 );
 
 typedef struct _SEC_IDT_TABLE {
-  EFI_PEI_SERVICES  *PeiService;
+  //
+  // Reserved 8 bytes preceding IDT to store EFI_PEI_SERVICES**, since IDT base
+  // address should be 8-byte alignment.
+  // Note: For IA32, only the 4 bytes immediately preceding IDT is used to store
+  // EFI_PEI_SERVICES**
+  //
+  UINT64            PeiService;
   UINT64            IdtTable[FixedPcdGet8 (PcdFspMaxInterruptSupported)];
 } SEC_IDT_TABLE;
 
-- 
2.13.3.windows.1

Re: [edk2-devel] [PATCH] IntelFsp2Pkg: PeiService pointer not reset to 0 in SecMain

[Nate DeSimone]

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu, Chasel
Sent: Thursday, July 25, 2019 8:51 AM
To: devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2-devel] [PATCH] IntelFsp2Pkg: PeiService pointer not reset to 0 in SecMain

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2018

Current FSP SEC_IDT_TABLE structure is not natural aligned:
  typedef struct _SEC_IDT_TABLE {
    EFI_PEI_SERVICES  *PeiService;
    UINT64            IdtTable[];
  } SEC_IDT_TABLE;
Compiler will insert DWORD padding between 2 elements and
GetPeiServicesTablePointer() in early phase then always returns padding data from stack, which was not rest to 0 in SecMain.

Solution is to align FSP SEC_IDT_TABLE structure to UefiCpuPkg to have UINT64 as PeiService field and rest it to 0.

Test: Verified on internal platform and booting successfully
      with FSP API mode.

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
---
 IntelFsp2Pkg/FspSecCore/SecMain.c |  2 +-  IntelFsp2Pkg/FspSecCore/SecMain.h | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c b/IntelFsp2Pkg/FspSecCore/SecMain.c
index a63d1336e4..7169afc6c7 100644
--- a/IntelFsp2Pkg/FspSecCore/SecMain.c
+++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
@@ -114,7 +114,7 @@ SecStartup (
   // |                   |
   // |                   |
   // |-------------------|---->  TempRamBase
-  IdtTableInStack.PeiService  = NULL;
+  IdtTableInStack.PeiService = 0;
   AsmReadIdtr (&IdtDescriptor);
   if (IdtDescriptor.Base == 0) {
     ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h b/IntelFsp2Pkg/FspSecCore/SecMain.h
index 6fb16febab..af7f387960 100644
--- a/IntelFsp2Pkg/FspSecCore/SecMain.h
+++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2019, Intel Corporation. All rights 
+ reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -29,7 +29,13 @@ typedef VOID (*PEI_CORE_ENTRY) ( \  );
 
 typedef struct _SEC_IDT_TABLE {
-  EFI_PEI_SERVICES  *PeiService;
+  //
+  // Reserved 8 bytes preceding IDT to store EFI_PEI_SERVICES**, since 
+ IDT base  // address should be 8-byte alignment.
+  // Note: For IA32, only the 4 bytes immediately preceding IDT is used 
+ to store  // EFI_PEI_SERVICES**  //
+  UINT64            PeiService;
   UINT64            IdtTable[FixedPcdGet8 (PcdFspMaxInterruptSupported)];
 } SEC_IDT_TABLE;
 
--
2.13.3.windows.1

Re: [edk2-devel] [PATCH] IntelFsp2Pkg: PeiService pointer not reset to 0 in SecMain

[Zeng, Star]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Chiu, Chasel
> Sent: Thursday, July 25, 2019 11:51 PM
> To: devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg: PeiService pointer not reset to 0
> in SecMain
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2018
> 
> Current FSP SEC_IDT_TABLE structure is not natural aligned:
>   typedef struct _SEC_IDT_TABLE {
>     EFI_PEI_SERVICES  *PeiService;
>     UINT64            IdtTable[];
>   } SEC_IDT_TABLE;
> Compiler will insert DWORD padding between 2 elements and
> GetPeiServicesTablePointer() in early phase then always returns padding
> data from stack, which was not rest to 0 in SecMain.

'rest' -> 'reset'

> 
> Solution is to align FSP SEC_IDT_TABLE structure to UefiCpuPkg to have
> UINT64 as PeiService field and rest it to 0.

'rest' to 'reset'

Please correct the typo 'rest' to 'reset' in the commit message.
After that, Reviewed-by: Star Zeng <star.zeng@intel.com>.

Thanks,
Star

> 
> Test: Verified on internal platform and booting successfully
>       with FSP API mode.
> 
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> ---
>  IntelFsp2Pkg/FspSecCore/SecMain.c |  2 +-
> IntelFsp2Pkg/FspSecCore/SecMain.h | 10 ++++++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c
> b/IntelFsp2Pkg/FspSecCore/SecMain.c
> index a63d1336e4..7169afc6c7 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
> @@ -114,7 +114,7 @@ SecStartup (
>    // |                   |
>    // |                   |
>    // |-------------------|---->  TempRamBase
> -  IdtTableInStack.PeiService  = NULL;
> +  IdtTableInStack.PeiService = 0;
>    AsmReadIdtr (&IdtDescriptor);
>    if (IdtDescriptor.Base == 0) {
>      ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h
> b/IntelFsp2Pkg/FspSecCore/SecMain.h
> index 6fb16febab..af7f387960 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2019, Intel Corporation. All rights
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -29,7 +29,13 @@ typedef VOID (*PEI_CORE_ENTRY) ( \  );
> 
>  typedef struct _SEC_IDT_TABLE {
> -  EFI_PEI_SERVICES  *PeiService;
> +  //
> +  // Reserved 8 bytes preceding IDT to store EFI_PEI_SERVICES**, since
> + IDT base  // address should be 8-byte alignment.
> +  // Note: For IA32, only the 4 bytes immediately preceding IDT is used
> + to store  // EFI_PEI_SERVICES**  //
> +  UINT64            PeiService;
>    UINT64            IdtTable[FixedPcdGet8 (PcdFspMaxInterruptSupported)];
>  } SEC_IDT_TABLE;
> 
> --
> 2.13.3.windows.1
> 
> 
>