From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by ml01.01.org (Postfix) with ESMTP id 44E931A1DEB for ; Thu, 4 Aug 2016 01:44:50 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 04 Aug 2016 01:44:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,469,1464678000"; d="scan'208";a="1008229621" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga001.jf.intel.com with ESMTP; 04 Aug 2016 01:44:50 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 4 Aug 2016 01:44:49 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 4 Aug 2016 01:44:49 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.8]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.150]) with mapi id 14.03.0248.002; Thu, 4 Aug 2016 16:44:47 +0800 From: "Wu, Hao A" To: Laszlo Ersek CC: "edk2-devel@ml01.01.org" , "Leif Lindholm (Linaro address)" , Ard Biesheuvel Thread-Topic: [edk2] [PATCH 0/3] Add APIs IsZeroBuffer and IsZeroGuid in BaseMemoryLib Thread-Index: AQHR7e76Hf/mYYwtpE6v/M0MsFvSjaA37G4AgACNwbA= Date: Thu, 4 Aug 2016 08:44:46 +0000 Message-ID: References: <1470273870-14376-1-git-send-email-hao.a.wu@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 0/3] Add APIs IsZeroBuffer and IsZeroGuid in BaseMemoryLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Aug 2016 08:44:50 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of La= szlo > Ersek > Sent: Thursday, August 04, 2016 4:07 PM > To: Wu, Hao A > Cc: edk2-devel@ml01.01.org; Leif Lindholm (Linaro address); Ard Biesheuve= l > Subject: Re: [edk2] [PATCH 0/3] Add APIs IsZeroBuffer and IsZeroGuid in > BaseMemoryLib >=20 > Hello Hao, >=20 > On 08/04/16 03:24, Hao Wu wrote: > > The patch series will add two APIs in BaseMemoryLib: > > 1. IsZeroBuffer() > > The API is used to check if the contents of a buffer are all zeros. > > > > 2. IsZeroGuid() > > The API is used to check if the given GUID is a zero GUID. > > > > In order to resolve build issues in SecurityPkg, the series will also > > remove the internal implementation of IsZeroBuffer() in modules within > > SecurityPkg\Tcg and use the one in BaseMemoryLib instead. >=20 > (1) Do you plan to add optimized implementations of IsZeroBuffer() later > on? >=20 > For example, in QEMU, the buffer_is_zero() function has optimized > implementations for: > - AVX2 > - SSE2 > - AArch64 >=20 > I see one of the library instances is called BaseMemoryLibSse2, so an > SSE2 optimized implementation could be possible. >=20 > Also, as far as I understand the example in the QEMU code, for AArch64, > the optimized implementation could go in all of the library instances > that build on AArch64. (QEMU calls the vgetq_lane_u64() function -- I'm > unsure how it would be available in edk2. Perhaps AArch64 assembly would > be necessary.) >=20 > Just an idea, of course. I will hold this patch series and do more research on the optimized implementations of IsZeroBuffer() for SSE2 and other library instances. >=20 > (2) The edk2 tree contains a number of zero GUID comparisons: >=20 > $ git grep -i -e zeroguid --and -e compareguid >=20 > > BaseTools/Source/C/GenFfs/GenFfs.c:749: if (CompareGuid (&FileGuid, > &mZeroGuid) =3D=3D 0) { > > BaseTools/Source/C/GenFv/GenFvInternalLib.c:4134: if (CompareGuid > (&mCapDataInfo.CapGuid, &mZeroGuid) =3D=3D 0) { > > BaseTools/Source/C/GenSec/GenSec.c:854: if (CompareGuid (VendorGuid, > &mZeroGuid) =3D=3D 0) { > > BaseTools/Source/C/GenSec/GenSec.c:900: if (CompareGuid (VendorGuid, > &mZeroGuid) =3D=3D 0) { > > BaseTools/Source/C/GenSec/GenSec.c:1368: if ((SectType !=3D > EFI_SECTION_GUID_DEFINED) && (CompareGuid (&VendorGuid, > &mZeroGuid) !=3D 0)) { > > BaseTools/Source/C/GenSec/GenSec.c:1421: if (InputFileAlign !=3D NUL= L && > (CompareGuid (&VendorGuid, &mZeroGuid) !=3D 0)) { > > > EdkCompatibilityPkg/Compatibility/FrameworkHiiOnUefiHiiThunk/Utility.c:71= 7: > if (FormSetGuid =3D=3D NULL || CompareGuid (FormSetGuid, &gZeroGuid)) { > > > EdkCompatibilityPkg/Compatibility/PiSmbiosRecordOnDataHubSmbiosRecordT > hunk/Translate.c:85: for (; !CompareGuid > (&(mConversionTable[Index].SubClass), &gZeroGuid); Index++) { > > > EdkCompatibilityPkg/Compatibility/PiSmbiosRecordOnDataHubSmbiosRecordT > hunk/Translate.c:96: if (CompareGuid (&(mConversionTable[Index].SubCla= ss), > &gZeroGuid)) { > > > EdkCompatibilityPkg/Sample/Tools/Source/GenAprioriFile/GenAprioriFile.c:1= 96: > if (CompareGuid (&GuidIn, &ZeroGuid) !=3D 0) { > > EdkCompatibilityPkg/Sample/Tools/Source/GenFfsFile/GenFfsFile.c:1328: > if (CompareGuid (&SignGuid, &mZeroGuid) !=3D 0) { > > EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c:1348: if (CompareGuid (&= File- > >FvNameGuid, &gZeroGuid)) { > > IntelFrameworkModulePkg/Universal/DataHubDxe/DataHub.c:142: > (CompareGuid (&FilterEntry->FilterDataRecordGuid, &gZeroGuid) || > > MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c:258: if > (!CompareGuid (&DriverInfo->FileName, &gZeroGuid)) { > > MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c:449: if > (CompareGuid (PcdGetPtr (PcdDriverHealthConfigureForm), &gZeroGuid)) { > > MdeModulePkg/Library/VarCheckHiiLib/VarCheckHiiGenFromFv.c:375: for > (Index =3D 0; !CompareGuid (&DriverGuidArray[Index], &gZeroGuid); Index++= ) { > > MdeModulePkg/Library/VarCheckHiiLib/VarCheckHiiGenFromFv.c:424: if > (CompareGuid (&DriverGuidArray[0], &gZeroGuid)) { > > MdeModulePkg/Universal/SetupBrowserDxe/Expression.c:2832: } else i= f > (CompareGuid (&OpCode->Guid, &gZeroGuid) !=3D 0) { > > MdeModulePkg/Universal/SetupBrowserDxe/Presentation.c:361: if > (!CompareGuid (&Statement->RefreshGuid, &gZeroGuid)) { > > MdeModulePkg/Universal/SetupBrowserDxe/Presentation.c:376: if > ((!CompareGuid (&Statement->RefreshGuid, &gZeroGuid)) || (Statement- > >RefreshInterval !=3D 0)) { > > MdeModulePkg/Universal/SetupBrowserDxe/Presentation.c:631: if > (!CompareGuid (&gCurrentSelection->Form->RefreshGuid, &gZeroGuid)) { > > MdeModulePkg/Universal/SetupBrowserDxe/Presentation.c:1413: } else if > (!CompareGuid (&Statement->HiiValue.Value.ref.FormSetGuid, &gZeroGuid)) { > > MdeModulePkg/Universal/SetupBrowserDxe/Setup.c:184: if > (CompareGuid (&MenuList->FormSetGuid, &gZeroGuid)) { > > MdeModulePkg/Universal/SetupBrowserDxe/Setup.c:5659: if > (CompareGuid (FormSetGuid, &gZeroGuid) || > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c:376: if > (CompareGuid (&VendorGuid, &gZeroGuid)) { > > > SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c:205: > if (!CompareGuid (&PartitionEntry->PartitionTypeGUID, &gZeroGuid)) { > > > SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c:241: > if (!CompareGuid (&PartitionEntry->PartitionTypeGUID, &gZeroGuid)) { > > SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c:205: > if (!CompareGuid (&PartitionEntry->PartitionTypeGUID, &gZeroGuid)) { > > SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c:239: > if (!CompareGuid (&PartitionEntry->PartitionTypeGUID, &gZeroGuid)) { >=20 > Do you plan to migrate these source files to the new IsZeroGuid() > function? >=20 > (There might be more -- "zeroguid" and "compareguid" could be on > different lines, so perhaps it's better to search for just "zeroguid", > and audit each location separately.) Yes, we plan to replace the usages of "compareguid with zeroguid" with the new API. It will be done independently by another patch later. For the above-listed results, I think we won't handle the cases used in tools. Since these codes won't be able to use the BaseMemoryLib. >=20 > (3) I think patch #3 should be implemented differently -- I believe the > current series can break bisection between patch #2 and patch #3. >=20 > I suggest to first rename the IsZeroBuffer() functions in > SecurityPkg/Tcg to IsZeroBufferInternal(), as patch #1. >=20 > Then add IsZeroBuffer() and IsZeroGuid() to the BaseMemoryLib instances, > as patch #2 and patch #3. >=20 > Finally, switch SecurityPkg/Tcg from IsZeroBufferInternal() to > IsZeroBuffer() in patch #4. >=20 > What do you think? Yes, I will follow this approach when sending out the next version of patch= . Best Regards, Hao Wu >=20 > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel