Sorry missing that one. I'll update it. Thanks for the comments. Regards, Jian
> -----Original Message----- > From: Zeng, Star > Sent: Friday, October 26, 2018 9:18 AM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel <edk2-devel- > boun...@lists.01.org>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Ni, Ruiyu > <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Laszlo Ersek > <ler...@redhat.com>; Zeng, Star <star.z...@intel.com> > Subject: Re: [edk2] [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire > GCD memory lock > > On 2018/10/25 15:20, Wang, Jian J wrote: > > Sorry, forgot to update commit message: > > > >> 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 moving the memory allocation in CoreGetMemorySpaceMap() > >> to be out of the GCD memory map lock. > > > > Regards, > > Jian > > > > > >> -----Original Message----- > >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] > >> Sent: Thursday, October 25, 2018 3:18 PM > >> To: edk2-devel@lists.01.org > >> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Ni, Ruiyu > >> <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Zeng, Star > >> <star.z...@intel.com>; Laszlo Ersek <ler...@redhat.com> > >> Subject: [edk2] [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD > >> memory lock > >> > >>> v4 changes: > >>> a. add more comments from Laszlo > >> > >> 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() > >> and CoreGetIoSpaceMap() to be out of the GCD memory map 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 | 87 > +++++++++++++++++++++++++++++- > >> ----------- > >> 1 file changed, 62 insertions(+), 25 deletions(-) > >> > >> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > >> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > >> index d9c65a8045..f99d6bb933 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,75 @@ CoreGetMemorySpaceMap ( > >> return EFI_INVALID_PARAMETER; > >> } > >> > >> - CoreAcquireGcdMemoryLock (); > >> + *NumberOfDescriptors = 0; > >> + *MemorySpaceMap = NULL; > >> > >> // > >> - // Count the number of descriptors > >> + // Take the lock, for entering the loop with the lock held. > >> // > >> - *NumberOfDescriptors = CoreCountGcdMapEntry > (&mGcdMemorySpaceMap); > >> + CoreAcquireGcdMemoryLock (); > >> + while (TRUE) { > >> + // > >> + // Count the number of descriptors. It might be done more than once > > How about using "Count the descriptors" here? No need to send new patch > series for it. > > >> because > >> + // there's code which has to be running outside the GCD lock. > > I have given comment for this line at V3 series. > How about using "AllocatePool() calling code below" instead of "there's > code" to be more specific? > > Thanks, > Star > > >> + // > >> + 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; > >> + } > >> + // > >> + // We're done; exit the loop with the lock held. > >> + // > >> + break; > >> + } > >> > >> - // > >> - // Allocate the MemorySpaceMap > >> - // > >> - *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof > >> (EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); > >> - if (*MemorySpaceMap == NULL) { > >> - Status = EFI_OUT_OF_RESOURCES; > >> - goto Done; > >> - } > >> + // > >> + // 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 count got before for another round of check to > make > >> + // sure we won't miss any, since we have code running outside the GCD > lock. > >> + // > >> + *NumberOfDescriptors = DescriptorCount; > >> + // > >> + // Re-acquire the lock, for the next iteration. > >> + // > >> + CoreAcquireGcdMemoryLock (); > >> + } > >> // > >> - // Fill in the MemorySpaceMap > >> + // We exited the loop with the lock held, release it. > >> // > >> - 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; > >> - } > >> - Status = EFI_SUCCESS; > >> - > >> -Done: > >> CoreReleaseGcdMemoryLock (); > >> - return Status; > >> + > >> + return EFI_SUCCESS; > >> } > >> > >> > >> -- > >> 2.16.2.windows.1 > >> > >> _______________________________________________ > >> 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
