* [PATCH v2 0/3] MdePkg/BaseMemoryLibOptDxe: generic and ARM/AARCH64 fixes
@ 2016-09-19 8:13 Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 1/3] MdePkg/BaseMemoryLibOptDxe ARM: fix arithmetic bugs in CompareMem() Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-19 8:13 UTC (permalink / raw)
To: edk2-devel, liming.gao; +Cc: leif.lindholm, vishalo, Ard Biesheuvel
Some pending fixes for BaseMemoryLib* and BaseMemoryLibOptDxe, now resent
as a single series.
Patch #1 fixes an arithmetic bug in BaseMemoryLibOptDxe for ARM, which renders
it unusable at the moment.
Patch #2 adds some asserts that are mentioned in the respective comment blocks
but are missing from the code.
Patch #3 fixes a performance regressions observed by moving from the (soon to be
deprecated) BaseMemoryLibStm in ArmPkg to BaseMemoryLibOptDxe. The culprit has
been identified to be CompareGuid(), which needlessly uses unaligned accessors
(since this library can only be used in a context where unaligned accesses are
allowed)
Ard Biesheuvel (3):
MdePkg/BaseMemoryLibOptDxe ARM: fix arithmetic bugs in CompareMem()
MdePkg/BaseMemoryLib*: add missing ASSERT()s
MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: implement accelerated GUID
functions
MdePkg/Library/BaseMemoryLib/MemLibGuid.c | 8 ++
MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c | 8 ++
MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S | 40 ++++++
MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 66 +++++++++
MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.asm | 69 +++++++++
MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S | 4 +-
MdePkg/Library/BaseMemoryLibOptDxe/Arm/MemLibGuid.c | 152 ++++++++++++++++++++
MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf | 7 +-
MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c | 8 ++
MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c | 8 ++
MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c | 8 ++
MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c | 8 ++
MdePkg/Library/PeiMemoryLib/MemLibGuid.c | 8 ++
MdePkg/Library/UefiMemoryLib/MemLibGuid.c | 8 ++
14 files changed, 399 insertions(+), 3 deletions(-)
create mode 100644 MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S
create mode 100644 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
create mode 100644 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.asm
create mode 100644 MdePkg/Library/BaseMemoryLibOptDxe/Arm/MemLibGuid.c
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/3] MdePkg/BaseMemoryLibOptDxe ARM: fix arithmetic bugs in CompareMem()
2016-09-19 8:13 [PATCH v2 0/3] MdePkg/BaseMemoryLibOptDxe: generic and ARM/AARCH64 fixes Ard Biesheuvel
@ 2016-09-19 8:13 ` Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 3/3] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: implement accelerated GUID functions Ard Biesheuvel
2 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-19 8:13 UTC (permalink / raw)
To: edk2-devel, liming.gao; +Cc: leif.lindholm, vishalo, Ard Biesheuvel
Fix two bugs:
- Erroneous shift of 2 in a bytes to bits conversion.
- Use reverse subtract rather than negate for value that is subsequently
used as operand #2 in a shift operation.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
index 951d15777a38..3aadebace30f 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
@@ -110,9 +110,9 @@ ASM_PFX(InternalMemCompareMem):
bic src1, src1, #3
bic src2, src2, #3
add limit, limit, tmp1 // Adjust the limit for the extra.
- lsl tmp1, tmp1, #2 // Bytes beyond alignment -> bits.
+ lsl tmp1, tmp1, #3 // Bytes beyond alignment -> bits.
ldr data1, [src1], #4
- neg tmp1, tmp1 // Bits to alignment -32.
+ rsb tmp1, tmp1, #32 // Bits to alignment -32.
ldr data2, [src2], #4
mov tmp2, #~0
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
2016-09-19 8:13 [PATCH v2 0/3] MdePkg/BaseMemoryLibOptDxe: generic and ARM/AARCH64 fixes Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 1/3] MdePkg/BaseMemoryLibOptDxe ARM: fix arithmetic bugs in CompareMem() Ard Biesheuvel
@ 2016-09-19 8:13 ` Ard Biesheuvel
2016-09-20 2:00 ` Wu, Hao A
2016-09-19 8:13 ` [PATCH v2 3/3] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: implement accelerated GUID functions Ard Biesheuvel
2 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-19 8:13 UTC (permalink / raw)
To: edk2-devel, liming.gao; +Cc: leif.lindholm, vishalo, Ard Biesheuvel
Add the ASSERT() statements to CopyGuid (), CompareGuid() and
IsZeroGuid() that are mentioned in the respective comments but
were missing from the actual code.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdePkg/Library/BaseMemoryLib/MemLibGuid.c | 8 ++++++++
MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c | 8 ++++++++
MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c | 8 ++++++++
MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c | 8 ++++++++
MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c | 8 ++++++++
MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c | 8 ++++++++
MdePkg/Library/PeiMemoryLib/MemLibGuid.c | 8 ++++++++
MdePkg/Library/UefiMemoryLib/MemLibGuid.c | 8 ++++++++
8 files changed, 64 insertions(+)
diff --git a/MdePkg/Library/BaseMemoryLib/MemLibGuid.c b/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
index b2590f83caef..dff9bde653a9 100644
--- a/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
diff --git a/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
diff --git a/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
diff --git a/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
diff --git a/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
diff --git a/MdePkg/Library/PeiMemoryLib/MemLibGuid.c b/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
+++ b/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
diff --git a/MdePkg/Library/UefiMemoryLib/MemLibGuid.c b/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
+++ b/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
@@ -47,6 +47,9 @@ CopyGuid (
IN CONST GUID *SourceGuid
)
{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
WriteUnaligned64 (
(UINT64*)DestinationGuid,
ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@ CompareGuid (
UINT64 HighPartOfGuid1;
UINT64 HighPartOfGuid2;
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@ IsZeroGuid (
UINT64 LowPartOfGuid;
UINT64 HighPartOfGuid;
+ ASSERT (Guid != NULL);
+
LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: implement accelerated GUID functions
2016-09-19 8:13 [PATCH v2 0/3] MdePkg/BaseMemoryLibOptDxe: generic and ARM/AARCH64 fixes Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 1/3] MdePkg/BaseMemoryLibOptDxe ARM: fix arithmetic bugs in CompareMem() Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s Ard Biesheuvel
@ 2016-09-19 8:13 ` Ard Biesheuvel
2 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-19 8:13 UTC (permalink / raw)
To: edk2-devel, liming.gao; +Cc: leif.lindholm, vishalo, Ard Biesheuvel
As reported by Vishal, CompareGuid() is a hotspot, and switching from
BaseMemoryLibStm in ArmPkg/ to BaseMemoryLibOptDxe causes a noticeable
performance regression due to the fact that BaseMemoryLibOptDxe uses
unaligned accessors explicitly to implement CompareGuid() and the related
functions.
Since BaseMemoryLibOptDxe on ARM and AARCH64 can only be used in contexts
where unaligned accesses are allowed, reimplement these functions for ARM
and AARCH64 specifically, using wide accessors that can tolerate any
misalignment.
Reported-by: "Oliyil Kunnil, Vishal" <vishalo@qti.qualcomm.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S | 40 ++++++
MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 66 +++++++++
MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.asm | 69 +++++++++
MdePkg/Library/BaseMemoryLibOptDxe/Arm/MemLibGuid.c | 152 ++++++++++++++++++++
MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf | 7 +-
5 files changed, 333 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S
new file mode 100644
index 000000000000..9e1b55cdf589
--- /dev/null
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/AArch64/CompareGuid.S
@@ -0,0 +1,40 @@
+//
+// Copyright (c) 2016, Linaro Limited
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+// * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above copyright
+// notice, this list of conditions and the following disclaimer in the
+// documentation and/or other materials provided with the distribution.
+// * Neither the name of the Linaro nor the
+// names of its contributors may be used to endorse or promote products
+// derived from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+
+ .text
+ .align 5
+ASM_GLOBAL ASM_PFX(InternalMemCompareGuid)
+ASM_PFX(InternalMemCompareGuid):
+ mov x2, xzr
+ ldp x3, x4, [x0]
+ cbz x1, 0f
+ ldp x1, x2, [x1]
+0: cmp x1, x3
+ ccmp x2, x4, #0, eq
+ cset w0, eq
+ ret
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
new file mode 100644
index 000000000000..dca79b423a40
--- /dev/null
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
@@ -0,0 +1,66 @@
+//
+// Copyright (c) 2016, Linaro Limited
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+// * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above copyright
+// notice, this list of conditions and the following disclaimer in the
+// documentation and/or other materials provided with the distribution.
+// * Neither the name of the Linaro nor the
+// names of its contributors may be used to endorse or promote products
+// derived from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+
+ .text
+ .thumb
+ .syntax unified
+ .align 5
+ASM_GLOBAL ASM_PFX(InternalMemCompareGuid)
+ASM_PFX(InternalMemCompareGuid):
+ push {r4, lr}
+ ldr r2, [r0]
+ ldr r3, [r0, #4]
+ ldr r4, [r0, #8]
+ ldr r0, [r0, #12]
+ cbz r1, 1f
+ ldr ip, [r1]
+ ldr lr, [r1, #4]
+ cmp r2, ip
+ it eq
+ cmpeq r3, lr
+ itt ne
+ movne r0, #0
+ popne {r4, pc}
+
+ ldr r5, [r1, #8]
+ ldr r1, [r1, #12]
+ cmp r4, r5
+ it eq
+ cmpeq r0, r1
+ ite eq
+ moveq r0, #1
+ movne r0, #0
+ pop {r4, pc}
+
+1: orrs r4, r4, r0
+ movs r0, #0
+ cbnz r2, 2f
+ cbnz r3, 2f
+ cbnz r4, 2f
+ movs r0, #1
+2: pop {r4, pc}
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.asm b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.asm
new file mode 100644
index 000000000000..c6d7248ef16e
--- /dev/null
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.asm
@@ -0,0 +1,69 @@
+;
+; Copyright (c) 2016, Linaro Limited
+; All rights reserved.
+;
+; Redistribution and use in source and binary forms, with or without
+; modification, are permitted provided that the following conditions are met:
+; * Redistributions of source code must retain the above copyright
+; notice, this list of conditions and the following disclaimer.
+; * Redistributions in binary form must reproduce the above copyright
+; notice, this list of conditions and the following disclaimer in the
+; documentation and/or other materials provided with the distribution.
+; * Neither the name of the Linaro nor the
+; names of its contributors may be used to endorse or promote products
+; derived from this software without specific prior written permission.
+;
+; THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+; "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+; LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+; A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+; HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+; SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+; LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+; DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+; THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+; (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+; OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+;
+
+ EXPORT InternalMemCompareGuid
+ THUMB
+ AREA CompareGuid, CODE, READONLY, CODEALIGN, ALIGN=5
+
+InternalMemCompareGuid
+ push {r4, lr}
+ ldr r2, [r0]
+ ldr r3, [r0, #4]
+ ldr r4, [r0, #8]
+ ldr r0, [r0, #12]
+ cbz r1, L1
+ ldr ip, [r1]
+ ldr lr, [r1, #4]
+ cmp r2, ip
+ it eq
+ cmpeq r3, lr
+ itt ne
+ movne r0, #0
+ popne {r4, pc}
+
+ ldr r5, [r1, #8]
+ ldr r1, [r1, #12]
+ cmp r4, r5
+ it eq
+ cmpeq r0, r1
+ ite eq
+ moveq r0, #1
+ movne r0, #0
+ pop {r4, pc}
+
+L1
+ orrs r4, r4, r0
+ movs r0, #0
+ cbnz r2, L2
+ cbnz r3, L2
+ cbnz r4, L2
+ movs r0, #1
+L2
+ pop {r4, pc}
+
+ END
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/MemLibGuid.c
new file mode 100644
index 000000000000..8f1e50b2ed02
--- /dev/null
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/MemLibGuid.c
@@ -0,0 +1,152 @@
+/** @file
+ Implementation of GUID functions for ARM and AARCH64
+
+ Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "MemLibInternals.h"
+
+BOOLEAN
+EFIAPI
+InternalMemCompareGuid (
+ IN CONST GUID *Guid1,
+ IN CONST GUID *Guid2
+ );
+
+/**
+ Copies a source GUID to a destination GUID.
+
+ This function copies the contents of the 128-bit GUID specified by SourceGuid to
+ DestinationGuid, and returns DestinationGuid.
+
+ If DestinationGuid is NULL, then ASSERT().
+ If SourceGuid is NULL, then ASSERT().
+
+ @param DestinationGuid The pointer to the destination GUID.
+ @param SourceGuid The pointer to the source GUID.
+
+ @return DestinationGuid.
+
+**/
+GUID *
+EFIAPI
+CopyGuid (
+ OUT GUID *DestinationGuid,
+ IN CONST GUID *SourceGuid
+ )
+{
+ ASSERT (DestinationGuid != NULL);
+ ASSERT (SourceGuid != NULL);
+
+ return InternalMemCopyMem (DestinationGuid, SourceGuid, sizeof (GUID));
+}
+
+/**
+ Compares two GUIDs.
+
+ This function compares Guid1 to Guid2. If the GUIDs are identical then TRUE is returned.
+ If there are any bit differences in the two GUIDs, then FALSE is returned.
+
+ If Guid1 is NULL, then ASSERT().
+ If Guid2 is NULL, then ASSERT().
+
+ @param Guid1 A pointer to a 128 bit GUID.
+ @param Guid2 A pointer to a 128 bit GUID.
+
+ @retval TRUE Guid1 and Guid2 are identical.
+ @retval FALSE Guid1 and Guid2 are not identical.
+
+**/
+BOOLEAN
+EFIAPI
+CompareGuid (
+ IN CONST GUID *Guid1,
+ IN CONST GUID *Guid2
+ )
+{
+ ASSERT (Guid1 != NULL);
+ ASSERT (Guid2 != NULL);
+
+ return InternalMemCompareGuid (Guid1, Guid2);
+}
+
+/**
+ Scans a target buffer for a GUID, and returns a pointer to the matching GUID
+ in the target buffer.
+
+ This function searches the target buffer specified by Buffer and Length from
+ the lowest address to the highest address at 128-bit increments for the 128-bit
+ GUID value that matches Guid. If a match is found, then a pointer to the matching
+ GUID in the target buffer is returned. If no match is found, then NULL is returned.
+ If Length is 0, then NULL is returned.
+
+ If Length > 0 and Buffer is NULL, then ASSERT().
+ If Buffer is not aligned on a 32-bit boundary, then ASSERT().
+ If Length is not aligned on a 128-bit boundary, then ASSERT().
+ If Length is greater than (MAX_ADDRESS - Buffer + 1), then ASSERT().
+
+ @param Buffer The pointer to the target buffer to scan.
+ @param Length The number of bytes in Buffer to scan.
+ @param Guid The value to search for in the target buffer.
+
+ @return A pointer to the matching Guid in the target buffer or NULL otherwise.
+
+**/
+VOID *
+EFIAPI
+ScanGuid (
+ IN CONST VOID *Buffer,
+ IN UINTN Length,
+ IN CONST GUID *Guid
+ )
+{
+ CONST GUID *GuidPtr;
+
+ ASSERT (((UINTN)Buffer & (sizeof (Guid->Data1) - 1)) == 0);
+ ASSERT (Length <= (MAX_ADDRESS - (UINTN)Buffer + 1));
+ ASSERT ((Length & (sizeof (*GuidPtr) - 1)) == 0);
+
+ GuidPtr = (GUID*)Buffer;
+ Buffer = GuidPtr + Length / sizeof (*GuidPtr);
+ while (GuidPtr < (CONST GUID*)Buffer) {
+ if (InternalMemCompareGuid (GuidPtr, Guid)) {
+ return (VOID*)GuidPtr;
+ }
+ GuidPtr++;
+ }
+ return NULL;
+}
+
+/**
+ Checks if the given GUID is a zero GUID.
+
+ This function checks whether the given GUID is a zero GUID. If the GUID is
+ identical to a zero GUID then TRUE is returned. Otherwise, FALSE is returned.
+
+ If Guid is NULL, then ASSERT().
+
+ @param Guid The pointer to a 128 bit GUID.
+
+ @retval TRUE Guid is a zero GUID.
+ @retval FALSE Guid is not a zero GUID.
+
+**/
+BOOLEAN
+EFIAPI
+IsZeroGuid (
+ IN CONST GUID *Guid
+ )
+{
+ ASSERT (Guid != NULL);
+
+ return InternalMemCompareGuid (Guid, NULL);
+}
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf b/MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf
index 5ddc0cbc2d77..bc5ec2f25310 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/BaseMemoryLibOptDxe.inf
@@ -79,6 +79,7 @@ [Sources.Ia32]
Ia32/CopyMem.nasm
Ia32/CopyMem.asm
Ia32/IsZeroBuffer.nasm
+ MemLibGuid.c
[Sources.X64]
X64/ScanMem64.nasm
@@ -115,6 +116,7 @@ [Sources.X64]
X64/CopyMem.asm
X64/CopyMem.S
X64/IsZeroBuffer.nasm
+ MemLibGuid.c
[Defines.ARM, Defines.AARCH64]
#
@@ -130,20 +132,24 @@ [Sources.ARM]
Arm/SetMem.S |GCC
Arm/CopyMem.S |GCC
Arm/CompareMem.S |GCC
+ Arm/CompareGuid.S |GCC
Arm/ScanMem.asm |RVCT
Arm/SetMem.asm |RVCT
Arm/CopyMem.asm |RVCT
Arm/CompareMem.asm |RVCT
+ Arm/CompareGuid.asm |RVCT
[Sources.AARCH64]
AArch64/ScanMem.S
AArch64/SetMem.S
AArch64/CopyMem.S
AArch64/CompareMem.S
+ AArch64/CompareGuid.S
[Sources.ARM, Sources.AARCH64]
Arm/ScanMemGeneric.c
+ Arm/MemLibGuid.c
[Sources]
ScanMem64Wrapper.c
@@ -158,7 +164,6 @@ [Sources]
SetMemWrapper.c
CopyMemWrapper.c
IsZeroBufferWrapper.c
- MemLibGuid.c
[Packages]
MdePkg/MdePkg.dec
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
2016-09-19 8:13 ` [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s Ard Biesheuvel
@ 2016-09-20 2:00 ` Wu, Hao A
2016-09-20 8:38 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Wu, Hao A @ 2016-09-20 2:00 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Gao, Liming
Cc: vishalo@qti.qualcomm.com, leif.lindholm@linaro.org
Hi Ard,
The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and
IsZeroGuid() are implicitly done within calls to BaseLib APIs
ReadUnaligned64() and WriteUnaligned64().
So I think the functions behavior matches with their comments. What do you
think?
Best Regards,
Hao Wu
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Monday, September 19, 2016 4:14 PM
> To: edk2-devel@lists.01.org; Gao, Liming
> Cc: vishalo@qti.qualcomm.com; leif.lindholm@linaro.org; Ard Biesheuvel
> Subject: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing
> ASSERT()s
>
> Add the ASSERT() statements to CopyGuid (), CompareGuid() and
> IsZeroGuid() that are mentioned in the respective comments but
> were missing from the actual code.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdePkg/Library/BaseMemoryLib/MemLibGuid.c | 8 ++++++++
> MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c | 8 ++++++++
> MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c | 8 ++++++++
> MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c | 8 ++++++++
> MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c | 8 ++++++++
> MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c | 8 ++++++++
> MdePkg/Library/PeiMemoryLib/MemLibGuid.c | 8 ++++++++
> MdePkg/Library/UefiMemoryLib/MemLibGuid.c | 8 ++++++++
> 8 files changed, 64 insertions(+)
>
> diff --git a/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
> b/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
> index b2590f83caef..dff9bde653a9 100644
> --- a/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
> +++ b/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> diff --git a/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
> b/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
> index cbb385fddfba..60babaf0dc49 100644
> --- a/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
> +++ b/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
> b/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
> index cbb385fddfba..60babaf0dc49 100644
> --- a/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
> +++ b/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> diff --git a/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
> b/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
> index cbb385fddfba..60babaf0dc49 100644
> --- a/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
> +++ b/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> diff --git a/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
> b/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
> index cbb385fddfba..60babaf0dc49 100644
> --- a/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
> +++ b/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> diff --git a/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
> b/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
> index cbb385fddfba..60babaf0dc49 100644
> --- a/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
> +++ b/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> diff --git a/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
> b/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
> index cbb385fddfba..60babaf0dc49 100644
> --- a/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
> +++ b/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> diff --git a/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
> b/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
> index cbb385fddfba..60babaf0dc49 100644
> --- a/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
> +++ b/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
> @@ -47,6 +47,9 @@ CopyGuid (
> IN CONST GUID *SourceGuid
> )
> {
> + ASSERT (DestinationGuid != NULL);
> + ASSERT (SourceGuid != NULL);
> +
> WriteUnaligned64 (
> (UINT64*)DestinationGuid,
> ReadUnaligned64 ((CONST UINT64*)SourceGuid)
> @@ -86,6 +89,9 @@ CompareGuid (
> UINT64 HighPartOfGuid1;
> UINT64 HighPartOfGuid2;
>
> + ASSERT (Guid1 != NULL);
> + ASSERT (Guid2 != NULL);
> +
> LowPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1);
> LowPartOfGuid2 = ReadUnaligned64 ((CONST UINT64*) Guid2);
> HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
> @@ -164,6 +170,8 @@ IsZeroGuid (
> UINT64 LowPartOfGuid;
> UINT64 HighPartOfGuid;
>
> + ASSERT (Guid != NULL);
> +
> LowPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid);
> HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
>
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
2016-09-20 2:00 ` Wu, Hao A
@ 2016-09-20 8:38 ` Ard Biesheuvel
2016-09-20 12:02 ` Wu, Hao A
0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-20 8:38 UTC (permalink / raw)
To: Wu, Hao A
Cc: edk2-devel@lists.01.org, Gao, Liming, vishalo@qti.qualcomm.com,
leif.lindholm@linaro.org
On 20 September 2016 at 03:00, Wu, Hao A <hao.a.wu@intel.com> wrote:
> Hi Ard,
>
> The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and
> IsZeroGuid() are implicitly done within calls to BaseLib APIs
> ReadUnaligned64() and WriteUnaligned64().
>
> So I think the functions behavior matches with their comments. What do you
> think?
>
I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically
be implemented by a version of BaseLib that does not contain such
ASSERT()s
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
2016-09-20 8:38 ` Ard Biesheuvel
@ 2016-09-20 12:02 ` Wu, Hao A
2016-09-20 12:16 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Wu, Hao A @ 2016-09-20 12:02 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: leif.lindholm@linaro.org, edk2-devel@lists.01.org,
vishalo@qti.qualcomm.com, Gao, Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, September 20, 2016 4:38 PM
> To: Wu, Hao A
> Cc: leif.lindholm@linaro.org; edk2-devel@lists.01.org;
> vishalo@qti.qualcomm.com; Gao, Liming
> Subject: Re: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing
> ASSERT()s
>
> On 20 September 2016 at 03:00, Wu, Hao A <hao.a.wu@intel.com> wrote:
> > Hi Ard,
> >
> > The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and
> > IsZeroGuid() are implicitly done within calls to BaseLib APIs
> > ReadUnaligned64() and WriteUnaligned64().
> >
> > So I think the functions behavior matches with their comments. What do you
> > think?
> >
>
> I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically
> be implemented by a version of BaseLib that does not contain such
> ASSERT()s
The comments for APIs ReadUnaligned64 and WriteUnaligned64 in BaseLib
mention the ASSERT() for inputting a NULL buffer.
I think instances of BaseLib should follow the comments.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
2016-09-20 12:02 ` Wu, Hao A
@ 2016-09-20 12:16 ` Ard Biesheuvel
2016-09-20 12:40 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-20 12:16 UTC (permalink / raw)
To: Wu, Hao A
Cc: leif.lindholm@linaro.org, edk2-devel@lists.01.org,
vishalo@qti.qualcomm.com, Gao, Liming
On 20 September 2016 at 13:02, Wu, Hao A <hao.a.wu@intel.com> wrote:
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>> Biesheuvel
>> Sent: Tuesday, September 20, 2016 4:38 PM
>> To: Wu, Hao A
>> Cc: leif.lindholm@linaro.org; edk2-devel@lists.01.org;
>> vishalo@qti.qualcomm.com; Gao, Liming
>> Subject: Re: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing
>> ASSERT()s
>>
>> On 20 September 2016 at 03:00, Wu, Hao A <hao.a.wu@intel.com> wrote:
>> > Hi Ard,
>> >
>> > The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and
>> > IsZeroGuid() are implicitly done within calls to BaseLib APIs
>> > ReadUnaligned64() and WriteUnaligned64().
>> >
>> > So I think the functions behavior matches with their comments. What do you
>> > think?
>> >
>>
>> I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically
>> be implemented by a version of BaseLib that does not contain such
>> ASSERT()s
>
> The comments for APIs ReadUnaligned64 and WriteUnaligned64 in BaseLib
> mention the ASSERT() for inputting a NULL buffer.
>
> I think instances of BaseLib should follow the comments.
>
I agree with this. But that does not justify omitting them here.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
2016-09-20 12:16 ` Ard Biesheuvel
@ 2016-09-20 12:40 ` Ard Biesheuvel
2016-09-21 1:27 ` Gao, Liming
0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-20 12:40 UTC (permalink / raw)
To: Wu, Hao A
Cc: leif.lindholm@linaro.org, edk2-devel@lists.01.org,
vishalo@qti.qualcomm.com, Gao, Liming
On 20 September 2016 at 13:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 20 September 2016 at 13:02, Wu, Hao A <hao.a.wu@intel.com> wrote:
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>>> Biesheuvel
>>> Sent: Tuesday, September 20, 2016 4:38 PM
>>> To: Wu, Hao A
>>> Cc: leif.lindholm@linaro.org; edk2-devel@lists.01.org;
>>> vishalo@qti.qualcomm.com; Gao, Liming
>>> Subject: Re: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing
>>> ASSERT()s
>>>
>>> On 20 September 2016 at 03:00, Wu, Hao A <hao.a.wu@intel.com> wrote:
>>> > Hi Ard,
>>> >
>>> > The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and
>>> > IsZeroGuid() are implicitly done within calls to BaseLib APIs
>>> > ReadUnaligned64() and WriteUnaligned64().
>>> >
>>> > So I think the functions behavior matches with their comments. What do you
>>> > think?
>>> >
>>>
>>> I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically
>>> be implemented by a version of BaseLib that does not contain such
>>> ASSERT()s
>>
>> The comments for APIs ReadUnaligned64 and WriteUnaligned64 in BaseLib
>> mention the ASSERT() for inputting a NULL buffer.
>>
>> I think instances of BaseLib should follow the comments.
>>
>
> I agree with this. But that does not justify omitting them here.
... but actually, it makes no sense to argue about this, since the
user visible outcome is the same. I will withdraw the patch.
Thanks,
Ard.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
2016-09-20 12:40 ` Ard Biesheuvel
@ 2016-09-21 1:27 ` Gao, Liming
0 siblings, 0 replies; 10+ messages in thread
From: Gao, Liming @ 2016-09-21 1:27 UTC (permalink / raw)
To: Ard Biesheuvel, Wu, Hao A
Cc: edk2-devel@lists.01.org, vishalo@qti.qualcomm.com,
leif.lindholm@linaro.org
Ard:
Thanks! Other two patches are good. Reviewed-by: Liming Gao <liming.gao@intel.com>
Thanks
Liming
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Tuesday, September 20, 2016 8:41 PM
To: Wu, Hao A <hao.a.wu@intel.com>
Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; vishalo@qti.qualcomm.com; leif.lindholm@linaro.org
Subject: Re: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s
On 20 September 2016 at 13:16, Ard Biesheuvel wrote:
> On 20 September 2016 at 13:02, Wu, Hao A wrote:
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>>> Biesheuvel
>>> Sent: Tuesday, September 20, 2016 4:38 PM
>>> To: Wu, Hao A
>>> Cc: leif.lindholm@linaro.org<mailto:leif.lindholm@linaro.org>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>;
>>> vishalo@qti.qualcomm.com<mailto:vishalo@qti.qualcomm.com>; Gao, Liming
>>> Subject: Re: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing
>>> ASSERT()s
>>>
>>> On 20 September 2016 at 03:00, Wu, Hao A wrote:
>>> > Hi Ard,
>>> >
>>> > The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and
>>> > IsZeroGuid() are implicitly done within calls to BaseLib APIs
>>> > ReadUnaligned64() and WriteUnaligned64().
>>> >
>>> > So I think the functions behavior matches with their comments. What do you
>>> > think?
>>> >
>>>
>>> I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically
>>> be implemented by a version of BaseLib that does not contain such
>>> ASSERT()s
>>
>> The comments for APIs ReadUnaligned64 and WriteUnaligned64 in BaseLib
>> mention the ASSERT() for inputting a NULL buffer.
>>
>> I think instances of BaseLib should follow the comments.
>>
>
> I agree with this. But that does not justify omitting them here.
... but actually, it makes no sense to argue about this, since the
user visible outcome is the same. I will withdraw the patch.
Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-09-21 1:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-19 8:13 [PATCH v2 0/3] MdePkg/BaseMemoryLibOptDxe: generic and ARM/AARCH64 fixes Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 1/3] MdePkg/BaseMemoryLibOptDxe ARM: fix arithmetic bugs in CompareMem() Ard Biesheuvel
2016-09-19 8:13 ` [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s Ard Biesheuvel
2016-09-20 2:00 ` Wu, Hao A
2016-09-20 8:38 ` Ard Biesheuvel
2016-09-20 12:02 ` Wu, Hao A
2016-09-20 12:16 ` Ard Biesheuvel
2016-09-20 12:40 ` Ard Biesheuvel
2016-09-21 1:27 ` Gao, Liming
2016-09-19 8:13 ` [PATCH v2 3/3] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: implement accelerated GUID functions Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox