I'm fine with them too. So we have two minor changes: 1. Change EnableNonExec to IsEnableNonExecNeeded 2. Move IsExecuteDisableBitAvailable() to VirtualMemory.c
I think v3 is not necessary. I'll push the patch after enough test. Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, September 21, 2018 6:14 PM > To: Zeng, Star <star.z...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com> > Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX > related PCDs > > On 09/21/18 10:42, Zeng, Star wrote: > > Another minor suggestion is to move IsExecuteDisableBitAvailable() to > > VirtualMemory.c, then there will be no need to declare it in > > VirtualMemeory.h. > > I'm fine with both ideas (name change as you see fit, and code movement). > > Thanks > Laszlo > > > > > > > Thanks, > > Star > > On 2018/9/21 14:00, Zeng, Star wrote: > >> Jian and Laszlo, > >> > >> There is also a superficial comment at below. > >> > >> On 2018/9/20 14:02, Jian J Wang wrote: > >>>> v2 changes: > >>>> a. remove macros no longer needed > >>>> b. remove DEBUG and ASSERT in ToEnableExecuteDisableFeature() > >>>> c. change ToEnableExecuteDisableFeature to EnableNonExec > >>> > >>> BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 > >>> > >>> Currently IA32_EFER.NXE is only set against PcdSetNxForStack. This > >>> confuses developers because following two other PCDs also need NXE > >>> to be set, but actually not. > >>> > >>> PcdDxeNxMemoryProtectionPolicy > >>> PcdImageProtectionPolicy > >>> > >>> This patch solves this issue by adding logic to enable IA32_EFER.NXE > >>> if any of those PCDs have anything enabled. > >>> > >>> Cc: Star Zeng <star.z...@intel.com> > >>> Cc: Laszlo Ersek <ler...@redhat.com> > >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > >>> Cc: Ruiyu Ni <ruiyu...@intel.com> > >>> Cc: Jiewen Yao <jiewen....@intel.com> > >>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>> Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > >>> --- > >>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 2 ++ > >>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 4 ++-- > >>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 30 > >>> +++++++++++++++++++++++- > >>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 24 > >>> +++++++++++++++++++ > >>> 4 files changed, 57 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >>> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >>> index fd82657404..068e700074 100644 > >>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >>> @@ -117,6 +117,8 @@ > >>> [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64] > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## > >>> SOMETIMES_CONSUMES > >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy > ## > >>> SOMETIMES_CONSUMES > >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## > >>> SOMETIMES_CONSUMES > >>> [Depex] > >>> gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid > >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > >>> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > >>> index d28baa3615..ccd30f964b 100644 > >>> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > >>> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > >>> @@ -245,7 +245,7 @@ ToBuildPageTable ( > >>> return TRUE; > >>> } > >>> - if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable > >>> ()) { > >>> + if (EnableNonExec ()) { > >>> return TRUE; > >>> } > >>> @@ -436,7 +436,7 @@ HandOffToDxeCore ( > >>> BuildPageTablesIa32Pae = ToBuildPageTable (); > >>> if (BuildPageTablesIa32Pae) { > >>> PageTables = Create4GPageTablesIa32Pae (BaseOfStack, > >>> STACK_SIZE); > >>> - if (IsExecuteDisableBitAvailable ()) { > >>> + if (EnableNonExec ()) { > >>> EnableExecuteDisableBit(); > >>> } > >>> } > >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > >>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > >>> index 496e219913..73b0f67c6b 100644 > >>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > >>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > >>> @@ -106,6 +106,31 @@ IsNullDetectionEnabled ( > >>> return ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != > >>> 0); > >>> } > >>> +/** > >>> + Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or > >>> not. > >>> + > >>> + @retval TRUE IA32_EFER.NXE should be enabled. > >>> + @retval FALSE IA32_EFER.NXE should not be enabled. > >>> + > >>> +**/ > >>> +BOOLEAN > >>> +EnableNonExec ( > >>> + VOID > >>> + ) > >>> +{ > >>> + if (!IsExecuteDisableBitAvailable ()) { > >>> + return FALSE; > >>> + } > >>> + > >>> + // > >>> + // XD flag (BIT63) in page table entry is only valid if > >>> IA32_EFER.NXE is set. > >>> + // Features controlled by Following PCDs need this feature to be > >>> enabled. > >>> + // > >>> + return (PcdGetBool (PcdSetNxForStack) || > >>> + PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0 || > >>> + PcdGet32 (PcdImageProtectionPolicy) != 0); > >>> +} > >> > >> I am a little confused by this function name compared with > >> EnableExecuteDisableBit(). This function is not really to enable NX, > >> but just to check whether enable NX is needed or not. How about using > >> name IsEnableNonExecNeeded or IsEnableNxNeeded or > IsDisableExecuteNeeded? > >> > >> > >> Sorry I did not raise this comment in V1 patch thread. > >> If you agree with the name changing, Reviewed-by: Star Zeng > >> <star.z...@intel.com>. > >> > >> > >> Thanks, > >> Star > >>> + > >>> /** > >>> Enable Execute Disable Bit. > >>> @@ -755,7 +780,10 @@ CreateIdentityMappingPageTables ( > >>> // > >>> EnablePageTableProtection ((UINTN)PageMap, TRUE); > >>> - if (PcdGetBool (PcdSetNxForStack)) { > >>> + // > >>> + // Set IA32_EFER.NXE if necessary. > >>> + // > >>> + if (EnableNonExec ()) { > >>> EnableExecuteDisableBit (); > >>> } > >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h > >>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h > >>> index 85457ff937..09085312aa 100644 > >>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h > >>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h > >>> @@ -179,6 +179,30 @@ typedef struct { > >>> UINTN FreePages; > >>> } PAGE_TABLE_POOL; > >>> +/** > >>> + Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or > >>> not. > >>> + > >>> + @retval TRUE IA32_EFER.NXE should be enabled. > >>> + @retval FALSE IA32_EFER.NXE should not be enabled. > >>> + > >>> +**/ > >>> +BOOLEAN > >>> +EnableNonExec ( > >>> + VOID > >>> + ); > >>> + > >>> +/** > >>> + The function will check if Execute Disable Bit is available. > >>> + > >>> + @retval TRUE Execute Disable Bit is available. > >>> + @retval FALSE Execute Disable Bit is not available. > >>> + > >>> +**/ > >>> +BOOLEAN > >>> +IsExecuteDisableBitAvailable ( > >>> + VOID > >>> + ); > >>> + > >>> /** > >>> Enable Execute Disable Bit. > >>> > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
