On 05/05/16 07:08, Ni, Ruiyu wrote:
>
>
> Regards,
> Ray
>
>> -----Original Message-----
>> From: Daniil Egranov [mailto:[email protected]]
>> Sent: Thursday, May 5, 2016 7:57 AM
>> To: Ni, Ruiyu <[email protected]>; [email protected]
>> Cc: Fan, Jeff <[email protected]>
>> 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:[email protected]] On Behalf Of
>>>> Daniil Egranov
>>>> Sent: Wednesday, May 4, 2016 9:34 AM
>>>> To: [email protected]
>>>> Cc: Fan, Jeff <[email protected]>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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.
Right, that's what I wanted to mention. It is not obvious (it was not
obvious to me either -- someone on this list advised me about the
correct solution), but the point is that platform BDS code has to call
PlatformBdsEnterFrontPage() explicitly, with the desired timeout.
Thanks
Laszlo
>
>>>
>>>
>>>> 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
>>>> [email protected]
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> _______________________________________________
>>> edk2-devel mailing list
>>> [email protected]
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
> _______________________________________________
> edk2-devel mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
