public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Zeng, Star" <star.zeng@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	"Yao, Jiewen" <jiewen.yao@intel.com>,
	"leif.lindholm@linaro.org" <leif.lindholm@linaro.org>
Cc: "Tian, Feng" <feng.tian@intel.com>,
	"afish@apple.com" <afish@apple.com>,
	"Gao, Liming" <liming.gao@intel.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>,
	"lersek@redhat.com" <lersek@redhat.com>,
	"Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
Date: Mon, 27 Feb 2017 01:56:54 +0000	[thread overview]
Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B82C082@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <1488133805-4773-5-git-send-email-ard.biesheuvel@linaro.org>

Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
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: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


  parent reply	other threads:[~2017-02-27  1:56 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 1/6] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
2017-02-27  6:43   ` Gao, Liming
2017-02-27  8:13     ` Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 3/6] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
2017-02-26 19:52   ` Ard Biesheuvel
2017-02-27  1:56   ` Zeng, Star [this message]
2017-02-27  8:15     ` Ard Biesheuvel
2017-02-27  8:20       ` Zeng, Star
2017-02-27  6:43   ` Gao, Liming
2017-02-27  6:50     ` Zeng, Star
2017-02-27  8:15       ` Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 5/6] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 6/6] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
2017-02-27  6:46   ` Gao, Liming
2017-02-27  9:56   ` Laszlo Ersek
2017-02-27  9:57     ` Ard Biesheuvel
2017-02-27  5:19 ` [PATCH v3 0/6] RFC: increased memory protection Yao, Jiewen
2017-02-27 13:14 ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0C09AFA07DD0434D9E2A0C6AEB0483103B82C082@shsmsx102.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox