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

["Laszlo Ersek" <lersek@redhat.com>]

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.

Thanks
Laszlo