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
On 26 February 2017 at 18:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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 ();
This should probably be
EFI_STATUS Status;
Status = CoreAcquireLockOrFail (&gMemoryLock);
if (EFI_ERROR (Status)) {
return NULL;
}
to preserve the old behavior.
> + 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
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 27 February 2017 at 01:56, Zeng, Star <star.zeng@intel.com> wrote:
> Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?
>
I need it after patch 6. But perhaps it is better to only add it
there, not here.
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
If it is really needed, I am fine to either this patch or another patch to add 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 4:16 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com
Subject: Re: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
On 27 February 2017 at 01:56, Zeng, Star <star.zeng@intel.com> wrote:
> Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?
>
I need it after patch 6. But perhaps it is better to only add it there, not here.
> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
CoreAllocatePoolPages() could not be updated simply by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(), it is also used by AllocateMemoryMapEntry() with the lock locked.
Thanks,
Star
-----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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.