Regards, Ray
>-----Original Message----- >From: Daniil Egranov [mailto:daniil.egra...@arm.com] >Sent: Thursday, May 5, 2016 7:57 AM >To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org >Cc: Fan, Jeff <jeff....@intel.com> >Subject: Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg/BdsDxe: Fix for the >BDS boot timeout > >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. You could refer to the OvmfPkg/Library/PlatformBdsLib. PlatformBdsEnterFrontPage() is called by PlatformBdsPolicyBehavior() with the timeout. >> >> >>> 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. The IntelFrameworkModulePkg/Bds requires platform map the PcdPlatformBootTimeOut to L"Timeout". >> >> >>> >>> // >>> // 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
