From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 81C942117FD7A; Thu, 25 Oct 2018 00:21:01 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Oct 2018 00:21:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,423,1534834800"; d="scan'208";a="84016511" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga007.jf.intel.com with ESMTP; 25 Oct 2018 00:21:00 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 25 Oct 2018 00:20:59 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.161]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.199]) with mapi id 14.03.0415.000; Thu, 25 Oct 2018 15:20:57 +0800 From: "Wang, Jian J" To: edk2-devel , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , "Ni, Ruiyu" , "Yao, Jiewen" , "Zeng, Star" , Laszlo Ersek Thread-Topic: [edk2] [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock Thread-Index: AQHUbDMDpajmZJdy7UCBmTs8MZm+iqUvjfRw Date: Thu, 25 Oct 2018 07:20:57 +0000 Message-ID: References: <20181025071805.6692-1-jian.j.wang@intel.com> <20181025071805.6692-6-jian.j.wang@intel.com> In-Reply-To: <20181025071805.6692-6-jian.j.wang@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTQ5OWI0ZjYtODFjNC00NDAzLTlkOWItODQ1ODUxMjNmNGIwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidVNNSG5hbDNLUWJTdjNFbTlKWFpOVmNNQnNZeGs2RTVuNHkwXC9cL2grcHhiYUxVQk5PWXBJamxYQktlRE03eGNcLyJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Oct 2018 07:21:01 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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. >=20 > 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-bounces@lists.01.org] > Sent: Thursday, October 25, 2018 3:18 PM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D ; Ni, Ruiyu > ; Yao, Jiewen ; Zeng, Star > ; Laszlo Ersek > Subject: [edk2] [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD > memory lock >=20 > > v4 changes: > > a. add more comments from Laszlo >=20 > 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. >=20 > The solution is move the memory allocation in CoreGetMemorySpaceMap() > and CoreGetIoSpaceMap() to be out of the GCD memory map lock. >=20 > CoreDumpGcdMemorySpaceMap() > =3D> CoreGetMemorySpaceMap() > =3D> CoreAcquireGcdMemoryLock () * > AllocatePool() > =3D> InternalAllocatePool() > =3D> CoreAllocatePool() > =3D> CoreAllocatePoolI() > =3D> CoreAllocatePoolPagesI() > =3D> CoreAllocatePoolPages() > =3D> FindFreePages() > =3D> PromoteMemoryResource() > =3D> CoreAcquireGcdMemoryLock() ** >=20 > Cc: Star Zeng > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 87 +++++++++++++++++++++++++++++- > ----------- > 1 file changed, 62 insertions(+), 25 deletions(-) >=20 > 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; >=20 > // > // Make sure parameters are valid > @@ -1706,38 +1706,75 @@ CoreGetMemorySpaceMap ( > return EFI_INVALID_PARAMETER; > } >=20 > - CoreAcquireGcdMemoryLock (); > + *NumberOfDescriptors =3D 0; > + *MemorySpaceMap =3D NULL; >=20 > // > - // Count the number of descriptors > + // Take the lock, for entering the loop with the lock held. > // > - *NumberOfDescriptors =3D CoreCountGcdMapEntry (&mGcdMemorySpaceMap); > + CoreAcquireGcdMemoryLock (); > + while (TRUE) { > + // > + // Count the number of descriptors. It might be done more than once > because > + // there's code which has to be running outside the GCD lock. > + // > + DescriptorCount =3D CoreCountGcdMapEntry (&mGcdMemorySpaceMap); > + if (DescriptorCount =3D=3D *NumberOfDescriptors) { > + // > + // Fill in the MemorySpaceMap if no memory space map change. > + // > + Descriptor =3D *MemorySpaceMap; > + Link =3D mGcdMemorySpaceMap.ForwardLink; > + while (Link !=3D &mGcdMemorySpaceMap) { > + Entry =3D CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATU= RE); > + BuildMemoryDescriptor (Descriptor, Entry); > + Descriptor++; > + Link =3D Link->ForwardLink; > + } > + // > + // We're done; exit the loop with the lock held. > + // > + break; > + } >=20 > - // > - // Allocate the MemorySpaceMap > - // > - *MemorySpaceMap =3D AllocatePool (*NumberOfDescriptors * sizeof > (EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); > - if (*MemorySpaceMap =3D=3D NULL) { > - Status =3D 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 alr= eady > + // allocated if there's map descriptor change during memory allocati= on at > + // last time. > + // > + if (*MemorySpaceMap !=3D NULL) { > + FreePool (*MemorySpaceMap); > + } >=20 > + *MemorySpaceMap =3D AllocatePool (DescriptorCount * > + sizeof (EFI_GCD_MEMORY_SPACE_DESCRIP= TOR)); > + if (*MemorySpaceMap =3D=3D NULL) { > + *NumberOfDescriptors =3D 0; > + return EFI_OUT_OF_RESOURCES; > + } > + > + // > + // Save the descriptor count got before for another round of check t= o make > + // sure we won't miss any, since we have code running outside the GC= D lock. > + // > + *NumberOfDescriptors =3D 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 =3D *MemorySpaceMap; > - Link =3D mGcdMemorySpaceMap.ForwardLink; > - while (Link !=3D &mGcdMemorySpaceMap) { > - Entry =3D CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE); > - BuildMemoryDescriptor (Descriptor, Entry); > - Descriptor++; > - Link =3D Link->ForwardLink; > - } > - Status =3D EFI_SUCCESS; > - > -Done: > CoreReleaseGcdMemoryLock (); > - return Status; > + > + return EFI_SUCCESS; > } >=20 >=20 > -- > 2.16.2.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel