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

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
>


  reply	other threads:[~2017-02-27  8:15 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
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 [this message]
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=CAKv+Gu-TA0tKMEYs2CYtj5aOpLpxt1Fkf5+Zaj98VA+vdtv3Wg@mail.gmail.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