Hi Laszlo, Regards, Jian
> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, June 13, 2018 11:10 PM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Dong, > Eric <eric.d...@intel.com> > Subject: Re: [edk2] [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) > page table in SMM mode > > Hi Jian, > > I have three comments: > > On 06/13/18 07:35, Jian J Wang wrote: > >> v2: > >> a. add more specific explanations in commit message > >> b. add more comments in code > >> c. remove redundant logic in IsInSmm() > >> d. fix a logic hole in GetCurrentPagingContext() > >> e. replace meanless constant macro with meaning ones > > > > The MdePkg/Library/SmmMemoryAllocationLib, used only by > DXE_SMM_DRIVER, > > allows to free memory allocated in DXE (before EndOfDxe). This is done > > by checking the memory range and calling gBS services to do real > > operation if the memory to free is out of SMRAM. If some memory related > > features, like Heap Guard, are enabled, gBS interface will turn to > > EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), provided by > > DXE driver UefiCpuPkg/CpuDxe, to change memory paging attributes. This > > means we have part of DXE code running in SMM mode in certain > > circumstances. > > > > Because page table in SMM mode is different from DXE mode and CpuDxe > > always uses current registers (CR0, CR3, etc.) to get memory paging > > attributes, it cannot get the correct attributes of DXE memory in SMM > > mode from SMM page table. This will cause incorrect memory manipulations, > > like fail the releasing of Guard pages if Heap Guard is enabled. > > > > The solution in this patch is to store the DXE page table information > > (e.g. value of CR0, CR3 registers, etc.) in a global variable of CpuDxe > > driver. If CpuDxe detects it's in SMM mode, it will use this global > > variable to access page table instead of current processor registers. > > This can avoid retrieving wrong DXE memory paging attributes and changing > > SMM page table attributes unexpectedly. > > > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > > --- > > UefiCpuPkg/CpuDxe/CpuDxe.inf | 1 + > > UefiCpuPkg/CpuDxe/CpuPageTable.c | 159 > ++++++++++++++++++++++++++++++--------- > > 2 files changed, 123 insertions(+), 37 deletions(-) > > > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf > > index 3c938cee53..ce2bd3627c 100644 > > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf > > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf > > @@ -66,6 +66,7 @@ > > [Protocols] > > gEfiCpuArchProtocolGuid ## PRODUCES > > gEfiMpServiceProtocolGuid ## PRODUCES > > + gEfiSmmBase2ProtocolGuid ## CONSUMES > > (1) In my v1 review, I suggested "SOMETIMES_CONSUMES" for this hint. > > And that's because CpuDxe can be included in platform builds that don't > include the SMM driver stack at all; in that case, > gEfiSmmBase2ProtocolGuid will not be available and will not be consumed. > In other words, we cannot say that the protocol is always consumed. > Make sense. > > > > [Guids] > > gIdleLoopEventGuid ## CONSUMES ## > > Event > > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c > b/UefiCpuPkg/CpuDxe/CpuPageTable.c > > index e2595b4d89..b7e75922b6 100644 > > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > > @@ -23,10 +23,21 @@ > > #include <Library/DebugLib.h> > > #include <Library/UefiBootServicesTableLib.h> > > #include <Protocol/MpService.h> > > +#include <Protocol/SmmBase2.h> > > +#include <Register/Cpuid.h> > > +#include <Register/Msr.h> > > > > #include "CpuDxe.h" > > #include "CpuPageTable.h" > > > > +/// > > +/// Paging registers > > +/// > > +#define CR0_WP BIT16 > > +#define CR0_PG BIT31 > > +#define CR4_PSE BIT4 > > +#define CR4_PAE BIT5 > > (2a) I think the BITxx -> CRx_xx macro replacements should have been > split to a separate patch. > Sure. > > + > > /// > > /// Page Table Entry > > /// > > @@ -87,7 +98,46 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { > > {Page1G, SIZE_1GB, PAGING_1G_ADDRESS_MASK_64}, > > }; > > > > -PAGE_TABLE_POOL *mPageTablePool = NULL; > > +PAGE_TABLE_POOL *mPageTablePool = NULL; > > +PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext; > > +EFI_SMM_BASE2_PROTOCOL *mSmmBase2 = NULL; > > + > > +/** > > + Check if current execution environment is in SMM mode or not, via > > + EFI_SMM_BASE2_PROTOCOL. > > + > > + This is necessary because of the fact that > MdePkg\Library\SmmMemoryAllocationLib > > + supports to free memory outside SMRAM. The library will call > > gBS->FreePool() > or > > + gBS->FreePages() and then SetMemorySpaceAttributes interface in turn to > change > > + memory paging attributes during free operation, if some memory related > features > > + are enabled (like Heap Guard). > > + > > + This means that SetMemorySpaceAttributes() has chance to run in SMM > mode. This > > + will cause incorrect result because SMM mode always loads its own page > tables, > > + which are usually different from DXE. This function can be used to detect > such > > + situation and help to avoid further misoperations. > > + > > + @retval TRUE In SMM mode. > > + @retval FALSE Not in SMM mode. > > +**/ > > +BOOLEAN > > +IsInSmm ( > > + VOID > > + ) > > +{ > > + BOOLEAN InSmm; > > + > > + InSmm = FALSE; > > + if (mSmmBase2 == NULL) { > > + gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL, (VOID > **)&mSmmBase2); > > + } > > + > > + if (mSmmBase2 != NULL) { > > + mSmmBase2->InSmm (mSmmBase2, &InSmm); > > + } > > + > > + return InSmm; > > +} > > > > /** > > Return current paging context. > > @@ -99,45 +149,61 @@ GetCurrentPagingContext ( > > IN OUT PAGE_TABLE_LIB_PAGING_CONTEXT *PagingContext > > ) > > { > > - UINT32 RegEax; > > - UINT32 RegEdx; > > + UINT32 RegEax; > > + CPUID_EXTENDED_CPU_SIG_EDX RegEdx; > > + MSR_IA32_EFER_REGISTER MsrEfer; > > > > - ZeroMem(PagingContext, sizeof(*PagingContext)); > > - if (sizeof(UINTN) == sizeof(UINT64)) { > > - PagingContext->MachineType = IMAGE_FILE_MACHINE_X64; > > - } else { > > - PagingContext->MachineType = IMAGE_FILE_MACHINE_I386; > > - } > > - if ((AsmReadCr0 () & BIT31) != 0) { > > - PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () & > PAGING_4K_ADDRESS_MASK_64); > > - } else { > > - PagingContext->ContextData.X64.PageTableBase = 0; > > - } > > + // > > + // Don't retrieve current paging context from processor if in SMM mode. > > + // > > + if (!IsInSmm ()) { > > + ZeroMem (&mPagingContext, sizeof(mPagingContext)); > > > > - if ((AsmReadCr4 () & BIT4) != 0) { > > - PagingContext->ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE; > > - } > > - if ((AsmReadCr4 () & BIT5) != 0) { > > - PagingContext->ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE; > > - } > > - if ((AsmReadCr0 () & BIT16) != 0) { > > - PagingContext->ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE; > > - } > > + if (sizeof(UINTN) == sizeof(UINT64)) { > > + mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64; > > + } else { > > + mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386; > > + } > > + if ((AsmReadCr0 () & CR0_PG) != 0) { > > + mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & > PAGING_4K_ADDRESS_MASK_64); > > + } else { > > + mPagingContext.ContextData.X64.PageTableBase = 0; > > + } > > > > - AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL); > > - if (RegEax > 0x80000000) { > > - AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx); > > - if ((RegEdx & BIT20) != 0) { > > - // XD supported > > - if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) { > > - // XD activated > > - PagingContext->ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED; > > - } > > + if ((AsmReadCr4 () & CR4_PSE) != 0) { > > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE; > > + } > > + if ((AsmReadCr4 () & CR4_PAE) != 0) { > > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE; > > + } > > + if ((AsmReadCr0 () & CR0_WP) != 0) { > > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE; > > } > > - if ((RegEdx & BIT26) != 0) { > > - PagingContext->ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPO > RT; > > + > > + AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL); > > + if (RegEax >= CPUID_EXTENDED_CPU_SIG) { > > (2b) Similarly to (2a), this cleanup (which is correct, and welcome) > should have been split to a separate patch. > Ok. > > + AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, (UINT32 > *)&RegEdx); > > (3) I think it would be more idiomatic to pass &RegEdx.Uint32 as the > last argument. However, this method works as well. > Your way is better. At least it saves a type cast. > > I don't think that posting a v3 is necessary just because of these > remarks. Please fix (1) though, just before you push the patch. > > * For this patch, with (1) updated: > > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > * For the series: > > Regression-tested-by: Laszlo Ersek <ler...@redhat.com> > > For the regression testing, I used OVMF built both with and without SMM. > Also, for patch #2, please remember that OVMF does not enable the page > guard feature. > Thank you very much for the test. > Thanks! > Laszlo > > > > + > > + if (RegEdx.Bits.NX != 0) { > > + // XD supported > > + MsrEfer.Uint64 = AsmReadMsr64(MSR_CORE_IA32_EFER); > > + if (MsrEfer.Bits.NXE != 0) { > > + // XD activated > > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED; > > + } > > + } > > + > > + if (RegEdx.Bits.Page1GB != 0) { > > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPO > RT; > > + } > > } > > } > > + > > + // > > + // This can avoid getting SMM paging context if in SMM mode. We cannot > assume > > + // SMM mode shares the same paging context as DXE. > > + // > > + CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext)); > > } > > > > /** > > @@ -507,7 +573,14 @@ IsReadOnlyPageWriteProtected ( > > VOID > > ) > > { > > - return ((AsmReadCr0 () & BIT16) != 0); > > + // > > + // To avoid unforseen consequences, don't touch paging settings in SMM > mode > > + // in this driver. > > + // > > + if (!IsInSmm ()) { > > + return ((AsmReadCr0 () & CR0_WP) != 0); > > + } > > + return FALSE; > > } > > > > /** > > @@ -518,7 +591,13 @@ DisableReadOnlyPageWriteProtect ( > > VOID > > ) > > { > > - AsmWriteCr0 (AsmReadCr0() & ~BIT16); > > + // > > + // To avoid unforseen consequences, don't touch paging settings in SMM > mode > > + // in this driver. > > + // > > + if (!IsInSmm ()) { > > + AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP); > > + } > > } > > > > /** > > @@ -529,7 +608,13 @@ EnableReadOnlyPageWriteProtect ( > > VOID > > ) > > { > > - AsmWriteCr0 (AsmReadCr0() | BIT16); > > + // > > + // To avoid unforseen consequences, don't touch paging settings in SMM > mode > > + // in this driver. > > + // > > + if (!IsInSmm ()) { > > + AsmWriteCr0 (AsmReadCr0 () | CR0_WP); > > + } > > } > > > > /** > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
