Re: [edk2-devel] [PATCH v6 1/6] MdePkg: Add macro definitions for CET feature for NASM files.

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

On 11/21/23 08:53, Ni, Ray wrote:
> You might need to drop Reviewed-by from Laszlo as the patch content is so different than the reviewed version.
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> 
> 
> Thanks,
> Ray
>> -----Original Message-----
>> From: Sheng, W <w.sheng@intel.com>
>> Sent: Tuesday, November 21, 2023 3:03 PM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
>> Ersek <lersek@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Tan, Dun
>> <dun.tan@intel.com>
>> Subject: [PATCH v6 1/6] MdePkg: Add macro definitions for CET feature for
>> NASM files.
>>
>> 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>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  MdePkg/Include/Ia32/Cet.inc | 26 ++++++++++++++++++++++++++
>>  MdePkg/Include/X64/Cet.inc  | 26 ++++++++++++++++++++++++++
>>  2 files changed, 52 insertions(+)
>>  create mode 100644 MdePkg/Include/Ia32/Cet.inc
>>  create mode 100644 MdePkg/Include/X64/Cet.inc

Referring back to Ray's v5 comments here
<https://edk2.groups.io/g/devel/message/111512> -- because v6 was posted
before I could have reacted to v5 --, I have the following opinion:

I agree with all observations except the code duplication under Ia32 and
X64. Ray himself noted that the duplication would be questionable.

How about this: unify the macro definitions in a single file, but call
that file:

  MdePkg/Include/X86Cet.inc

This eliminates the code duplication and also clearly shows that the
include is only for IA32 and X64 -- for *both* of them, actually.

I have to reasons for thinking that the "X86" prefix should work fine:

- the X86 prefix is already used in the filenames

  MdePkg/Library/BaseCacheMaintenanceLib/X86*
  MdePkg/Library/BaseCpuLib/X86*
  MdePkg/Library/BaseLib/X86*
  MdePkg/Library/SecPeiDxeTimerLibCpu/X86*

- in BaseLib.h, we have identifiers such as X86_ASSEMBLY_PATCH_LABEL and
PatchInstructionX86().

All of these apply to both Ia32 and X64.


If there will be no *other* changes in *v7* 1/6 than this file
unification, then you can add, at once, to v7 1/6:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(Of course the %include references will have to be updated in the other
patches, but that doesn't invalidate my R-b's on those patches.)

Thanks!
Laszlo


>>
>> diff --git a/MdePkg/Include/Ia32/Cet.inc b/MdePkg/Include/Ia32/Cet.inc
>> new file mode 100644
>> index 0000000000..41c99988c9
>> --- /dev/null
>> +++ b/MdePkg/Include/Ia32/Cet.inc
>> @@ -0,0 +1,26 @@
>> +;------------------------------------------------------------------------------
>>
>> +;
>>
>> +; Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
>>
>> +; SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> +;
>>
>> +; Abstract:
>>
>> +;
>>
>> +;   This file provides macro definitions for CET feature for NASM files.
>>
>> +;
>>
>> +;------------------------------------------------------------------------------
>>
>> +
>>
>> +%define MSR_IA32_U_CET                     0x6A0
>>
>> +%define MSR_IA32_S_CET                     0x6A2
>>
>> +%define MSR_IA32_CET_SH_STK_EN             (1<<0)
>>
>> +%define MSR_IA32_CET_WR_SHSTK_EN           (1<<1)
>>
>> +%define MSR_IA32_CET_ENDBR_EN              (1<<2)
>>
>> +%define MSR_IA32_CET_LEG_IW_EN             (1<<3)
>>
>> +%define MSR_IA32_CET_NO_TRACK_EN           (1<<4)
>>
>> +%define MSR_IA32_CET_SUPPRESS_DIS          (1<<5)
>>
>> +%define MSR_IA32_CET_SUPPRESS              (1<<10)
>>
>> +%define MSR_IA32_CET_TRACKER               (1<<11)
>>
>> +%define MSR_IA32_PL0_SSP                   0x6A4
>>
>> +%define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR  0x6A8
>>
>> +
>>
>> +%define CR4_CET_BIT                        23
>>
>> +%define CR4_CET                            (1<<CR4_CET_BIT)
>>
>> diff --git a/MdePkg/Include/X64/Cet.inc b/MdePkg/Include/X64/Cet.inc
>> new file mode 100644
>> index 0000000000..41c99988c9
>> --- /dev/null
>> +++ b/MdePkg/Include/X64/Cet.inc
>> @@ -0,0 +1,26 @@
>> +;------------------------------------------------------------------------------
>>
>> +;
>>
>> +; Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
>>
>> +; SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> +;
>>
>> +; Abstract:
>>
>> +;
>>
>> +;   This file provides macro definitions for CET feature for NASM files.
>>
>> +;
>>
>> +;------------------------------------------------------------------------------
>>
>> +
>>
>> +%define MSR_IA32_U_CET                     0x6A0
>>
>> +%define MSR_IA32_S_CET                     0x6A2
>>
>> +%define MSR_IA32_CET_SH_STK_EN             (1<<0)
>>
>> +%define MSR_IA32_CET_WR_SHSTK_EN           (1<<1)
>>
>> +%define MSR_IA32_CET_ENDBR_EN              (1<<2)
>>
>> +%define MSR_IA32_CET_LEG_IW_EN             (1<<3)
>>
>> +%define MSR_IA32_CET_NO_TRACK_EN           (1<<4)
>>
>> +%define MSR_IA32_CET_SUPPRESS_DIS          (1<<5)
>>
>> +%define MSR_IA32_CET_SUPPRESS              (1<<10)
>>
>> +%define MSR_IA32_CET_TRACKER               (1<<11)
>>
>> +%define MSR_IA32_PL0_SSP                   0x6A4
>>
>> +%define MSR_IA32_INTERRUPT_SSP_TABLE_ADDR  0x6A8
>>
>> +
>>
>> +%define CR4_CET_BIT                        23
>>
>> +%define CR4_CET                            (1<<CR4_CET_BIT)
>>
>> --
>> 2.26.2.windows.1
> 
> 
> 
> 
> 
> 



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