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

["Andrew Fish" <afish@apple.com>]

> On Oct 11, 2019, at 9:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> Hi Andrew,
> 
> On 10/11/19 15:13, Andrew Fish wrote:
> 
>> I'm with Pete on this as my expectation would be the progress bar
>> completes when you are done. I'd also point out that a common progress
>> bar UI implementation is to show the area that is going to be updated.
>> I don't think our UI does that, but it could get updated at some
>> point. Thus I would posit that if PlatformBootManagerWaitCallback() is
>> called by passing in PcdPlatformBootTimeOut you should always call
>> PlatformBootManagerWaitCallback() with zero to terminate the progress
>> bar.
> 
> OK.
> 
>> What makes sense to me is to skip the UI if PcdPlatformBootTimeOut is
>> zero. The PlatformBootManagerWaitCallback (0) is only really needed if
>> PlatformBootManagerWaitCallback (0) has not been called (TimeoutRemain
>> != 0).
> 
> Let me paste your patch below, formatted with the
> "--ignore-space-change" option (it hides changes due to simple
> re-indentation of code):
> 
>> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> index 7968a58f3454..fd551d17e15d 100644
>> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -310,48 +310,52 @@ VOID
>> BdsWait (
>>   IN EFI_EVENT      HotkeyTriggered
>>   )
>> {
>>   EFI_STATUS            Status;
>>   UINT16                TimeoutRemain;
>> 
>>   DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Zzzzzzzzzzzz...\n"));
>> 
>>   TimeoutRemain = PcdGet16 (PcdPlatformBootTimeOut);
>> +  if (TimeoutRemain != 0) {
>>     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--;
>>       }
>>     }
>> +    if (TimeoutRemain != 0) {
>>       PlatformBootManagerWaitCallback (0);
>> +    }
>> +  }
>>   DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>> }
> 
> This would solve the (PcdPlatformBootTimeOut==0) case well.
> 
> However, it would regress Pete's use case. Namely, if you allow the loop
> to time out (the "while" statement sees a zero "TimeoutRemain"), then
> the final PlatformBootManagerWaitCallback (0) is skipped.
> 
> And that's the problem Pete intended to fix in the first place: when the
> loop times out, PlatformBootManagerWaitCallback (0) is *never* called,
> inside the loop. So we start booting e.g. an OS, without the progress
> bar filling the screen to the right edge.
> 
> I think if we eliminate the *inner* "if" statement -- call
> PlatformBootManagerWaitCallback (0) unconditionally --, and preserve the
> outer "if" statement, it should work.
> 

Laszlo,

Good point. I was trying to prevent 2 calls with 0 but now I notice that is not possible in the while loop. So I agree with your suggestion. I also like Pete's idea of adding the error check in PlatformBootManagerWaitCallback().

Thanks,

Andrew Fish


> Thanks
> Laszlo
> 
> 
>