OVMF is crashing for me in master

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* OVMF is crashing for me in master
@ 2019-10-10 21:12 Andrew Fish
  2019-10-11  1:19 ` [edk2-devel] " Liming Gao
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Fish @ 2019-10-10 21:12 UTC (permalink / raw)
  To: devel, Pete Batard

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

This is my flavor of OVMF:  build -p OvmfPkg/OvmfPkgX64.dsc -a X64 -t XCODE5

It looks like I took an exception? Is it expected that an unhandled exception just hang in a dead loop? I would have expected some serial  output about the failure? 

Looks like a divide by zero exception. The exception context has PC and FP so I can manually walk the stack. Yikes I see PlatformBootManagerWaitCallback() will fault if PcdPlatformBootTimeOut is zero? 
/Volumes/Case/UDK2018(master)>git grep PcdPlatformBootTimeOut -- *.dsc
ArmVirtPkg/ArmVirtQemu.dsc:194:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
ArmVirtPkg/ArmVirtQemuKernel.dsc:191:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
ArmVirtPkg/ArmVirtXen.dsc:122:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
EmulatorPkg/EmulatorPkg.dsc:236:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
OvmfPkg/OvmfPkgIa32.dsc:541:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
OvmfPkg/OvmfPkgIa32X64.dsc:553:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
OvmfPkg/OvmfPkgX64.dsc:552:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
OvmfPkg/OvmfXen.dsc:470:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
UefiPayloadPkg/UefiPayloadPkgIa32.dsc:344:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc:345:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3

OK PcdPlatformBootTimeOut is zero on Ovmf, so how did this ever work?

Ahhh gotom.... 
/Volumes/Case/UDK2018(master)>git blame -L344,344  /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
2de1f611be0 (Pete Batard 2019-09-25 23:50:05 +0800 344)   PlatformBootManagerWaitCallback (0);

This call causes a divide by zero if PcdPlatformBootTimeOut == 0. 

This fixes my crash:
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 70df6b841a..d6ae43e900 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1634,6 +1634,11 @@ PlatformBootManagerWaitCallback (
   UINT16                              Timeout;
 
   Timeout = PcdGet16 (PcdPlatformBootTimeOut);
+  if (Timeout ==  0) {
+    Timeout = 100;
+  } else {
+    Timeout = (Timeout - TimeoutRemain) * 100 / Timeout;
+  }
 
   Black.Raw = 0x00000000;
   White.Raw = 0x00FFFFFF;
@@ -1643,7 +1648,7 @@ PlatformBootManagerWaitCallback (
     Black.Pixel,
     L"Start boot option",
     White.Pixel,
-    (Timeout - TimeoutRemain) * 100 / Timeout,
+    Timeout,
     0
     );
 }


lldb debugger output:

(lldb) bt
* thread #1, stop reason = signal SIGTRAP
  * frame #0: 0x0000000007b58a70 CpuDxe.dll:CpuDeadLoop() + 13 at /Volumes/Case/UDK2018/MdePkg/Library/BaseLib/CpuDeadLoop.c:31
    frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
    frame #2: 0x0000000007b61624 CpuDxe.dll:CommonExceptionHandler() + 36 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c:40
    frame #3: 0x0000000007b5ff26 CpuDxe.dll:HasErrorCode() + 230
(lldb) fr sel 1
frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
   91  	    (ExternalInterruptHandler[ExceptionType]) (ExceptionType, SystemContext);
   92  	  } else if (ExceptionType < CPU_EXCEPTION_NUM) {
   93  	    //
   94  	    // Get Spinlock to display CPU information
   95  	    //
   96  	    while (!AcquireSpinLockOrFail (&ExceptionHandlerData->DisplayMessageSpinLock)) {
   97  	      CpuPause ();
   98  	    }
   99  	    //
   100 	    // Initialize the serial port before dumping.
   101 	    //
   102 	    SerialPortInitialize ();
   103 	    //
   104 	    // Display ExceptionType, CPU information and Image information
   105 	    //
   106 	    DumpImageAndCpuContent (ExceptionType, SystemContext);
   107 	    //
   108 	    // Release Spinlock of output message
   109 	    //
   110 	    ReleaseSpinLock (&ExceptionHandlerData->DisplayMessageSpinLock);
   111 	    //
   112 	    // Enter a dead loop if needn't to execute old IDT handler further
   113 	    //
   114 	    if (ReservedVectors[ExceptionType].Attribute != EFI_VECTOR_HANDOFF_HOOK_BEFORE) {
-> 115 	      CpuDeadLoop ();
   116 	    }
   117 	  }
   118 	}
   119 	
(lldb) p ExceptionType
(EFI_EXCEPTION_TYPE) $0 = 0
(lldb) p SystemContext.SystemContextX64->Rip
(UINT64) $1 = 0x0000000007a9cc38
(lldb) p SystemContext.SystemContextX64->Rbp
(UINT64) $2 = 0x0000000007e8fc20
(lldb) efi_backtrace --pc 0x0000000007a9cc38 --frame 0x0000000007e8fc20 --symbols
  frame  0: 0x07a9cc38 BdsDxe:PlatformBootManagerWaitCallback + 35 at /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646:37
  frame  1: 0x07a9e744 BdsDxe:BdsWait + 269 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344:3
  frame  2: 0x07a9dff9 BdsDxe:BdsEntry + 2620 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:1002:5
  frame  3: 0x07eb367d DxeCore:DxeMain + 2680 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c:544:3
  frame  4: 0x07e943cb DxeCore:_ModuleEntryPoint + 20 at /Volumes/Case/UDK2018/MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.c:48:3
  frame  5: 0x07edc947 DxeIpl.dll`AsmEnableCache
  frame  6: 0x07ee1e4e DxeIpl:HandOffToDxeCore + 509 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c:113:3
  frame  7: 0x07ee0604 DxeIpl:DxeLoadCore + 1354 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:449:3
  frame  8: 0x07eeff2f PeiCore.dll`PeiCore.cold.3 + 847
  frame  9: 0x07ee9d04 PeiCore:PeiCore + 163 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c:502:3
  frame 10: 0x0082c387 0x0082c387
  frame 11: 0x00825dd7 0x00825dd7
  frame 12: 0x0082ad27 0x0082ad27
  frame 13: 0x0082b3a8 0x0082b3a8
  frame 14: 0x0082bf23 0x0082bf23
  frame 15: 0x00825e24 0x00825e24
  frame 16: 0x00823af2 0x00823af2
  frame 17: 0xfffd1db8 SecMain:SecStartupPhase2 + 67 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:858:3
  frame 18: 0xfffd1d67 SecMain:SecCoreStartupWithStack + 420 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:821:3
  frame 19: 0xfffd1e14 SecMain:ProcessLibraryConstructorList + 0 at /Volumes/Case/UDK2018/Build/OvmfX64/DEBUG_XCODE5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:201

(lldb) l /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646
   1646	    (Timeout - TimeoutRemain) * 100 / Timeout,
   1647	    0
   1648	    );
   1649	}
   1650	
   1651	/**
   1652	  The function is called when no boot option could be launched,
   1653	  including platform recovery options and options pointing to applications
   1654	  built into firmware volumes.
   1655	
(lldb) l /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344
   344 	  PlatformBootManagerWaitCallback (0);
   345 	  DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
   346 	}
   347 	
   348 	/**
   349 	  Attempt to boot each boot option in the BootOptions array.
   350 	
   351 	  @param BootOptions       Input boot option array.
   352 	  @param BootOptionCount   Input boot option count.
   353 	  @param BootManagerMenu   Input boot manager menu.
   354 	


Thanks,

Andrew Fish


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-10 21:12 OVMF is crashing for me in master Andrew Fish
@ 2019-10-11  1:19 ` Liming Gao
  2019-10-11  4:59   ` Andrew Fish
  2019-10-11 11:50   ` Laszlo Ersek
  0 siblings, 2 replies; 10+ messages in thread
From: Liming Gao @ 2019-10-11  1:19 UTC (permalink / raw)
  To: devel@edk2.groups.io, afish@apple.com, Pete Batard

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

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?

Thanks
Liming
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.Io
Sent: Friday, October 11, 2019 5:12 AM
To: devel@edk2.groups.io; Pete Batard <pete@akeo.ie>
Subject: [edk2-devel] OVMF is crashing for me in master

This is my flavor of OVMF:  build -p OvmfPkg/OvmfPkgX64.dsc -a X64 -t XCODE5

It looks like I took an exception? Is it expected that an unhandled exception just hang in a dead loop? I would have expected some serial  output about the failure?

Looks like a divide by zero exception. The exception context has PC and FP so I can manually walk the stack. Yikes I see PlatformBootManagerWaitCallback() will fault if PcdPlatformBootTimeOut is zero?
/Volumes/Case/UDK2018(master)>git grep PcdPlatformBootTimeOut -- *.dsc
ArmVirtPkg/ArmVirtQemu.dsc:194:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
ArmVirtPkg/ArmVirtQemuKernel.dsc:191:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
ArmVirtPkg/ArmVirtXen.dsc:122:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
EmulatorPkg/EmulatorPkg.dsc:236:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
OvmfPkg/OvmfPkgIa32.dsc:541:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
OvmfPkg/OvmfPkgIa32X64.dsc:553:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
OvmfPkg/OvmfPkgX64.dsc:552:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
OvmfPkg/OvmfXen.dsc:470:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
UefiPayloadPkg/UefiPayloadPkgIa32.dsc:344:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc:345:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3


OK PcdPlatformBootTimeOut is zero on Ovmf, so how did this ever work?


Ahhh gotom....
/Volumes/Case/UDK2018(master)>git blame -L344,344  /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
2de1f611be0 (Pete Batard 2019-09-25 23:50:05 +0800 344)   PlatformBootManagerWaitCallback (0);


This call causes a divide by zero if PcdPlatformBootTimeOut == 0.

This fixes my crash:
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 70df6b841a..d6ae43e900 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1634,6 +1634,11 @@ PlatformBootManagerWaitCallback (
   UINT16                              Timeout;

   Timeout = PcdGet16 (PcdPlatformBootTimeOut);
+  if (Timeout ==  0) {
+    Timeout = 100;
+  } else {
+    Timeout = (Timeout - TimeoutRemain) * 100 / Timeout;
+  }

   Black.Raw = 0x00000000;
   White.Raw = 0x00FFFFFF;
@@ -1643,7 +1648,7 @@ PlatformBootManagerWaitCallback (
     Black.Pixel,
     L"Start boot option",
     White.Pixel,
-    (Timeout - TimeoutRemain) * 100 / Timeout,
+    Timeout,
     0
     );
 }



lldb debugger output:

(lldb) bt
* thread #1, stop reason = signal SIGTRAP
  * frame #0: 0x0000000007b58a70 CpuDxe.dll:CpuDeadLoop() + 13 at /Volumes/Case/UDK2018/MdePkg/Library/BaseLib/CpuDeadLoop.c:31
    frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
    frame #2: 0x0000000007b61624 CpuDxe.dll:CommonExceptionHandler() + 36 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c:40
    frame #3: 0x0000000007b5ff26 CpuDxe.dll:HasErrorCode() + 230
(lldb) fr sel 1
frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
   91             (ExternalInterruptHandler[ExceptionType]) (ExceptionType, SystemContext);
   92           } else if (ExceptionType < CPU_EXCEPTION_NUM) {
   93             //
   94             // Get Spinlock to display CPU information
   95             //
   96             while (!AcquireSpinLockOrFail (&ExceptionHandlerData->DisplayMessageSpinLock)) {
   97               CpuPause ();
   98             }
   99             //
   100           // Initialize the serial port before dumping.
   101           //
   102           SerialPortInitialize ();
   103           //
   104           // Display ExceptionType, CPU information and Image information
   105           //
   106           DumpImageAndCpuContent (ExceptionType, SystemContext);
   107           //
   108           // Release Spinlock of output message
   109           //
   110           ReleaseSpinLock (&ExceptionHandlerData->DisplayMessageSpinLock);
   111           //
   112           // Enter a dead loop if needn't to execute old IDT handler further
   113           //
   114           if (ReservedVectors[ExceptionType].Attribute != EFI_VECTOR_HANDOFF_HOOK_BEFORE) {
-> 115            CpuDeadLoop ();
   116           }
   117         }
   118       }
   119
(lldb) p ExceptionType
(EFI_EXCEPTION_TYPE) $0 = 0
(lldb) p SystemContext.SystemContextX64->Rip
(UINT64) $1 = 0x0000000007a9cc38
(lldb) p SystemContext.SystemContextX64->Rbp
(UINT64) $2 = 0x0000000007e8fc20
(lldb) efi_backtrace --pc 0x0000000007a9cc38 --frame 0x0000000007e8fc20 --symbols
  frame  0: 0x07a9cc38 BdsDxe:PlatformBootManagerWaitCallback + 35 at /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646:37
  frame  1: 0x07a9e744 BdsDxe:BdsWait + 269 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344:3
  frame  2: 0x07a9dff9 BdsDxe:BdsEntry + 2620 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:1002:5
  frame  3: 0x07eb367d DxeCore:DxeMain + 2680 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c:544:3
  frame  4: 0x07e943cb DxeCore:_ModuleEntryPoint + 20 at /Volumes/Case/UDK2018/MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.c:48:3
  frame  5: 0x07edc947 DxeIpl.dll`AsmEnableCache
  frame  6: 0x07ee1e4e DxeIpl:HandOffToDxeCore + 509 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c:113:3
  frame  7: 0x07ee0604 DxeIpl:DxeLoadCore + 1354 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:449:3
  frame  8: 0x07eeff2f PeiCore.dll`PeiCore.cold.3 + 847
  frame  9: 0x07ee9d04 PeiCore:PeiCore + 163 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c:502:3
  frame 10: 0x0082c387 0x0082c387
  frame 11: 0x00825dd7 0x00825dd7
  frame 12: 0x0082ad27 0x0082ad27
  frame 13: 0x0082b3a8 0x0082b3a8
  frame 14: 0x0082bf23 0x0082bf23
  frame 15: 0x00825e24 0x00825e24
  frame 16: 0x00823af2 0x00823af2
  frame 17: 0xfffd1db8 SecMain:SecStartupPhase2 + 67 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:858:3
  frame 18: 0xfffd1d67 SecMain:SecCoreStartupWithStack + 420 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:821:3
  frame 19: 0xfffd1e14 SecMain:ProcessLibraryConstructorList + 0 at /Volumes/Case/UDK2018/Build/OvmfX64/DEBUG_XCODE5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:201

(lldb) l /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646
   1646         (Timeout - TimeoutRemain) * 100 / Timeout,
   1647         0
   1648         );
   1649     }
   1650
   1651     /**
   1652       The function is called when no boot option could be launched,
   1653       including platform recovery options and options pointing to applications
   1654       built into firmware volumes.
   1655
(lldb) l /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344
   344         PlatformBootManagerWaitCallback (0);
   345         DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
   346       }
   347
   348       /**
   349         Attempt to boot each boot option in the BootOptions array.
   350
   351         @param BootOptions       Input boot option array.
   352         @param BootOptionCount   Input boot option count.
   353         @param BootManagerMenu   Input boot manager menu.
   354


Thanks,

Andrew Fish



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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11  1:19 ` [edk2-devel] " Liming Gao
@ 2019-10-11  4:59   ` Andrew Fish
  2019-10-11 11:53     ` Laszlo Ersek
  2019-10-11 11:50   ` Laszlo Ersek
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Fish @ 2019-10-11  4:59 UTC (permalink / raw)
  To: Gao, Liming; +Cc: devel@edk2.groups.io, Pete Batard

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

Liming,

Thanks for looking into this!

Can someone also answer my question about the expected behavior of taking an exception in OVMF?  Is the CpuDeadloop() expected? 

Thanks,

Andrew Fish

> On Oct 10, 2019, at 6:19 PM, Gao, Liming <liming.gao@intel.com> 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?
>
> Thanks
> Liming
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.Io
> Sent: Friday, October 11, 2019 5:12 AM
> To: devel@edk2.groups.io; Pete Batard <pete@akeo.ie>
> Subject: [edk2-devel] OVMF is crashing for me in master
>
> This is my flavor of OVMF:  build -p OvmfPkg/OvmfPkgX64.dsc -a X64 -t XCODE5
>
> It looks like I took an exception? Is it expected that an unhandled exception just hang in a dead loop? I would have expected some serial  output about the failure? 
>
> Looks like a divide by zero exception. The exception context has PC and FP so I can manually walk the stack. Yikes I see PlatformBootManagerWaitCallback() will fault if PcdPlatformBootTimeOut is zero? 
> /Volumes/Case/UDK2018(master)>git grep PcdPlatformBootTimeOut -- *.dsc
> ArmVirtPkg/ArmVirtQemu.dsc:194:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
> ArmVirtPkg/ArmVirtQemuKernel.dsc:191:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
> ArmVirtPkg/ArmVirtXen.dsc:122:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
> EmulatorPkg/EmulatorPkg.dsc:236:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
> OvmfPkg/OvmfPkgIa32.dsc:541:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
> OvmfPkg/OvmfPkgIa32X64.dsc:553:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
> OvmfPkg/OvmfPkgX64.dsc:552:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
> OvmfPkg/OvmfXen.dsc:470:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
> UefiPayloadPkg/UefiPayloadPkgIa32.dsc:344:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc:345:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
> 
> 
> OK PcdPlatformBootTimeOut is zero on Ovmf, so how did this ever work?
> 
> 
> Ahhh gotom.... 
> /Volumes/Case/UDK2018(master)>git blame -L344,344  /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
> 2de1f611be0 (Pete Batard 2019-09-25 23:50:05 +0800 344)   PlatformBootManagerWaitCallback (0);
> 
> 
> This call causes a divide by zero if PcdPlatformBootTimeOut == 0. 
>
> This fixes my crash:
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 70df6b841a..d6ae43e900 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -1634,6 +1634,11 @@ PlatformBootManagerWaitCallback (
>    UINT16                              Timeout;
>
>    Timeout = PcdGet16 (PcdPlatformBootTimeOut);
> +  if (Timeout ==  0) {
> +    Timeout = 100;
> +  } else {
> +    Timeout = (Timeout - TimeoutRemain) * 100 / Timeout;
> +  }
>
>    Black.Raw = 0x00000000;
>    White.Raw = 0x00FFFFFF;
> @@ -1643,7 +1648,7 @@ PlatformBootManagerWaitCallback (
>      Black.Pixel,
>      L"Start boot option",
>      White.Pixel,
> -    (Timeout - TimeoutRemain) * 100 / Timeout,
> +    Timeout,
>      0
>      );
>  }
> 
> 
>
> lldb debugger output:
>
> (lldb) bt
> * thread #1, stop reason = signal SIGTRAP
>   * frame #0: 0x0000000007b58a70 CpuDxe.dll:CpuDeadLoop() + 13 at /Volumes/Case/UDK2018/MdePkg/Library/BaseLib/CpuDeadLoop.c:31
>     frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
>     frame #2: 0x0000000007b61624 CpuDxe.dll:CommonExceptionHandler() + 36 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c:40
>     frame #3: 0x0000000007b5ff26 CpuDxe.dll:HasErrorCode() + 230
> (lldb) fr sel 1
> frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
>    91             (ExternalInterruptHandler[ExceptionType]) (ExceptionType, SystemContext);
>    92           } else if (ExceptionType < CPU_EXCEPTION_NUM) {
>    93             //
>    94             // Get Spinlock to display CPU information
>    95             //
>    96             while (!AcquireSpinLockOrFail (&ExceptionHandlerData->DisplayMessageSpinLock)) {
>    97               CpuPause ();
>    98             }
>    99             //
>    100           // Initialize the serial port before dumping.
>    101           //
>    102           SerialPortInitialize ();
>    103           //
>    104           // Display ExceptionType, CPU information and Image information
>    105           //
>    106           DumpImageAndCpuContent (ExceptionType, SystemContext);
>    107           //
>    108           // Release Spinlock of output message
>    109           //
>    110           ReleaseSpinLock (&ExceptionHandlerData->DisplayMessageSpinLock);
>    111           //
>    112           // Enter a dead loop if needn't to execute old IDT handler further
>    113           //
>    114           if (ReservedVectors[ExceptionType].Attribute != EFI_VECTOR_HANDOFF_HOOK_BEFORE) {
> -> 115            CpuDeadLoop ();
>    116           }
>    117         }
>    118       }
>    119
> (lldb) p ExceptionType
> (EFI_EXCEPTION_TYPE) $0 = 0
> (lldb) p SystemContext.SystemContextX64->Rip
> (UINT64) $1 = 0x0000000007a9cc38
> (lldb) p SystemContext.SystemContextX64->Rbp
> (UINT64) $2 = 0x0000000007e8fc20
> (lldb) efi_backtrace --pc 0x0000000007a9cc38 --frame 0x0000000007e8fc20 --symbols
>   frame  0: 0x07a9cc38 BdsDxe:PlatformBootManagerWaitCallback + 35 at /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646:37
>   frame  1: 0x07a9e744 BdsDxe:BdsWait + 269 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344:3
>   frame  2: 0x07a9dff9 BdsDxe:BdsEntry + 2620 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:1002:5
>   frame  3: 0x07eb367d DxeCore:DxeMain + 2680 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c:544:3
>   frame  4: 0x07e943cb DxeCore:_ModuleEntryPoint + 20 at /Volumes/Case/UDK2018/MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.c:48:3
>   frame  5: 0x07edc947 DxeIpl.dll`AsmEnableCache
>   frame  6: 0x07ee1e4e DxeIpl:HandOffToDxeCore + 509 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c:113:3
>   frame  7: 0x07ee0604 DxeIpl:DxeLoadCore + 1354 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:449:3
>   frame  8: 0x07eeff2f PeiCore.dll`PeiCore.cold.3 + 847
>   frame  9: 0x07ee9d04 PeiCore:PeiCore + 163 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c:502:3
>   frame 10: 0x0082c387 0x0082c387
>   frame 11: 0x00825dd7 0x00825dd7
>   frame 12: 0x0082ad27 0x0082ad27
>   frame 13: 0x0082b3a8 0x0082b3a8
>   frame 14: 0x0082bf23 0x0082bf23
>   frame 15: 0x00825e24 0x00825e24
>   frame 16: 0x00823af2 0x00823af2
>   frame 17: 0xfffd1db8 SecMain:SecStartupPhase2 + 67 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:858:3
>   frame 18: 0xfffd1d67 SecMain:SecCoreStartupWithStack + 420 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:821:3
>   frame 19: 0xfffd1e14 SecMain:ProcessLibraryConstructorList + 0 at /Volumes/Case/UDK2018/Build/OvmfX64/DEBUG_XCODE5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:201
>
> (lldb) l /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646
>    1646         (Timeout - TimeoutRemain) * 100 / Timeout,
>    1647         0
>    1648         );
>    1649     }
>    1650
>    1651     /**
>    1652       The function is called when no boot option could be launched,
>    1653       including platform recovery options and options pointing to applications
>    1654       built into firmware volumes.
>    1655
> (lldb) l /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344
>    344         PlatformBootManagerWaitCallback (0);
>    345         DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>    346       }
>    347
>    348       /**
>    349         Attempt to boot each boot option in the BootOptions array.
>    350
>    351         @param BootOptions       Input boot option array.
>    352         @param BootOptionCount   Input boot option count.
>    353         @param BootManagerMenu   Input boot manager menu.
>    354
>
>
> Thanks,
>
> Andrew Fish
>
> 


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11  4:59   ` Andrew Fish
@ 2019-10-11 11:53     ` Laszlo Ersek
  2019-10-11 12:49       ` Andrew Fish
  0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2019-10-11 11:53 UTC (permalink / raw)
  To: devel, afish, Gao, Liming; +Cc: Pete Batard

On 10/11/19 06:59, Andrew Fish via Groups.Io wrote:
> Liming,
> 
> Thanks for looking into this!
> 
> Can someone also answer my question about the expected behavior of taking an exception in OVMF?  Is the CpuDeadloop() expected? 

Yes, it is.

The exception handler dumps the register file and the stack to the
emulated serial port, not to the QEMU debug port.

When looking for OVMF debug messages, people usually (and rightly) check
wherever the QEMU debug port has been redirected. (Unless they built
OVMF with -D DEBUG_ON_SERIAL_PORT.) However, the exception handler
always dumps the state to the serial port, regardless of
DEBUG_ON_SERIAL_PORT. This can be confusing, as the normally consulted
debug log has no indication of the issue.

Thanks
Laszlo

> 
> Thanks,
> 
> Andrew Fish
> 
>> On Oct 10, 2019, at 6:19 PM, Gao, Liming <liming.gao@intel.com> 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?
>>
>> Thanks
>> Liming
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.Io
>> Sent: Friday, October 11, 2019 5:12 AM
>> To: devel@edk2.groups.io; Pete Batard <pete@akeo.ie>
>> Subject: [edk2-devel] OVMF is crashing for me in master
>>
>> This is my flavor of OVMF:  build -p OvmfPkg/OvmfPkgX64.dsc -a X64 -t XCODE5
>>
>> It looks like I took an exception? Is it expected that an unhandled exception just hang in a dead loop? I would have expected some serial  output about the failure? 
>>
>> Looks like a divide by zero exception. The exception context has PC and FP so I can manually walk the stack. Yikes I see PlatformBootManagerWaitCallback() will fault if PcdPlatformBootTimeOut is zero? 
>> /Volumes/Case/UDK2018(master)>git grep PcdPlatformBootTimeOut -- *.dsc
>> ArmVirtPkg/ArmVirtQemu.dsc:194:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>> ArmVirtPkg/ArmVirtQemuKernel.dsc:191:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>> ArmVirtPkg/ArmVirtXen.dsc:122:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>> EmulatorPkg/EmulatorPkg.dsc:236:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
>> OvmfPkg/OvmfPkgIa32.dsc:541:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>> OvmfPkg/OvmfPkgIa32X64.dsc:553:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>> OvmfPkg/OvmfPkgX64.dsc:552:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>> OvmfPkg/OvmfXen.dsc:470:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>> UefiPayloadPkg/UefiPayloadPkgIa32.dsc:344:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc:345:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>>
>>
>> OK PcdPlatformBootTimeOut is zero on Ovmf, so how did this ever work?
>>
>>
>> Ahhh gotom.... 
>> /Volumes/Case/UDK2018(master)>git blame -L344,344  /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
>> 2de1f611be0 (Pete Batard 2019-09-25 23:50:05 +0800 344)   PlatformBootManagerWaitCallback (0);
>>
>>
>> This call causes a divide by zero if PcdPlatformBootTimeOut == 0. 
>>
>> This fixes my crash:
>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>> index 70df6b841a..d6ae43e900 100644
>> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>> @@ -1634,6 +1634,11 @@ PlatformBootManagerWaitCallback (
>>    UINT16                              Timeout;
>>
>>    Timeout = PcdGet16 (PcdPlatformBootTimeOut);
>> +  if (Timeout ==  0) {
>> +    Timeout = 100;
>> +  } else {
>> +    Timeout = (Timeout - TimeoutRemain) * 100 / Timeout;
>> +  }
>>
>>    Black.Raw = 0x00000000;
>>    White.Raw = 0x00FFFFFF;
>> @@ -1643,7 +1648,7 @@ PlatformBootManagerWaitCallback (
>>      Black.Pixel,
>>      L"Start boot option",
>>      White.Pixel,
>> -    (Timeout - TimeoutRemain) * 100 / Timeout,
>> +    Timeout,
>>      0
>>      );
>>  }
>>
>>
>>
>> lldb debugger output:
>>
>> (lldb) bt
>> * thread #1, stop reason = signal SIGTRAP
>>   * frame #0: 0x0000000007b58a70 CpuDxe.dll:CpuDeadLoop() + 13 at /Volumes/Case/UDK2018/MdePkg/Library/BaseLib/CpuDeadLoop.c:31
>>     frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
>>     frame #2: 0x0000000007b61624 CpuDxe.dll:CommonExceptionHandler() + 36 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c:40
>>     frame #3: 0x0000000007b5ff26 CpuDxe.dll:HasErrorCode() + 230
>> (lldb) fr sel 1
>> frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
>>    91             (ExternalInterruptHandler[ExceptionType]) (ExceptionType, SystemContext);
>>    92           } else if (ExceptionType < CPU_EXCEPTION_NUM) {
>>    93             //
>>    94             // Get Spinlock to display CPU information
>>    95             //
>>    96             while (!AcquireSpinLockOrFail (&ExceptionHandlerData->DisplayMessageSpinLock)) {
>>    97               CpuPause ();
>>    98             }
>>    99             //
>>    100           // Initialize the serial port before dumping.
>>    101           //
>>    102           SerialPortInitialize ();
>>    103           //
>>    104           // Display ExceptionType, CPU information and Image information
>>    105           //
>>    106           DumpImageAndCpuContent (ExceptionType, SystemContext);
>>    107           //
>>    108           // Release Spinlock of output message
>>    109           //
>>    110           ReleaseSpinLock (&ExceptionHandlerData->DisplayMessageSpinLock);
>>    111           //
>>    112           // Enter a dead loop if needn't to execute old IDT handler further
>>    113           //
>>    114           if (ReservedVectors[ExceptionType].Attribute != EFI_VECTOR_HANDOFF_HOOK_BEFORE) {
>> -> 115            CpuDeadLoop ();
>>    116           }
>>    117         }
>>    118       }
>>    119
>> (lldb) p ExceptionType
>> (EFI_EXCEPTION_TYPE) $0 = 0
>> (lldb) p SystemContext.SystemContextX64->Rip
>> (UINT64) $1 = 0x0000000007a9cc38
>> (lldb) p SystemContext.SystemContextX64->Rbp
>> (UINT64) $2 = 0x0000000007e8fc20
>> (lldb) efi_backtrace --pc 0x0000000007a9cc38 --frame 0x0000000007e8fc20 --symbols
>>   frame  0: 0x07a9cc38 BdsDxe:PlatformBootManagerWaitCallback + 35 at /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646:37
>>   frame  1: 0x07a9e744 BdsDxe:BdsWait + 269 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344:3
>>   frame  2: 0x07a9dff9 BdsDxe:BdsEntry + 2620 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:1002:5
>>   frame  3: 0x07eb367d DxeCore:DxeMain + 2680 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c:544:3
>>   frame  4: 0x07e943cb DxeCore:_ModuleEntryPoint + 20 at /Volumes/Case/UDK2018/MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.c:48:3
>>   frame  5: 0x07edc947 DxeIpl.dll`AsmEnableCache
>>   frame  6: 0x07ee1e4e DxeIpl:HandOffToDxeCore + 509 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c:113:3
>>   frame  7: 0x07ee0604 DxeIpl:DxeLoadCore + 1354 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:449:3
>>   frame  8: 0x07eeff2f PeiCore.dll`PeiCore.cold.3 + 847
>>   frame  9: 0x07ee9d04 PeiCore:PeiCore + 163 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c:502:3
>>   frame 10: 0x0082c387 0x0082c387
>>   frame 11: 0x00825dd7 0x00825dd7
>>   frame 12: 0x0082ad27 0x0082ad27
>>   frame 13: 0x0082b3a8 0x0082b3a8
>>   frame 14: 0x0082bf23 0x0082bf23
>>   frame 15: 0x00825e24 0x00825e24
>>   frame 16: 0x00823af2 0x00823af2
>>   frame 17: 0xfffd1db8 SecMain:SecStartupPhase2 + 67 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:858:3
>>   frame 18: 0xfffd1d67 SecMain:SecCoreStartupWithStack + 420 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:821:3
>>   frame 19: 0xfffd1e14 SecMain:ProcessLibraryConstructorList + 0 at /Volumes/Case/UDK2018/Build/OvmfX64/DEBUG_XCODE5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:201
>>
>> (lldb) l /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646
>>    1646         (Timeout - TimeoutRemain) * 100 / Timeout,
>>    1647         0
>>    1648         );
>>    1649     }
>>    1650
>>    1651     /**
>>    1652       The function is called when no boot option could be launched,
>>    1653       including platform recovery options and options pointing to applications
>>    1654       built into firmware volumes.
>>    1655
>> (lldb) l /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344
>>    344         PlatformBootManagerWaitCallback (0);
>>    345         DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>>    346       }
>>    347
>>    348       /**
>>    349         Attempt to boot each boot option in the BootOptions array.
>>    350
>>    351         @param BootOptions       Input boot option array.
>>    352         @param BootOptionCount   Input boot option count.
>>    353         @param BootManagerMenu   Input boot manager menu.
>>    354
>>
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11 11:53     ` Laszlo Ersek
@ 2019-10-11 12:49       ` Andrew Fish
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Fish @ 2019-10-11 12:49 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: devel, Gao, Liming, Pete Batard

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



> On Oct 11, 2019, at 4:53 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 10/11/19 06:59, Andrew Fish via Groups.Io wrote:
>> Liming,
>> 
>> Thanks for looking into this!
>> 
>> Can someone also answer my question about the expected behavior of taking an exception in OVMF?  Is the CpuDeadloop() expected? 
> 
> Yes, it is.
> 
> The exception handler dumps the register file and the stack to the
> emulated serial port, not to the QEMU debug port.
> 
> When looking for OVMF debug messages, people usually (and rightly) check
> wherever the QEMU debug port has been redirected. (Unless they built
> OVMF with -D DEBUG_ON_SERIAL_PORT.) However, the exception handler
> always dumps the state to the serial port, regardless of
> DEBUG_ON_SERIAL_PORT. This can be confusing, as the normally consulted
> debug log has no indication of the issue.
> 

Laszlo,

Thanks if I add -serial file:serial.log to the QEMU command line I see the exception in serial.log. 

Thanks,

Andrew Fish

> Thanks
> Laszlo
> 
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>>> On Oct 10, 2019, at 6:19 PM, Gao, Liming <liming.gao@intel.com> 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?
>>> 
>>> Thanks
>>> Liming
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.Io
>>> Sent: Friday, October 11, 2019 5:12 AM
>>> To: devel@edk2.groups.io; Pete Batard <pete@akeo.ie>
>>> Subject: [edk2-devel] OVMF is crashing for me in master
>>> 
>>> This is my flavor of OVMF:  build -p OvmfPkg/OvmfPkgX64.dsc -a X64 -t XCODE5
>>> 
>>> It looks like I took an exception? Is it expected that an unhandled exception just hang in a dead loop? I would have expected some serial  output about the failure? 
>>> 
>>> Looks like a divide by zero exception. The exception context has PC and FP so I can manually walk the stack. Yikes I see PlatformBootManagerWaitCallback() will fault if PcdPlatformBootTimeOut is zero? 
>>> /Volumes/Case/UDK2018(master)>git grep PcdPlatformBootTimeOut -- *.dsc
>>> ArmVirtPkg/ArmVirtQemu.dsc:194:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>>> ArmVirtPkg/ArmVirtQemuKernel.dsc:191:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>>> ArmVirtPkg/ArmVirtXen.dsc:122:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>>> EmulatorPkg/EmulatorPkg.dsc:236:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|10
>>> OvmfPkg/OvmfPkgIa32.dsc:541:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>>> OvmfPkg/OvmfPkgIa32X64.dsc:553:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>>> OvmfPkg/OvmfPkgX64.dsc:552:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>>> OvmfPkg/OvmfXen.dsc:470:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>>> UefiPayloadPkg/UefiPayloadPkgIa32.dsc:344:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>>> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc:345:  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>>> 
>>> 
>>> OK PcdPlatformBootTimeOut is zero on Ovmf, so how did this ever work?
>>> 
>>> 
>>> Ahhh gotom.... 
>>> /Volumes/Case/UDK2018(master)>git blame -L344,344  /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
>>> 2de1f611be0 (Pete Batard 2019-09-25 23:50:05 +0800 344)   PlatformBootManagerWaitCallback (0);
>>> 
>>> 
>>> This call causes a divide by zero if PcdPlatformBootTimeOut == 0. 
>>> 
>>> This fixes my crash:
>>> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>>> index 70df6b841a..d6ae43e900 100644
>>> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>>> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>>> @@ -1634,6 +1634,11 @@ PlatformBootManagerWaitCallback (
>>>   UINT16                              Timeout;
>>> 
>>>   Timeout = PcdGet16 (PcdPlatformBootTimeOut);
>>> +  if (Timeout ==  0) {
>>> +    Timeout = 100;
>>> +  } else {
>>> +    Timeout = (Timeout - TimeoutRemain) * 100 / Timeout;
>>> +  }
>>> 
>>>   Black.Raw = 0x00000000;
>>>   White.Raw = 0x00FFFFFF;
>>> @@ -1643,7 +1648,7 @@ PlatformBootManagerWaitCallback (
>>>     Black.Pixel,
>>>     L"Start boot option",
>>>     White.Pixel,
>>> -    (Timeout - TimeoutRemain) * 100 / Timeout,
>>> +    Timeout,
>>>     0
>>>     );
>>> }
>>> 
>>> 
>>> 
>>> lldb debugger output:
>>> 
>>> (lldb) bt
>>> * thread #1, stop reason = signal SIGTRAP
>>>  * frame #0: 0x0000000007b58a70 CpuDxe.dll:CpuDeadLoop() + 13 at /Volumes/Case/UDK2018/MdePkg/Library/BaseLib/CpuDeadLoop.c:31
>>>    frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
>>>    frame #2: 0x0000000007b61624 CpuDxe.dll:CommonExceptionHandler() + 36 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c:40
>>>    frame #3: 0x0000000007b5ff26 CpuDxe.dll:HasErrorCode() + 230
>>> (lldb) fr sel 1
>>> frame #1: 0x0000000007b61222 CpuDxe.dll:CommonExceptionHandlerWorker() + 674 at /Volumes/Case/UDK2018/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c:115
>>>   91             (ExternalInterruptHandler[ExceptionType]) (ExceptionType, SystemContext);
>>>   92           } else if (ExceptionType < CPU_EXCEPTION_NUM) {
>>>   93             //
>>>   94             // Get Spinlock to display CPU information
>>>   95             //
>>>   96             while (!AcquireSpinLockOrFail (&ExceptionHandlerData->DisplayMessageSpinLock)) {
>>>   97               CpuPause ();
>>>   98             }
>>>   99             //
>>>   100           // Initialize the serial port before dumping.
>>>   101           //
>>>   102           SerialPortInitialize ();
>>>   103           //
>>>   104           // Display ExceptionType, CPU information and Image information
>>>   105           //
>>>   106           DumpImageAndCpuContent (ExceptionType, SystemContext);
>>>   107           //
>>>   108           // Release Spinlock of output message
>>>   109           //
>>>   110           ReleaseSpinLock (&ExceptionHandlerData->DisplayMessageSpinLock);
>>>   111           //
>>>   112           // Enter a dead loop if needn't to execute old IDT handler further
>>>   113           //
>>>   114           if (ReservedVectors[ExceptionType].Attribute != EFI_VECTOR_HANDOFF_HOOK_BEFORE) {
>>> -> 115            CpuDeadLoop ();
>>>   116           }
>>>   117         }
>>>   118       }
>>>   119
>>> (lldb) p ExceptionType
>>> (EFI_EXCEPTION_TYPE) $0 = 0
>>> (lldb) p SystemContext.SystemContextX64->Rip
>>> (UINT64) $1 = 0x0000000007a9cc38
>>> (lldb) p SystemContext.SystemContextX64->Rbp
>>> (UINT64) $2 = 0x0000000007e8fc20
>>> (lldb) efi_backtrace --pc 0x0000000007a9cc38 --frame 0x0000000007e8fc20 --symbols
>>>  frame  0: 0x07a9cc38 BdsDxe:PlatformBootManagerWaitCallback + 35 at /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646:37
>>>  frame  1: 0x07a9e744 BdsDxe:BdsWait + 269 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344:3
>>>  frame  2: 0x07a9dff9 BdsDxe:BdsEntry + 2620 at /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:1002:5
>>>  frame  3: 0x07eb367d DxeCore:DxeMain + 2680 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c:544:3
>>>  frame  4: 0x07e943cb DxeCore:_ModuleEntryPoint + 20 at /Volumes/Case/UDK2018/MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.c:48:3
>>>  frame  5: 0x07edc947 DxeIpl.dll`AsmEnableCache
>>>  frame  6: 0x07ee1e4e DxeIpl:HandOffToDxeCore + 509 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c:113:3
>>>  frame  7: 0x07ee0604 DxeIpl:DxeLoadCore + 1354 at /Volumes/Case/UDK2018/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:449:3
>>>  frame  8: 0x07eeff2f PeiCore.dll`PeiCore.cold.3 + 847
>>>  frame  9: 0x07ee9d04 PeiCore:PeiCore + 163 at /Volumes/Case/UDK2018/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c:502:3
>>>  frame 10: 0x0082c387 0x0082c387
>>>  frame 11: 0x00825dd7 0x00825dd7
>>>  frame 12: 0x0082ad27 0x0082ad27
>>>  frame 13: 0x0082b3a8 0x0082b3a8
>>>  frame 14: 0x0082bf23 0x0082bf23
>>>  frame 15: 0x00825e24 0x00825e24
>>>  frame 16: 0x00823af2 0x00823af2
>>>  frame 17: 0xfffd1db8 SecMain:SecStartupPhase2 + 67 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:858:3
>>>  frame 18: 0xfffd1d67 SecMain:SecCoreStartupWithStack + 420 at /Volumes/Case/UDK2018/OvmfPkg/Sec/SecMain.c:821:3
>>>  frame 19: 0xfffd1e14 SecMain:ProcessLibraryConstructorList + 0 at /Volumes/Case/UDK2018/Build/OvmfX64/DEBUG_XCODE5/X64/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c:201
>>> 
>>> (lldb) l /Volumes/Case/UDK2018/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c:1646
>>>   1646         (Timeout - TimeoutRemain) * 100 / Timeout,
>>>   1647         0
>>>   1648         );
>>>   1649     }
>>>   1650
>>>   1651     /**
>>>   1652       The function is called when no boot option could be launched,
>>>   1653       including platform recovery options and options pointing to applications
>>>   1654       built into firmware volumes.
>>>   1655
>>> (lldb) l /Volumes/Case/UDK2018/MdeModulePkg/Universal/BdsDxe/BdsEntry.c:344
>>>   344         PlatformBootManagerWaitCallback (0);
>>>   345         DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
>>>   346       }
>>>   347
>>>   348       /**
>>>   349         Attempt to boot each boot option in the BootOptions array.
>>>   350
>>>   351         @param BootOptions       Input boot option array.
>>>   352         @param BootOptionCount   Input boot option count.
>>>   353         @param BootManagerMenu   Input boot manager menu.
>>>   354
>>> 
>>> 
>>> Thanks,
>>> 
>>> Andrew Fish
>>> 
>>> 
>> 
>> 
>> 


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11  1:19 ` [edk2-devel] " Liming Gao
  2019-10-11  4:59   ` Andrew Fish
@ 2019-10-11 11:50   ` Laszlo Ersek
  2019-10-11 12:16     ` Pete Batard
  2019-10-11 13:13     ` Andrew Fish
  1 sibling, 2 replies; 10+ messages in thread
From: Laszlo Ersek @ 2019-10-11 11:50 UTC (permalink / raw)
  To: devel, liming.gao, afish@apple.com, Pete Batard

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

Thanks!
Laszlo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11 11:50   ` Laszlo Ersek
@ 2019-10-11 12:16     ` Pete Batard
  2019-10-11 13:13     ` Andrew Fish
  1 sibling, 0 replies; 10+ messages in thread
From: Pete Batard @ 2019-10-11 12:16 UTC (permalink / raw)
  To: Laszlo Ersek, devel, liming.gao, afish@apple.com

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
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11 11:50   ` Laszlo Ersek
  2019-10-11 12:16     ` Pete Batard
@ 2019-10-11 13:13     ` Andrew Fish
  2019-10-11 16:33       ` Laszlo Ersek
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Fish @ 2019-10-11 13:13 UTC (permalink / raw)
  To: devel, lersek; +Cc: liming.gao, Pete Batard

[-- 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 --]

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11 13:13     ` Andrew Fish
@ 2019-10-11 16:33       ` Laszlo Ersek
  2019-10-11 19:07         ` Andrew Fish
  0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2019-10-11 16:33 UTC (permalink / raw)
  To: Andrew Fish, devel; +Cc: liming.gao, Pete Batard

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [edk2-devel] OVMF is crashing for me in master
  2019-10-11 16:33       ` Laszlo Ersek
@ 2019-10-11 19:07         ` Andrew Fish
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Fish @ 2019-10-11 19:07 UTC (permalink / raw)
  To: devel, lersek; +Cc: liming.gao, Pete Batard



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-10-11 19:07 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-10 21:12 OVMF is crashing for me in master Andrew Fish
2019-10-11  1:19 ` [edk2-devel] " Liming Gao
2019-10-11  4:59   ` Andrew Fish
2019-10-11 11:53     ` Laszlo Ersek
2019-10-11 12:49       ` Andrew Fish
2019-10-11 11:50   ` Laszlo Ersek
2019-10-11 12:16     ` Pete Batard
2019-10-11 13:13     ` Andrew Fish
2019-10-11 16:33       ` Laszlo Ersek
2019-10-11 19:07         ` Andrew Fish

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox