Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 27 February 2017 at 06:50, Zeng, Star <star.zeng@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.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
> Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@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.biesheuvel@linaro.org]
>>Sent: Monday, February 27, 2017 2:30 AM
>>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>>leif.lindholm@linaro.org
>>Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>;
>>Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng
>><feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>><ard.biesheuvel@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.biesheuvel@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
>