Date   

Re: [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

Ard Biesheuvel
 

On 4/2/20 12:57 PM, Leif Lindholm wrote:
On Thu, Apr 02, 2020 at 12:29:40 +0200, Ard Biesheuvel wrote:
On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
TT_ATTR_INDX_INVALID is #define'd but never used so drop it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index a82596d290f1..222ff817956f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -19,9 +19,6 @@
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
-// We use this index definition to define an invalid block entry
-#define TT_ATTR_INDX_INVALID ((UINT32)~0)
-
Since this is separately defined also in
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
could this patch be tweaked to instead "consolidate" the definitions
to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
all of the other TT_ATTR_INDX_ definitions live?
That would imply that this value is somehow architected, which it is not.

ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this
constant, and it only has meaning within the context of the routines
therein.
Hmm, ok. So all of those definitions should really move *out* of
ArmPkg/Include/Chipset/AArch64Mmu.h?
No, they can't, because some of the others are used in
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
So I'm still feeling that #defines using the same namespace should
live together in order to reduce risk of future screwups.
So, unrelated to this patch
(Reviewed-by: Leif Lindholm <leif@...>)
should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be
moved to ArmPkg/Include/Chipset/AArch64Mmu.h?
It should simply be renamed. They are not part of the same namespace, it is simply a local hack in CpuDxe to distinguish between valid indexes into the MAIR table and an 'unknown' placeholder value.

CpuDxe is on my list of things I'd like to clean up as well.


Re: [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

Leif Lindholm
 

On Thu, Apr 02, 2020 at 12:29:30 +0200, Ard Biesheuvel wrote:
On 4/2/20 12:28 PM, Leif Lindholm wrote:
On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
Before getting rid of GetRootTranslationTableInfo() and the related
LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
version of the former to CpuDxe, which will be its only remaining
user. While at it, simplify it a bit, since in the CpuDxe cases,
both OUT arguments are always provided.
Summary after discussing off-list:
This patch isn't *just* moving a public function private, but it's
replacing use of a function that was never meant to be public and had
a completely bonkers forward-declaration in a different module.

Ard has agreed to make the commit message more explicit on that point,
and I'm happy with that. So with that change, for the series:
Reviewed-by: Leif Lindholm <leif@...>

So, this may be super picky, but:
Deleting the prototype without making the definition also STATIC would
cause build errors with -Wmissing-prototypes (which someone might be
enabling explicitly or implicitly if say doing some code hardening on
the side).

Now, it's a valid point to say that -Wmissing-prototypes isn't in our
current CFLAGS, but I think it would be a good habit to get into.
So you get a:

Reviewed-by: Leif Lindholm <leif@...>

regardless, but I'd appreciate if you could sling a STATIC into
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
pushing.
Well, while I see your point, please note that the prototype only exists in
a local header file that only gets included by CpuDxe, and not by any of the
other consumers of ArmMmuLib.
What I'm saying is that with -Wmissing-prototypes added, building
after applying this patch (but before applying the one that deletes
the function) gives:

"aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
GetRootTranslationTableInfo (
^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

So it breaks bisect if you're experimenting with hardening.

As I said, I don't insist, but I want to be sure you realise that bit
before you decide.
But none of the other consumers of ArmMmuLib do an #include of
ArmPkg/Drivers/CpuDxe/CpuDxe.h, right? So I am slightly puzzled by this
result.


Re: [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

Leif Lindholm
 

On Thu, Apr 02, 2020 at 12:29:40 +0200, Ard Biesheuvel wrote:
On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
TT_ATTR_INDX_INVALID is #define'd but never used so drop it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index a82596d290f1..222ff817956f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -19,9 +19,6 @@
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
-// We use this index definition to define an invalid block entry
-#define TT_ATTR_INDX_INVALID ((UINT32)~0)
-
Since this is separately defined also in
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
could this patch be tweaked to instead "consolidate" the definitions
to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
all of the other TT_ATTR_INDX_ definitions live?
That would imply that this value is somehow architected, which it is not.

ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this
constant, and it only has meaning within the context of the routines
therein.
Hmm, ok. So all of those definitions should really move *out* of
ArmPkg/Include/Chipset/AArch64Mmu.h?

No, they can't, because some of the others are used in
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

So I'm still feeling that #defines using the same namespace should
live together in order to reduce risk of future screwups.
So, unrelated to this patch
(Reviewed-by: Leif Lindholm <leif@...>)
should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be
moved to ArmPkg/Include/Chipset/AArch64Mmu.h?

/
Leif


Re: [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

Ard Biesheuvel
 

On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
TT_ATTR_INDX_INVALID is #define'd but never used so drop it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index a82596d290f1..222ff817956f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -19,9 +19,6 @@
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
-// We use this index definition to define an invalid block entry
-#define TT_ATTR_INDX_INVALID ((UINT32)~0)
-
Since this is separately defined also in
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
could this patch be tweaked to instead "consolidate" the definitions
to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
all of the other TT_ATTR_INDX_ definitions live?
That would imply that this value is somehow architected, which it is not.

ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this constant, and it only has meaning within the context of the routines therein.


Re: [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

Ard Biesheuvel
 

On 4/2/20 12:28 PM, Leif Lindholm wrote:
On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
Before getting rid of GetRootTranslationTableInfo() and the related
LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
version of the former to CpuDxe, which will be its only remaining
user. While at it, simplify it a bit, since in the CpuDxe cases,
both OUT arguments are always provided.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 3b6c5e733709..24eb1c4221e3 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define TT_ATTR_INDX_INVALID ((UINT32)~0)
+#define MIN_T0SZ 16
+#define BITS_PER_LEVEL 9
+
+STATIC
+VOID
+GetRootTranslationTableInfo (
+ IN UINTN T0SZ,
+ OUT UINTN *RootTableLevel,
+ OUT UINTN *RootTableEntryCount
+ )
+{
+ *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
+ *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
+
STATIC
UINT64
GetFirstPageAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index b627c3c50ff8..3fe5c24d5e5b 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -134,13 +134,6 @@ GetMemoryRegion (
OUT UINTN *RegionAttributes
);
-VOID
-GetRootTranslationTableInfo (
So, this may be super picky, but:
Deleting the prototype without making the definition also STATIC would
cause build errors with -Wmissing-prototypes (which someone might be
enabling explicitly or implicitly if say doing some code hardening on
the side).

Now, it's a valid point to say that -Wmissing-prototypes isn't in our
current CFLAGS, but I think it would be a good habit to get into.
So you get a:

Reviewed-by: Leif Lindholm <leif@...>

regardless, but I'd appreciate if you could sling a STATIC into
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
pushing.
Well, while I see your point, please note that the prototype only exists in
a local header file that only gets included by CpuDxe, and not by any of the
other consumers of ArmMmuLib.
What I'm saying is that with -Wmissing-prototypes added, building
after applying this patch (but before applying the one that deletes
the function) gives:
"aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
GetRootTranslationTableInfo (
^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
So it breaks bisect if you're experimenting with hardening.
As I said, I don't insist, but I want to be sure you realise that bit
before you decide.
But none of the other consumers of ArmMmuLib do an #include of ArmPkg/Drivers/CpuDxe/CpuDxe.h, right? So I am slightly puzzled by this result.


Re: [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

Leif Lindholm
 

On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
Before getting rid of GetRootTranslationTableInfo() and the related
LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
version of the former to CpuDxe, which will be its only remaining
user. While at it, simplify it a bit, since in the CpuDxe cases,
both OUT arguments are always provided.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 3b6c5e733709..24eb1c4221e3 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define TT_ATTR_INDX_INVALID ((UINT32)~0)
+#define MIN_T0SZ 16
+#define BITS_PER_LEVEL 9
+
+STATIC
+VOID
+GetRootTranslationTableInfo (
+ IN UINTN T0SZ,
+ OUT UINTN *RootTableLevel,
+ OUT UINTN *RootTableEntryCount
+ )
+{
+ *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
+ *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
+
STATIC
UINT64
GetFirstPageAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index b627c3c50ff8..3fe5c24d5e5b 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -134,13 +134,6 @@ GetMemoryRegion (
OUT UINTN *RegionAttributes
);
-VOID
-GetRootTranslationTableInfo (
So, this may be super picky, but:
Deleting the prototype without making the definition also STATIC would
cause build errors with -Wmissing-prototypes (which someone might be
enabling explicitly or implicitly if say doing some code hardening on
the side).

Now, it's a valid point to say that -Wmissing-prototypes isn't in our
current CFLAGS, but I think it would be a good habit to get into.
So you get a:

Reviewed-by: Leif Lindholm <leif@...>

regardless, but I'd appreciate if you could sling a STATIC into
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
pushing.
Well, while I see your point, please note that the prototype only exists in
a local header file that only gets included by CpuDxe, and not by any of the
other consumers of ArmMmuLib.
What I'm saying is that with -Wmissing-prototypes added, building
after applying this patch (but before applying the one that deletes
the function) gives:

"aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
GetRootTranslationTableInfo (
^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

So it breaks bisect if you're experimenting with hardening.

As I said, I don't insist, but I want to be sure you realise that bit
before you decide.

/
Leif



- IN UINTN T0SZ,
- OUT UINTN *TableLevel,
- OUT UINTN *TableEntryCount
- );
-
EFI_STATUS
SetGcdMemorySpaceAttributes (
IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap,
--
2.17.1


Re: [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

Leif Lindholm
 

On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
TT_ATTR_INDX_INVALID is #define'd but never used so drop it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index a82596d290f1..222ff817956f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -19,9 +19,6 @@
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>

-// We use this index definition to define an invalid block entry
-#define TT_ATTR_INDX_INVALID ((UINT32)~0)
-
Since this is separately defined also in
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
could this patch be tweaked to instead "consolidate" the definitions
to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
all of the other TT_ATTR_INDX_ definitions live?

/
Leif

STATIC
UINT64
ArmMemoryAttributeToPageAttribute (
--
2.17.1


Re: [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

Ard Biesheuvel
 

On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
Before getting rid of GetRootTranslationTableInfo() and the related
LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
version of the former to CpuDxe, which will be its only remaining
user. While at it, simplify it a bit, since in the CpuDxe cases,
both OUT arguments are always provided.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 3b6c5e733709..24eb1c4221e3 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define TT_ATTR_INDX_INVALID ((UINT32)~0)
+#define MIN_T0SZ 16
+#define BITS_PER_LEVEL 9
+
+STATIC
+VOID
+GetRootTranslationTableInfo (
+ IN UINTN T0SZ,
+ OUT UINTN *RootTableLevel,
+ OUT UINTN *RootTableEntryCount
+ )
+{
+ *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
+ *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
+
STATIC
UINT64
GetFirstPageAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index b627c3c50ff8..3fe5c24d5e5b 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -134,13 +134,6 @@ GetMemoryRegion (
OUT UINTN *RegionAttributes
);
-VOID
-GetRootTranslationTableInfo (
So, this may be super picky, but:
Deleting the prototype without making the definition also STATIC would
cause build errors with -Wmissing-prototypes (which someone might be
enabling explicitly or implicitly if say doing some code hardening on
the side).
Now, it's a valid point to say that -Wmissing-prototypes isn't in our
current CFLAGS, but I think it would be a good habit to get into.
So you get a:
Reviewed-by: Leif Lindholm <leif@...>
regardless, but I'd appreciate if you could sling a STATIC into
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
pushing.
Well, while I see your point, please note that the prototype only exists in a local header file that only gets included by CpuDxe, and not by any of the other consumers of ArmMmuLib.


- IN UINTN T0SZ,
- OUT UINTN *TableLevel,
- OUT UINTN *TableEntryCount
- );
-
EFI_STATUS
SetGcdMemorySpaceAttributes (
IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap,
--
2.17.1


Re: [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

Leif Lindholm
 

On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
Before getting rid of GetRootTranslationTableInfo() and the related
LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
version of the former to CpuDxe, which will be its only remaining
user. While at it, simplify it a bit, since in the CpuDxe cases,
both OUT arguments are always provided.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 7 -------
2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 3b6c5e733709..24eb1c4221e3 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#define TT_ATTR_INDX_INVALID ((UINT32)~0)

+#define MIN_T0SZ 16
+#define BITS_PER_LEVEL 9
+
+STATIC
+VOID
+GetRootTranslationTableInfo (
+ IN UINTN T0SZ,
+ OUT UINTN *RootTableLevel,
+ OUT UINTN *RootTableEntryCount
+ )
+{
+ *RootTableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
+ *RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
+
STATIC
UINT64
GetFirstPageAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index b627c3c50ff8..3fe5c24d5e5b 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -134,13 +134,6 @@ GetMemoryRegion (
OUT UINTN *RegionAttributes
);

-VOID
-GetRootTranslationTableInfo (
So, this may be super picky, but:
Deleting the prototype without making the definition also STATIC would
cause build errors with -Wmissing-prototypes (which someone might be
enabling explicitly or implicitly if say doing some code hardening on
the side).

Now, it's a valid point to say that -Wmissing-prototypes isn't in our
current CFLAGS, but I think it would be a good habit to get into.
So you get a:

Reviewed-by: Leif Lindholm <leif@...>

regardless, but I'd appreciate if you could sling a STATIC into
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
pushing.

- IN UINTN T0SZ,
- OUT UINTN *TableLevel,
- OUT UINTN *TableEntryCount
- );
-
EFI_STATUS
SetGcdMemorySpaceAttributes (
IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap,
--
2.17.1


[edk2-staging][PATCH] BaseTools/Fmmt: Enhance for check input FD size

Feng, YunhuaX
 

Enhance for check input FD if empty file or not.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <liming.gao@...>
Signed-off-by: Yunhua Feng <yunhuax.feng@...>
---
BaseTools/Source/C/FMMT/FirmwareModuleManagement.c | 2 +-
BaseTools/Source/C/FMMT/FmmtLib.c | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
index db9b585541..4252c698aa 100644
--- a/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
+++ b/BaseTools/Source/C/FMMT/FirmwareModuleManagement.c
@@ -833,11 +833,11 @@ FmmtImageView (
}

Status = LibFindFvInFd (InputFile, &LocalFdData);

if (EFI_ERROR(Status)) {
- Error("FMMT", 0, 1001, "Error while search FV in FD", "");
+ Error("FMMT", 0, 1001, "Error while search FV in FD", FdInName);
fclose (InputFile);
return EFI_ABORTED;
}

CurrentFv = LocalFdData->Fv;
diff --git a/BaseTools/Source/C/FMMT/FmmtLib.c b/BaseTools/Source/C/FMMT/FmmtLib.c
index cdbee3d629..30deec532f 100644
--- a/BaseTools/Source/C/FMMT/FmmtLib.c
+++ b/BaseTools/Source/C/FMMT/FmmtLib.c
@@ -265,10 +265,15 @@ LibFindFvInFd (
}

FdBufferOri = FdBuffer;
FdBufferEnd = FdBuffer + FdSize;

+ if (FdSize < sizeof(EFI_FIRMWARE_VOLUME_HEADER)) {
+ Error ("FMMT", 0, 0002, "Error Check the input FD, Please make sure the FD is valid", "Check FD size error!");
+ return EFI_ABORTED;
+ }
+
while (FdBuffer <= FdBufferEnd - sizeof (EFI_FIRMWARE_VOLUME_HEADER)) {
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) FdBuffer;
//
// Copy 4 bytes of fd data to check the _FVH signature
//
--
2.12.2.windows.2


[PATCH v2] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()

Maciej Rabeda
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2655

This patch fixes reversed logic of recently added ASSERTs which should
ensure that Ip6IsNDOptionValid() implementation properly reacts to invalid
packets.

Cc: Jiaxin Wu <jiaxin.wu@...>
Cc: Siyuan Fu <siyuan.fu@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
Reviewed-by: Siyuan Fu <siyuan.fu@...>
Tested-by: Laszlo Ersek <lersek@...>
Fixes: 9c20342eed70ec99ec50cd73cb81804299f05403
---
NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index fd7f60b2f92c..0780a98cb325 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().=0D
//=0D
ASSERT (LinkLayerOption.Length !=3D 0);=0D
- ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >=3D (UINT32) H=
ead->PayloadLength);=0D
+ ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <=3D (UINT32) H=
ead->PayloadLength);=0D
=0D
ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));=0D
CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);=0D
@@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().=0D
//=0D
ASSERT (PrefixOption.Length =3D=3D 4);=0D
- ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >=3D (UINT32) Head=
->PayloadLength);=0D
+ ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <=3D (UINT32) Head=
->PayloadLength);=0D
=0D
PrefixOption.ValidLifetime =3D NTOHL (PrefixOption.ValidLifetime=
);=0D
PrefixOption.PreferredLifetime =3D NTOHL (PrefixOption.PreferredLife=
time);=0D
@@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().=0D
//=0D
ASSERT (MTUOption.Length =3D=3D 1);=0D
- ASSERT (Offset + (UINT32) MTUOption.Length * 8 >=3D (UINT32) Head->P=
ayloadLength);=0D
+ ASSERT (Offset + (UINT32) MTUOption.Length * 8 <=3D (UINT32) Head->P=
ayloadLength);=0D
=0D
//=0D
// Use IPv6 minimum link MTU 1280 bytes as the maximum packet size i=
n order=0D
--=20
2.24.0.windows.2


Re: [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

Gao, Zhichao
 

Summarize the issue, if anything incorrect, please help to correct it. Thanks.

 

Background:

Uefi spec ambiguous description:

> When interpreting the data from this function, it should be noted that if a class

> of printable characters that are normally adjusted by shift modifiers (e.g. Shift

> Key + "f" key) would be presented solely as a KeyData.Key.UnicodeChar without the

> associated shift state. So in the previous example of a Shift Key + "f" key being

> pressed, the only pertinent data returned would be KeyData.Key.UnicodeChar with

> the value of "F". This of course would not typically be the case for non-printable

> characters such as the pressing of the Right Shift Key + F10 key since the

> corresponding returned data would be reflected both in the

> KeyData.KeyState.KeyShiftState and KeyData.Key.ScanCode values.

 

Some firmware vendor see the “if” as an option to implement the keyboard driver and they choose to implement in the other way.

Ideal: shift + UnicodeKey/UnScancodeKey == report as ==> “shifted” UnicodeKey/UnScancodeKey

                Ctrl + UnicodeKey == report as ==> UnicodeKey – ‘A’/’a’ + 1

Other way: shift + UnicodeKey/UnScancodeKey == report as ==> shift state + “shifted” UnicodeKey/UnScancodeKey

                Ctrl + UnicodeKey == report as ==> ctrl state + UnicodeKey – ‘A’/’a’ + 1

I.e. do not remove the shift state.

That would cause the shell edit/Hexedit missing handling the “shifted” key when run shell in such firmware.

 

Vitaly’s solution add the support the handle “other implementation”.

 

If we take Vitaly’s solution, we should fixed all the applications in edk2 not only in shell edit/Hexeidt function.

And when we catch another request same with this issue, we would follow the same progress.

 

Thanks,

Zhichao

 

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, Zhichao
Sent: Thursday, April 2, 2020 2:57 PM
To: devel@edk2.groups.io; vit9696@...; Rothman, Michael A <michael.a.rothman@...>
Cc: Andrew Fish <afish@...>; Laszlo Ersek <lersek@...>; Marvin Häuser <mhaeuser@...>; Kinney, Michael D <michael.d.kinney@...>; Ni, Ray <ray.ni@...>
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

 

Hi Micheal Rothman,

 

Can you help to review this issue? Because of the uefi spec issue, some firmware implemented the different behavior.

Should the edk2 code to handle such issue? If yes, here maybe the situation:

1.       Change for all apps -> uefi spec update and accept  such behavior with description -> Done.

2.       Change for all apps -> uefi spec update to remove the ambiguous and reject other behavior -> removal the change in first step.

 

Hi Vitaly,

 

I used to think it is an additional support for different implementation because of the spec. But if we approve this patch, all the app in edk2 using the combo key function should be update.

Using shell’s ctrl+’c’ as an example, it need to update at the same time. Same with SCT tool.

 

Thanks,

Zhichao

 

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, March 27, 2020 7:01 PM
To: Gao, Zhichao <
zhichao.gao@...>
Cc:
devel@edk2.groups.io; Andrew Fish <afish@...>; Laszlo Ersek <lersek@...>; Marvin Häuser <mhaeuser@...>; Kinney, Michael D <michael.d.kinney@...>; Ni, Ray <ray.ni@...>
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

 

Hello,

 

Requesting to merge this into edk2-stable202005 for the reasons explained in BZ[1]. I assume there is no real objection for this, only the approach we make such changes in the future, but this can be postponed as we encounter more of such problems.

 

Best regards,

Vitaly

 

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2510

 

20 февр. 2020 г., в 03:27, Gao, Zhichao <zhichao.gao@...> написал(а):

 

Sorry for my mistake. Then I have no other comments for this patch.

Reviewed-by: Zhichao Gao <zhichao.gao@...>

 

Thanks,

Zhichao

 

From: vit9696 <vit9696@...> 
Sent: Wednesday, February 19, 2020 8:15 PM
To: Gao, Zhichao <
zhichao.gao@...>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

 

Zhichao,

 

Thanks for your review. The comment is correct, as ShiftOnlyState means the state where only shift (and no other modifiers) can be pressed or not.

 

Best wishes,

Vitaly

 

On Wed, Feb 19, 2020 at 09:55, Gao, Zhichao <zhichao.gao@...> wrote:

Hi Vitaly,

See the comment below:

> -----Original Message-----
> From: 
devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
> Cheptsov via Groups.Io
> Sent: Monday, February 10, 2020 6:18 PM
> To: 
devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with
> separately reported modifiers
>
> REF: 
https://bugzilla.tianocore.org/show_bug.cgi?id=2510
>
> Some firmwares:
> - Report Shift modifier even when they report upper-case unicode letter.
> - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
>
> This change provides support for these firmwares preserving the compatibility
> with the previous input handling.
>
> Signed-off-by: Michael Belyaev <
usrsse2@...>
> Reviewed-by: Vitaly Cheptsov <
vit9696@...>
> ---
> ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37
> ++++++++++++++------
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28
> ++++++++++-----
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 ++++
> ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11
> +++---
> 4 files changed, 58 insertions(+), 24 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> index df530f1119..9615a4dfbd 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> @@ -1381,8 +1381,8 @@ MainCommandDisplayHelp (
> continue;
> }
>
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
> - (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> + if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> //
> // For consoles that don't support/report shift state,
> // CTRL+W is translated to L'W' - L'A' + 1.
> @@ -1390,14 +1390,17 @@ MainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == L'w') || (KeyData.Key.UnicodeChar == L'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> @@ -1834,7 +1837,8 @@ MainEditorKeyInput (
> EFI_KEY_DATA KeyData;
> EFI_STATUS Status;
> EFI_SIMPLE_POINTER_STATE MouseState;
> - BOOLEAN NoShiftState;
> + BOOLEAN NoModifierState;
> + BOOLEAN ShiftOnlyState;
>
> do {
>
> @@ -1886,17 +1890,28 @@ MainEditorKeyInput (
> //
> StatusBarSetRefresh();
> //
> - // NoShiftState: TRUE when no shift key is pressed.
> + // NoModifierState: TRUE when no modifier key is pressed.
> //
> - NoShiftState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0) || (KeyData.KeyState.KeyShiftState ==
> EFI_SHIFT_STATE_VALID);
> + NoModifierState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID);
> + //
> + // ShiftOnlyState: TRUE when no modifier key except Shift is pressed.
> + //
> + ShiftOnlyState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || ((KeyData.KeyState.KeyShiftState
> + & ~(EFI_SHIFT_STATE_VALID |
> + EFI_LEFT_SHIFT_PRESSED | EFI_RIGHT_SHIFT_PRESSED)) == 0);
> //
> // dispatch to different components' key handling function
> //
> if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey(&KeyData)) {
> Status = EFI_SUCCESS;
> - } else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) ||
> ((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <=
> SCAN_PAGE_DOWN)))) {
> + } else if ((ShiftOnlyState && (KeyData.Key.ScanCode == SCAN_NULL))
> + || (NoModifierState && (KeyData.Key.ScanCode >= SCAN_UP) &&
> (KeyData.Key.ScanCode <= SCAN_PAGE_DOWN))) {
> + //
> + // alphanumeric keys with or without shift, or arrow keys without shift
> + //

This is unmatched with the comments. It only handles the alphanumeric keys with shift.

Thanks,
Zhichao

> Status = FileBufferHandleInput (&KeyData.Key);
> - } else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) &&
> (KeyData.Key.ScanCode <= SCAN_F12)) {
> + } else if (NoModifierState && (KeyData.Key.ScanCode >= SCAN_F1)
> + && (KeyData.Key.ScanCode <= SCAN_F12)) {
> Status = MenuBarDispatchFunctionKey (&KeyData.Key);
> } else {
> StatusBarSetStatusString (L"Unknown Command"); diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> index 35b0b701e8..d053059220 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> @@ -130,6 +130,8 @@ InputBarRefresh (
> UINTN EventIndex;
> UINTN CursorRow;
> UINTN CursorCol;
> + BOOLEAN ShiftPressed;
> + BOOLEAN ModifiersPressed;
>
> //
> // variable initialization
> @@ -180,17 +182,23 @@ InputBarRefresh (
> if (EFI_ERROR (Status)) {
> continue;
> }
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0) &&
> - (KeyData.KeyState.KeyShiftState != EFI_SHIFT_STATE_VALID)) {
> - //
> - // Shift key pressed.
> - //
> + ModifiersPressed = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) != 0)
> + && (KeyData.KeyState.KeyShiftState !=
> + EFI_SHIFT_STATE_VALID);
> +
> + //
> + // TRUE if Shift is pressed and no other modifiers are pressed
> + //
> + ShiftPressed = ModifiersPressed &&
> + ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_SHIFT_PRESSED |
> + EFI_RIGHT_SHIFT_PRESSED)) == 0);
> +
> + if (ModifiersPressed && !ShiftPressed) {
> continue;
> }
> //
> // pressed ESC
> //
> - if (KeyData.Key.ScanCode == SCAN_ESC) {
> + if (!ModifiersPressed && KeyData.Key.ScanCode == SCAN_ESC) {
> Size = 0;
> Status = EFI_NOT_READY;
> break;
> @@ -198,9 +206,10 @@ InputBarRefresh (
> //
> // return pressed
> //
> - if (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN) {
> + if (!ModifiersPressed
> + && (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> + KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN)) {
> break;
> - } else if (KeyData.Key.UnicodeChar == CHAR_BACKSPACE) {
> + } else if (!ModifiersPressed && KeyData.Key.UnicodeChar ==
> + CHAR_BACKSPACE) {
> //
> // backspace
> //
> @@ -213,7 +222,8 @@ InputBarRefresh (
>
> }
> }
> - } else if (KeyData.Key.UnicodeChar <= 127 && KeyData.Key.UnicodeChar >=
> 32) {
> + } else if ((!ModifiersPressed || ShiftPressed)
> + && KeyData.Key.UnicodeChar <= 127 &&
> + KeyData.Key.UnicodeChar >= 32) {
> //
> // VALID ASCII char pressed
> //
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> index ca8bc506d9..eabbf3c571 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> @@ -190,11 +190,17 @@ MenuBarDispatchControlHotKey (
> //
> // For consoles that supports/reports shift state,
> // make sure only CONTROL is pressed.
> + // For some consoles that report shift state,
> + // Ctrl+A is still translated to 1 (UnicodeChar).
> //
> if ((KeyData->Key.UnicodeChar >= L'A') && (KeyData->Key.UnicodeChar <=
> L'Z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'A' + 1;
> } else if ((KeyData->Key.UnicodeChar >= L'a') && (KeyData->Key.UnicodeChar
> <= L'z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'a' + 1;
> + } else if ((KeyData->Key.UnicodeChar >= L'A' - L'A' + 1) && (KeyData-
> >Key.UnicodeChar <= L'Z' - L'A' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> + } else if ((KeyData->Key.UnicodeChar >= L'a' - L'a' + 1) && (KeyData-
> >Key.UnicodeChar <= L'z' - L'z' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> }
> }
> if ((SCAN_CONTROL_Z < ControlIndex)
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> index a00df49d38..394e531c16 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.
> +++ c
> @@ -137,14 +137,17 @@ HMainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> --
> 2.21.1 (Apple Git-122.3)
>
>
> -=-=-=-=-=-=
>
Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#54122): 
https://edk2.groups.io/g/devel/message/54122
> Mute This Topic: 
https://groups.io/mt/71133729/1768756
> Group Owner: 
devel+owner@edk2.groups.io
> Unsubscribe: 
https://edk2.groups.io/g/devel/unsub [zhichao.gao@...]
> -=-=-=-=-=-=

 


[PATCH] BaseTools/Scripts: Add scripts to set PACKAGES_PATH environment

Heng Luo
 

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2656

1. Add GetPackagesPath.py, it will be used to get package pathes from
special directories. A sub directory is a qualified package path
when an EDKII Package can be found under it.
2. Add SetPackagesPath.bat and SetPackagesPath.sh, these scripts call
GetPackagesPath.py to collect all package paths under specified
directories and append them to PACKAGES_PATH environment variable.

Cc: Ray Ni <ray.ni@...>
Cc: Eric Dong <eric.dong@...>
Cc: Liming Gao <liming.gao@...>
Cc: Bob Feng <bob.c.feng@...>
Cc: Amy Chan <amy.chan@...>
Signed-off-by: Heng Luo <heng.luo@...>
---
BaseTools/Scripts/GetPackagesPath.py | 94 +++++++++++++++++++++++++++++++=
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
BaseTools/Scripts/SetPackagesPath.bat | 33 +++++++++++++++++++++++++++++++=
++
BaseTools/Scripts/SetPackagesPath.sh | 40 +++++++++++++++++++++++++++++++=
+++++++++
3 files changed, 167 insertions(+)

diff --git a/BaseTools/Scripts/GetPackagesPath.py b/BaseTools/Scripts/GetPa=
ckagesPath.py
new file mode 100644
index 0000000000..82d66a6106
--- /dev/null
+++ b/BaseTools/Scripts/GetPackagesPath.py
@@ -0,0 +1,94 @@
+## @file=0D
+# Get all recursive package paths from special directories.=0D
+#=0D
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>=0D
+# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+#=0D
+=0D
+import os=0D
+import glob=0D
+import argparse=0D
+=0D
+#=0D
+# Globals for help information=0D
+#=0D
+__prog__ =3D 'GetPackagesPath.py'=0D
+__copyright__ =3D 'Copyright (c) 2020, Intel Corporation. All rights reser=
ved.'=0D
+__description__ =3D 'Gets all recursive package paths in specified directo=
ry.\n'=0D
+=0D
+def list_packages_path(root):=0D
+ """ Gets all recursive package paths in specified directory.=0D
+ A directory is a package path if it satisfies conditions below:=0D
+ 1. it is a directory=0D
+ 2. it is not an EDK II Package. An EDK II Package (directory) is=0D
+ a directory that contains an EDK II package declaration (DEC) f=
ile.=0D
+ 3. it contains at least one first level EDK II Package.=0D
+ Note: A directory is not package path but its subdirectory could b=
e.=0D
+ Example: edk2-platforms/Features is not package path=0D
+ but edk2-platforms/Features/Intel is.=0D
+=0D
+ :param root: The specified directory to find package paths in it=0D
+ :type root: String=0D
+ :returns: Return all recursive package paths=0D
+ :rtype: String list=0D
+ """=0D
+=0D
+ if (not os.path.exists(root)) or (not os.path.isdir(root)):=0D
+ return []=0D
+=0D
+ if glob.glob(os.path.join(root, '*.dec')):=0D
+ # it is an EDK II Package=0D
+ return []=0D
+=0D
+ paths =3D []=0D
+ contain_package =3D False=0D
+ for filename in os.listdir(root):=0D
+ filepath =3D os.path.join(root, filename)=0D
+ if os.path.isdir(filepath):=0D
+ if glob.glob(os.path.join(filepath, '*.dec')):=0D
+ # it is an EDK II Package=0D
+ contain_package =3D True=0D
+ else:=0D
+ # gets package paths for subdirectory if it is not package=
=0D
+ paths =3D paths + list_packages_path(filepath)=0D
+=0D
+ if contain_package:=0D
+ # root is a package path because it contains EDK II Package=0D
+ # in first level folder, inset it to head of list=0D
+ paths.insert(0, root)=0D
+=0D
+ # return package paths=0D
+ return paths=0D
+=0D
+def get_packages_path(directories):=0D
+ """ For each direcory in directories, gets all recursive package paths=
=0D
+ in this directory and joins them into one string.=0D
+=0D
+ :param directories: the list of directory=0D
+ :type directories: String list=0D
+ :returns: Return string of package paths=0D
+ :rtype: String=0D
+ """=0D
+=0D
+ packages_path =3D ''=0D
+ for directory in directories:=0D
+ directory =3D os.path.abspath(directory)=0D
+ paths =3D list_packages_path(directory)=0D
+ for path in paths:=0D
+ if packages_path =3D=3D '':=0D
+ packages_path =3D path=0D
+ else:=0D
+ packages_path +=3D os.pathsep + path=0D
+ return packages_path=0D
+=0D
+if __name__ =3D=3D '__main__':=0D
+ # Create command line argument parser object=0D
+ parser =3D argparse.ArgumentParser(=0D
+ prog=3D__prog__,=0D
+ description=3D__description__ + __copyright__,=0D
+ conflict_handler=3D'resolve'=0D
+ )=0D
+ parser.add_argument('directory', nargs=3D'+',=0D
+ help=3D'Specified directory where package packages are got fro=
m')=0D
+ args =3D parser.parse_args()=0D
+ print(get_packages_path(args.directory))=0D
diff --git a/BaseTools/Scripts/SetPackagesPath.bat b/BaseTools/Scripts/SetP=
ackagesPath.bat
new file mode 100644
index 0000000000..a8c45612c9
--- /dev/null
+++ b/BaseTools/Scripts/SetPackagesPath.bat
@@ -0,0 +1,33 @@
+@REM @file=0D
+@REM Windows batch file to set PACKAGES_PATH environment=0D
+@REM=0D
+@REM Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>=0D
+@REM SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+@REM=0D
+@REM This script calls GetPackagesPath.py to collect all package paths und=
er=0D
+@REM specified directories and append them to PACKAGES_PATH environment=0D
+@REM variable. A sub directory is a qualified package path when an EDKII=0D
+@REM Package can be found under it.=0D
+=0D
+@echo off=0D
+@if /I "%1"=3D=3D"" @goto Usage=0D
+@if /I "%1"=3D=3D"-h" @goto Usage=0D
+@if /I "%1"=3D=3D"--help" @goto Usage=0D
+@if /I "%1"=3D=3D"/?" @goto Usage=0D
+=0D
+for /f %%i in ('python %EDK_TOOLS_PATH%\Scripts\GetPackagesPath.py %*') do=
(=0D
+ if defined PACKAGES_PATH (=0D
+ set "PACKAGES_PATH=3D%PACKAGES_PATH%;%%i"=0D
+ ) else (=0D
+ set "PACKAGES_PATH=3D%%i"=0D
+ )=0D
+)=0D
+@goto End=0D
+=0D
+:Usage=0D
+@echo Usage: SetPackagesPath.bat directory [directory ...]=0D
+@echo Copyright(c) 2020, Intel Corporation. All rights reserved.=0D
+@echo Options:=0D
+@echo --help, -h Print this help screen and exit=0D
+=0D
+:End=0D
diff --git a/BaseTools/Scripts/SetPackagesPath.sh b/BaseTools/Scripts/SetPa=
ckagesPath.sh
new file mode 100644
index 0000000000..e3e337254e
--- /dev/null
+++ b/BaseTools/Scripts/SetPackagesPath.sh
@@ -0,0 +1,40 @@
+
+#
+# Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+# This script calls GetPackagesPath.py to collect all package paths under
+# specified directories and append them to PACKAGES_PATH environment
+# variable. A sub directory is a qualified package path when an EDKII
+# Package can be found under it.
+#
+# Note: This script must be \'sourced\' so the environment can be changed:
+# source SetPackagesPath.sh
+# . SetPackagesPath.sh
+
+function Usage()
+{
+ echo "Usage: source SetPackagesPath.sh directory [directory ...]"
+ echo "Copyright(c) 2020, Intel Corporation. All rights reserved."
+ echo "Options:"
+ echo " --help, -h Print this help screen and exit"
+ echo "Please note: This script must be \'sourced\' so the environment =
can be changed."
+ echo ". SetPackagesPath.sh"
+ echo "source SetPackagesPath.sh"
+}
+
+function SetEnv()
+{
+ local paths=3D$(python $EDK_TOOLS_PATH/Scripts/GetPackagesPath.py $@)
+ if [ "$PACKAGES_PATH" ]; then
+ PACKAGES_PATH=3D$PACKAGES_PATH:$paths
+ else
+ PACKAGES_PATH=3D$paths
+ fi
+}
+
+if [ $# -eq 0 -o "$1" =3D=3D "-h" -o "$1" =3D=3D "--help" -o "$1" =3D=3D "=
/?" ]; then
+ Usage
+else
+ SetEnv $@
+fi
--=20
2.24.0.windows.2


[PATCH v2] MdeModulePkg/RegularExpressionDxe: Make oniguruma a submodule in edk2.

Zhang, Shenglei
 

Use submodule way to access oniguruma. And upgrade oniguruma
version from v6.9.3 to v6.9.4_mark1.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2073

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <liming.gao@...>
Signed-off-by: Shenglei Zhang <shenglei.zhang@...>
---

v2:1.Update the inf file to make VS2019 build pass.
2.Add ignore files in MdeModulePkg.ci.yaml.

The patch is too big, so I put the change on my forked tree.
https://github.com/shenglei10/edk2/tree/oniguruma

--
2.18.0.windows.1


Re: [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

Gao, Zhichao
 

Hi Micheal Rothman,

 

Can you help to review this issue? Because of the uefi spec issue, some firmware implemented the different behavior.

Should the edk2 code to handle such issue? If yes, here maybe the situation:

1.      Change for all apps -> uefi spec update and accept  such behavior with description -> Done.

2.      Change for all apps -> uefi spec update to remove the ambiguous and reject other behavior -> removal the change in first step.

 

Hi Vitaly,

 

I used to think it is an additional support for different implementation because of the spec. But if we approve this patch, all the app in edk2 using the combo key function should be update.

Using shell’s ctrl+’c’ as an example, it need to update at the same time. Same with SCT tool.

 

Thanks,

Zhichao

 

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Friday, March 27, 2020 7:01 PM
To: Gao, Zhichao <zhichao.gao@...>
Cc: devel@edk2.groups.io; Andrew Fish <afish@...>; Laszlo Ersek <lersek@...>; Marvin Häuser <mhaeuser@...>; Kinney, Michael D <michael.d.kinney@...>; Ni, Ray <ray.ni@...>
Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

 

Hello,

 

Requesting to merge this into edk2-stable202005 for the reasons explained in BZ[1]. I assume there is no real objection for this, only the approach we make such changes in the future, but this can be postponed as we encounter more of such problems.

 

Best regards,

Vitaly

 

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2510



20 февр. 2020 г., в 03:27, Gao, Zhichao <zhichao.gao@...> написал(а):

 

Sorry for my mistake. Then I have no other comments for this patch.

Reviewed-by: Zhichao Gao <zhichao.gao@...>

 

Thanks,

Zhichao

 

From: vit9696 <vit9696@...> 
Sent: Wednesday, February 19, 2020 8:15 PM
To: Gao, Zhichao <zhichao.gao@...>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers

 

Zhichao,

 

Thanks for your review. The comment is correct, as ShiftOnlyState means the state where only shift (and no other modifiers) can be pressed or not.

 

Best wishes,

Vitaly

 

On Wed, Feb 19, 2020 at 09:55, Gao, Zhichao <zhichao.gao@...> wrote:

Hi Vitaly,

See the comment below:

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
> Cheptsov via Groups.Io
> Sent: Monday, February 10, 2020 6:18 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with
> separately reported modifiers
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2510
>
> Some firmwares:
> - Report Shift modifier even when they report upper-case unicode letter.
> - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
>
> This change provides support for these firmwares preserving the compatibility
> with the previous input handling.
>
> Signed-off-by: Michael Belyaev <usrsse2@...>
> Reviewed-by: Vitaly Cheptsov <vit9696@...>
> ---
> ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37
> ++++++++++++++------
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28
> ++++++++++-----
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 ++++
> ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11
> +++---
> 4 files changed, 58 insertions(+), 24 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> index df530f1119..9615a4dfbd 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> @@ -1381,8 +1381,8 @@ MainCommandDisplayHelp (
> continue;
> }
>
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
> - (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> + if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> //
> // For consoles that don't support/report shift state,
> // CTRL+W is translated to L'W' - L'A' + 1.
> @@ -1390,14 +1390,17 @@ MainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == L'w') || (KeyData.Key.UnicodeChar == L'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> @@ -1834,7 +1837,8 @@ MainEditorKeyInput (
> EFI_KEY_DATA KeyData;
> EFI_STATUS Status;
> EFI_SIMPLE_POINTER_STATE MouseState;
> - BOOLEAN NoShiftState;
> + BOOLEAN NoModifierState;
> + BOOLEAN ShiftOnlyState;
>
> do {
>
> @@ -1886,17 +1890,28 @@ MainEditorKeyInput (
> //
> StatusBarSetRefresh();
> //
> - // NoShiftState: TRUE when no shift key is pressed.
> + // NoModifierState: TRUE when no modifier key is pressed.
> //
> - NoShiftState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0) || (KeyData.KeyState.KeyShiftState ==
> EFI_SHIFT_STATE_VALID);
> + NoModifierState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID);
> + //
> + // ShiftOnlyState: TRUE when no modifier key except Shift is pressed.
> + //
> + ShiftOnlyState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || ((KeyData.KeyState.KeyShiftState
> + & ~(EFI_SHIFT_STATE_VALID |
> + EFI_LEFT_SHIFT_PRESSED | EFI_RIGHT_SHIFT_PRESSED)) == 0);
> //
> // dispatch to different components' key handling function
> //
> if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey(&KeyData)) {
> Status = EFI_SUCCESS;
> - } else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) ||
> ((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <=
> SCAN_PAGE_DOWN)))) {
> + } else if ((ShiftOnlyState && (KeyData.Key.ScanCode == SCAN_NULL))
> + || (NoModifierState && (KeyData.Key.ScanCode >= SCAN_UP) &&
> (KeyData.Key.ScanCode <= SCAN_PAGE_DOWN))) {
> + //
> + // alphanumeric keys with or without shift, or arrow keys without shift
> + //

This is unmatched with the comments. It only handles the alphanumeric keys with shift.

Thanks,
Zhichao

> Status = FileBufferHandleInput (&KeyData.Key);
> - } else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) &&
> (KeyData.Key.ScanCode <= SCAN_F12)) {
> + } else if (NoModifierState && (KeyData.Key.ScanCode >= SCAN_F1)
> + && (KeyData.Key.ScanCode <= SCAN_F12)) {
> Status = MenuBarDispatchFunctionKey (&KeyData.Key);
> } else {
> StatusBarSetStatusString (L"Unknown Command"); diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> index 35b0b701e8..d053059220 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> @@ -130,6 +130,8 @@ InputBarRefresh (
> UINTN EventIndex;
> UINTN CursorRow;
> UINTN CursorCol;
> + BOOLEAN ShiftPressed;
> + BOOLEAN ModifiersPressed;
>
> //
> // variable initialization
> @@ -180,17 +182,23 @@ InputBarRefresh (
> if (EFI_ERROR (Status)) {
> continue;
> }
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0) &&
> - (KeyData.KeyState.KeyShiftState != EFI_SHIFT_STATE_VALID)) {
> - //
> - // Shift key pressed.
> - //
> + ModifiersPressed = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) != 0)
> + && (KeyData.KeyState.KeyShiftState !=
> + EFI_SHIFT_STATE_VALID);
> +
> + //
> + // TRUE if Shift is pressed and no other modifiers are pressed
> + //
> + ShiftPressed = ModifiersPressed &&
> + ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_SHIFT_PRESSED |
> + EFI_RIGHT_SHIFT_PRESSED)) == 0);
> +
> + if (ModifiersPressed && !ShiftPressed) {
> continue;
> }
> //
> // pressed ESC
> //
> - if (KeyData.Key.ScanCode == SCAN_ESC) {
> + if (!ModifiersPressed && KeyData.Key.ScanCode == SCAN_ESC) {
> Size = 0;
> Status = EFI_NOT_READY;
> break;
> @@ -198,9 +206,10 @@ InputBarRefresh (
> //
> // return pressed
> //
> - if (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN) {
> + if (!ModifiersPressed
> + && (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> + KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN)) {
> break;
> - } else if (KeyData.Key.UnicodeChar == CHAR_BACKSPACE) {
> + } else if (!ModifiersPressed && KeyData.Key.UnicodeChar ==
> + CHAR_BACKSPACE) {
> //
> // backspace
> //
> @@ -213,7 +222,8 @@ InputBarRefresh (
>
> }
> }
> - } else if (KeyData.Key.UnicodeChar <= 127 && KeyData.Key.UnicodeChar >=
> 32) {
> + } else if ((!ModifiersPressed || ShiftPressed)
> + && KeyData.Key.UnicodeChar <= 127 &&
> + KeyData.Key.UnicodeChar >= 32) {
> //
> // VALID ASCII char pressed
> //
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> index ca8bc506d9..eabbf3c571 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> @@ -190,11 +190,17 @@ MenuBarDispatchControlHotKey (
> //
> // For consoles that supports/reports shift state,
> // make sure only CONTROL is pressed.
> + // For some consoles that report shift state,
> + // Ctrl+A is still translated to 1 (UnicodeChar).
> //
> if ((KeyData->Key.UnicodeChar >= L'A') && (KeyData->Key.UnicodeChar <=
> L'Z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'A' + 1;
> } else if ((KeyData->Key.UnicodeChar >= L'a') && (KeyData->Key.UnicodeChar
> <= L'z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'a' + 1;
> + } else if ((KeyData->Key.UnicodeChar >= L'A' - L'A' + 1) && (KeyData-
> >Key.UnicodeChar <= L'Z' - L'A' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> + } else if ((KeyData->Key.UnicodeChar >= L'a' - L'a' + 1) && (KeyData-
> >Key.UnicodeChar <= L'z' - L'z' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> }
> }
> if ((SCAN_CONTROL_Z < ControlIndex)
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> index a00df49d38..394e531c16 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.
> +++ c
> @@ -137,14 +137,17 @@ HMainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> --
> 2.21.1 (Apple Git-122.3)
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#54122): https://edk2.groups.io/g/devel/message/54122
> Mute This Topic: https://groups.io/mt/71133729/1768756
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@...]
> -=-=-=-=-=-=

 


Re: [edk2-staging/EdkRepo] [PATCH] EdkRepo: Allow pin file to be checked out when combo is not in project manifest

Nate DeSimone
 

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@...>

-----Original Message-----
From: Desimone, Ashley E <ashley.e.desimone@...>
Sent: Tuesday, March 31, 2020 5:51 PM
To: devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@...>; Pandya, Puja <puja.pandya@...>; Bjorge, Erik C <erik.c.bjorge@...>; Bret Barkelew <Bret.Barkelew@...>; Agyeman, Prince <prince.agyeman@...>
Subject: [edk2-staging/EdkRepo] [PATCH] EdkRepo: Allow pin file to be checked out when combo is not in project manifest

When a pin file is based on a combo that is not in the project manifest file but otherwise matches the project print a warning instead of throwing and exception and allow the pin to be checked out.

Signed-off-by: Ashley E Desimone <ashley.e.desimone@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Puja Pandya <puja.pandya@...>
Cc: Erik Bjorge <erik.c.bjorge@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Prince Agyeman <prince.agyeman@...>
---
edkrepo/commands/checkout_pin_command.py | 10 ++++++++--
edkrepo/commands/humble/checkout_pin_humble.py | 4 +++-
2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/edkrepo/commands/checkout_pin_command.py b/edkrepo/commands/checkout_pin_command.py
index 619fcf8..72075bf 100644
--- a/edkrepo/commands/checkout_pin_command.py
+++ b/edkrepo/commands/checkout_pin_command.py
@@ -90,11 +90,17 @@ class CheckoutPinCommand(EdkrepoCommand):
elif not set(pin.remotes).issubset(set(manifest.remotes)):
raise EdkrepoProjectMismatchException(humble.MANIFEST_MISMATCH)
elif pin.general_config.current_combo not in [c.name for c in manifest.combinations]:
- raise EdkrepoProjectMismatchException(humble.MANIFEST_MISMATCH)
+
+ print(humble.COMBO_NOT_FOUND.format(pin.general_config.current_combo))
combo_name = pin.general_config.current_combo
pin_sources = pin.get_repo_sources(combo_name)
pin_root_remote = {source.root:source.remote_name for source in pin_sources}
- manifest_sources = manifest.get_repo_sources(combo_name)
+ try:
+ # If the pin and the project manifest have the same combo get the
+ # repo sources from that combo. Otherwise get the default combo's
+ # repo sources
+ manifest_sources = manifest.get_repo_sources(combo_name)
+ except ValueError:
+ manifest_sources =
+ manifest.get_repo_sources(manifest.general_config.default_combo)
manifest_root_remote = {source.root:source.remote_name for source in manifest_sources}
if set(pin_root_remote.items()).isdisjoint(set(manifest_root_remote.items())):
raise EdkrepoProjectMismatchException(humble.MANIFEST_MISMATCH)
diff --git a/edkrepo/commands/humble/checkout_pin_humble.py b/edkrepo/commands/humble/checkout_pin_humble.py
index ac7467d..ec6938d 100644
--- a/edkrepo/commands/humble/checkout_pin_humble.py
+++ b/edkrepo/commands/humble/checkout_pin_humble.py
@@ -12,4 +12,6 @@ NOT_FOUND = 'The selected PIN file was not found.'
MANIFEST_MISMATCH = ('The selected PIN file does not refer to the same project '
'as the local manifest file. {}'.format(CHP_EXIT)) COMMIT_NOT_FOUND = 'The commit referenced by the PIN file does not exist. {}'.format(CHP_EXIT) -PIN_COMBO = 'Pin: {}'
\ No newline at end of file
+PIN_COMBO = 'Pin: {}'
+COMBO_NOT_FOUND = ('Warning: The combo listed in PIN file: {} is no longer '
+ 'listed in the project manifest file.')
\ No newline at end of file
--
2.16.2.windows.1


Re: [edk2-staging/EdkRepo] [PATCH v1 0/7] Adding support for archiving branch combos

Nate DeSimone
 

Erik,

Please remember to Cc: the reviewers on the cover letter as well next time.

For the series...
Reviewed-by: Nate DeSimone <nathaniel.l.desimone@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bjorge, Erik C
Sent: Tuesday, March 31, 2020 3:42 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] [edk2-staging/EdkRepo] [PATCH v1 0/7] Adding support for archiving branch combos

Adding the ability to mark a branch combination as archived. This will remove it from the list of valid combinations by default. It should not limit users from accessing the branch combination. The archive flag will allow users to list archived branch combinations in the combo command.

Erik Bjorge (7):
EdkRepo: Adding support for archiving combos
EdkRepo: Added ability to display archived combinations
EdkRepo: Update Checkout for archived combos
EdkRepo: Update Sync for archived combos
EdkRepo: Update Checkout Pin for archived combos
EdkRepo: Update clone for archived combos
EdkRepo: Update List Repos for archived combos

edkrepo/commands/arguments/combo_args.py | 5 ++-- edkrepo/commands/checkout_pin_command.py | 4 +--
edkrepo/commands/clone_command.py | 6 ++--
edkrepo/commands/combo_command.py | 19 ++++++++++--
edkrepo/commands/list_repos_command.py | 37 +++++++++++++++++-------
edkrepo/commands/sync_command.py | 16 +++++-----
edkrepo/common/common_repo_functions.py | 10 +++++-- edkrepo_manifest_parser/edk_manifest.py | 10 ++++++-
8 files changed, 76 insertions(+), 31 deletions(-)

--
2.21.0.windows.1


Re: [edk2-staging/EdkRepo] [PATCH] EdkRepo: Improve state tracking when checking out pin files

Nate DeSimone
 

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Desimone, Ashley E
Sent: Tuesday, March 31, 2020 2:00 PM
To: devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@...>; Pandya, Puja <puja.pandya@...>; Bjorge, Erik C <erik.c.bjorge@...>; Bret Barkelew <Bret.Barkelew@...>; Agyeman, Prince <prince.agyeman@...>
Subject: [edk2-devel] [edk2-staging/EdkRepo] [PATCH] EdkRepo: Improve state tracking when checking out pin files

Improves the state tracking when checking out onto a pin file
by: (1)moving the call to write_current_combo() after the succesfull checkout, (2)changing the name of the combo written to the format:
'Pin: {pinfilename}', (3)If the current combo is a knon pin file (starts with 'Pin:') get_repo_sources() will return the repo sources from the default combo

Signed-off-by: Ashley E Desimone <ashley.e.desimone@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Puja Pandya <puja.pandya@...>
Cc: Erik Bjorge <erik.c.bjorge@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Prince Agyeman <prince.agyeman@...>
---
edkrepo/commands/checkout_pin_command.py | 2 +-
edkrepo/commands/humble/checkout_pin_humble.py | 3 ++-
edkrepo_manifest_parser/edk_manifest.py | 4 ++++
3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/edkrepo/commands/checkout_pin_command.py b/edkrepo/commands/checkout_pin_command.py
index a2afc41..619fcf8 100644
--- a/edkrepo/commands/checkout_pin_command.py
+++ b/edkrepo/commands/checkout_pin_command.py
@@ -53,7 +53,6 @@ class CheckoutPinCommand(EdkrepoCommand):
origin = repo.remotes.origin
origin.fetch()
self.__pin_matches_project(pin, manifest, workspace_path)
- manifest.write_current_combo(pin.general_config.current_combo)
sparse_enabled = sparse_checkout_enabled(workspace_path, manifest_sources)
if sparse_enabled:
print(SPARSE_RESET)
@@ -61,6 +60,7 @@ class CheckoutPinCommand(EdkrepoCommand):
pin_repo_sources = pin.get_repo_sources(pin.general_config.current_combo)
try:
checkout_repos(args.verbose, args.override, pin_repo_sources, workspace_path, manifest)
+
+ manifest.write_current_combo(humble.PIN_COMBO.format(args.pinfile))
finally:
if sparse_enabled:
print(SPARSE_CHECKOUT)
diff --git a/edkrepo/commands/humble/checkout_pin_humble.py b/edkrepo/commands/humble/checkout_pin_humble.py
index b5a9cfb..ac7467d 100644
--- a/edkrepo/commands/humble/checkout_pin_humble.py
+++ b/edkrepo/commands/humble/checkout_pin_humble.py
@@ -11,4 +11,5 @@ CHP_EXIT = 'Exiting without checkout out PIN data.'
NOT_FOUND = 'The selected PIN file was not found.'
MANIFEST_MISMATCH = ('The selected PIN file does not refer to the same project '
'as the local manifest file. {}'.format(CHP_EXIT)) -COMMIT_NOT_FOUND = 'The commit referenced by the PIN file does not exist. {}'.format(CHP_EXIT) \ No newline at end of file
+COMMIT_NOT_FOUND = 'The commit referenced by the PIN file does not
+exist. {}'.format(CHP_EXIT) PIN_COMBO = 'Pin: {}'
\ No newline at end of file
diff --git a/edkrepo_manifest_parser/edk_manifest.py b/edkrepo_manifest_parser/edk_manifest.py
index dd3512b..2d3e79e 100644
--- a/edkrepo_manifest_parser/edk_manifest.py
+++ b/edkrepo_manifest_parser/edk_manifest.py
@@ -311,6 +311,10 @@ class ManifestXml(BaseXmlHelper):
def get_repo_sources(self, combo_name):
if combo_name in self.__combo_sources:
return self._tuple_list(self.__combo_sources[combo_name])
+ elif combo_name.startswith('Pin:'):
+ # If currently checked out onto a pin file reture the sources in the
+ # default combo
+ return
+ self._tuple_list(self.__combo_sources[self.general_config.default_comb
+ o])
else:
raise ValueError(COMB_INVALIDINPUT_ERROR.format(combo_name))

--
2.16.2.windows.1


Query about VS2012

Zhang, Shenglei
 

Hi All,

 

We have a build failure with VS2012(https://bugzilla.tianocore.org/show_bug.cgi?id=2631).

If no body uses VS2012, I would like to ignore this issue. I send this mail to collect information whether you are using VS2012.

 

Thanks,

Shenglei


Re: [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()

Siyuan, Fu
 

With Laszlo's comments.
Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: 2020年4月1日 20:02
To: Maciej Rabeda <maciej.rabeda@...>; devel@edk2.groups.io
Cc: Wu, Jiaxin <jiaxin.wu@...>; Fu, Siyuan <siyuan.fu@...>
Subject: Re: [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in
Ip6ProcessRouterAdvertise()

Hi Maciej,

On 04/01/20 11:53, Maciej Rabeda wrote:
This patch fixes reversed logic of recently added ASSERTs which should
ensure that Ip6IsNDOptionValid() implementation properly reacts to
invalid
packets.

Cc: Jiaxin Wu <jiaxin.wu@...>
Cc: Siyuan Fu <siyuan.fu@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
Tested-by: Laszlo Ersek <lersek@...>
---
NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Thanks for the patch. Two meta-suggestions:

(1) we should do one of the following:

(1a) Create a new BZ for this issue, cross-link it -- via the See Also
field -- with TianoCore#2174, and reference the new BZ in this commit
message. If we file this new BZ, it should get the Regression keyword.

(1b) Or else we should reopen TianoCore#2174, and reference *that* BZ in
this commit message.

(2) Independently of (1), the commit message should carry the following tag:

Fixes: 9c20342eed70ec99ec50cd73cb81804299f05403

Regarding this patch, the above updates only affect the commit message,
so I'm not asking for a v2 -- you can implement the commit message
changes right before pushing.

Thanks!
Laszlo



diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index fd7f60b2f92c..0780a98cb325 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().
//
ASSERT (LinkLayerOption.Length != 0);
- ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32)
Head->PayloadLength);
+ ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <= (UINT32)
Head->PayloadLength);

ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));
CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);
@@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().
//
ASSERT (PrefixOption.Length == 4);
- ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head-
PayloadLength);
+ ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <= (UINT32) Head-
PayloadLength);

PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime);
PrefixOption.PreferredLifetime = NTOHL
(PrefixOption.PreferredLifetime);
@@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().
//
ASSERT (MTUOption.Length == 1);
- ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head-
PayloadLength);
+ ASSERT (Offset + (UINT32) MTUOption.Length * 8 <= (UINT32) Head-
PayloadLength);

//
// Use IPv6 minimum link MTU 1280 bytes as the maximum packet size
in order