Hi Laszlo, Thank you for the review comments. (1) This change is used to fix a hung issue when enable kernel CET-IBT. Kernel will enable CET-IBT by set IA32_U_CET.bit2. The issue only happens when it enters SMI with the state machine == WAIT_FOR_ENDBRANCH state. In SMI handler, when Set CR4.CET bit, CPU will check the next assembly code, if the next code is not ENDBR. It will trigger #CP exception. So, we need to backup current IA32_U_CET, and clear IA32_U_CET before enable CR4.CET. And when exit SMI, we need to restore the value in IA32_U_CET.
(2) Yes, I have separated it to 3 patches. But last patch will be removed because of (3) (3) It is global variable. It is initialized to zero. I will remove this change. I will raise patch V3. Thank you. BR Sheng Wei > -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Friday, November 3, 2023 9:19 PM > To: devel@edk2.groups.io; Sheng, W <w.sh...@intel.com> > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Wu, Jiaxin > <jiaxin...@intel.com>; Tan, Dun <dun....@intel.com> > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear > CR4.CET before restoring MSR IA32_S_CET > > On 11/3/23 06:35, Sheng Wei wrote: > > Clear CR4.CET bit before restoring MSR IA32_S_CET. > > Backup/restore MSR IA32_U_CET in SMI. > > Use current CR4 value when changing CR4.CET. > > (1) Why? > > (It's fine if you can provide a reference from the Intel SDM, but then please > do > provide it.) > > No problem has been stated. What is broken, and how does the proposed > patch solve the issue? > > (2) I could be mistaken, but the above changes are three separate fixes. > If you agree, then please split the patch in three patches. > > > Initial mSmmInterruptSspTables to 0. > > (3) The "mSmmInterruptSspTables" object has static storage duration (it is a > "global variable"), and its current definition > > UINTN mSmmInterruptSspTables; > > already ensures that it is initialized to zero. Therefore this change is > unnecessary. > > It does not hurt either, of course, so if you argument is that we should > improve readability, I don't mind, but then it too belongs in a separate > patch. > > Laszlo > > > > > Signed-off-by: Sheng Wei <w.sh...@intel.com> > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Wu Jiaxin <jiaxin...@intel.com> > > Cc: Tan Dun <dun....@intel.com> > > --- > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 62 > +++++++++++++---- > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 72 > ++++++++++++++++---- > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 2 +- > > 3 files changed, 107 insertions(+), 29 deletions(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > > index 19de5f614e..a087576a54 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > > @@ -16,18 +16,19 @@ > > %include "StuffRsbNasm.inc" > > %include "Nasm.inc" > > > > +%define MSR_IA32_U_CET 0x6A0 > > %define MSR_IA32_S_CET 0x6A2 > > -%define MSR_IA32_CET_SH_STK_EN 0x1 > > -%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > > -%define MSR_IA32_CET_ENDBR_EN 0x4 > > -%define MSR_IA32_CET_LEG_IW_EN 0x8 > > -%define MSR_IA32_CET_NO_TRACK_EN 0x10 > > -%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > > -%define MSR_IA32_CET_SUPPRESS 0x400 > > -%define MSR_IA32_CET_TRACKER 0x800 > > +%define MSR_IA32_CET_SH_STK_EN 0x1 > > +%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > > +%define MSR_IA32_CET_ENDBR_EN 0x4 > > +%define MSR_IA32_CET_LEG_IW_EN 0x8 > > +%define MSR_IA32_CET_NO_TRACK_EN 0x10 > > +%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > > +%define MSR_IA32_CET_SUPPRESS 0x400 > > +%define MSR_IA32_CET_TRACKER 0x800 > > %define MSR_IA32_PL0_SSP 0x6A4 > > > > -%define CR4_CET 0x800000 > > +%define CR4_CET_BIT 23 > > > > %define MSR_IA32_MISC_ENABLE 0x1A0 > > %define MSR_EFER 0xc0000080 > > @@ -214,11 +215,21 @@ ASM_PFX(mPatchCetSupported): > > push edx > > push eax > > > > + mov ecx, MSR_IA32_U_CET > > + rdmsr > > + push edx > > + push eax > > + > > mov ecx, MSR_IA32_PL0_SSP > > rdmsr > > push edx > > push eax > > > > + mov ecx, MSR_IA32_U_CET > > + xor eax, eax > > + xor edx, edx > > + wrmsr > > + > > mov ecx, MSR_IA32_S_CET > > mov eax, MSR_IA32_CET_SH_STK_EN > > xor edx, edx > > @@ -249,7 +260,8 @@ CetInterruptDone: > > bts ecx, 16 ; set WP > > mov cr0, ecx > > > > - mov eax, 0x668 | CR4_CET > > + mov eax, cr4 > > + bts eax, CR4_CET_BIT > > mov cr4, eax > > > > setssbsy > > @@ -276,18 +288,30 @@ CetDone: > > cmp al, 0 > > jz CetDone2 > > > > - mov eax, 0x668 > > - mov cr4, eax ; disable CET > > + mov ecx, MSR_IA32_S_CET > > + xor eax, eax > > + xor edx, edx > > + wrmsr > > + > > + ; clear CR4.CET bit > > + mov eax, cr4 > > + btr eax, CR4_CET_BIT > > + mov cr4, eax > > > > mov ecx, MSR_IA32_PL0_SSP > > pop eax > > pop edx > > wrmsr > > > > - mov ecx, MSR_IA32_S_CET > > + mov ecx, MSR_IA32_U_CET > > pop eax > > pop edx > > wrmsr > > + > > + mov ecx, MSR_IA32_S_CET > > + pop eax > > + pop edx > > + mov ebx, eax > > CetDone2: > > > > mov eax, ASM_PFX(mXdSupported) > > @@ -305,6 +329,18 @@ CetDone2: > > .7: > > > > StuffRsb32 > > + > > + mov eax, ASM_PFX(mCetSupported) > > + mov al, [eax] > > + cmp al, 0 > > + jz CetDone3 > > + > > + mov ecx, MSR_IA32_S_CET > > + mov eax, ebx > > + xor edx, edx > > + wrmsr > > +CetDone3: > > + > > rsm > > > > ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint diff --git > > a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > > index d302ca8d01..7aed7c8dda 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > > @@ -20,19 +20,20 @@ > > ; Variables referenced by C code > > ; > > > > +%define MSR_IA32_U_CET 0x6A0 > > %define MSR_IA32_S_CET 0x6A2 > > -%define MSR_IA32_CET_SH_STK_EN 0x1 > > -%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > > -%define MSR_IA32_CET_ENDBR_EN 0x4 > > -%define MSR_IA32_CET_LEG_IW_EN 0x8 > > -%define MSR_IA32_CET_NO_TRACK_EN 0x10 > > -%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > > -%define MSR_IA32_CET_SUPPRESS 0x400 > > -%define MSR_IA32_CET_TRACKER 0x800 > > +%define MSR_IA32_CET_SH_STK_EN 0x1 > > +%define MSR_IA32_CET_WR_SHSTK_EN 0x2 > > +%define MSR_IA32_CET_ENDBR_EN 0x4 > > +%define MSR_IA32_CET_LEG_IW_EN 0x8 > > +%define MSR_IA32_CET_NO_TRACK_EN 0x10 > > +%define MSR_IA32_CET_SUPPRESS_DIS 0x20 > > +%define MSR_IA32_CET_SUPPRESS 0x400 > > +%define MSR_IA32_CET_TRACKER 0x800 > > %define MSR_IA32_PL0_SSP 0x6A4 > > %define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR 0x6A8 > > > > -%define CR4_CET 0x800000 > > +%define CR4_CET_BIT 23 > > > > %define MSR_IA32_MISC_ENABLE 0x1A0 > > %define MSR_EFER 0xc0000080 > > @@ -230,6 +231,11 @@ ASM_PFX(mPatchCetSupported): > > push rdx > > push rax > > > > + mov ecx, MSR_IA32_U_CET > > + rdmsr > > + push rdx > > + push rax > > + > > mov ecx, MSR_IA32_PL0_SSP > > rdmsr > > push rdx > > @@ -240,6 +246,11 @@ ASM_PFX(mPatchCetSupported): > > push rdx > > push rax > > > > + mov ecx, MSR_IA32_U_CET > > + xor eax, eax > > + xor edx, edx > > + wrmsr > > + > > mov ecx, MSR_IA32_S_CET > > mov eax, MSR_IA32_CET_SH_STK_EN > > xor edx, edx > > @@ -276,7 +287,8 @@ CetInterruptDone: > > bts ecx, 16 ; set WP > > mov cr0, rcx > > > > - mov eax, 0x668 | CR4_CET > > + mov rax, cr4 > > + bts rax, CR4_CET_BIT > > mov cr4, rax > > > > setssbsy > > @@ -316,13 +328,20 @@ CpuSmmDebugExitAbsAddr: > > add rsp, 0x200 > > > > mov rax, strict qword 0 ; mov rax, > > ASM_PFX(mCetSupported) > > -mCetSupportedAbsAddr: > > +mCetSupportedAbsAddr1: > > mov al, [rax] > > cmp al, 0 > > jz CetDone2 > > > > - mov eax, 0x668 > > - mov cr4, rax ; disable CET > > + mov ecx, MSR_IA32_S_CET > > + xor eax, eax > > + xor edx, edx > > + wrmsr > > + > > + ; clear CR4.CET bit > > + mov rax, cr4 > > + btr rax, CR4_CET_BIT > > + mov cr4, rax > > > > mov ecx, MSR_IA32_INTERRUPT_SSP_TABLE_ADDR > > pop rax > > @@ -334,10 +353,15 @@ mCetSupportedAbsAddr: > > pop rdx > > wrmsr > > > > - mov ecx, MSR_IA32_S_CET > > + mov ecx, MSR_IA32_U_CET > > pop rax > > pop rdx > > wrmsr > > + > > + mov ecx, MSR_IA32_S_CET > > + pop rax > > + pop rdx > > + mov ebx, eax > > CetDone2: > > > > mov rax, strict qword 0 ; lea rax, > > [ASM_PFX(mXdSupported)] > > @@ -356,6 +380,19 @@ mXdSupportedAbsAddr: > > .1: > > > > StuffRsb64 > > + > > + mov rax, strict qword 0 ; mov rax, > > ASM_PFX(mCetSupported) > > +mCetSupportedAbsAddr2: > > + mov al, [rax] > > + cmp al, 0 > > + jz CetDone3 > > + > > + mov ecx, MSR_IA32_S_CET > > + mov eax, ebx > > + xor edx, edx > > + wrmsr > > +CetDone3: > > + > > rsm > > > > ASM_PFX(gcSmiHandlerSize) DW $ - _SmiEntryPoint > > @@ -391,6 +428,11 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress): > > mov qword [rcx - 8], rax > > > > lea rax, [ASM_PFX(mCetSupported)] > > - lea rcx, [mCetSupportedAbsAddr] > > + lea rcx, [mCetSupportedAbsAddr1] > > mov qword [rcx - 8], rax > > + > > + lea rax, [ASM_PFX(mCetSupported)] > > + lea rcx, [mCetSupportedAbsAddr2] > > + mov qword [rcx - 8], rax > > + > > ret > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > index c4f21e2155..6c53213b0b 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > > @@ -20,7 +20,7 @@ UINT32 mCetPl0Ssp; > > UINT32 mCetInterruptSsp; > > UINT32 mCetInterruptSspTable; > > > > -UINTN mSmmInterruptSspTables; > > +UINTN mSmmInterruptSspTables = 0; > > > > /** > > Initialize IDT IST Field. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110882): https://edk2.groups.io/g/devel/message/110882 Mute This Topic: https://groups.io/mt/102358752/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
