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

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

[-- Attachment #1: Type: text/plain, Size: 10066 bytes --]



> On Oct 11, 2019, at 4:50 AM, Laszlo Ersek <lersek@redhat.com> 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 <http://mid.mail-archive.com/8bacbb53-98df-724e-b704-b5059d11e378@redhat.com>
>  https://edk2.groups.io/g/devel/message/48133 <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.
> 

Laszlo,

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. 

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). 

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 7968a58f34..fd551d17e1 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -317,31 +317,35 @@ BdsWait (
   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);
+  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.
+      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;
+      if (HotkeyTriggered != NULL) {
+        Status = BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERIOD_SECONDS (1));
+        if (!EFI_ERROR (Status)) {
+          break;
+        }
+      } else {
+        gBS->Stall (1000000);
       }
-    } 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--;
+      //
+      // 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);
     }
   }
-  PlatformBootManagerWaitCallback (0);
   DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
 }
 
Thanks,

Andrew Fish

> 
> 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"));
>> }
>> 
> 
> Thanks!
> Laszlo
> 
> 


[-- Attachment #2: Type: text/html, Size: 79693 bytes --]