Some minor comments are added below.
With them addressed, Reviewed-by: Star Zeng <star.z...@intel.com>.
On 2018/10/24 13:26, Jian J Wang wrote:
v3 changes:
a. drop the changes to CoreGetIoSpaceMap() because it won't cause
problem
b. refine the logic in changes of CoreGetMemorySpaceMap()
and add more comments
This issue is hidden in current code but exposed by introduction
of freed-memory guard feature due to the fact that the feature
will turn all pool allocation to page allocation.
The solution is move the memory allocation in CoreGetMemorySpaceMap()
Use 'moving' instead of 'move'?
and CoreGetIoSpaceMap() to be out of the GCD memory map lock.
Please remove CoreGetIoSpaceMap() as the code does not touch it at all.
Although it's rare, a while-loop is added to make sure that the count
of descriptor of memory map is the same after memory allocation, because
it's executed outside the GCD memory lock.
CoreDumpGcdMemorySpaceMap()
=> CoreGetMemorySpaceMap()
=> CoreAcquireGcdMemoryLock () *
AllocatePool()
=> InternalAllocatePool()
=> CoreAllocatePool()
=> CoreAllocatePoolI()
=> CoreAllocatePoolPagesI()
=> CoreAllocatePoolPages()
=> FindFreePages()
=> PromoteMemoryResource()
=> CoreAcquireGcdMemoryLock() **
Cc: Star Zeng <star.z...@intel.com>
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Ruiyu Ni <ruiyu...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 80 +++++++++++++++++++++++++++--------------
1 file changed, 54 insertions(+), 26 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index d9c65a8045..bc02945721 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR **MemorySpaceMap
)
{
- EFI_STATUS Status;
LIST_ENTRY *Link;
EFI_GCD_MAP_ENTRY *Entry;
EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor;
+ UINTN DescriptorCount;
//
// Make sure parameters are valid
@@ -1706,38 +1706,66 @@ CoreGetMemorySpaceMap (
return EFI_INVALID_PARAMETER;
}
+ *NumberOfDescriptors = 0;
+ *MemorySpaceMap = NULL;
+
CoreAcquireGcdMemoryLock ();
Better to add comment for it like below quoted from the example at
https://lists.01.org/pipermail/edk2-devel/2018-October/031279.html given
by Laszlo.
//
// Take the lock, for entering the loop with the lock held.
//
- //
- // Count the number of descriptors
- //
- *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
+ while (TRUE) {
+ //
+ // Count the number of descriptors. It might be done more than once because
Use 'count' instead of 'number' to match the variable name?
+ // there's code which has to be running outside the GCD lock.
Use "AllocatePool() calling code below" instead of "there's code"?
+ //
+ DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
+ if (DescriptorCount == *NumberOfDescriptors) {
+ //
+ // Fill in the MemorySpaceMap if no memory space map change.
+ //
+ Descriptor = *MemorySpaceMap;
+ Link = mGcdMemorySpaceMap.ForwardLink;
+ while (Link != &mGcdMemorySpaceMap) {
+ Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
+ BuildMemoryDescriptor (Descriptor, Entry);
+ Descriptor++;
+ Link = Link->ForwardLink;
+ }
- //
- // Allocate the MemorySpaceMap
- //
- *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof
(EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
- if (*MemorySpaceMap == NULL) {
- Status = EFI_OUT_OF_RESOURCES;
- goto Done;
- }
+ break;
+ }
- //
- // Fill in the MemorySpaceMap
- //
- Descriptor = *MemorySpaceMap;
- Link = mGcdMemorySpaceMap.ForwardLink;
- while (Link != &mGcdMemorySpaceMap) {
- Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
- BuildMemoryDescriptor (Descriptor, Entry);
- Descriptor++;
- Link = Link->ForwardLink;
+ //
+ // Release the lock before memory allocation, because it might cause
+ // GCD lock conflict in one of calling path in AllocatPool().
+ //
+ CoreReleaseGcdMemoryLock ();
+
+ //
+ // Allocate memory to store the MemorySpaceMap. Note it might be already
+ // allocated if there's map descriptor change during memory allocation at
+ // last time.
+ //
+ if (*MemorySpaceMap != NULL) {
+ FreePool (*MemorySpaceMap);
+ }
+
+ *MemorySpaceMap = AllocatePool (DescriptorCount *
+ sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
+ if (*MemorySpaceMap == NULL) {
+ *NumberOfDescriptors = 0;
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ //
+ // Save the descriptor number got before for another round of check to make
Use 'count' instead of 'number' to match the variable name?
+ // sure we won't miss any, since we have code running outside the GCD lock.
+ //
+ *NumberOfDescriptors = DescriptorCount;
+ CoreAcquireGcdMemoryLock ();
Better to add comment for it like below quoted from the example at
https://lists.01.org/pipermail/edk2-devel/2018-October/031279.html given
by Laszlo.
//
// Re-acquire the lock, for the next iteration.
//
}
- Status = EFI_SUCCESS;
-Done:
CoreReleaseGcdMemoryLock ();
Better to add comment for it like below quoted from the example at
https://lists.01.org/pipermail/edk2-devel/2018-October/031279.html given
by Laszlo.
//
// We exited the loop with the lock held, release it.
//
Thanks,
Star
- return Status;
+
+ return EFI_SUCCESS;
}
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
