Hi Jian, On 06/11/18 09:08, Jian J Wang wrote: > The SMM version of MemoryAllocationLib 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. This would cause problem if some memory related features, like > Heap Guard, have to update page table to change memory attributes. > Because page table in SMM mode is different from DXE mode, gBS memory > services cannot get the correct attributes of DXE memory from SMM page > table and then cause incorrect memory manipulations.
(1) I think this description makes sense, but it does not highlight the involvement of CpuDxe. (1a) Please reference "MdePkg/Library/SmmMemoryAllocationLib" specifically. (1b) The SmmMemoryAllocationLib instance implies that the call chain starts with a DXE_SMM_DRIVER calling FreePool() or FreePages(). DXE_SMM_DRIVERs can only call boot services, and protocols located with boot services, in their entry point functions. CpuDxe is a DXE_DRIVER and it provides the CPU arch protocol (and the MP services protocol). Thus, our call chain can only occur if: - a DXE_SMM_DRIVER calls the FreePool() or FreePages() library API, - in its entry point function, - releasing memory that's not SMRAM, - and the heap guard feature is enabled, so the FreePool() or FreePages() boot service ends up calling EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(). Please include this information in the commit message. (2) Can you remind me how the SMM page tables map non-SMRAM memory? Because, I assume that, after a FreePages() libary API call, the freed memory should not be accessible to either SMM code or normal DXE code. This seems to imply that both sets of page tables should be modified. What am I missing? > > 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. > > Change-Id: If810bb1828160b8bdd8cb616d86df2859c74971f (3) I think this comes from Gerrit. Do you really need the "Change-Id" tag in the commit message? It doesn't say anything to the upstream edk2 users. If you really need it, I don't mind it though; just please let's not add it due to oversight. (4) Please make sure that you don't *push* the patch with Gerrit. Gerrit does bad things to git metadata. Please see <4A89E2EF3DFEDB4C8BFDE51014F606A14E22344D@SHSMSX104.ccr.corp.intel.com">http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E22344D@SHSMSX104.ccr.corp.intel.com>. > 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.c | 2 +- > UefiCpuPkg/CpuDxe/CpuDxe.inf | 1 + > UefiCpuPkg/CpuDxe/CpuPageTable.c | 108 > ++++++++++++++++++++++++++------------- > 3 files changed, 75 insertions(+), 36 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c > index 6ae2dcd1c7..1fd996fc3f 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c > @@ -404,7 +404,7 @@ CpuSetMemoryAttributes ( > // to avoid unnecessary computing. > // > if (mIsFlushingGCD) { > - DEBUG((DEBUG_INFO, " Flushing GCD\n")); > + DEBUG((DEBUG_GCD, " Flushing GCD\n")); > return EFI_SUCCESS; > } (5) I think it would be cleaner if we fixed the debug level for this message in a separate patch. You are not touching "CpuDxe.c" for any other reason. > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf > index 3c938cee53..8c8773af90 100644 > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf > @@ -66,6 +66,7 @@ > [Protocols] > gEfiCpuArchProtocolGuid ## PRODUCES > gEfiMpServiceProtocolGuid ## PRODUCES > + gEfiSmmBase2ProtocolGuid (6) protocol usage note is missing (should be "## SOMETIMES_CONSUMES" I think.) > > [Guids] > gIdleLoopEventGuid ## CONSUMES ## > Event > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c > b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index e2595b4d89..bf420d3792 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -23,6 +23,7 @@ > #include <Library/DebugLib.h> > #include <Library/UefiBootServicesTableLib.h> > #include <Protocol/MpService.h> > +#include <Protocol/SmmBase2.h> > > #include "CpuDxe.h" > #include "CpuPageTable.h" > @@ -87,7 +88,33 @@ 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; > + > +BOOLEAN > +IsInSmm ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + BOOLEAN InSmm; > + > + InSmm = FALSE; > + if (mSmmBase2 == NULL) { > + Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL, > + (VOID **)&mSmmBase2); (7) Indentation is incorrect. > + if (EFI_ERROR (Status)) { > + mSmmBase2 = NULL; > + } (8) I think you can simply drop the EFI_ERROR (Status) branch; "mSmmBase2" will simply remain NULL. > + } > + > + if (mSmmBase2 != NULL) { > + mSmmBase2->InSmm (mSmmBase2, &InSmm); > + } > + > + return InSmm; > +} (9) I don't understand how the InSmm() member function is supposed to work. According to the PI spec (v1.6): """ EFI_MM_BASE_PROTOCOL.InMm() [...] Service to indicate whether the driver is currently executing in the MM Driver Initialization phase. [...] InMmram Pointer to a Boolean which, on return, indicates that the driver is currently executing inside of MMRAM (TRUE) or outside of MMRAM (FALSE). [...] This service returns whether the caller is being executed in the MM Driver Initialization phase. For MM Drivers, this will return TRUE in InMmram while inside the driver’s entry point and otherwise FALSE. For combination MM/DXE drivers, this will return FALSE in the DXE launch. For the MM launch, it behaves as an MM Driver. """ First of all, to my knowledge, we don't have any combination MM/DXE drivers in edk2 (with dual entry point launches, binary type 0x0C). We have traditional MM drivers (binary type 0x0A) that are launched directly from SMRAM. However, it is not guaranteed that they are launched with the processor operating in System Management Mode. Secondly, focusing on traditional MM drivers, the InMm() specification doesn't seem to say anything as to whether the processor is running in SMM (system management mode) or normal mode. Initially, SMRAM is open, and thus the processor can execute MM driver code out of it without actually operating in SMM (see above). Furthermore, CpuDxe is a DXE_DRIVER, thus it is never loaded into SMRAM. I notice that you call the "InMmram" parameter "InSmm". Are you saying that the PI spec is wrong, and edk2 actually returns whether the processor is executing CpuDxe code in Management Mode? (Due to a call from the entry point function of a traditional MM driver?) Third, the spec is inconsistent even with itself. The general description says, "For MM Drivers, this will return TRUE in InMmram while inside the driver’s entry point and otherwise FALSE". But a DXE protocol must not even be invoked from a traditional MM driver *except* from the entry point! Which would imply that, for traditional MM drivers, the output parameter could only be set to TRUE. So basically what I'd like to see here is evidence that (9a) the IsInSmm() helper function is never called outside of the entry point of an MM driver, due to the restrictions on gBS->LocateProtocol() and mSmmBase2->InSmm(), (9b) the mSmmBase2->InSmm() protocol member determines whether CpuDxe is being executed by the processor in Management Mode, and *not* whether the CpuDxe driver is being executed from within SMRAM. > > /** > Return current paging context. > @@ -102,42 +129,45 @@ GetCurrentPagingContext ( > UINT32 RegEax; > UINT32 RegEdx; > > - 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; > - } > + if (!IsInSmm ()) { > + if (sizeof(UINTN) == sizeof(UINT64)) { > + mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64; > + } else { > + mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386; > + } > + if ((AsmReadCr0 () & BIT31) != 0) { > + mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & > PAGING_4K_ADDRESS_MASK_64); > + } else { > + mPagingContext.ContextData.X64.PageTableBase = 0; > + } > > - 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 ((AsmReadCr4 () & BIT4) != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE; > + } > + if ((AsmReadCr4 () & BIT5) != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE; > + } > + if ((AsmReadCr0 () & BIT16) != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE; > + } > > - 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; > + 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 > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED; > + } > + } > + if ((RegEdx & BIT26) != 0) { > + mPagingContext.ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT; > } > - } > - if ((RegEdx & BIT26) != 0) { > - PagingContext->ContextData.Ia32.Attributes |= > PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT; > } > } > + > + CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext)); > } (10) Even assuming that IsInSmm() always returns false -- for example because the platform does not include the SMM drivers --, this change does not look like a no-op to me. The ZeroMem() call at the top is removed by the patch, but attributes like PSE, PAE, WP_ENABLE, XD_ACTIVATED and 1G_SUPPORT are never *cleared* in "mPagingContext", once they are set. I guess the idea is that a few (how many?) initial calls to GetCurrentPagingContext() set up "mPagingContext", and after that, GetCurrentPagingContext() just keeps returning the last set context. But I don't think that those "initial calls" work correctly. > > /** > @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected ( > VOID > ) > { > - return ((AsmReadCr0 () & BIT16) != 0); > + if (!IsInSmm ()) { > + return ((AsmReadCr0 () & BIT16) != 0); > + } > + return FALSE; > } > > /** > @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect ( > VOID > ) > { > - AsmWriteCr0 (AsmReadCr0() & ~BIT16); > + if (!IsInSmm ()) { > + AsmWriteCr0 (AsmReadCr0 () & ~BIT16); > + } > } > > /** > @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect ( > VOID > ) > { > - AsmWriteCr0 (AsmReadCr0() | BIT16); > + if (!IsInSmm ()) { > + AsmWriteCr0 (AsmReadCr0 () | BIT16); > + } > } (11) I don't understand these changes at all. Why are these functions no-ops when CpuDxe is invoked in Management Mode? Thanks Laszlo > > /** > @@ -1054,6 +1091,7 @@ InitializePageTableLib ( > { > PAGE_TABLE_LIB_PAGING_CONTEXT CurrentPagingContext; > > + ZeroMem (&mPagingContext, sizeof(mPagingContext)); > GetCurrentPagingContext (&CurrentPagingContext); > > // > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
