Hi Jiewen,
Thanks for the good catch!
Since some platforms might have smaller temporary memory that cannot support
255 interrupt, could we define a FixedAtBuild PCD so platform decides maximum
supported interrupt?
If FSP found boot loader IDT table too large, it will "deadloop" to let boot
loader know that it cannot support.
Thanks!
Chasel
-----Original Message-----
From: Yao, Jiewen
Sent: Tuesday, October 23, 2018 2:09 PM
To: Chiu, Chasel <chasel.c...@intel.com>; edk2-devel@lists.01.org
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>
Subject: RE: [PATCH] IntelFsp2Pkg: FSP should not override IDT
Hi Chasel
Good idea to reserve bootloader IDT.
I have a little concern on the code below. It unconditionally limits the boot
loader IDT to our IdtTableInStack.
#define SEC_IDT_ENTRY_COUNT 34
typedef struct _SEC_IDT_TABLE {
EFI_PEI_SERVICES *PeiService;
UINT64 IdtTable[SEC_IDT_ENTRY_COUNT];
} SEC_IDT_TABLE;
Is that right assumption that 34 entries is enough?
Can we enlarge the IdtTableInStack to hold or bootloader IDT?
Or can we enlarge IdtTableInStack to hold all 0x100 entries, and use ASSERT for
that?
Silence failure seems not the best choice.
> + // Get minimum size
> + if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
> + IdtSize = sizeof (IdtTableInStack.IdtTable);
> + } else {
Thank you
Yao Jiewen
> -----Original Message-----
> From: Chiu, Chasel
> Sent: Friday, October 19, 2018 5:43 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen....@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desim...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>
> Subject: [PATCH] IntelFsp2Pkg: FSP should not override IDT
>
> 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.
>
> Test: Verified on internal platform and boots successfully.
>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Desimone Nathaniel L <nathaniel.l.desim...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu <chasel.c...@intel.com>
> ---
> IntelFsp2Pkg/FspSecCore/SecMain.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c
> b/IntelFsp2Pkg/FspSecCore/SecMain.c
> index 37fd4dfdeb..5912221204 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,24 @@ 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)) {
> + ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> + for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
> + CopyMem ((VOID*)&IdtTableInStack.IdtTable[Index],
> (VOID*)&ExceptionHandler, sizeof (UINT64));
> + }
> + IdtSize = sizeof (IdtTableInStack.IdtTable); } else {
> + // Get minimum size
> + if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
> + IdtSize = sizeof (IdtTableInStack.IdtTable);
> + } 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);
>
> --
> 2.13.3.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
