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...@intel.com> > > > > Thanks, > Ray >> -----Original Message----- >> From: Sheng, W <w.sh...@intel.com> >> Sent: Tuesday, November 21, 2023 3:03 PM >> To: devel@edk2.groups.io >> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Laszlo >> Ersek <ler...@redhat.com>; Wu, Jiaxin <jiaxin...@intel.com>; Tan, Dun >> <dun....@intel.com> >> Subject: [PATCH v6 1/6] MdePkg: Add macro definitions for CET feature for >> NASM files. >> >> 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> >> Reviewed-by: Laszlo Ersek <ler...@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 <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
