[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

Jiewen,

For 1), this is good question for both global variable solution and new API 
solution.
My suggestion is only to use one global stack for BSP, since only BSP is 
support before CpuDxe produced Cpu MP service Protocol.
And in InitializeCpuInterruptHandlers() (consumed by CpuDxe) to register 
callback function on CPU MP Protocol to allocate stack for Aps.

For 2),we could consider new API solution if PEI phase does support this new 
feature.

Jeff

发件人: Yao, Jiewen<mailto:jiewen@intel.com>
发送时间: 2017年11月6日 9:55
收件人: Wang, Jian J<mailto:jian.j.w...@intel.com>
抄送: Fan Jeff<mailto:vanjeff_...@hotmail.com>; Kinney, Michael 
D<mailto:michael.d.kin...@intel.com>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Dong, 
Eric<mailto:eric.d...@intel.com>; Zeng, Star<mailto:star.z...@intel.com>
主题: Re: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

I think we need consider 2 additional things:

1)  how to support different stack for ap?


2) how to support pei phase, when the global variable is read only?

thank you!
Yao, Jiewen


在 2017年11月6日，上午8:30，Wang, Jian J 
<jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>> 写道：
Jeff,

That’s a good suggestion. Do you know if there’s any pitfall in using data 
segment memory as stack?

Thanks,
Jian

From: Fan Jeff [mailto:vanjeff_...@hotmail.com]
Sent: Friday, November 03, 2017 4:59 PM
To: Yao, Jiewen <jiewen@intel.com<mailto:jiewen@intel.com>>; Kinney, 
Michael D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; 
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: Dong, Eric <eric.d...@intel.com<mailto:eric.d...@intel.com>>; Zeng, Star 
<star.z...@intel.com<mailto:star.z...@intel.com>>
Subject: 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support


Jian,



For example, you could use the global variable UINT8 NewStack[256] instead of 
allocate memory for stack and try to use the other global variables for other 
allocated data.



Does it work for your case?



Thanks!

Jeff



From: edk2-devel 
<edk2-devel-boun...@lists.01.org<mailto:edk2-devel-boun...@lists.01.org>> on 
behalf of Fan Jeff <vanjeff_...@hotmail.com<mailto:vanjeff_...@hotmail.com>>
Sent: Friday, November 3, 2017 4:21:22 PM
To: Yao, Jiewen; Kinney, Michael D; Wang, Jian J; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Dong, Eric; Zeng, Star
Subject: [edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack 
switch support

Jian,

Could you use some global variables in 
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
 to avoid adding new API?

Jeff
发件人: Yao, Jiewen<mailto:jiewen@intel.com>
发送时间: 2017年11月3日 9:24
收件人: Kinney, Michael D<mailto:michael.d.kin...@intel.com>; Fan 
Jeff<mailto:vanjeff_...@hotmail.com>; Wang, Jian 
J<mailto:jian.j.w...@intel.com>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
抄送: Dong, Eric<mailto:eric.d...@intel.com>; Zeng, 
Star<mailto:star.z...@intel.com>
主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

HI Jian
There is another potential problem.

This feature is enabled in InitializeCpuInterruptHandlers(). However, we expect 
this is enabled in InitializeCpuExceptionHandlers().

In order to get the exception stack in InitializeCpuExceptionHandlers(). We can 
have 2 ways:
A) Let InitializeCpuExceptionHandlers() allocate the exception stack.
B) Let caller to pass the exception stack to InitializeCpuExceptionHandlers().

For A), it is hard, because DxeCore calls InitializeCpuExceptionHandlers() 
earlier than CoreInitializeMemoryServices().
For B), InitializeCpuExceptionHandlers() interface does not contain such 
information. We might need add a new API to add the exception stack 
information, such as InitializeCpuSwitchStackExceptionHandlers ().

Once this feature is done, can we clean up the SMM code to use the new API, 
instead of duplicate the SMM copy of exception handler ? The existing SMM copy 
has some assumption on CR4 and FXSAVE/RESTORE, and it broke Quark platform.

Thank you
Yao Jiewen



> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 1, 2017 11:43 PM
> To: Fan Jeff <vanjeff_...@hotmail.com<mailto:vanjeff_...@hotmail.com>>; Yao, 
> Jiewen <jiewen@intel.com<mailto:jiewen@intel.com>>;
> 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>; Kinney, Michael
> D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
>

[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

Jian,

In EDKII, DS == SS on most of cases. I don’t think there is any issue to use 
data range for stack.

Jeff


From: Wang, Jian J <jian.j.w...@intel.com>
Sent: Monday, November 6, 2017 8:30:29 AM
To: Fan Jeff; Yao, Jiewen; Kinney, Michael D; edk2-devel@lists.01.org
Cc: Dong, Eric; Zeng, Star
Subject: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

Jeff,

That’s a good suggestion. Do you know if there’s any pitfall in using data 
segment memory as stack?

Thanks,
Jian

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


Jian,



For example, you could use the global variable UINT8 NewStack[256] instead of 
allocate memory for stack and try to use the other global variables for other 
allocated data.



Does it work for your case?



Thanks!

Jeff




From: edk2-devel 
<edk2-devel-boun...@lists.01.org<mailto:edk2-devel-boun...@lists.01.org>> on 
behalf of Fan Jeff <vanjeff_...@hotmail.com<mailto:vanjeff_...@hotmail.com>>
Sent: Friday, November 3, 2017 4:21:22 PM
To: Yao, Jiewen; Kinney, Michael D; Wang, Jian J; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Dong, Eric; Zeng, Star
Subject: [edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack 
switch support

Jian,

Could you use some global variables in 
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
 to avoid adding new API?

Jeff
发件人: Yao, Jiewen<mailto:jiewen@intel.com>
发送时间: 2017年11月3日 9:24
收件人: Kinney, Michael D<mailto:michael.d.kin...@intel.com>; Fan 
Jeff<mailto:vanjeff_...@hotmail.com>; Wang, Jian 
J<mailto:jian.j.w...@intel.com>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
抄送: Dong, Eric<mailto:eric.d...@intel.com>; Zeng, 
Star<mailto:star.z...@intel.com>
主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

HI Jian
There is another potential problem.

This feature is enabled in InitializeCpuInterruptHandlers(). However, we expect 
this is enabled in InitializeCpuExceptionHandlers().

In order to get the exception stack in InitializeCpuExceptionHandlers(). We can 
have 2 ways:
A) Let InitializeCpuExceptionHandlers() allocate the exception stack.
B) Let caller to pass the exception stack to InitializeCpuExceptionHandlers().

For A), it is hard, because DxeCore calls InitializeCpuExceptionHandlers() 
earlier than CoreInitializeMemoryServices().
For B), InitializeCpuExceptionHandlers() interface does not contain such 
information. We might need add a new API to add the exception stack 
information, such as InitializeCpuSwitchStackExceptionHandlers ().

Once this feature is done, can we clean up the SMM code to use the new API, 
instead of duplicate the SMM copy of exception handler ? The existing SMM copy 
has some assumption on CR4 and FXSAVE/RESTORE, and it broke Quark platform.

Thank you
Yao Jiewen



> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 1, 2017 11:43 PM
> To: Fan Jeff <vanjeff_...@hotmail.com<mailto:vanjeff_...@hotmail.com>>; Yao, 
> Jiewen <jiewen@intel.com<mailto:jiewen@intel.com>>;
> 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>; Kinney, Michael
> D <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
> Cc: Dong, Eric <eric.d...@intel.com<mailto:eric.d...@intel.com>>; Zeng, Star 
> <star.z...@intel.com<mailto:star.z...@intel.com>>
> Subject: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> support
>
> I agree that adding AsmWriteTr, IA32_TASK_STATE_SEGMENT,
> IA32_TSS_DESCRIPTOR to MdePkg BaseLib is a good idea.
>
> Mike
>
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Fan
> Jeff
> Sent: Tuesday, October 31, 2017 7:33 PM
> To: Yao, Jiewen <jiewen@intel.com<mailto:jiewen@intel.com>>; 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: Kinney, Michael D 
> <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Dong, Eric
> <eric.d...@intel.com<mailto:eric.d...@intel.com>

[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

Jian,

Could you use some global variables in 
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
 to avoid adding new API?

Jeff
发件人: Yao, Jiewen<mailto:jiewen@intel.com>
发送时间: 2017年11月3日 9:24
收件人: Kinney, Michael D<mailto:michael.d.kin...@intel.com>; Fan 
Jeff<mailto:vanjeff_...@hotmail.com>; Wang, Jian 
J<mailto:jian.j.w...@intel.com>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
抄送: Dong, Eric<mailto:eric.d...@intel.com>; Zeng, 
Star<mailto:star.z...@intel.com>
主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

HI Jian
There is another potential problem.

This feature is enabled in InitializeCpuInterruptHandlers(). However, we expect 
this is enabled in InitializeCpuExceptionHandlers().

In order to get the exception stack in InitializeCpuExceptionHandlers(). We can 
have 2 ways:
A) Let InitializeCpuExceptionHandlers() allocate the exception stack.
B) Let caller to pass the exception stack to InitializeCpuExceptionHandlers().

For A), it is hard, because DxeCore calls InitializeCpuExceptionHandlers() 
earlier than CoreInitializeMemoryServices().
For B), InitializeCpuExceptionHandlers() interface does not contain such 
information. We might need add a new API to add the exception stack 
information, such as InitializeCpuSwitchStackExceptionHandlers ().

Once this feature is done, can we clean up the SMM code to use the new API, 
instead of duplicate the SMM copy of exception handler ? The existing SMM copy 
has some assumption on CR4 and FXSAVE/RESTORE, and it broke Quark platform.

Thank you
Yao Jiewen



> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 1, 2017 11:43 PM
> To: Fan Jeff <vanjeff_...@hotmail.com>; Yao, Jiewen <jiewen@intel.com>;
> Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org; Kinney, Michael
> D <michael.d.kin...@intel.com>
> Cc: Dong, Eric <eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> support
>
> I agree that adding AsmWriteTr, IA32_TASK_STATE_SEGMENT,
> IA32_TSS_DESCRIPTOR to MdePkg BaseLib is a good idea.
>
> Mike
>
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Fan
> Jeff
> Sent: Tuesday, October 31, 2017 7:33 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: [edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add
> stack switch support
>
> Per https://bugzilla.tianocore.org/show_bug.cgi?id=109, TR should be setup
> (Such as in DxeIplPeim) even though NULL Cpu Exception Handler instance is
> chosen.
>
> For long term, I agree we need to move AsmWriteTr,
> IA32_TASK_STATE_SEGMENT, IA32_TSS_DESCRIPTOR to MdePkg(Such as BaseLib)
> For this patch, I have no strong opinion.
>
>
> 发件人: Yao, Jiewen<mailto:jiewen@intel.com>
> 发送时间: 2017年11月1日 9:56
> 收件人: Wang, Jian J<mailto:jian.j.w...@intel.com>;
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> 抄送: Kinney, Michael D<mailto:michael.d.kin...@intel.com>; Dong,
> Eric<mailto:eric.d...@intel.com>; Zeng, Star<mailto:star.z...@intel.com>
> 主题: Re: [edk2] [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
>
> Hi Jian
> Thanks for the patch.
>
> Can we move all IA32 defined data structure or function to MdePkg?
> Such as: AsmWriteTr, IA32_TASK_STATE_SEGMENT, IA32_TSS_DESCRIPTOR
>
> I am also curious why we use different policy for other boot mode.
> Can we use consistent policy?
> > +  if (PcdGetBool (PcdCpuStackGuard)) {
> > +//
> > +// Stack Guard works with the support of page table established and
> > +// memory management. So we have to exclude those boot modes
> > without
> > +// them.
> > +//
> > +switch (GetBootModeHob()) {
> > +case BOOT_ON_FLASH_UPDATE:
> > +case BOOT_IN_RECOVERY_MODE:
> > +case BOOT_ON_S3_RESUME:
> > +  break;
> > +
> > +default:
> > +  ArchSetupExcpetionStack (IdtTable);
> > +  break;
> > +}
> > +  }
>
>
> Thank you
> Yao Jiewen
>
>
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Tuesday, October 31, 2017 10:24 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric
> > <eric.d...@intel.com>; Yao, Jiewen <jiewen@intel.

[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

Jian,

You are right! The exception handler setup in DxeIplPeim does not need stack at 
all. Thanks your clarification.

Jeff

发件人: Wang, Jian J
发送时间: 2017年11月1日 11:12
收件人: Wang, Jian J; Fan 
Jeff; Yao, Jiewen; 
edk2-devel@lists.01.org
抄送: Kinney, Michael D; Dong, 
Eric; Zeng, Star
主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

Jeff,

Sorry for the misunderstanding. I think stack switch cannot work with NULL 
exception handler. But without exception handler, what do we need stack switch 
for?

Thanks,
Jian

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Wednesday, November 01, 2017 11:09 AM
> To: Fan Jeff ; Yao, Jiewen ;
> edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Dong, Eric
> ; Zeng, Star 
> Subject: Re: [edk2] [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
>
> Jeff,
>
> Stack guard will still work. But the developer cannot know what’s going on
> without exception dumping message when stack overflow occurs.
>
> Thanks,
> Jian
>
> From: Fan Jeff [mailto:vanjeff_...@hotmail.com]
> Sent: Wednesday, November 01, 2017 10:59 AM
> To: Wang, Jian J ; Yao, Jiewen ;
> edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Dong, Eric
> ; Zeng, Star 
> Subject: 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
>
> Jian,
>
> No. I suggest to fix #109 in another separate patch.
>
> But to fix #109, we have to makes sure AsmWriteTr() and some definitions are 
> in
> MdePkg.
> But I have no strong opinion to add them into MdePkg in this patch.
>
> I have another question: If NULL CPU exception handler instance is chosen,
> could Stack Switch work if PCD PcdCpuStackGuard is set to TRUE.
>
> Thanks!
> Jeff
>
> 发件人: Wang, Jian J
> 发送时间: 2017年11月1日 10:48
> 收件人: Fan Jeff; Yao,
> Jiewen; edk2-devel@lists.01.org de...@lists.01.org>
> 抄送: Kinney, Michael D; Dong,
> Eric; Zeng, Star
> 主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> support
>
> Hi Jeff,
>
> Thanks for the feedback. Are you suggesting to fix Bugzilla 109 in this patch?
>
> Thanks,
> Jian
>
> From: Fan Jeff [mailto:vanjeff_...@hotmail.com]
> Sent: Wednesday, November 01, 2017 10:33 AM
> To: Yao, Jiewen >;
> Wang, Jian J >; edk2-
> de...@lists.01.org
> Cc: Kinney, Michael D
> >; Dong, Eric
> >; Zeng, Star
> >
> Subject: 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
>
> Per https://bugzilla.tianocore.org/show_bug.cgi?id=109, TR should be setup
> (Such as in DxeIplPeim) even though NULL Cpu Exception Handler instance is
> chosen.
>
> For long term, I agree we need to move AsmWriteTr,
> IA32_TASK_STATE_SEGMENT, IA32_TSS_DESCRIPTOR to MdePkg(Such as
> BaseLib)
> For this patch, I have no strong opinion.
>
>
> 发件人: Yao, Jiewen
> 发送时间: 2017年11月1日 9:56
> 收件人: Wang, Jian J; edk2-
> de...@lists.01.org
> 抄送: Kinney, Michael D; Dong,
> Eric; Zeng, Star
> 主题: Re: [edk2] [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack
> switch support
>
> Hi Jian
> Thanks for the patch.
>
> Can we move all IA32 defined data structure or function to MdePkg?
> Such as: AsmWriteTr, IA32_TASK_STATE_SEGMENT, IA32_TSS_DESCRIPTOR
>
> I am also curious why we use different policy for other boot mode.
> Can we use consistent policy?
> > +  if (PcdGetBool (PcdCpuStackGuard)) {
> > +//
> > +// Stack Guard works with the support of page table established and
> > +// memory management. So we have to exclude those boot modes
> > without
> > +// them.
> > +//
> > +switch (GetBootModeHob()) {
> > +case BOOT_ON_FLASH_UPDATE:
> > +case BOOT_IN_RECOVERY_MODE:
> > +case BOOT_ON_S3_RESUME:
> > +  break;
> > +
> > +default:
> > +  ArchSetupExcpetionStack (IdtTable);
> > +  break;
> > +}
> > +  }
>
>
> 

[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

That’s fine. Thanks

Developers should have such assumption without exception dumping message if 
NULL instance used.

Jeff

发件人: Wang, Jian J
发送时间: 2017年11月1日 11:08
收件人: Fan Jeff; Yao, 
Jiewen; 
edk2-devel@lists.01.org
抄送: Kinney, Michael D; Dong, 
Eric; Zeng, Star
主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

Jeff,

Stack guard will still work. But the developer cannot know what’s going on 
without exception dumping message when stack overflow occurs.

Thanks,
Jian

From: Fan Jeff [mailto:vanjeff_...@hotmail.com]
Sent: Wednesday, November 01, 2017 10:59 AM
To: Wang, Jian J ; Yao, Jiewen ; 
edk2-devel@lists.01.org
Cc: Kinney, Michael D ; Dong, Eric 
; Zeng, Star 
Subject: 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

Jian,

No. I suggest to fix #109 in another separate patch.

But to fix #109, we have to makes sure AsmWriteTr() and some definitions are in 
MdePkg.
But I have no strong opinion to add them into MdePkg in this patch.

I have another question: If NULL CPU exception handler instance is chosen, 
could Stack Switch work if PCD PcdCpuStackGuard is set to TRUE.

Thanks!
Jeff

发件人: Wang, Jian J
发送时间: 2017年11月1日 10:48
收件人: Fan Jeff; Yao, 
Jiewen; 
edk2-devel@lists.01.org
抄送: Kinney, Michael D; Dong, 
Eric; Zeng, Star
主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

Hi Jeff,

Thanks for the feedback. Are you suggesting to fix Bugzilla 109 in this patch?

Thanks,
Jian

From: Fan Jeff [mailto:vanjeff_...@hotmail.com]
Sent: Wednesday, November 01, 2017 10:33 AM
To: Yao, Jiewen >; Wang, Jian 
J >; 
edk2-devel@lists.01.org
Cc: Kinney, Michael D 
>; Dong, Eric 
>; Zeng, Star 
>
Subject: 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

Per https://bugzilla.tianocore.org/show_bug.cgi?id=109, TR should be setup 
(Such as in DxeIplPeim) even though NULL Cpu Exception Handler instance is 
chosen.

For long term, I agree we need to move AsmWriteTr, IA32_TASK_STATE_SEGMENT, 
IA32_TSS_DESCRIPTOR to MdePkg(Such as BaseLib)
For this patch, I have no strong opinion.


发件人: Yao, Jiewen
发送时间: 2017年11月1日 9:56
收件人: Wang, Jian J; 
edk2-devel@lists.01.org
抄送: Kinney, Michael D; Dong, 
Eric; Zeng, Star
主题: Re: [edk2] [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

Hi Jian
Thanks for the patch.

Can we move all IA32 defined data structure or function to MdePkg?
Such as: AsmWriteTr, IA32_TASK_STATE_SEGMENT, IA32_TSS_DESCRIPTOR

I am also curious why we use different policy for other boot mode.
Can we use consistent policy?
> +  if (PcdGetBool (PcdCpuStackGuard)) {
> +//
> +// Stack Guard works with the support of page table established and
> +// memory management. So we have to exclude those boot modes
> without
> +// them.
> +//
> +switch (GetBootModeHob()) {
> +case BOOT_ON_FLASH_UPDATE:
> +case BOOT_IN_RECOVERY_MODE:
> +case BOOT_ON_S3_RESUME:
> +  break;
> +
> +default:
> +  ArchSetupExcpetionStack (IdtTable);
> +  break;
> +}
> +  }


Thank you
Yao Jiewen


> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, October 31, 2017 10:24 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star >; Dong, Eric 
> >; Yao,
> Jiewen >; Kinney, Michael D
> >
> Subject: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> support
>
> 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 

[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

Jian,

No. I suggest to fix #109 in another separate patch.

But to fix #109, we have to makes sure AsmWriteTr() and some definitions are in 
MdePkg.
But I have no strong opinion to add them into MdePkg in this patch.

I have another question: If NULL CPU exception handler instance is chosen, 
could Stack Switch work if PCD PcdCpuStackGuard is set to TRUE.

Thanks!
Jeff

发件人: Wang, Jian J
发送时间: 2017年11月1日 10:48
收件人: Fan Jeff; Yao, 
Jiewen; 
edk2-devel@lists.01.org
抄送: Kinney, Michael D; Dong, 
Eric; Zeng, Star
主题: RE: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

Hi Jeff,

Thanks for the feedback. Are you suggesting to fix Bugzilla 109 in this patch?

Thanks,
Jian

From: Fan Jeff [mailto:vanjeff_...@hotmail.com]
Sent: Wednesday, November 01, 2017 10:33 AM
To: Yao, Jiewen ; Wang, Jian J ; 
edk2-devel@lists.01.org
Cc: Kinney, Michael D ; Dong, Eric 
; Zeng, Star 
Subject: 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

Per https://bugzilla.tianocore.org/show_bug.cgi?id=109, TR should be setup 
(Such as in DxeIplPeim) even though NULL Cpu Exception Handler instance is 
chosen.

For long term, I agree we need to move AsmWriteTr, IA32_TASK_STATE_SEGMENT, 
IA32_TSS_DESCRIPTOR to MdePkg(Such as BaseLib)
For this patch, I have no strong opinion.


发件人: Yao, Jiewen
发送时间: 2017年11月1日 9:56
收件人: Wang, Jian J; 
edk2-devel@lists.01.org
抄送: Kinney, Michael D; Dong, 
Eric; Zeng, Star
主题: Re: [edk2] [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

Hi Jian
Thanks for the patch.

Can we move all IA32 defined data structure or function to MdePkg?
Such as: AsmWriteTr, IA32_TASK_STATE_SEGMENT, IA32_TSS_DESCRIPTOR

I am also curious why we use different policy for other boot mode.
Can we use consistent policy?
> +  if (PcdGetBool (PcdCpuStackGuard)) {
> +//
> +// Stack Guard works with the support of page table established and
> +// memory management. So we have to exclude those boot modes
> without
> +// them.
> +//
> +switch (GetBootModeHob()) {
> +case BOOT_ON_FLASH_UPDATE:
> +case BOOT_IN_RECOVERY_MODE:
> +case BOOT_ON_S3_RESUME:
> +  break;
> +
> +default:
> +  ArchSetupExcpetionStack (IdtTable);
> +  break;
> +}
> +  }


Thank you
Yao Jiewen


> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, October 31, 2017 10:24 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star >; Dong, Eric 
> >; Yao,
> Jiewen >; Kinney, Michael D
> >
> Subject: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> support
>
> 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).
>
> Note: Stack switch needs to allocate memory pages to be new stacks. So this
>   functionality works only in the boot phases capable of memory
>   allocation (besides the paging, for the sake of Stack Guard). In
>   other words, only DXE phase can supports Stack Guard with stack switch.
>
> Cc: Star Zeng >
> Cc: Eric Dong >
> Cc: Jiewen Yao >
> Cc: Michael Kinney 
> 

[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

1. Should add EFIAPI for the following function.
+VOID
+AsmWriteTr (
+  UINT16 Selector
+  );


2. Should not add EFIAPI for the following function.
+VOID
+EFIAPI
+ArchSetupExcpetionStack (
+  IN IA32_IDT_GATE_DESCRIPTOR   *IdtTable
+  )

发件人: Jian J Wang
发送时间: 2017年10月31日 22:26
收件人: edk2-devel@lists.01.org
抄送: Michael Kinney; Jiewen 
Yao; Eric Dong; Star 
Zeng
主题: [edk2] [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

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).

Note: Stack switch needs to allocate memory pages to be new stacks. So this
  functionality works only in the boot phases capable of memory
  allocation (besides the paging, for the sake of Stack Guard). In
  other words, only DXE phase can supports Stack Guard with stack switch.

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|  22 ++
 .../DxeCpuExceptionHandlerLib.inf  |   5 +
 .../Library/CpuExceptionHandlerLib/DxeException.c  |  19 +
 .../Ia32/ArchExceptionHandler.c| 135 +++
 .../Ia32/ArchInterruptDefs.h   | 136 +++
 .../Ia32/ExceptionTssEntryAsm.nasm | 398 +
 .../PeiCpuExceptionHandlerLib.inf  |   1 +
 .../SecPeiCpuExceptionHandlerLib.inf   |   3 +
 .../SmmCpuExceptionHandlerLib.inf  |   1 +
 .../X64/ArchExceptionHandler.c | 108 ++
 .../CpuExceptionHandlerLib/X64/ArchInterruptDefs.h |  40 +++
 .../X64/ExceptionHandlerAsm.S  |  12 +
 .../X64/ExceptionHandlerAsm.asm|  12 +
 .../X64/ExceptionHandlerAsm.nasm   |  12 +
 14 files changed, 904 insertions(+)
 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..fd4a26a458 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 

 #define  CPU_EXCEPTION_NUM  32
 #define  CPU_INTERRUPT_NUM 256
@@ -288,5 +289,26 @@ CommonExceptionHandlerWorker (
   IN EXCEPTION_HANDLER_DATA  *ExceptionHandlerData
   );

+/**
+  Load given selector into TR register
+
+  @param Selector Task segment selector
+**/
+VOID
+AsmWriteTr (
+  UINT16 Selector
+  );
+
+/**
+  Setup separate stack for specific exceptions.
+
+  @param[in] IdtTableIDT table base.
+**/
+VOID
+EFIAPI
+ArchSetupExcpetionStack (
+  IN IA32_IDT_GATE_DESCRIPTOR   *IdtTable
+  );
+
 #endif

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
index f4a8d01c80..b099ef4dad 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
@@ -30,6 +30,7 @@
 [Sources.Ia32]
   Ia32/ExceptionHandlerAsm.asm
   Ia32/ExceptionHandlerAsm.nasm
+  Ia32/ExceptionTssEntryAsm.nasm
   Ia32/ExceptionHandlerAsm.S
   Ia32/ArchExceptionHandler.c
   Ia32/ArchInterruptDefs.h
@@ -47,6 +48,9 @@
   PeiDxeSmmCpuException.c
   DxeException.c

+[Pcd]
+  

[edk2] 答复: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support

[Fan Jeff]

Per https://bugzilla.tianocore.org/show_bug.cgi?id=109, TR should be setup 
(Such as in DxeIplPeim) even though NULL Cpu Exception Handler instance is 
chosen.

For long term, I agree we need to move AsmWriteTr, IA32_TASK_STATE_SEGMENT, 
IA32_TSS_DESCRIPTOR to MdePkg(Such as BaseLib)
For this patch, I have no strong opinion.


发件人: Yao, Jiewen
发送时间: 2017年11月1日 9:56
收件人: Wang, Jian J; 
edk2-devel@lists.01.org
抄送: Kinney, Michael D; Dong, 
Eric; Zeng, Star
主题: Re: [edk2] [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch 
support

Hi Jian
Thanks for the patch.

Can we move all IA32 defined data structure or function to MdePkg?
Such as: AsmWriteTr, IA32_TASK_STATE_SEGMENT, IA32_TSS_DESCRIPTOR

I am also curious why we use different policy for other boot mode.
Can we use consistent policy?
> +  if (PcdGetBool (PcdCpuStackGuard)) {
> +//
> +// Stack Guard works with the support of page table established and
> +// memory management. So we have to exclude those boot modes
> without
> +// them.
> +//
> +switch (GetBootModeHob()) {
> +case BOOT_ON_FLASH_UPDATE:
> +case BOOT_IN_RECOVERY_MODE:
> +case BOOT_ON_S3_RESUME:
> +  break;
> +
> +default:
> +  ArchSetupExcpetionStack (IdtTable);
> +  break;
> +}
> +  }


Thank you
Yao Jiewen


> -Original Message-
> From: Wang, Jian J
> Sent: Tuesday, October 31, 2017 10:24 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric ; Yao,
> Jiewen ; Kinney, Michael D
> 
> Subject: [PATCH 3/3] UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch
> support
>
> 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).
>
> Note: Stack switch needs to allocate memory pages to be new stacks. So this
>   functionality works only in the boot phases capable of memory
>   allocation (besides the paging, for the sake of Stack Guard). In
>   other words, only DXE phase can supports Stack Guard with stack switch.
>
> 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|  22 ++
>  .../DxeCpuExceptionHandlerLib.inf  |   5 +
>  .../Library/CpuExceptionHandlerLib/DxeException.c  |  19 +
>  .../Ia32/ArchExceptionHandler.c| 135 +++
>  .../Ia32/ArchInterruptDefs.h   | 136 +++
>  .../Ia32/ExceptionTssEntryAsm.nasm | 398
> +
>  .../PeiCpuExceptionHandlerLib.inf  |   1 +
>  .../SecPeiCpuExceptionHandlerLib.inf   |   3 +
>  .../SmmCpuExceptionHandlerLib.inf  |   1 +
>  .../X64/ArchExceptionHandler.c | 108 ++
>  .../CpuExceptionHandlerLib/X64/ArchInterruptDefs.h |  40 +++
>  .../X64/ExceptionHandlerAsm.S  |  12 +
>  .../X64/ExceptionHandlerAsm.asm|  12 +
>  .../X64/ExceptionHandlerAsm.nasm   |  12 +
>  14 files changed, 904 insertions(+)
>  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..fd4a26a458 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> +++