Re: [PATCH 0/3] Add APIs IsZeroBuffer and IsZeroGuid in BaseMemoryLib

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Thursday, August 04, 2016 4:07 PM
> To: Wu, Hao A
> Cc: edk2-devel@ml01.01.org; Leif Lindholm (Linaro address); Ard Biesheuvel
> Subject: Re: [edk2] [PATCH 0/3] Add APIs IsZeroBuffer and IsZeroGuid in
> BaseMemoryLib
> 
> 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.

I will hold this patch series and do more research on the optimized
implementations of IsZeroBuffer() for SSE2 and other library instances.

> 
> (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/PiSmbiosRecordOnDataHubSmbiosRecordT
> hunk/Translate.c:85:    for (; !CompareGuid
> (&(mConversionTable[Index].SubClass), &gZeroGuid); Index++) {
> >
> EdkCompatibilityPkg/Compatibility/PiSmbiosRecordOnDataHubSmbiosRecordT
> hunk/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.)

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.

> 
> (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?

Yes, I will follow this approach when sending out the next version of patch.

Best Regards,
Hao Wu

> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel