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


Wu, Hao A
 

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

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