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

[Laszlo Ersek <lersek@redhat.com>]

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