Hi Ray,
Thank you for review and comments. Please see my answers below.
Regards,
Daniil
On 05/04/2016 12:04 AM, Ni, Ruiyu wrote:
3 comments below.
Regards,
Ray
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Daniil
Egranov
Sent: Wednesday, May 4, 2016 9:34 AM
To: edk2-devel@lists.01.org
Cc: Fan, Jeff <jeff....@intel.com>
Subject: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg/BdsDxe: Fix for the BDS
boot timeout
The patch loads timeout value from the "Timeout" global variable and passes
it to PlatformBdsEnterFrontPage(), which handles delay and key input.
The PcdPlatformBootTimeOut is only used at the BDS entry point and updates
the "Timeout" value. This will allow the modification of the timeout value
through the BDS menu and overwrite it if PcdPlatformBootTimeOut has been
set.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daniil Egranov <daniil.egra...@arm.com>
---
IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 18 +++++++++++++++---
.../Universal/BdsDxe/BootMaint/BootMaint.c | 17 ++++++++++++++++-
.../Universal/BdsDxe/BootMaint/UpdatePage.c | 13 +++++++------
3 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
index ae7ad21..1d80fca 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -123,6 +123,7 @@ BdsBootDeviceSelect (
BOOLEAN BootNextExist;
LIST_ENTRY *LinkBootNext;
EFI_EVENT ConnectConInEvent;
+ UINTN Size;
//
// Got the latest boot option
@@ -214,6 +215,18 @@ BdsBootDeviceSelect (
if (Link == NULL) {
return ;
}
+
+ //Read boot timeout variable
+ Status = gRT->GetVariable (L"Timeout",
+ &gEfiGlobalVariableGuid,
+ NULL,
+ &Size,
+ (VOID *) &Timeout);
+
+ if (EFI_ERROR (Status)) {
+ Timeout = 0xFFFF;
+ }
+
//
// Here we make the boot in a loop, every boot success will
// return to the front page
@@ -222,7 +235,7 @@ BdsBootDeviceSelect (
//
// Check the boot option list first
//
- if (Link == &BootLists) {
+ if (Link == &BootLists || Timeout != 0xFFFF) {
//
// When LazyConIn enabled, signal connect ConIn event before enter UI
//
@@ -238,12 +251,11 @@ BdsBootDeviceSelect (
// one is success to boot, then we back here to allow user
// add new active boot option
//
- Timeout = 0xffff;
PlatformBdsEnterFrontPage (Timeout, FALSE);
+ Timeout = 0xffff;
1. The old code is to enter front page unconditionally and immediately while you
changed the behavior to wait for Timeout seconds before entering front page.
Why?
The previous logic allows entry into the BDS boot menu in two cases: no
active boot options and all attempts to boot from the current boot
options fail. If there is a valid bootable device in the boot options, i
found no way to interrupt the boot process. In a lot of cases, the boot
options have to be changed before any boot attempts have been made. This
patch checks if boot timeout is set and uses PlatformBdsEnterFrontPage
to manage time and key inputs before using current boot options.
InitializeListHead (&BootLists);
BdsLibBuildOptionFromVar (&BootLists, L"BootOrder");
Link = BootLists.ForwardLink;
- continue;
}
//
// Get the boot option from the link list
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootMaint.c
b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootMaint.c
index d4b4475..cc2d656 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootMaint.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootMaint.c
@@ -111,6 +111,9 @@ InitializeBmmConfig (
BM_MENU_ENTRY *NewMenuEntry;
BM_LOAD_CONTEXT *NewLoadContext;
UINT16 Index;
+ UINT16 BootTimeOut;
+ EFI_STATUS Status;
+ UINTN Size;
ASSERT (CallbackData != NULL);
@@ -128,7 +131,19 @@ InitializeBmmConfig (
}
}
- CallbackData->BmmFakeNvData.BootTimeOut = PcdGet16 (PcdPlatformBootTimeOut);
+ //Read boot timeout variable. If PcdPlatformBootTimeOut has been set,
+ //the Timeout variable will be initialized as part of the BDS startup
procedure
+ Status = gRT->GetVariable ( L"Timeout",
+ &gEfiGlobalVariableGuid,
+ NULL,
+ &Size,
+ (VOID *) &BootTimeOut);
+
+ if (EFI_ERROR (Status)) {
+ BootTimeOut = 0;
+ }
+
+ CallbackData->BmmFakeNvData.BootTimeOut = BootTimeOut;
2. With the platform DSC maps the PcdPlatformBootTimeout to L"Timeout" variable
correctly,
the above change is not necessary. right?
I agree, but what about the case when PcdPlatformBootTimeOut is not
mapped or the value is not set? The L"Timeout" is initialized by
PcdPlatformBootTimeOut in the BdsEntry() call so it has the timeout
value anyway. In the current implementation, two different variables are
used for the same purpose during the run time. I think the code can be
cleaner if only one of them is used across the code. In case
PcdPlatformBootTimeOut is preferred, it has to be synchronized with UI
timeout menu input. Otherwise, the modified timeout value will be reset
back to the original PcdPlatformBootTimeOut value. Also, the
PcdPlatformBootTimeOut variable may have to be synchronized with Bmm
BootTimeOut between reboots.
//
// Initialize data which located in Boot Options Menu
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/UpdatePage.c
b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/UpdatePage.c
index b13ed11..c872766 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/UpdatePage.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/UpdatePage.c
@@ -722,18 +722,21 @@ UpdateTimeOutPage (
IN BMM_CALLBACK_DATA *CallbackData
)
{
- UINT16 BootTimeOut;
VOID *DefaultOpCodeHandle;
CallbackData->BmmAskSaveOrNot = TRUE;
UpdatePageStart (CallbackData);
- BootTimeOut = PcdGet16 (PcdPlatformBootTimeOut);
-
DefaultOpCodeHandle = HiiAllocateOpCodeHandle ();
ASSERT (DefaultOpCodeHandle != NULL);
- HiiCreateDefaultOpCode (DefaultOpCodeHandle, EFI_HII_DEFAULT_CLASS_STANDARD,
EFI_IFR_TYPE_NUM_SIZE_16,
BootTimeOut);
+
+ HiiCreateDefaultOpCode (
+ DefaultOpCodeHandle,
+ EFI_HII_DEFAULT_CLASS_STANDARD,
+ EFI_IFR_TYPE_NUM_SIZE_16,
+ CallbackData->BmmFakeNvData.BootTimeOut
+ );
3. I agree it's not necessary to re-get the timeout value from PCD again.
So in summary I think only #3 is necessary.
HiiCreateNumericOpCode (
mStartOpCodeHandle,
@@ -752,8 +755,6 @@ UpdateTimeOutPage (
HiiFreeOpCodeHandle (DefaultOpCodeHandle);
- //CallbackData->BmmFakeNvData.BootTimeOut = BootTimeOut;
-
UpdatePageEnd (CallbackData);
}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
