Re: [PATCH v3 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock

["Wang, Jian J" <jian.j.wang@intel.com>]

Sure. I'll change them. Thanks.

Regards,
Jian


> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, October 25, 2018 11:23 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH v3 5/6] MdeModulePkg/Core: prevent re-acquire
> GCD memory lock
> 
> Some minor comments are added below.
> With them addressed, Reviewed-by: Star Zeng <star.zeng@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.zeng@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@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;
> >   }
> >
> >
> >