Re: [PATCH v2] IntelFsp2Pkg: FSP should not override IDT

["Chiu, Chasel" <chasel.chiu@intel.com>]

Hello,

Please see my reply below inline.

Thanks!
Chasel


-----Original Message-----
From: afish@apple.com [mailto:afish@apple.com] 
Sent: Tuesday, October 23, 2018 6:29 PM
To: Chiu, Chasel <chasel.chiu@intel.com>
Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH v2] IntelFsp2Pkg: FSP should not override IDT



> On Oct 23, 2018, at 2:33 AM, Chasel, Chiu <chasel.chiu@intel.com> wrote:
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1265
> 
> FSP should not override IDT table when it is initialized by boot 
> loader. IDT should be re-initialized in FSP only when it is invalid.
> To mitigate temporary memory usage a PCD PcdFspMaxInterruptSupported 
> created for platform to decide how many interrupts the FSP IDT table 
> can support.
> 
> Test: Verified on internal platform and boots successfully.
> 
> Cc: Jiewen Yao <Jiewen.yao@intel.com>
> Cc: Desimone Nathaniel L <nathaniel.l.desimone@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> ---
> IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf |  1 +
> IntelFsp2Pkg/FspSecCore/SecMain.c       | 24 +++++++++++++++++++-----
> IntelFsp2Pkg/FspSecCore/SecMain.h       |  6 ++----
> IntelFsp2Pkg/IntelFsp2Pkg.dec           |  4 ++++
> 4 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf 
> b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> index c61af10b8a..dafe6f5993 100644
> --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> @@ -62,6 +62,7 @@
>   gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize              ## CONSUMES
>   gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize           ## CONSUMES
>   gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage         ## CONSUMES
> +  gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported      ## CONSUMES
> 
> [Ppis]
>   gEfiTemporaryRamSupportPpiGuid                              ## PRODUCES
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c 
> b/IntelFsp2Pkg/FspSecCore/SecMain.c
> index 37fd4dfdeb..ddbfc4fcdf 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
> @@ -70,6 +70,7 @@ SecStartup (
>   UINT32                      Index;
>   FSP_GLOBAL_DATA             PeiFspData;
>   UINT64                      ExceptionHandler;
> +  UINTN                       IdtSize;
> 
>   //
>   // Process all libraries constructor function linked to SecCore.
> @@ -98,13 +99,26 @@ SecStartup (
>   // |                   |
>   // |-------------------|---->  TempRamBase
>   IdtTableInStack.PeiService  = NULL;
> -  ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> -  for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
> -    CopyMem ((VOID*)&IdtTableInStack.IdtTable[Index], (VOID*)&ExceptionHandler, sizeof (UINT64));
> +  AsmReadIdtr (&IdtDescriptor);
> +  if ((IdtDescriptor.Base == 0) && (IdtDescriptor.Limit == 0xFFFF)) {

Are these architectural value at reset?

Thanks,

Andrew Fish

Chasel: Yes, these are default values from reset.

> +    ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> +    for (Index = 0; Index < FixedPcdGet8(PcdFspMaxInterruptSupported); Index ++) {
> +      CopyMem ((VOID*)&IdtTableInStack.IdtTable[Index], (VOID*)&ExceptionHandler, sizeof (UINT64));
> +    }
> +    IdtSize = sizeof (IdtTableInStack.IdtTable);  } else {
> +    if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
> +      //
> +      // ERROR: IDT table size from boot loader is larger than FSP can support, DeadLoop here!
> +      //
> +      CpuDeadLoop();
> +    } else {
> +      IdtSize = IdtDescriptor.Limit + 1;
> +    }
> +    CopyMem ((VOID *) (UINTN) &IdtTableInStack.IdtTable, (VOID *) 
> + IdtDescriptor.Base, IdtSize);
>   }
> -
>   IdtDescriptor.Base  = (UINTN) &IdtTableInStack.IdtTable;
> -  IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 
> 1);
> +  IdtDescriptor.Limit = (UINT16)(IdtSize - 1);
> 
>   AsmWriteIdtr (&IdtDescriptor);
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h 
> b/IntelFsp2Pkg/FspSecCore/SecMain.h
> index 291bc5ca5c..19ac2fbfc1 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
> @@ -1,6 +1,6 @@
> /** @file
> 
> -  Copyright (c) 2014 - 2016, Intel Corporation. All rights 
> reserved.<BR>
> +  Copyright (c) 2014 - 2018, Intel Corporation. All rights 
> + reserved.<BR>
>   This program and the accompanying materials
>   are licensed and made available under the terms and conditions of the BSD License
>   which accompanies this distribution.  The full text of the license 
> may be found at @@ -29,8 +29,6 @@ #include <Library/FspCommonLib.h> 
> #include <FspEas.h>
> 
> -#define SEC_IDT_ENTRY_COUNT    34
> -
> typedef VOID (*PEI_CORE_ENTRY) ( \
>   IN CONST  EFI_SEC_PEI_HAND_OFF    *SecCoreData, \
>   IN CONST  EFI_PEI_PPI_DESCRIPTOR  *PpiList \ @@ -38,7 +36,7 @@ 
> typedef VOID (*PEI_CORE_ENTRY) ( \
> 
> typedef struct _SEC_IDT_TABLE {
>   EFI_PEI_SERVICES  *PeiService;
> -  UINT64            IdtTable[SEC_IDT_ENTRY_COUNT];
> +  UINT64            IdtTable[FixedPcdGet8 (PcdFspMaxInterruptSupported)];
> } SEC_IDT_TABLE;
> 
> /**
> diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec 
> b/IntelFsp2Pkg/IntelFsp2Pkg.dec index 5b037d65e2..50496241da 100644
> --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec
> +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec
> @@ -86,6 +86,10 @@
>   # x % of FSP temporary memory will be used for heap
>   # (100 - x) % of FSP temporary memory will be used for stack
>   gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage    |        50| UINT8|0x10000004
> +  #
> +  # Maximal Interrupt supported in IDT table.
> +  #
> +  gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported |        34| UINT8|0x10000005
> 
> [PcdsFixedAtBuild,PcdsDynamic,PcdsDynamicEx]
>   gIntelFsp2PkgTokenSpaceGuid.PcdFspReservedMemoryLength 
> |0x00100000|UINT32|0x46530000
> --
> 2.13.3.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel