Hi Laszlo, Please ignore the patch V3. I will refine the patches and raise patch V4. Thank you. BR Sheng Wei
> -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Thursday, November 9, 2023 5:16 AM > 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 v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: > Clear CR4.CET before restoring MSR IA32_S_CET > > On 11/6/23 10:07, Sheng Wei wrote: > > Clear CR4.CET bit before restoring MSR IA32_S_CET. > > Backup/restore MSR IA32_U_CET in SMI. > > (1) As far as I understand, these are still two separate fixes. And I think > this > patch has issues due to trying to fix both issues at the same time. (I could > be > wrong of course, I'm not familiar with CET, but this is my impression.) More > details on this below. > > (2) Each issue / fix (i.e., the one issue / fix per patch) should be > explained in > detail, even if you think the issue that each patch fixes is obvious. > > > > > 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 | 53 > ++++++++++++--- > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 69 > ++++++++++++++++---- > > 2 files changed, 98 insertions(+), 24 deletions(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > > index 19de5f614e..68332e2c3f 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 > > (3) These assembly language macros should have been introduced in an > include file (*.inc). > > These "SmiEntry.nasm" files already %include "StuffRsbNasm.inc" and > "Nasm.inc", so placing the CET-related macros side-by-side with those files, > for > example in a new file called "Cet.inc", would be the right thing. It would > eliminate the duplication between the IA32 and X64 nasm files. > > Please prepend a patch to the series that moves the existent macros to > "Cet.nasm", and then in this patch, add the new macros to "Cet.nasm" / > modify the old ones inside "Cet.nasm". > > > > @@ -214,11 +215,21 @@ ASM_PFX(mPatchCetSupported): > > push edx > > push eax > > > > + mov ecx, MSR_IA32_U_CET > > + rdmsr > > + push edx > > + push eax > > + > > So this is related to saving CET_U state; we're pushing the MSR contents to > the > stack right after having saving CET_S state similarly. > > > 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 > > This seems to clear CET_U state. Why is that necessary? > > The commit message only says "backup/restore"; it does not say "clear". > > I assume your main goal is *clearing* CET_U state for the duration of the SMI, > and that's why you want to backup/restore (so that the clearing does not > destroy information). I guess. > > (4) But this should be explained in the commit message. > > > @@ -276,6 +287,11 @@ CetDone: > > cmp al, 0 > > jz CetDone2 > > > > + mov ecx, MSR_IA32_S_CET > > + xor eax, eax > > + xor edx, edx > > + wrmsr > > + > > mov eax, 0x668 > > mov cr4, eax ; disable CET > > > > (5) This logic then appears to belong to the *other* fix, namely nulling > (?) CET_S state before clearing CR4.CET. > > > @@ -284,10 +300,15 @@ CetDone: > > 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) > > I admit again that I know effectively nothing about CET, but this looks like a > bug. > > The patch tries to pop CET_U (for the purpose of restoring it) before it > (already) pops CET_S (for the purpose of restoring it). > > The ordering seems fine; it mirrors the pushing. > > (6) However, the last instruction before the CetDone2 label should be > "wrmsr"; should it not? > > I have no idea why we move EAX to EBX instead; it makes no sense to me. > > > @@ -305,6 +326,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 > > This makes my head spin! > > First, it explains why we store EAX to EBX under (6). The reason is that we > want to delay the CET_S restoration right until the RSM. And because the > MSR_IA32_MISC_ENABLE restoration clobbers EAX, we stash it in EBX. > > (7) But *why* do we have to delay CET_S restoration right until the RSM? > It is not explained anywhere at all in the patch. > > (8) The stashing of EAX in EBX needs a conspicuous comment in the code, so > that future code additions don't clobber EBX. > > (9) Why don't we stash EDX similary? Why is it safe to set EDX to 0 before the > WRMSR? > > If we don't care about the EDX that we just popped from the stack, then why > do we push it originally? > > (10) Given that we skip the CET_S restoration if "mCetSupported" is zero, why > do we *not* skip the CET_U restoration similarly? > > Sorry, I'm totally confused by this patch. It feels like the patch is trying > to do at > least three separate things. > > I don't think I can sensibly review the X64 counterpart until this patch is > further split into *minimal* actions. (In fact the "clear CR4.CET" in the > subject > line and in the commit message seems to apply only to the > X64 code!) > > Thanks > Laszlo > > > +CetDone3: > > + > > rsm > > > > ASM_PFX(gcSmiHandlerSize): DW $ - _SmiEntryPoint diff --git > > a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > > index d302ca8d01..007fbff640 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 > > @@ -316,13 +327,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 +352,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 +379,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 +427,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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110951): https://edk2.groups.io/g/devel/message/110951 Mute This Topic: https://groups.io/mt/102416572/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
