On 27 February 2017 at 06:50, Zeng, Star <star.z...@intel.com> wrote: > CoreAllocatePoolPages() could not be updated simply by adding > CoreAcquireMemoryLock() and CoreReleaseMemoryLock(), it is also used by > AllocateMemoryMapEntry() with the lock locked. >
Indeed. But I am wondering now if that means some code paths don't set the protection correctly. If EfiBootServicesData and EfiConventionalMemory use the same policy (which should be the case 99.9% of the time) it doesn't matter, but if the configured policy is different, the permissions will go out of sync. > -----Original Message----- > From: Gao, Liming > Sent: Monday, February 27, 2017 2:43 PM > To: Ard Biesheuvel <ard.biesheu...@linaro.org>; edk2-devel@lists.01.org; Yao, > Jiewen <jiewen....@intel.com>; leif.lindh...@linaro.org > Cc: af...@apple.com; Kinney, Michael D <michael.d.kin...@intel.com>; > ler...@redhat.com; Tian, Feng <feng.t...@intel.com>; Zeng, Star > <star.z...@intel.com> > Subject: RE: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool > allocations > > Ard: > I agree to separate lock for pool allocations. I suggest you update > CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding > CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to > introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI (). > > Thanks > Liming >>-----Original Message----- >>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >>Sent: Monday, February 27, 2017 2:30 AM >>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen....@intel.com>; >>leif.lindh...@linaro.org >>Cc: af...@apple.com; Kinney, Michael D <michael.d.kin...@intel.com>; >>Gao, Liming <liming....@intel.com>; ler...@redhat.com; Tian, Feng >><feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>; Ard Biesheuvel >><ard.biesheu...@linaro.org> >>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for >>pool allocations >> >>In preparation of adding memory permission attribute management to the >>pool allocator, split off the locking of the pool metadata into a >>separate lock. This is an improvement in itself, given that pool >>allocation can only interfere with the page allocation bookkeeping if >>pool pages are allocated or released. But it is also required to ensure >>that the permission attribute management does not deadlock, given that >>it may trigger page table splits leading to additional page tables >>being allocated. >> >>Contributed-under: TianoCore Contribution Agreement 1.0 >>Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> >>--- >> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++---- >> 1 file changed, 43 insertions(+), 10 deletions(-) >> >>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c >>b/MdeModulePkg/Core/Dxe/Mem/Pool.c >>index 7afd2d312c1d..410615e0dee9 100644 >>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c >>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c >>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, >>EITHER EXPRESS OR IMPLIED. >> #include "DxeMain.h" >> #include "Imem.h" >> >>+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE >>(TPL_NOTIFY); >>+ >> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0') >> typedef struct { >> UINT32 Signature; >>@@ -239,13 +241,13 @@ CoreInternalAllocatePool ( >> // >> // Acquire the memory lock and make the allocation >> // >>- Status = CoreAcquireLockOrFail (&gMemoryLock); >>+ Status = CoreAcquireLockOrFail (&mPoolMemoryLock); >> if (EFI_ERROR (Status)) { >> return EFI_OUT_OF_RESOURCES; >> } >> >> *Buffer = CoreAllocatePoolI (PoolType, Size); >>- CoreReleaseMemoryLock (); >>+ CoreReleaseLock (&mPoolMemoryLock); >> return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; } >> >>@@ -289,6 +291,23 @@ CoreAllocatePool ( >> return Status; >> } >> >>+STATIC >>+VOID * >>+CoreAllocatePoolPagesI ( >>+ IN EFI_MEMORY_TYPE PoolType, >>+ IN UINTN NoPages, >>+ IN UINTN Granularity >>+ ) >>+{ >>+ VOID *Buffer; >>+ >>+ CoreAcquireMemoryLock (); >>+ Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity); >>+ CoreReleaseMemoryLock (); >>+ >>+ return Buffer; >>+} >>+ >> /** >> Internal function to allocate pool of a particular type. >> Caller must have the memory lock held @@ -317,7 +336,7 @@ >>CoreAllocatePoolI ( >> UINTN NoPages; >> UINTN Granularity; >> >>- ASSERT_LOCKED (&gMemoryLock); >>+ ASSERT_LOCKED (&mPoolMemoryLock); >> >> if (PoolType == EfiACPIReclaimMemory || >> PoolType == EfiACPIMemoryNVS || >>@@ -355,7 +374,7 @@ CoreAllocatePoolI ( >> if (Index >= SIZE_TO_LIST (Granularity)) { >> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES >>(Granularity) - 1; >> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1); >>- Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity); >>+ Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity); >> goto Done; >> } >> >>@@ -383,7 +402,7 @@ CoreAllocatePoolI ( >> // >> // Get another page >> // >>- NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES >>(Granularity), Granularity); >>+ NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES >>(Granularity), Granularity); >> if (NewPage == NULL) { >> goto Done; >> } >>@@ -486,9 +505,9 @@ CoreInternalFreePool ( >> return EFI_INVALID_PARAMETER; >> } >> >>- CoreAcquireMemoryLock (); >>+ CoreAcquireLock (&mPoolMemoryLock); >> Status = CoreFreePoolI (Buffer, PoolType); >>- CoreReleaseMemoryLock (); >>+ CoreReleaseLock (&mPoolMemoryLock); >> return Status; >> } >> >>@@ -525,6 +544,19 @@ CoreFreePool ( >> return Status; >> } >> >>+STATIC >>+VOID >>+CoreFreePoolPagesI ( >>+ IN EFI_MEMORY_TYPE PoolType, >>+ IN EFI_PHYSICAL_ADDRESS Memory, >>+ IN UINTN NoPages >>+ ) >>+{ >>+ CoreAcquireMemoryLock (); >>+ CoreFreePoolPages (Memory, NoPages); >>+ CoreReleaseMemoryLock (); >>+} >>+ >> /** >> Internal function to free a pool entry. >> Caller must have the memory lock held @@ -573,7 +605,7 @@ >>CoreFreePoolI ( >> // >> ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE); >> ASSERT (Head->Size == Tail->Size); >>- ASSERT_LOCKED (&gMemoryLock); >>+ ASSERT_LOCKED (&mPoolMemoryLock); >> >> if (Tail->Signature != POOL_TAIL_SIGNATURE) { >> return EFI_INVALID_PARAMETER; >>@@ -624,7 +656,7 @@ CoreFreePoolI ( >> // >> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES >>(Granularity) - 1; >> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1); >>- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages); >>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) >>(UINTN) Head, NoPages); >> >> } else { >> >>@@ -680,7 +712,8 @@ CoreFreePoolI ( >> // >> // Free the page >> // >>- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, >>EFI_SIZE_TO_PAGES (Granularity)); >>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) >>(UINTN)NewPage, >>+ EFI_SIZE_TO_PAGES (Granularity)); >> } >> } >> } >>-- >>2.7.4 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
