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


Laszlo Ersek
 

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

Join devel@edk2.groups.io to automatically receive all group messages.