Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Yao, Jiewen]

Thanks Jian.

I just talked to some other person, who think it is valuable to have stack 
guard in PEI as well, because they have seen stack overflow issue in PEI. I do 
think we need consider that, although it is not implemented in this patch 
series.

My thought for API is that the API design for PEI/DXE/SMM should be consistent.
When people look at the MdeModulePkg\Include\Library\CpuExceptionHandlerLib.h, 
he can know clearly on which API should be called and what is done in each API.

If we call InitializeCpuExceptionStackSwitchHandlers() inside 
InitializeCpuExceptionHandlers(), we should document this in .H file and make 
PEI/DXE/SMM version all implement in this way.
If we did one way for DXE, the other way for PEI, and another way for SMM, it 
might bring confusing.


Thank you
Yao Jiewen

> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, November 28, 2017 9:39 AM
> To: Yao, Jiewen <jiewen@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
> 
> Sorry, 1.3 should be for your 1.4. I just noticed I missed your 1.3 comment.
> Here's my opinion for it:
> 
> Current changes (use global variables to reserve resources) to
> InitializeCpuExceptionHandlers() is for DXE only. For PEI, if we really need 
> to
> worry
> about the stack overflow and stack switch, we can have a different way to do 
> it.
> For
> example, we don't need to call InitializeCpuExceptionStackSwitchHandlers()
> inside
> InitializeCpuExceptionHandlers(). We could call it whenever we can reserve
> memory blocks
> and pass them to InitializeCpuExceptionStackSwitchHandlers() via parameter. I
> think this
> is one of reason we have a separate method to initialize the exception 
> handlers
> for the
> sake of stack switch.
> 
> Calling InitializeCpuExceptionStackSwitchHandlers() inside
> InitializeCpuExceptionHandlers()
> is just for the consideration of backward compatibility. Because this new API 
> is
> just
> implemented for IA32 processor at this time, calling it in DXE core will 
> break the
> build of
> other type of processors. This is another reason we have a separate method to
> do
> exception handler initialization in a different way.
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Wang,
> > Jian J
> > Sent: Thursday, November 23, 2017 2:44 PM
> > To: Yao, Jiewen <jiewen@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> > <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> > Subject: Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add
> > stack switch support
> >
> > 1.1) Got your point. I'll add dummy function in this patch.
> > 1.2) Yep, we're on the same page.
> > 1.3) Here's my opinion:
> >
> > Actually almost all MP code has such assumption: any AP configuration will
> copy
> >  from BSP. If we allow AP to call InitializeCpuExceptionHandlers(), we have 
> > to
> do
> > a lot
> > of other changes than just updating InitializeCpuExceptionHandlers(). If 
> > so, it
> > may
> > be premature to figure out a solution at this patch.
> >
> > In addition, CpuDxe actually calls InitializeCpuInterruptHandlers() which 
> > covers
> > the
> > functionalities of InitializeCpuExceptionHandlers() (its settings will be
> > overwritten).
> > If we want AP to initialize interrupt and exception individually, maybe we
> should
> > let AP call InitializeCpuInterruptHandlers() instead.
> >
> > > -Original Message-
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 23, 2017 2:16 PM
> > > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> > > Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric <eric.d...@intel.com>;
> > > Kinney, Michael D <michael.d.kin...@intel.com>
> > > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > > switch support
> > >
> > > Here is my thought for 1)
> > >
> > > 1.1) We must provide the InitializeCpuExceptionStackSwitchHandlers()
> > > implementation in Pei instance and Smm instance.
> > >
> > > The basic requirement is a library instance must provide symbol for 
> > > functions
> > > declared in header file.
> > > It is ok to return unsupported. But we MUST provid

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Wang, Jian J]

Sorry, 1.3 should be for your 1.4. I just noticed I missed your 1.3 comment.
Here's my opinion for it:

Current changes (use global variables to reserve resources) to  
InitializeCpuExceptionHandlers() is for DXE only. For PEI, if we really need to 
worry 
about the stack overflow and stack switch, we can have a different way to do 
it. For
example, we don't need to call InitializeCpuExceptionStackSwitchHandlers() 
inside
InitializeCpuExceptionHandlers(). We could call it whenever we can reserve 
memory blocks
and pass them to InitializeCpuExceptionStackSwitchHandlers() via parameter. I 
think this
is one of reason we have a separate method to initialize the exception handlers 
for the
sake of stack switch.

Calling InitializeCpuExceptionStackSwitchHandlers() inside 
InitializeCpuExceptionHandlers()
is just for the consideration of backward compatibility. Because this new API 
is just
implemented for IA32 processor at this time, calling it in DXE core will break 
the build of
other type of processors. This is another reason we have a separate method to do
exception handler initialization in a different way.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Thursday, November 23, 2017 2:44 PM
> To: Yao, Jiewen <jiewen@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add
> stack switch support
> 
> 1.1) Got your point. I'll add dummy function in this patch.
> 1.2) Yep, we're on the same page.
> 1.3) Here's my opinion:
> 
> Actually almost all MP code has such assumption: any AP configuration will 
> copy
>  from BSP. If we allow AP to call InitializeCpuExceptionHandlers(), we have 
> to do
> a lot
> of other changes than just updating InitializeCpuExceptionHandlers(). If so, 
> it
> may
> be premature to figure out a solution at this patch.
> 
> In addition, CpuDxe actually calls InitializeCpuInterruptHandlers() which 
> covers
> the
> functionalities of InitializeCpuExceptionHandlers() (its settings will be
> overwritten).
> If we want AP to initialize interrupt and exception individually, maybe we 
> should
> let AP call InitializeCpuInterruptHandlers() instead.
> 
> > -Original Message-
> > From: Yao, Jiewen
> > Sent: Thursday, November 23, 2017 2:16 PM
> > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric <eric.d...@intel.com>;
> > Kinney, Michael D <michael.d.kin...@intel.com>
> > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch support
> >
> > Here is my thought for 1)
> >
> > 1.1) We must provide the InitializeCpuExceptionStackSwitchHandlers()
> > implementation in Pei instance and Smm instance.
> >
> > The basic requirement is a library instance must provide symbol for 
> > functions
> > declared in header file.
> > It is ok to return unsupported. But we MUST provide the symbol.
> >
> > 1.2) For SMM, I think our ultimate goal is to remove SMM specific stack 
> > guard,
> > and use the common one. Duplicating code is completely unnecessary, and it 
> > is
> > easy to introduce bug. And unfortunately, we already have bug in existing
> SMM
> > exception handler. -- That is a good reason to remove duplication.
> >
> > Again, it is not necessary to do it in this patch. I am totally OK to do it 
> > in
> another
> > patch.
> >
> > 1.3) For PEI, I do not think we can use current way to allocate stack in 
> > data
> > section, because it might be readonly in pre-mem phase. We must use some
> > other way.
> >
> > 1.4) I believe this patch has a hidden assumption is that:
> > InitializeCpuExceptionHandlers() won't be called by multiple APs.
> > If 2 or more APs call the it at same time, it might be broken because you 
> > use
> > mNewStack for all the callers
> > Is that right?
> >
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -Original Message-
> > > From: Wang, Jian J
> > > Sent: Thursday, November 23, 2017 2:06 PM
> > > To: Yao, Jiewen <jiewen@intel.com>; edk2-devel@lists.01.org
> > > Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric <eric.d...@intel.com>;
> > Kinney,
> > > Michael D <michael.d.kin...@intel.com>
> > > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Wang, Jian J]

Good catch. I’ll add them. Thanks.

From: Fan Jeff [mailto:vanjeff_...@hotmail.com]
Sent: Saturday, November 25, 2017 9:28 PM
To: Wang, Jian J <jian.j.w...@intel.com>; Yao, Jiewen <jiewen@intel.com>; 
edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric 
<eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: 答复: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support


Hi,



About 1.1), I agree with Jiewen’s suggestion. Besides it, we also need to 
provide dummy function of InitializeCpuExceptionStackSwitchHandlers() in NULL 
instance in MdeModulePkg/Library/CpuExceptionHandlerLibNull.

But we need to think about the return status carefully. For example, if return 
EFI_UNSUPPORTED in Pei/SMM instrance, we need to update public header file to 
add this return type.



Jeff




From: edk2-devel 
<edk2-devel-boun...@lists.01.org<mailto:edk2-devel-boun...@lists.01.org>> on 
behalf of Wang, Jian J <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>
Sent: Thursday, November 23, 2017 2:43:44 PM
To: Yao, Jiewen; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Kinney, Michael D; Dong, Eric; Zeng, Star
Subject: Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack 
switch support

1.1) Got your point. I'll add dummy function in this patch.
1.2) Yep, we're on the same page.
1.3) Here's my opinion:

Actually almost all MP code has such assumption: any AP configuration will copy
 from BSP. If we allow AP to call InitializeCpuExceptionHandlers(), we have to 
do a lot
of other changes than just updating InitializeCpuExceptionHandlers(). If so, it 
may
be premature to figure out a solution at this patch.

In addition, CpuDxe actually calls InitializeCpuInterruptHandlers() which 
covers the
functionalities of InitializeCpuExceptionHandlers() (its settings will be 
overwritten).
If we want AP to initialize interrupt and exception individually, maybe we 
should
let AP call InitializeCpuInterruptHandlers() instead.

> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, November 23, 2017 2:16 PM
> To: Wang, Jian J <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; 
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Zeng, Star <star.z...@intel.com<mailto:star.z...@intel.com>>; Dong, Eric 
> <eric.d...@intel.com<mailto:eric.d...@intel.com>>;
> Kinney, Michael D 
> <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
>
> Here is my thought for 1)
>
> 1.1) We must provide the InitializeCpuExceptionStackSwitchHandlers()
> implementation in Pei instance and Smm instance.
>
> The basic requirement is a library instance must provide symbol for functions
> declared in header file.
> It is ok to return unsupported. But we MUST provide the symbol.
>
> 1.2) For SMM, I think our ultimate goal is to remove SMM specific stack guard,
> and use the common one. Duplicating code is completely unnecessary, and it is
> easy to introduce bug. And unfortunately, we already have bug in existing SMM
> exception handler. -- That is a good reason to remove duplication.
>
> Again, it is not necessary to do it in this patch. I am totally OK to do it 
> in another
> patch.
>
> 1.3) For PEI, I do not think we can use current way to allocate stack in data
> section, because it might be readonly in pre-mem phase. We must use some
> other way.
>
> 1.4) I believe this patch has a hidden assumption is that:
> InitializeCpuExceptionHandlers() won't be called by multiple APs.
> If 2 or more APs call the it at same time, it might be broken because you use
> mNewStack for all the callers
> Is that right?
>
>
> Thank you
> Yao Jiewen
>
>
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Thursday, November 23, 2017 2:06 PM
> > To: Yao, Jiewen <jiewen@intel.com<mailto:jiewen@intel.com>>; 
> > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Cc: Zeng, Star <star.z...@intel.com<mailto:star.z...@intel.com>>; Dong, 
> > Eric <eric.d...@intel.com<mailto:eric.d...@intel.com>>;
> Kinney,
> > Michael D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
> > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch support
> >
> >
> >
> > > -Original Message-
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 23, 2017 1:50 PM
> > > To: Wang, Jian J <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com&

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Wang, Jian J]

1) No impact at all.

2)
Page at stack base will be disabled.
If Arch == IA32,
The stack switch for handler of #PF/#DD will be setup for BSP and AP
Else
The handler of #PF/#DD keeps the same
EndIf

If StackOverFlow,
If Arch == IA32,
#PF is triggered and its handler is called with KnownGoodStack.
CPU context is dumped successfully.
Else
#PF handler is triggered but its handler is called with corrupted 
stack.
CPU context cannot be dumped.
EndIf
EndIf

3) 
If Cpu == BSP,
Only Exceptions will be handled. Interrupts will not.
Else,
No exceptions and interrupts will be handled.
EndIf

4) 
Page at stack base will be disabled.
If Cpu == BSP,
Only Exceptions will be handled. Interrupts will not.

If Arch == IA32,
The stack switch for handler of #PF/#DD will be setup for BSP
Else
The handler of #PF/#DD keeps the same
EndIf

If StackOverFlow,
If Arch == IA32,
#PF is triggered and its handler is called with KnownGoodStack.
CPU context is dumped successfully.
Else
#PF handler is triggered but its handler is called with 
corrupted stack.
CPU context cannot be dumped.
EndIf
EndIf
Else,
No exceptions and interrupts will be handled.
EndIf

> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, November 23, 2017 2:25 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
> 
> I am ok to keep FALSE by default. But I still suggest we test existing UEFI OS
> behavior.
> 
> Please help me understand below condition, if we do not change a platform
> specific CPU driver:
> 1) If PcdCpuStackGuard is FALSE, and CPU driver is still consuming existing 
> API in
> ExceptionLib. Is there any impact?
> 2) If PcdCpuStackGuard is TRUE, and CPU driver is still consuming existing 
> API in
> ExceptionLib. Is there any impact?
> 3) If PcdCpuStackGuard is FALSE, and CPU driver is not consuming existing API 
> in
> ExceptionLib. Is there any impact?
> 4) If PcdCpuStackGuard is TRUE, and CPU driver is not consuming existing API 
> in
> ExceptionLib. Is there any impact?
> 
> 
> Thank you
> Yao Jiewen
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Thursday, November 23, 2017 2:09 PM
> > To: Yao, Jiewen <jiewen@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> > <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch support
> >
> > If PcdCpuStackGuard is not enabled, there's no impact. If it's enabled, the 
> > only
> > issue is that the exception dump cannot be done but no other impact. From
> this
> > point of view, maybe PcdCpuStackGuard should be FALSE by default.
> >
> > > -Original Message-
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 23, 2017 1:59 PM
> > > To: Yao, Jiewen <jiewen@intel.com>; Wang, Jian J
> > <jian.j.w...@intel.com>;
> > > edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> > > <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> > > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > > switch support
> > >
> > > One more question:
> > > I notice not all platforms are using the CpuDxe in UefiCpuPkg.
> > > If so, is there any impact to the platform whose CPU driver does not have
> such
> > > InitializeCpuExceptionStackSwitchHandlers() call?
> > > Have you tested that condition?
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Yao,
> > > > Jiewen
> > > > Sent: Thursday, November 23, 2017 1:50 PM
> > > > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> > > > <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> > > > Subject: Re: [edk2

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Wang, Jian J]

1.1) Got your point. I'll add dummy function in this patch.
1.2) Yep, we're on the same page.
1.3) Here's my opinion:

Actually almost all MP code has such assumption: any AP configuration will copy
 from BSP. If we allow AP to call InitializeCpuExceptionHandlers(), we have to 
do a lot
of other changes than just updating InitializeCpuExceptionHandlers(). If so, it 
may
be premature to figure out a solution at this patch.

In addition, CpuDxe actually calls InitializeCpuInterruptHandlers() which 
covers the
functionalities of InitializeCpuExceptionHandlers() (its settings will be 
overwritten).
If we want AP to initialize interrupt and exception individually, maybe we 
should
let AP call InitializeCpuInterruptHandlers() instead.

> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, November 23, 2017 2:16 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric ;
> Kinney, Michael D 
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
> 
> Here is my thought for 1)
> 
> 1.1) We must provide the InitializeCpuExceptionStackSwitchHandlers()
> implementation in Pei instance and Smm instance.
> 
> The basic requirement is a library instance must provide symbol for functions
> declared in header file.
> It is ok to return unsupported. But we MUST provide the symbol.
> 
> 1.2) For SMM, I think our ultimate goal is to remove SMM specific stack guard,
> and use the common one. Duplicating code is completely unnecessary, and it is
> easy to introduce bug. And unfortunately, we already have bug in existing SMM
> exception handler. -- That is a good reason to remove duplication.
> 
> Again, it is not necessary to do it in this patch. I am totally OK to do it 
> in another
> patch.
> 
> 1.3) For PEI, I do not think we can use current way to allocate stack in data
> section, because it might be readonly in pre-mem phase. We must use some
> other way.
> 
> 1.4) I believe this patch has a hidden assumption is that:
> InitializeCpuExceptionHandlers() won't be called by multiple APs.
> If 2 or more APs call the it at same time, it might be broken because you use
> mNewStack for all the callers
> Is that right?
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Thursday, November 23, 2017 2:06 PM
> > To: Yao, Jiewen ; edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Dong, Eric ;
> Kinney,
> > Michael D 
> > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch support
> >
> >
> >
> > > -Original Message-
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 23, 2017 1:50 PM
> > > To: Wang, Jian J ; edk2-devel@lists.01.org
> > > Cc: Zeng, Star ; Dong, Eric ;
> > > Kinney, Michael D 
> > > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > > switch support
> > >
> > > Some thought:
> > >
> > > 1) I found InitializeCpuExceptionStackSwitchHandlers() is only implemented
> in
> > > DxeException.c.
> > > What about Pei/Smm instance?
> > >
> > > I think it is OK to not implement it at this moment. But we need make 
> > > sure no
> > > architecture issue if we want to enable it some time later.
> > >
> > Like what we discussed before, this series of patch is for Stack Guard 
> > feature
> > which
> > is only available for DXE (because Stack Guard needs paging to work). Stack
> > switch
> > is enabled for the sake of Stack Guard feature. So I think it's enough to
> > implement
> > it in DxeException.c. In addition, SMM has its own implementation of stack
> guard
> > and stack switch. It's not necessary to do it again.
> >
> > I agree with you that we should merge those common code but I think we
> should
> > do
> > it in a separate patch series since it's not Stack Guard relevant. And I've
> removed
> > all architecture issues I can think of. Current stack switch initialization 
> > should
> work
> > for both PEI and SMM as well.
> >
> > > 2) #define IA32_GDT_TYPE_TSS   0x9
> > > This is generic, can we move to BaseLib.h?
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -Original Message-
> > > > From: Wang, Jian J
> > > > Sent: Wednesday, November 22, 2017 4:46 PM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Zeng, Star ; Dong, Eric ;
> Yao,
> > > > Jiewen ; Kinney, Michael D
> > > > 
> > > > Subject: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > > switch
> > > > support
> > > >
> > > > > v2:
> > > > >a. Move common TSS structure and API definitions to BaseLib.h
> > > > >b. Add EXCEPTION_STACK_SWITCH_DATA to convery data used 

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Yao, Jiewen]

I am ok to keep FALSE by default. But I still suggest we test existing UEFI OS 
behavior.

Please help me understand below condition, if we do not change a platform 
specific CPU driver:
1) If PcdCpuStackGuard is FALSE, and CPU driver is still consuming existing API 
in ExceptionLib. Is there any impact?
2) If PcdCpuStackGuard is TRUE, and CPU driver is still consuming existing API 
in ExceptionLib. Is there any impact?
3) If PcdCpuStackGuard is FALSE, and CPU driver is not consuming existing API 
in ExceptionLib. Is there any impact?
4) If PcdCpuStackGuard is TRUE, and CPU driver is not consuming existing API in 
ExceptionLib. Is there any impact?


Thank you
Yao Jiewen

> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, November 23, 2017 2:09 PM
> To: Yao, Jiewen <jiewen@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
> 
> If PcdCpuStackGuard is not enabled, there's no impact. If it's enabled, the 
> only
> issue is that the exception dump cannot be done but no other impact. From this
> point of view, maybe PcdCpuStackGuard should be FALSE by default.
> 
> > -Original Message-
> > From: Yao, Jiewen
> > Sent: Thursday, November 23, 2017 1:59 PM
> > To: Yao, Jiewen <jiewen@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>;
> > edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> > <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch support
> >
> > One more question:
> > I notice not all platforms are using the CpuDxe in UefiCpuPkg.
> > If so, is there any impact to the platform whose CPU driver does not have 
> > such
> > InitializeCpuExceptionStackSwitchHandlers() call?
> > Have you tested that condition?
> >
> > Thank you
> > Yao Jiewen
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Yao,
> > > Jiewen
> > > Sent: Thursday, November 23, 2017 1:50 PM
> > > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> > > <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> > > Subject: Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add
> > > stack switch support
> > >
> > > Some thought:
> > >
> > > 1) I found InitializeCpuExceptionStackSwitchHandlers() is only 
> > > implemented in
> > > DxeException.c.
> > > What about Pei/Smm instance?
> > >
> > > I think it is OK to not implement it at this moment. But we need make 
> > > sure no
> > > architecture issue if we want to enable it some time later.
> > >
> > > 2) #define IA32_GDT_TYPE_TSS   0x9
> > > This is generic, can we move to BaseLib.h?
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -Original Message-
> > > > From: Wang, Jian J
> > > > Sent: Wednesday, November 22, 2017 4:46 PM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric <eric.d...@intel.com>;
> > Yao,
> > > > Jiewen <jiewen@intel.com>; Kinney, Michael D
> > > > <michael.d.kin...@intel.com>
> > > > Subject: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch
> > > > support
> > > >
> > > > > v2:
> > > > >a. Move common TSS structure and API definitions to BaseLib.h
> > > > >b. Add EXCEPTION_STACK_SWITCH_DATA to convery data used to
> setup
> > > > stack
> > > > >   switch. This can avoid allocating memory for it in this library.
> > > > >c. Add globals to reserve memory for stack switch initialized in 
> > > > > early
> > > > >   phase of DXE core.
> > > > >d. Remove the filter code used to exclude boot modes which doesn't
> > > > support
> > > > >   memory allocation because those memory can passed in by
> > > parameter
> > > > now.
> > > > >e. Remove the nasm macro to define exception handl

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Yao, Jiewen]

Here is my thought for 1)

1.1) We must provide the InitializeCpuExceptionStackSwitchHandlers() 
implementation in Pei instance and Smm instance.

The basic requirement is a library instance must provide symbol for functions 
declared in header file.
It is ok to return unsupported. But we MUST provide the symbol.

1.2) For SMM, I think our ultimate goal is to remove SMM specific stack guard, 
and use the common one. Duplicating code is completely unnecessary, and it is 
easy to introduce bug. And unfortunately, we already have bug in existing SMM 
exception handler. -- That is a good reason to remove duplication.

Again, it is not necessary to do it in this patch. I am totally OK to do it in 
another patch.

1.3) For PEI, I do not think we can use current way to allocate stack in data 
section, because it might be readonly in pre-mem phase. We must use some other 
way.

1.4) I believe this patch has a hidden assumption is that: 
InitializeCpuExceptionHandlers() won't be called by multiple APs.
If 2 or more APs call the it at same time, it might be broken because you use 
mNewStack for all the callers
Is that right?


Thank you
Yao Jiewen


> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, November 23, 2017 2:06 PM
> To: Yao, Jiewen ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric ; 
> Kinney,
> Michael D 
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
> 
> 
> 
> > -Original Message-
> > From: Yao, Jiewen
> > Sent: Thursday, November 23, 2017 1:50 PM
> > To: Wang, Jian J ; edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Dong, Eric ;
> > Kinney, Michael D 
> > Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch support
> >
> > Some thought:
> >
> > 1) I found InitializeCpuExceptionStackSwitchHandlers() is only implemented 
> > in
> > DxeException.c.
> > What about Pei/Smm instance?
> >
> > I think it is OK to not implement it at this moment. But we need make sure 
> > no
> > architecture issue if we want to enable it some time later.
> >
> Like what we discussed before, this series of patch is for Stack Guard feature
> which
> is only available for DXE (because Stack Guard needs paging to work). Stack
> switch
> is enabled for the sake of Stack Guard feature. So I think it's enough to
> implement
> it in DxeException.c. In addition, SMM has its own implementation of stack 
> guard
> and stack switch. It's not necessary to do it again.
> 
> I agree with you that we should merge those common code but I think we should
> do
> it in a separate patch series since it's not Stack Guard relevant. And I've 
> removed
> all architecture issues I can think of. Current stack switch initialization 
> should work
> for both PEI and SMM as well.
> 
> > 2) #define IA32_GDT_TYPE_TSS   0x9
> > This is generic, can we move to BaseLib.h?
> >
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -Original Message-
> > > From: Wang, Jian J
> > > Sent: Wednesday, November 22, 2017 4:46 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Zeng, Star ; Dong, Eric ; 
> > > Yao,
> > > Jiewen ; Kinney, Michael D
> > > 
> > > Subject: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> > switch
> > > support
> > >
> > > > v2:
> > > >a. Move common TSS structure and API definitions to BaseLib.h
> > > >b. Add EXCEPTION_STACK_SWITCH_DATA to convery data used to
> setup
> > > stack
> > > >   switch. This can avoid allocating memory for it in this library.
> > > >c. Add globals to reserve memory for stack switch initialized in 
> > > > early
> > > >   phase of DXE core.
> > > >d. Remove the filter code used to exclude boot modes which doesn't
> > > support
> > > >   memory allocation because those memory can passed in by
> parameter
> > > now.
> > > >e. Remove the nasm macro to define exception handler one by one
> and
> > > add a
> > > >   function to return the start address of each handler.
> > >
> > > If Stack Guard is enabled and there's really a stack overflow happened 
> > > during
> > > boot, a Page Fault exception will be triggered. Because the stack is out 
> > > of
> > > usage, the exception handler, which shares the stack with normal UEFI 
> > > driver,
> > > cannot be executed and cannot dump the processor information.
> > >
> > > Without those information, it's very difficult for the BIOS developers 
> > > locate
> > > the root cause of stack overflow. And without a workable stack, the
> developer
> > > cannot event use single step to debug the UEFI driver with JTAG debugger.
> > >
> > > In order to make sure the exception handler to execute normally after 
> > > stack
> > > overflow. We need 

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Wang, Jian J]

If PcdCpuStackGuard is not enabled, there's no impact. If it's enabled, the only
issue is that the exception dump cannot be done but no other impact. From this
point of view, maybe PcdCpuStackGuard should be FALSE by default.

> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, November 23, 2017 1:59 PM
> To: Yao, Jiewen <jiewen@intel.com>; Wang, Jian J <jian.j.w...@intel.com>;
> edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
> 
> One more question:
> I notice not all platforms are using the CpuDxe in UefiCpuPkg.
> If so, is there any impact to the platform whose CPU driver does not have such
> InitializeCpuExceptionStackSwitchHandlers() call?
> Have you tested that condition?
> 
> Thank you
> Yao Jiewen
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao,
> > Jiewen
> > Sent: Thursday, November 23, 2017 1:50 PM
> > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> > <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> > Subject: Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add
> > stack switch support
> >
> > Some thought:
> >
> > 1) I found InitializeCpuExceptionStackSwitchHandlers() is only implemented 
> > in
> > DxeException.c.
> > What about Pei/Smm instance?
> >
> > I think it is OK to not implement it at this moment. But we need make sure 
> > no
> > architecture issue if we want to enable it some time later.
> >
> > 2) #define IA32_GDT_TYPE_TSS   0x9
> > This is generic, can we move to BaseLib.h?
> >
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -Original Message-
> > > From: Wang, Jian J
> > > Sent: Wednesday, November 22, 2017 4:46 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric <eric.d...@intel.com>;
> Yao,
> > > Jiewen <jiewen@intel.com>; Kinney, Michael D
> > > <michael.d.kin...@intel.com>
> > > Subject: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch
> > > support
> > >
> > > > v2:
> > > >a. Move common TSS structure and API definitions to BaseLib.h
> > > >b. Add EXCEPTION_STACK_SWITCH_DATA to convery data used to setup
> > > stack
> > > >   switch. This can avoid allocating memory for it in this library.
> > > >c. Add globals to reserve memory for stack switch initialized in 
> > > > early
> > > >   phase of DXE core.
> > > >d. Remove the filter code used to exclude boot modes which doesn't
> > > support
> > > >   memory allocation because those memory can passed in by
> > parameter
> > > now.
> > > >e. Remove the nasm macro to define exception handler one by one and
> > > add a
> > > >   function to return the start address of each handler.
> > >
> > > If Stack Guard is enabled and there's really a stack overflow happened 
> > > during
> > > boot, a Page Fault exception will be triggered. Because the stack is out 
> > > of
> > > usage, the exception handler, which shares the stack with normal UEFI 
> > > driver,
> > > cannot be executed and cannot dump the processor information.
> > >
> > > Without those information, it's very difficult for the BIOS developers 
> > > locate
> > > the root cause of stack overflow. And without a workable stack, the
> developer
> > > cannot event use single step to debug the UEFI driver with JTAG debugger.
> > >
> > > In order to make sure the exception handler to execute normally after 
> > > stack
> > > overflow. We need separate stacks for exception handlers in case of
> unusable
> > > stack.
> > >
> > > IA processor allows to switch to a new stack during handling interrupt and
> > > exception. But X64 and IA32 provides different ways to make it. X64 
> > > provides
> > > interrupt stack table (IST) to allow maximum 7 different exceptions to 
> > > have
> > > new stack for its handler. IA32 doesn't have IST mechanism and can only 
> > > use
> > > task 

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Wang, Jian J]

> -Original Message-
> From: Yao, Jiewen
> Sent: Thursday, November 23, 2017 1:50 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric ;
> Kinney, Michael D 
> Subject: RE: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
> 
> Some thought:
> 
> 1) I found InitializeCpuExceptionStackSwitchHandlers() is only implemented in
> DxeException.c.
> What about Pei/Smm instance?
> 
> I think it is OK to not implement it at this moment. But we need make sure no
> architecture issue if we want to enable it some time later.
> 
Like what we discussed before, this series of patch is for Stack Guard feature 
which
is only available for DXE (because Stack Guard needs paging to work). Stack 
switch
is enabled for the sake of Stack Guard feature. So I think it's enough to 
implement
it in DxeException.c. In addition, SMM has its own implementation of stack guard
and stack switch. It's not necessary to do it again.

I agree with you that we should merge those common code but I think we should do
it in a separate patch series since it's not Stack Guard relevant. And I've 
removed
all architecture issues I can think of. Current stack switch initialization 
should work
for both PEI and SMM as well.

> 2) #define IA32_GDT_TYPE_TSS   0x9
> This is generic, can we move to BaseLib.h?
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Wednesday, November 22, 2017 4:46 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Dong, Eric ; Yao,
> > Jiewen ; Kinney, Michael D
> > 
> > Subject: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch
> > support
> >
> > > v2:
> > >a. Move common TSS structure and API definitions to BaseLib.h
> > >b. Add EXCEPTION_STACK_SWITCH_DATA to convery data used to setup
> > stack
> > >   switch. This can avoid allocating memory for it in this library.
> > >c. Add globals to reserve memory for stack switch initialized in early
> > >   phase of DXE core.
> > >d. Remove the filter code used to exclude boot modes which doesn't
> > support
> > >   memory allocation because those memory can passed in by parameter
> > now.
> > >e. Remove the nasm macro to define exception handler one by one and
> > add a
> > >   function to return the start address of each handler.
> >
> > If Stack Guard is enabled and there's really a stack overflow happened 
> > during
> > boot, a Page Fault exception will be triggered. Because the stack is out of
> > usage, the exception handler, which shares the stack with normal UEFI 
> > driver,
> > cannot be executed and cannot dump the processor information.
> >
> > Without those information, it's very difficult for the BIOS developers 
> > locate
> > the root cause of stack overflow. And without a workable stack, the 
> > developer
> > cannot event use single step to debug the UEFI driver with JTAG debugger.
> >
> > In order to make sure the exception handler to execute normally after stack
> > overflow. We need separate stacks for exception handlers in case of unusable
> > stack.
> >
> > IA processor allows to switch to a new stack during handling interrupt and
> > exception. But X64 and IA32 provides different ways to make it. X64 provides
> > interrupt stack table (IST) to allow maximum 7 different exceptions to have
> > new stack for its handler. IA32 doesn't have IST mechanism and can only use
> > task gate to do it since task switch allows to load a new stack through its
> > task-state segment (TSS).
> >
> > Cc: Star Zeng 
> > Cc: Eric Dong 
> > Cc: Jiewen Yao 
> > Cc: Michael Kinney 
> > Suggested-by: Ayellet Wolman 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  .../CpuExceptionHandlerLib/CpuExceptionCommon.h|  50 +++
> >  .../DxeCpuExceptionHandlerLib.inf  |   6 +
> >  .../Library/CpuExceptionHandlerLib/DxeException.c  |  53 ++-
> >  .../Ia32/ArchExceptionHandler.c| 167 +
> >  .../Ia32/ArchInterruptDefs.h   |   8 +
> >  .../Ia32/ExceptionTssEntryAsm.nasm | 398
> > +
> >  .../PeiCpuExceptionHandlerLib.inf  |   1 +
> >  .../SecPeiCpuExceptionHandlerLib.inf   |   1 +
> >  .../SmmCpuExceptionHandlerLib.inf  |   1 +
> >  .../X64/ArchExceptionHandler.c | 133 +++
> >  .../CpuExceptionHandlerLib/X64/ArchInterruptDefs.h |   3 +
> >  11 files changed, 820 insertions(+), 1 deletion(-)
> >  create mode 100644
> >
> 

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Yao, Jiewen]

One more question:
I notice not all platforms are using the CpuDxe in UefiCpuPkg.
If so, is there any impact to the platform whose CPU driver does not have such 
InitializeCpuExceptionStackSwitchHandlers() call?
Have you tested that condition?

Thank you
Yao Jiewen

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao,
> Jiewen
> Sent: Thursday, November 23, 2017 1:50 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add
> stack switch support
> 
> Some thought:
> 
> 1) I found InitializeCpuExceptionStackSwitchHandlers() is only implemented in
> DxeException.c.
> What about Pei/Smm instance?
> 
> I think it is OK to not implement it at this moment. But we need make sure no
> architecture issue if we want to enable it some time later.
> 
> 2) #define IA32_GDT_TYPE_TSS   0x9
> This is generic, can we move to BaseLib.h?
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Wednesday, November 22, 2017 4:46 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric <eric.d...@intel.com>; Yao,
> > Jiewen <jiewen@intel.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>
> > Subject: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> > support
> >
> > > v2:
> > >a. Move common TSS structure and API definitions to BaseLib.h
> > >b. Add EXCEPTION_STACK_SWITCH_DATA to convery data used to setup
> > stack
> > >   switch. This can avoid allocating memory for it in this library.
> > >c. Add globals to reserve memory for stack switch initialized in early
> > >   phase of DXE core.
> > >d. Remove the filter code used to exclude boot modes which doesn't
> > support
> > >   memory allocation because those memory can passed in by
> parameter
> > now.
> > >e. Remove the nasm macro to define exception handler one by one and
> > add a
> > >   function to return the start address of each handler.
> >
> > If Stack Guard is enabled and there's really a stack overflow happened 
> > during
> > boot, a Page Fault exception will be triggered. Because the stack is out of
> > usage, the exception handler, which shares the stack with normal UEFI 
> > driver,
> > cannot be executed and cannot dump the processor information.
> >
> > Without those information, it's very difficult for the BIOS developers 
> > locate
> > the root cause of stack overflow. And without a workable stack, the 
> > developer
> > cannot event use single step to debug the UEFI driver with JTAG debugger.
> >
> > In order to make sure the exception handler to execute normally after stack
> > overflow. We need separate stacks for exception handlers in case of unusable
> > stack.
> >
> > IA processor allows to switch to a new stack during handling interrupt and
> > exception. But X64 and IA32 provides different ways to make it. X64 provides
> > interrupt stack table (IST) to allow maximum 7 different exceptions to have
> > new stack for its handler. IA32 doesn't have IST mechanism and can only use
> > task gate to do it since task switch allows to load a new stack through its
> > task-state segment (TSS).
> >
> > Cc: Star Zeng <star.z...@intel.com>
> > Cc: Eric Dong <eric.d...@intel.com>
> > Cc: Jiewen Yao <jiewen@intel.com>
> > Cc: Michael Kinney <michael.d.kin...@intel.com>
> > Suggested-by: Ayellet Wolman <ayellet.wol...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
> > ---
> >  .../CpuExceptionHandlerLib/CpuExceptionCommon.h|  50 +++
> >  .../DxeCpuExceptionHandlerLib.inf  |   6 +
> >  .../Library/CpuExceptionHandlerLib/DxeException.c  |  53 ++-
> >  .../Ia32/ArchExceptionHandler.c| 167 +
> >  .../Ia32/ArchInterruptDefs.h   |   8 +
> >  .../Ia32/ExceptionTssEntryAsm.nasm | 398
> > +
> >  .../PeiCpuExceptionHandlerLib.inf  |   1 +
> >  .../SecPeiCpuExceptionHandlerLib.inf   |   1 +
> >  .../SmmCpuExceptionHandlerLib.inf 

Re: [edk2] [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Yao, Jiewen]

Some thought:

1) I found InitializeCpuExceptionStackSwitchHandlers() is only implemented in 
DxeException.c.
What about Pei/Smm instance?

I think it is OK to not implement it at this moment. But we need make sure no 
architecture issue if we want to enable it some time later.

2) #define IA32_GDT_TYPE_TSS   0x9
This is generic, can we move to BaseLib.h?


Thank you
Yao Jiewen


> -Original Message-
> From: Wang, Jian J
> Sent: Wednesday, November 22, 2017 4:46 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric ; Yao,
> Jiewen ; Kinney, Michael D
> 
> Subject: [PATCH v2 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> support
> 
> > v2:
> >a. Move common TSS structure and API definitions to BaseLib.h
> >b. Add EXCEPTION_STACK_SWITCH_DATA to convery data used to setup
> stack
> >   switch. This can avoid allocating memory for it in this library.
> >c. Add globals to reserve memory for stack switch initialized in early
> >   phase of DXE core.
> >d. Remove the filter code used to exclude boot modes which doesn't
> support
> >   memory allocation because those memory can passed in by parameter
> now.
> >e. Remove the nasm macro to define exception handler one by one and
> add a
> >   function to return the start address of each handler.
> 
> If Stack Guard is enabled and there's really a stack overflow happened during
> boot, a Page Fault exception will be triggered. Because the stack is out of
> usage, the exception handler, which shares the stack with normal UEFI driver,
> cannot be executed and cannot dump the processor information.
> 
> Without those information, it's very difficult for the BIOS developers locate
> the root cause of stack overflow. And without a workable stack, the developer
> cannot event use single step to debug the UEFI driver with JTAG debugger.
> 
> In order to make sure the exception handler to execute normally after stack
> overflow. We need separate stacks for exception handlers in case of unusable
> stack.
> 
> IA processor allows to switch to a new stack during handling interrupt and
> exception. But X64 and IA32 provides different ways to make it. X64 provides
> interrupt stack table (IST) to allow maximum 7 different exceptions to have
> new stack for its handler. IA32 doesn't have IST mechanism and can only use
> task gate to do it since task switch allows to load a new stack through its
> task-state segment (TSS).
> 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Cc: Michael Kinney 
> Suggested-by: Ayellet Wolman 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  .../CpuExceptionHandlerLib/CpuExceptionCommon.h|  50 +++
>  .../DxeCpuExceptionHandlerLib.inf  |   6 +
>  .../Library/CpuExceptionHandlerLib/DxeException.c  |  53 ++-
>  .../Ia32/ArchExceptionHandler.c| 167 +
>  .../Ia32/ArchInterruptDefs.h   |   8 +
>  .../Ia32/ExceptionTssEntryAsm.nasm | 398
> +
>  .../PeiCpuExceptionHandlerLib.inf  |   1 +
>  .../SecPeiCpuExceptionHandlerLib.inf   |   1 +
>  .../SmmCpuExceptionHandlerLib.inf  |   1 +
>  .../X64/ArchExceptionHandler.c | 133 +++
>  .../CpuExceptionHandlerLib/X64/ArchInterruptDefs.h |   3 +
>  11 files changed, 820 insertions(+), 1 deletion(-)
>  create mode 100644
> UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> index 740a58828b..30334105d2 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> @@ -48,6 +48,32 @@
>  0xb21d9148, 0x9211, 0x4d8f, { 0xad, 0xd3, 0x66, 0xb1, 0x89, 0xc9, 0x2c, 
> 0x83 }
> \
>}
> 
> +#define CPU_STACK_SWITCH_EXCEPTION_NUMBER \
> +  FixedPcdGetSize (PcdCpuStackSwitchExceptionList)
> +
> +#define CPU_STACK_SWITCH_EXCEPTION_LIST \
> +  FixedPcdGetPtr (PcdCpuStackSwitchExceptionList)
> +
> +#define CPU_KNOWN_GOOD_STACK_SIZE \
> +  FixedPcdGet32 (PcdCpuKnownGoodStackSize)
> +
> +#define CPU_TSS_GDT_SIZE (SIZE_2KB + CPU_TSS_DESC_SIZE + CPU_TSS_SIZE)
> +
> +#define IA32_GDT_TYPE_TSS   0x9
> +#define IA32_GDT_ALIGNMENT  8
> +
> +typedef struct {
> +  UINTN StackTop;
> +  UINTN StackSize;
> +  UINT8 *Exceptions;
> +  UINTN ExceptionNumber;
> +  IA32_IDT_GATE_DESCRIPTOR  *IdtTable;
> +  IA32_SEGMENT_DESCRIPTOR   *GdtTable;
> +  UINTN