| Message ID | 1488133805-4773-5-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RFC: increased memory protection | expand |
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(-) 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-develdiff --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 (
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
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
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)); } } }
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(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel