[PATCH v2 0/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

[PATCH v2 0/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

[JamesWang [王家明]]

This version has the fix for previous coding style.
Code changes can be found here: https://github.com/JamesWangAmiTw/edk2-platforms/tree/F7OverwriteBoot0000

James Wang (1):
  BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

 Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

--
2.13.2.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

[PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

[JamesWang [王家明]]

Check if mBootMenuOptionNumber is valid or not before
calling RegisterBootOptionHotkey to avoid overwriting Boot0000

Cc: Eric Dong <eric.dong@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: James Wang <jameswang@ami.com>
---
 Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
index 6e3c21f4f052..34c971f3780b 100644
--- a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
+++ b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
@@ -461,12 +461,17 @@ RegisterStaticHotkey (
     RegisterBootOptionHotkey ((UINT16) mSetupOptionNumber, &F2.Key, TRUE);
   }

-  F7.Key.ScanCode    = SCAN_F7;
-  F7.Key.UnicodeChar = CHAR_NULL;
-  F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
-  F7.KeyState.KeyToggleState = 0;
-  mBootMenuBoot  = !EnterSetup;
-  RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key, mBootMenuBoot);
+  //
+  // Register F7 only when the mBootMenuOptionNumber is valid
+  //
+  if (mBootMenuOptionNumber != LoadOptionNumberUnassigned) {
+    F7.Key.ScanCode    = SCAN_F7;
+    F7.Key.UnicodeChar = CHAR_NULL;
+    F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
+    F7.KeyState.KeyToggleState = 0;
+    mBootMenuBoot  = !EnterSetup;
+    RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key, mBootMenuBoot);
+  }

 }

--
2.13.2.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [edk2-devel] [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

[Benjamin Doron]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

I agree this is good, but an alternate option is to EfiBootManagerGetBootManagerMenu(), as I've implemented here: https://edk2.groups.io/g/devel/topic/84876596. Then, the boot menu is guaranteed to have it's boot option.

However, as Nate mentioned, there's the issue of https://github.com/tianocore/edk2-platforms/blob/784f7739f5afd268042d4d9e8ef570131620c82c/Platform/Intel/WhitleyOpenBoardPkg/PlatformPkg.dsc#L323 and https://github.com/tianocore/edk2-platforms/blob/784f7739f5afd268042d4d9e8ef570131620c82c/Platform/Intel/SimicsOpenBoardPkg/BoardX58Ich10/OpenBoardPkgPcd.dsc#L210. So, might we prefer this implementation?

[-- Attachment #2: Type: text/html, Size: 1127 bytes --]

Re: [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

[Isaac Oram]

Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>

James,

I cannot apply the patch.  I can fix it up to push though, so I don't need you to resolve.  But for a bigger patch, I would need help resolving.

I note that you have special characters in your name.  I doubt that matters, but I would probably try to keep it normal 8 bit ASCII.  I think that the script many maintainers use will lose those characters in favor of "?".
I note that you are using an old version of git.  Also probably not the issue.

I note that you seem to have LF line endings and I have CRLF when I generate and send patches.  Maybe try configuring: 
git config --global am.keepcr true
git config --global core.autocrlf          false​

These are my notes on config.  I am not sure if they are relevant still.  Especially the second one.  I don't seem to have that set anymore.  Setting keepcr false caused an error applying patch for me.

Regards,
Isaac

-----Original Message-----
From: JamesWang [王家明] <JamesWang@ami.com> 
Sent: Tuesday, September 13, 2022 2:36 AM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

Check if mBootMenuOptionNumber is valid or not before calling RegisterBootOptionHotkey to avoid overwriting Boot0000

Cc: Eric Dong <eric.dong@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: James Wang <jameswang@ami.com>
---
 Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
index 6e3c21f4f052..34c971f3780b 100644
--- a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
+++ b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOpt
+++ ion.c
@@ -461,12 +461,17 @@ RegisterStaticHotkey (
     RegisterBootOptionHotkey ((UINT16) mSetupOptionNumber, &F2.Key, TRUE);
   }

-  F7.Key.ScanCode    = SCAN_F7;
-  F7.Key.UnicodeChar = CHAR_NULL;
-  F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
-  F7.KeyState.KeyToggleState = 0;
-  mBootMenuBoot  = !EnterSetup;
-  RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key, mBootMenuBoot);
+  //
+  // Register F7 only when the mBootMenuOptionNumber is valid  //  if 
+ (mBootMenuOptionNumber != LoadOptionNumberUnassigned) {
+    F7.Key.ScanCode    = SCAN_F7;
+    F7.Key.UnicodeChar = CHAR_NULL;
+    F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
+    F7.KeyState.KeyToggleState = 0;
+    mBootMenuBoot  = !EnterSetup;
+    RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key, 
+ mBootMenuBoot);  }

 }

--
2.13.2.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [edk2-devel] [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

[Isaac Oram]

Pushed as 76fc44fecb..801d003703

Note I fixed two nearby comments and my editor cleaned up trailing whitespace automatically.
-  // [F2]/[F7]
+  // [F2]

And
-  // Register F7 only when the mBootMenuOptionNumber is valid
+  // Register [F7] only when the mBootMenuOptionNumber is valid

Regards,
Isaac

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Isaac Oram
Sent: Thursday, September 15, 2022 5:06 PM
To: JamesWang [王家明] <JamesWang@ami.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: Re: [edk2-devel] [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>

James,

I cannot apply the patch.  I can fix it up to push though, so I don't need you to resolve.  But for a bigger patch, I would need help resolving.

I note that you have special characters in your name.  I doubt that matters, but I would probably try to keep it normal 8 bit ASCII.  I think that the script many maintainers use will lose those characters in favor of "?".
I note that you are using an old version of git.  Also probably not the issue.

I note that you seem to have LF line endings and I have CRLF when I generate and send patches.  Maybe try configuring: 
git config --global am.keepcr true
git config --global core.autocrlf          false​

These are my notes on config.  I am not sure if they are relevant still.  Especially the second one.  I don't seem to have that set anymore.  Setting keepcr false caused an error applying patch for me.

Regards,
Isaac

-----Original Message-----
From: JamesWang [王家明] <JamesWang@ami.com>
Sent: Tuesday, September 13, 2022 2:36 AM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

Check if mBootMenuOptionNumber is valid or not before calling RegisterBootOptionHotkey to avoid overwriting Boot0000

Cc: Eric Dong <eric.dong@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: James Wang <jameswang@ami.com>
---
 Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
index 6e3c21f4f052..34c971f3780b 100644
--- a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
+++ b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOpt
+++ ion.c
@@ -461,12 +461,17 @@ RegisterStaticHotkey (
     RegisterBootOptionHotkey ((UINT16) mSetupOptionNumber, &F2.Key, TRUE);
   }

-  F7.Key.ScanCode    = SCAN_F7;
-  F7.Key.UnicodeChar = CHAR_NULL;
-  F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
-  F7.KeyState.KeyToggleState = 0;
-  mBootMenuBoot  = !EnterSetup;
-  RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key, mBootMenuBoot);
+  //
+  // Register F7 only when the mBootMenuOptionNumber is valid  //  if 
+ (mBootMenuOptionNumber != LoadOptionNumberUnassigned) {
+    F7.Key.ScanCode    = SCAN_F7;
+    F7.Key.UnicodeChar = CHAR_NULL;
+    F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
+    F7.KeyState.KeyToggleState = 0;
+    mBootMenuBoot  = !EnterSetup;
+    RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key, 
+ mBootMenuBoot);  }

 }

--
2.13.2.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

Re: [edk2-devel] [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

[JamesWang [王家明]]

Isaac,

Thank you for the review and help for this commit.  Yes my git version is old and I will update it to the latest one.  Also I have set the config for CRLF like you told me.

Thanks!

-----Original Message-----
From: Oram, Isaac W <isaac.w.oram@intel.com>
Sent: Friday, September 16, 2022 8:09 AM
To: devel@edk2.groups.io; Oram, Isaac W <isaac.w.oram@intel.com>; JamesWang [王家明] <JamesWang@ami.com>
Cc: Dong, Eric <eric.dong@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: [EXTERNAL] RE: [edk2-devel] [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000


**CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**

Pushed as 76fc44fecb..801d003703

Note I fixed two nearby comments and my editor cleaned up trailing whitespace automatically.
-  // [F2]/[F7]
+  // [F2]

And
-  // Register F7 only when the mBootMenuOptionNumber is valid
+  // Register [F7] only when the mBootMenuOptionNumber is valid

Regards,
Isaac

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Isaac Oram
Sent: Thursday, September 15, 2022 5:06 PM
To: JamesWang [王家明] <JamesWang@ami.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: Re: [edk2-devel] [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>

James,

I cannot apply the patch.  I can fix it up to push though, so I don't need you to resolve.  But for a bigger patch, I would need help resolving.

I note that you have special characters in your name.  I doubt that matters, but I would probably try to keep it normal 8 bit ASCII.  I think that the script many maintainers use will lose those characters in favor of "?".
I note that you are using an old version of git.  Also probably not the issue.

I note that you seem to have LF line endings and I have CRLF when I generate and send patches.  Maybe try configuring:
git config --global am.keepcr true
git config --global core.autocrlf          false​

These are my notes on config.  I am not sure if they are relevant still.  Especially the second one.  I don't seem to have that set anymore.  Setting keepcr false caused an error applying patch for me.

Regards,
Isaac

-----Original Message-----
From: JamesWang [王家明] <JamesWang@ami.com>
Sent: Tuesday, September 13, 2022 2:36 AM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Subject: [PATCH v2 1/1] BoardModulePkg/BoardBdsHookLib: Fix F7 overwrites Boot0000

Check if mBootMenuOptionNumber is valid or not before calling RegisterBootOptionHotkey to avoid overwriting Boot0000

Cc: Eric Dong <eric.dong@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: James Wang <jameswang@ami.com>
---
 Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
index 6e3c21f4f052..34c971f3780b 100644
--- a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
+++ b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOpt
+++ ion.c
@@ -461,12 +461,17 @@ RegisterStaticHotkey (
     RegisterBootOptionHotkey ((UINT16) mSetupOptionNumber, &F2.Key, TRUE);
   }

-  F7.Key.ScanCode    = SCAN_F7;
-  F7.Key.UnicodeChar = CHAR_NULL;
-  F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
-  F7.KeyState.KeyToggleState = 0;
-  mBootMenuBoot  = !EnterSetup;
-  RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key, mBootMenuBoot);
+  //
+  // Register F7 only when the mBootMenuOptionNumber is valid  //  if
+ (mBootMenuOptionNumber != LoadOptionNumberUnassigned) {
+    F7.Key.ScanCode    = SCAN_F7;
+    F7.Key.UnicodeChar = CHAR_NULL;
+    F7.KeyState.KeyShiftState = EFI_SHIFT_STATE_VALID;
+    F7.KeyState.KeyToggleState = 0;
+    mBootMenuBoot  = !EnterSetup;
+    RegisterBootOptionHotkey ((UINT16) mBootMenuOptionNumber, &F7.Key,
+ mBootMenuBoot);  }

 }

--
2.13.2.windows.1
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.





-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.