* [PATCH] IntelFsp2Pkg: FSP should not override IDT
@ 2018-10-19 9:42 Chasel, Chiu
2018-10-23 6:08 ` Yao, Jiewen
0 siblings, 1 reply; 3+ messages in thread
From: Chasel, Chiu @ 2018-10-19 9:42 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Desimone Nathaniel L, Chasel Chiu
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.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/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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] IntelFsp2Pkg: FSP should not override IDT
2018-10-19 9:42 [PATCH] IntelFsp2Pkg: FSP should not override IDT Chasel, Chiu
@ 2018-10-23 6:08 ` Yao, Jiewen
2018-10-23 7:04 ` Chiu, Chasel
0 siblings, 1 reply; 3+ messages in thread
From: Yao, Jiewen @ 2018-10-23 6:08 UTC (permalink / raw)
To: Chiu, Chasel, edk2-devel@lists.01.org
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.yao@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Chiu, Chasel <chasel.chiu@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.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/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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] IntelFsp2Pkg: FSP should not override IDT
2018-10-23 6:08 ` Yao, Jiewen
@ 2018-10-23 7:04 ` Chiu, Chasel
0 siblings, 0 replies; 3+ messages in thread
From: Chiu, Chasel @ 2018-10-23 7:04 UTC (permalink / raw)
To: Yao, Jiewen; +Cc: Desimone, Nathaniel L, edk2-devel@lists.01.org
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.chiu@intel.com>; edk2-devel@lists.01.org
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@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.yao@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Chiu, Chasel <chasel.chiu@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.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/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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-23 7:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-19 9:42 [PATCH] IntelFsp2Pkg: FSP should not override IDT Chasel, Chiu
2018-10-23 6:08 ` Yao, Jiewen
2018-10-23 7:04 ` Chiu, Chasel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox