From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 3DFBC1A1E00 for ; Thu, 4 Aug 2016 01:07:16 -0700 (PDT) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9D6684E320; Thu, 4 Aug 2016 08:07:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-59.phx2.redhat.com [10.3.116.59]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7487DDe027182; Thu, 4 Aug 2016 04:07:13 -0400 To: Hao Wu References: <1470273870-14376-1-git-send-email-hao.a.wu@intel.com> From: Laszlo Ersek Cc: edk2-devel@ml01.01.org, Ard Biesheuvel , "Leif Lindholm (Linaro address)" Message-ID: Date: Thu, 4 Aug 2016 10:07:12 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1470273870-14376-1-git-send-email-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 04 Aug 2016 08:07:15 +0000 (UTC) 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:07:16 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Hello Hao, 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. (1) Do you plan to add optimized implementations of IsZeroBuffer() later on? For example, in QEMU, the buffer_is_zero() function has optimized implementations for: - AVX2 - SSE2 - AArch64 I see one of the library instances is called BaseMemoryLibSse2, so an SSE2 optimized implementation could be possible. 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.) Just an idea, of course. (2) The edk2 tree contains a number of zero GUID comparisons: $ git grep -i -e zeroguid --and -e compareguid > BaseTools/Source/C/GenFfs/GenFfs.c:749: if (CompareGuid (&FileGuid, &mZeroGuid) == 0) { > BaseTools/Source/C/GenFv/GenFvInternalLib.c:4134: if (CompareGuid (&mCapDataInfo.CapGuid, &mZeroGuid) == 0) { > BaseTools/Source/C/GenSec/GenSec.c:854: if (CompareGuid (VendorGuid, &mZeroGuid) == 0) { > BaseTools/Source/C/GenSec/GenSec.c:900: if (CompareGuid (VendorGuid, &mZeroGuid) == 0) { > BaseTools/Source/C/GenSec/GenSec.c:1368: if ((SectType != EFI_SECTION_GUID_DEFINED) && (CompareGuid (&VendorGuid, &mZeroGuid) != 0)) { > BaseTools/Source/C/GenSec/GenSec.c:1421: if (InputFileAlign != NULL && (CompareGuid (&VendorGuid, &mZeroGuid) != 0)) { > EdkCompatibilityPkg/Compatibility/FrameworkHiiOnUefiHiiThunk/Utility.c:717: if (FormSetGuid == NULL || CompareGuid (FormSetGuid, &gZeroGuid)) { > EdkCompatibilityPkg/Compatibility/PiSmbiosRecordOnDataHubSmbiosRecordThunk/Translate.c:85: for (; !CompareGuid (&(mConversionTable[Index].SubClass), &gZeroGuid); Index++) { > EdkCompatibilityPkg/Compatibility/PiSmbiosRecordOnDataHubSmbiosRecordThunk/Translate.c:96: if (CompareGuid (&(mConversionTable[Index].SubClass), &gZeroGuid)) { > EdkCompatibilityPkg/Sample/Tools/Source/GenAprioriFile/GenAprioriFile.c:196: if (CompareGuid (&GuidIn, &ZeroGuid) != 0) { > EdkCompatibilityPkg/Sample/Tools/Source/GenFfsFile/GenFfsFile.c:1328: if (CompareGuid (&SignGuid, &mZeroGuid) != 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 = 0; !CompareGuid (&DriverGuidArray[Index], &gZeroGuid); Index++) { > MdeModulePkg/Library/VarCheckHiiLib/VarCheckHiiGenFromFv.c:424: if (CompareGuid (&DriverGuidArray[0], &gZeroGuid)) { > MdeModulePkg/Universal/SetupBrowserDxe/Expression.c:2832: } else if (CompareGuid (&OpCode->Guid, &gZeroGuid) != 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 != 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)) { Do you plan to migrate these source files to the new IsZeroGuid() function? (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.) (3) I think patch #3 should be implemented differently -- I believe the current series can break bisection between patch #2 and patch #3. I suggest to first rename the IsZeroBuffer() functions in SecurityPkg/Tcg to IsZeroBufferInternal(), as patch #1. Then add IsZeroBuffer() and IsZeroGuid() to the BaseMemoryLib instances, as patch #2 and patch #3. Finally, switch SecurityPkg/Tcg from IsZeroBufferInternal() to IsZeroBuffer() in patch #4. What do you think? Thanks Laszlo