Re: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Change CR4.CET bit only

["Laszlo Ersek" <lersek@redhat.com>]

On 11/6/23 10:07, Sheng Wei wrote:
> Do not use fixed CR4 value 0x668, change CR4.CET bit only.
> 
> Signed-off-by: Sheng Wei <w.sheng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Wu Jiaxin <jiaxin.wu@intel.com>
> Cc: Tan Dun <dun.tan@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 9 ++++++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm  | 3 ++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> index 68332e2c3f..a087576a54 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
> @@ -260,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
> @@ -292,8 +293,10 @@ CetDone:
>      xor     edx, edx
>      wrmsr
>  
> -    mov     eax, 0x668
> -    mov     cr4, eax       ; disable CET
> +    ; clear CR4.CET bit
> +    mov     eax, cr4
> +    btr     eax, CR4_CET_BIT
> +    mov     cr4, eax
>  
>      mov     ecx, MSR_IA32_PL0_SSP
>      pop     eax
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> index 007fbff640..7aed7c8dda 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> @@ -287,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

I didn't understand why the X64 code here didn't contain the "btr"
counterpart of "bts". Well the reason is that the "missing" btr is
actually introduced in the previous patch.

I find that confusing. I think that, once you have "Cet.inc", you should
separately replace CR4_CET with CR4_CET_BIT, both in "Cet.inc" and in
the three existent locations (two in the IA32 entry code and one in the
X64 entry code).

*Then* you could proceed to clearing CR4.CET in the subsequent patch,
using CR4_CET_BIT.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110927): https://edk2.groups.io/g/devel/message/110927
Mute This Topic: https://groups.io/mt/102416574/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-