Re: [edk2-devel] OVMF is crashing for me in master

["Pete Batard" <pete@akeo.ie>]

Hi Lazlo,

On 2019.10.11 12:50, Laszlo Ersek wrote:
> Hi,
> 
> On 10/11/19 03:19, Liming Gao wrote:
>> Andrew:
>>    I verify the change (2de1f611be06ded3a59726a4052a9039be7d459b
>>    MdeModulePkg/BdsDxe: Also call PlatformBootManagerWaitCallback on 0)
>>    in Emulator.
>>    It works, because PCD value is set to 10 in Emulator.
>>
>>    Before this change, if TimeOut PCD is zero, BdsEntry doesn't call
>>    PlatformBootManagerWaitCallback().
>>    After this change,  if TimeOut PCD is zero, BdsEntry still call
>>    PlatformBootManagerWaitCallback(). So, it trigs this issue. I agree
>>    your fix.
>>
>> Pete:
>>    Will you contribute the patch to fix this hang issue in OVMF?
> 
> So, when Pete submitted the BDS patch, I didn't test it, I only looked
> at the patch email. See my feedback here:
> 
>    http://mid.mail-archive.com/8bacbb53-98df-724e-b704-b5059d11e378@redhat.com
>    https://edk2.groups.io/g/devel/message/48133
> 
> It seemed OK to me. In particular, I reasoned about (TimeoutRemain==0),
> and the patch seemed to do the right thing for that.
> 
> Unfortunately, what I missed was the following case:
> 
> - if PcdPlatformBootTimeOut is zero, then the patch by Pete does not
>    simply make one more call to PlatformBootManagerWaitCallback(), with
>    argument zero, but it makes the *ONLY* call to
>    PlatformBootManagerWaitCallback().
> 
> I missed this because (like normal) the patch was sent with a 3-line
> context, and I didn't think it necessary to review the full function
> context.
> 
> Here is the commit, with full function context:
> 
>> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> index f3d5e5ac0615..7968a58f3454 100644
>> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -310,47 +310,48 @@ VOID
>>   BdsWait (
>>     IN EFI_EVENT      HotkeyTriggered
>>     )
>>   {
>>     EFI_STATUS            Status;
>>     UINT16                TimeoutRemain;
>>
>>     DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Zzzzzzzzzzzz...\n"));
>>
>>     TimeoutRemain = PcdGet16 (PcdPlatformBootTimeOut);
>>     while (TimeoutRemain != 0) {
>>       DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) TimeoutRemain));
>>       PlatformBootManagerWaitCallback (TimeoutRemain);
>>
>>       BdsReadKeys (); // BUGBUG: Only reading can signal HotkeyTriggered
>>                       //         Can be removed after all keyboard drivers invoke callback in timer callback.
>>
>>       if (HotkeyTriggered != NULL) {
>>         Status = BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERIOD_SECONDS (1));
>>         if (!EFI_ERROR (Status)) {
>>           break;
>>         }
>>       } else {
>>         gBS->Stall (1000000);
>>       }
>>
>>       //
>>       // 0xffff means waiting forever
>>       // BDS with no hotkey provided and 0xffff as timeout will "hang" in the loop
>>       //
>>       if (TimeoutRemain != 0xffff) {
>>         TimeoutRemain--;
>>       }
>>     }
>> +  PlatformBootManagerWaitCallback (0);
>>     DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>>   }
> 
> We can see that the function used to handle the
> PcdPlatformBootTimeOut==0 situation well, and make *no* call to
> PlatformBootManagerWaitCallback(). The patch broke that behavior.
> 
> Therefore, I disagree with the idea to fix this in OvmfPkg. Any number
> of platforms may exist that rely on PlatformBootManagerWaitCallback()
> not being called when PcdPlatformBootTimeOut is zero.
> 
> (To name just one other example, ArmVirtPkg too is affected.)
> 
> And for OVMF and ArmVirtPkg, it is definitely correct to *not* display
> any progress bar if PcdPlatformBootTimeOut is 0.
> 
> Please consult the definition of the PCD in "MdePkg/MdePkg.dec":
> 
>>    ## The number of seconds that the firmware will wait before initiating the original default boot selection.
>>    #  A value of 0 indicates that the default boot selection is to be initiated immediately on boot.
>>    #  The value of 0xFFFF then firmware will wait for user input before booting.
>>    # @Prompt Boot Timeout (s)
>>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
> 
> Also see the specification of PlatformBootManagerWaitCallback(), in
> "MdeModulePkg/Include/Library/PlatformBootManagerLib.h":
> 
>> /**
>>    This function is called each second during the boot manager waits the timeout.
>>
>>    @param TimeoutRemain  The remaining timeout.
>> **/
>> VOID
>> EFIAPI
>> PlatformBootManagerWaitCallback (
>>    UINT16          TimeoutRemain
>>    );
> 
> According to the DEC file, there is no waiting if the PCD is zero.
> Therefore, if the PCD is zero, the function should not be called at all.
> 
> 
> There is another complication, namely, if the PCD is 0xFFFF. In this
> case, the loop goes on until a hotkey is pressed. In every iteration
> (that is, every second), PlatformBootManagerWaitCallback() is called
> with argument 0xFFFF, invariably. Clearly, this *cannot* cause a
> platform to display any progress *changes*. In the most common callback
> implementation, namely in the formula
> 
>    (Timeout - TimeoutRemain) * 100 / Timeout,
> 
> it will mean
> 
>    (0xFFFF - 0xFFFF) * 100 / 0xFFFF,
> 
> that is, "zero percent", in every iteration.
> 
> Based on that, it looks wrong to suddenly call
> PlatformBootManagerWaitCallback() with a zero argument ("hundred percent
> completion"), after it's been called, let's say for a minute or more,
> with a constant 0xFFFF argument ("zero percent completion"), until the
> user finally presses a hotkey.
> 
> Jumping from zero to 100% is discontiguous and doesn't appear helpful to
> me.
> 
> 
> Further yet, what if there is an actual progress bar and timeout (like
> 10 seconds), and the user presses a hotkey after 5 seconds? Should the
> progress bar jump to 100% at once? I wouldn't think so.
> 
> 
> Therefore, I claim that the right fix is:
> 
>> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> index 7968a58f3454..d12829afeb8b 100644
>> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -341,7 +341,16 @@ BdsWait (
>>         TimeoutRemain--;
>>       }
>>     }
>> -  PlatformBootManagerWaitCallback (0);
>> +  //
>> +  // If the platform configured a nonzero and finite time-out, and we have
>> +  // actually reached that, report 100% completion to the platform.
>> +  //
>> +  // Note that the (TimeoutRemain == 0) condition excludes
>> +  // PcdPlatformBootTimeOut=0xFFFF, and that's deliberate.
>> +  //
>> +  if (PcdGet16 (PcdPlatformBootTimeOut) != 0 && TimeoutRemain == 0) {
>> +    PlatformBootManagerWaitCallback (0);
>> +  }
>>     DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>>   }
>>

I agree with the analysis and the proposed fix. My earlier patch was 
indeed altering existing behaviour when PcdPlatformBootTimeOut was 0, 
and I completely missed that.

Also, apologies for not replying to the direct request you sent to me 
earlier. The e-mail client I use does not currently deduplicate messages 
that are sent both to the list as well as directly to me as CC, so I 
only get one copy in the list which makes these requests easy to miss. 
I'll try to fix that.

Many thanks for looking into this!

Regards,

/Pete

> 
> Thanks!
> Laszlo
>