From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nwk-aaemail-lapp01.apple.com (nwk-aaemail-lapp01.apple.com [17.151.62.66]) by mx.groups.io with SMTP id smtpd.web12.299.1570799629399147179 for ; Fri, 11 Oct 2019 06:13:50 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=KTqTSwxA; spf=pass (domain: apple.com, ip: 17.151.62.66, mailfrom: afish@apple.com) Received: from pps.filterd (nwk-aaemail-lapp01.apple.com [127.0.0.1]) by nwk-aaemail-lapp01.apple.com (8.16.0.27/8.16.0.27) with SMTP id x9BD1wKe062706; Fri, 11 Oct 2019 06:13:46 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=g+AyfIZBPWAo2moG5bbwBqQCSES01zalHMp93w2pSQY=; b=KTqTSwxAn3RLSE/97YTSjZeGvUPVS5zoKU9jbJYIW8Mj4kcUoVQVtw+94eaxZXSXolo9 XtzTKJi76vDu70WJ0gsAsJHDMWCqLJxR8JY5djtgym5/7WOpctTVz0nX01OtC0dAL1+H M6OPM3U6D3lQasIhqbl/7dgDENyOgWvfTNP2aMYqQkEmyri+LDxUDTZ85SRyAdC9DiYF KPImqHjM4Mmpr8zfOPJmQcEAmYhHq4c/o6BOObsG/MQeTX3C/Ti1b7Jj6Dk99tf3X6kx U6fLXdQFIl/6TDh4i90t+z3FPva8L1444So6a0Xd8aEwhNxmAamEcyorGBfFhnm3Kl5M 8A== Received: from ma1-mtap-s02.corp.apple.com (ma1-mtap-s02.corp.apple.com [17.40.76.6]) by nwk-aaemail-lapp01.apple.com with ESMTP id 2vet4d49xh-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Fri, 11 Oct 2019 06:13:46 -0700 Received: from nwk-mmpp-sz10.apple.com (nwk-mmpp-sz10.apple.com [17.128.115.122]) by ma1-mtap-s02.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PZ700M2FOQX4J70@ma1-mtap-s02.corp.apple.com>; Fri, 11 Oct 2019 06:13:46 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz10.apple.com by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PZ700J00OI3GU00@nwk-mmpp-sz10.apple.com>; Fri, 11 Oct 2019 06:13:45 -0700 (PDT) X-Va-A: X-Va-T-CD: 08777febe38bb384cc57fda39d0586b7 X-Va-E-CD: c4384febc0b38a4c3ffc24bb55458dfb X-Va-R-CD: 39d1446bb985fb6c3d28e9c8d969a470 X-Va-CD: 0 X-Va-ID: 23438b2b-4e58-40d5-8d0a-5f6ef3067d18 X-V-A: X-V-T-CD: 08777febe38bb384cc57fda39d0586b7 X-V-E-CD: c4384febc0b38a4c3ffc24bb55458dfb X-V-R-CD: 39d1446bb985fb6c3d28e9c8d969a470 X-V-CD: 0 X-V-ID: a5c31d28-d8a0-42ed-818e-1c2793ae2ec0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-11_08:,, signatures=0 Received: from [17.235.46.243] (unknown [17.235.46.243]) by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PZ7001PMOQVJ380@nwk-mmpp-sz10.apple.com>; Fri, 11 Oct 2019 06:13:45 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: <65D5B022-0049-4BFA-AB15-B12B4D8F1E13@apple.com> MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] OVMF is crashing for me in master Date: Fri, 11 Oct 2019 06:13:41 -0700 In-reply-to: <0411bb4e-7d90-df7e-8fa0-5072275f9c62@redhat.com> Cc: liming.gao@intel.com, Pete Batard To: devel@edk2.groups.io, lersek@redhat.com References: <4A89E2EF3DFEDB4C8BFDE51014F606A14E515A05@SHSMSX104.ccr.corp.intel.com> <0411bb4e-7d90-df7e-8fa0-5072275f9c62@redhat.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-11_08:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_FA1659F4-CA68-4EA3-8DAE-1EEC4405F028" --Apple-Mail=_FA1659F4-CA68-4EA3-8DAE-1EEC4405F028 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii > On Oct 11, 2019, at 4:50 AM, Laszlo Ersek wrote: >=20 > Hi, >=20 > 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. >>=20 >> 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. >>=20 >> Pete: >> Will you contribute the patch to fix this hang issue in OVMF? >=20 > So, when Pete submitted the BDS patch, I didn't test it, I only looked > at the patch email. See my feedback here: >=20 > http://mid.mail-archive.com/8bacbb53-98df-724e-b704-b5059d11e378@redhat= .com > https://edk2.groups.io/g/devel/message/48133 >=20 > It seemed OK to me. In particular, I reasoned about (TimeoutRemain=3D=3D= 0), > and the patch seemed to do the right thing for that. >=20 > Unfortunately, what I missed was the following case: >=20 > - 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(). >=20 > 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. >=20 > Here is the commit, with full function context: >=20 >> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Un= iversal/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; >>=20 >> DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Zzzzzzzzzzzz...\n")); >>=20 >> TimeoutRemain =3D PcdGet16 (PcdPlatformBootTimeOut); >> while (TimeoutRemain !=3D 0) { >> DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) TimeoutR= emain)); >> PlatformBootManagerWaitCallback (TimeoutRemain); >>=20 >> BdsReadKeys (); // BUGBUG: Only reading can signal HotkeyTriggered >> // Can be removed after all keyboard driver= s invoke callback in timer callback. >>=20 >> if (HotkeyTriggered !=3D NULL) { >> Status =3D BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERI= OD_SECONDS (1)); >> if (!EFI_ERROR (Status)) { >> break; >> } >> } else { >> gBS->Stall (1000000); >> } >>=20 >> // >> // 0xffff means waiting forever >> // BDS with no hotkey provided and 0xffff as timeout will "hang" in= the loop >> // >> if (TimeoutRemain !=3D 0xffff) { >> TimeoutRemain--; >> } >> } >> + PlatformBootManagerWaitCallback (0); >> DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n")); >> } >=20 > We can see that the function used to handle the > PcdPlatformBootTimeOut=3D=3D0 situation well, and make *no* call to > PlatformBootManagerWaitCallback(). The patch broke that behavior. >=20 > 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. >=20 > (To name just one other example, ArmVirtPkg too is affected.) >=20 > And for OVMF and ArmVirtPkg, it is definitely correct to *not* display > any progress bar if PcdPlatformBootTimeOut is 0. >=20 > Please consult the definition of the PCD in "MdePkg/MdePkg.dec": >=20 >> ## 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 ini= tiated immediately on boot. >> # The value of 0xFFFF then firmware will wait for user input before b= ooting. >> # @Prompt Boot Timeout (s) >> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x000000= 2c >=20 > Also see the specification of PlatformBootManagerWaitCallback(), in > "MdeModulePkg/Include/Library/PlatformBootManagerLib.h": >=20 >> /** >> This function is called each second during the boot manager waits the = timeout. >>=20 >> @param TimeoutRemain The remaining timeout. >> **/ >> VOID >> EFIAPI >> PlatformBootManagerWaitCallback ( >> UINT16 TimeoutRemain >> ); >=20 > 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. >=20 >=20 > 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 >=20 > (Timeout - TimeoutRemain) * 100 / Timeout, >=20 > it will mean >=20 > (0xFFFF - 0xFFFF) * 100 / 0xFFFF, >=20 > that is, "zero percent", in every iteration. >=20 > 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. >=20 > Jumping from zero to 100% is discontiguous and doesn't appear helpful to > me. >=20 >=20 > 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. >=20 Laszlo, I'm with Pete on this as my expectation would be the progress bar complete= s when you are done. I'd also point out that a common progress bar UI imple= mentation is to show the area that is going to be updated. I don't think ou= r UI does that, but it could get updated at some point. Thus I would posit = that if PlatformBootManagerWaitCallback() is called by passing in PcdPlatfo= rmBootTimeOut you should always call PlatformBootManagerWaitCallback() with= zero to terminate the progress bar.=20 What makes sense to me is to skip the UI if PcdPlatformBootTimeOut is zero= . The PlatformBootManagerWaitCallback (0) is only really needed if Platform= BootManagerWaitCallback (0) has not been called (TimeoutRemain !=3D 0).=20 diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Unive= rsal/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")); =20 TimeoutRemain =3D PcdGet16 (PcdPlatformBootTimeOut); - while (TimeoutRemain !=3D 0) { - DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) TimeoutRem= ain)); - PlatformBootManagerWaitCallback (TimeoutRemain); + if (TimeoutRemain !=3D 0) { + while (TimeoutRemain !=3D 0) { + DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) TimeoutR= emain)); + PlatformBootManagerWaitCallback (TimeoutRemain); =20 - 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 driver= s invoke callback in timer callback. =20 - if (HotkeyTriggered !=3D NULL) { - Status =3D BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERIOD= _SECONDS (1)); - if (!EFI_ERROR (Status)) { - break; + if (HotkeyTriggered !=3D NULL) { + Status =3D BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERI= OD_SECONDS (1)); + if (!EFI_ERROR (Status)) { + break; + } + } else { + gBS->Stall (1000000); } - } else { - gBS->Stall (1000000); - } =20 - // - // 0xffff means waiting forever - // BDS with no hotkey provided and 0xffff as timeout will "hang" in t= he loop - // - if (TimeoutRemain !=3D 0xffff) { - TimeoutRemain--; + // + // 0xffff means waiting forever + // BDS with no hotkey provided and 0xffff as timeout will "hang" in= the loop + // + if (TimeoutRemain !=3D 0xffff) { + TimeoutRemain--; + } + } + if (TimeoutRemain !=3D 0) { + PlatformBootManagerWaitCallback (0); } } - PlatformBootManagerWaitCallback (0); DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n")); } =20 Thanks, Andrew Fish >=20 > Therefore, I claim that the right fix is: >=20 >> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Un= iversal/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 =3D=3D 0) condition excludes >> + // PcdPlatformBootTimeOut=3D0xFFFF, and that's deliberate. >> + // >> + if (PcdGet16 (PcdPlatformBootTimeOut) !=3D 0 && TimeoutRemain =3D=3D= 0) { >> + PlatformBootManagerWaitCallback (0); >> + } >> DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n")); >> } >>=20 >=20 > Thanks! > Laszlo >=20 >=20 --Apple-Mail=_FA1659F4-CA68-4EA3-8DAE-1EEC4405F028 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
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 (2de1f611be06ded3a5= 9726a4052a9039be7d459b
 MdeModulePkg/BdsDxe: Also call P= latformBootManagerWaitCallback on 0)
 in Emulator.
 It works, because PCD value is set to 10 in Emulator.

 Before this change, if TimeOut PCD is zero, Bd= sEntry doesn't call
 PlatformBootManagerWaitCallback().<= br class=3D""> After this change,  if TimeOut PCD is zero, BdsEnt= ry still call
 PlatformBootManagerWaitCallback(). So, it= trigs this issue. I agree
 your fix.

Pete:
 Will you contribute the patch to fix t= his 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-archiv= e.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=3D=3D0),
and the patch seemed to do the right thing f= or that.

Unfortunately, what I missed was the following cas= e:

- if PcdPlatformBootTimeOut is zero, then the patch by P= ete does not
 simp= ly make one more call to PlatformBootManagerWaitCallback(), with
 argument zero, but it makes= the *ONLY* call to
&nb= sp;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
con= text.

Here is the commit, with full function context:

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEn= try.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index f3d5e5= ac0615..7968a58f3454 100644
--- a/MdeModulePkg/Universal/BdsD= xe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c<= br class=3D"">@@ -310,47 +310,48 @@ VOID
BdsWait (
  IN EFI_EVENT      HotkeyTrigger= ed
  )
{
  EF= I_STATUS            = Status;
  UINT16      &nbs= p;         TimeoutRemain;

  DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Z= zzzzzzzzzzz...\n"));

  TimeoutRemain= =3D PcdGet16 (PcdPlatformBootTimeOut);
  while (Ti= meoutRemain !=3D 0) {
    DEBUG ((EFI_D_I= NFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) TimeoutRemain));
=     PlatformBootManagerWaitCallback (TimeoutRemain);
    BdsReadKeys (); // BUGBU= G: Only reading can signal HotkeyTriggered
   =             &nb= sp;    //        &nb= sp;Can be removed after all keyboard drivers invoke callback in timer callb= ack.

    if (HotkeyTrigger= ed !=3D NULL) {
      Status = =3D BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERIOD_SECONDS (1));=
      if (!EFI_ERROR (Status))= {
        break;
      }
  =   } else {
      gBS-= >Stall (1000000);
    }
=
    //
   &= nbsp;// 0xffff means waiting forever
    = // BDS with no hotkey provided and 0xffff as timeout will "hang" in the loo= p
    //
   =  if (TimeoutRemain !=3D 0xffff) {
   &nbs= p;  TimeoutRemain--;
    }
  }
+  PlatformBootManagerWaitCallba= ck (0);
  DEBUG ((EFI_D_INFO, "[Bds]Exit the waitin= g!\n"));
}

We can see that the function used to handle the
PcdPlatformBootTimeOut=3D= =3D0 situation well, and make *no* call to
PlatformBootManagerWaitCallback(). The patch broke tha= t 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 PcdPl= atformBootTimeOut 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.<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">
Please consult the definition of the PCD in "MdePkg/MdePkg.dec":

 ## The number of seconds that the firmwar= e 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 va= lue of 0xFFFF then firmware will wait for user input before booting.
 # @Prompt Boot Timeout (s)
 gEfiMdePkgToke= nSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c

Also see the speci= fication of PlatformBootManagerWaitCallback(), in
"MdeModulePkg/Include/Library/PlatformBootManage= rLib.h":

/**
 This function= is called each second during the boot manager waits the timeout.

 @param TimeoutRemain  The remaining timeou= t.
**/
VOID
EFIAPI
= PlatformBootManagerWaitCallback (
 UINT16   &n= bsp;      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, t= he loop goes on until a hotkey is pressed. In every iteration
(that is, every second), PlatformBoo= tManagerWaitCallback() is called
with argument 0xFFFF, invariably. Clearly, this *cannot* cause a<= /span>
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 o= n that, it looks wrong to suddenly call
PlatformBootManagerWaitCallback() with a zero argument ("h= undred percent
completi= on"), after it's been called, let's say for a minute or more,
with a constant 0xFFFF argument ("ze= ro percent completion"), until the
user finally presses a hotkey.

Jumping from z= ero 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<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">progress bar jump to 100% at o= nce? I wouldn't think so.


<= /div>
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 t= he 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 PlatformB= ootManagerWaitCallback() is called by passing in PcdPlatformBootTimeOu= t you should always call PlatformBootManagerWaitCallback() with zero to ter= minate the progress bar. 

What mak= es sense to me is to skip the UI if PcdPlatformBootTimeOut is zero. Th= e PlatformBootManagerWaitCallback (0) is only really needed if Platfor= mBootManagerWaitCallback (0) has not been called (TimeoutRemain !=3D 0= ). 

diff --git a/MdeModulePkg/Universal= /BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 7968a58f34..fd551d17e1 100644
--- a/MdeModulePkg/U= niversal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/Bds= Dxe/BdsEntry.c
@@ -317,31 +317,35 @@ BdsWait (
= &nbs= p;  DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Zzzzzzzzzzzz...\n"));<= /div>

 

   TimeoutRem= ain =3D PcdGet16 (PcdPlatformBootTimeOut);
-  while (TimeoutRemain != = =3D 0) {
-    DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n= ", (UINTN) TimeoutRemain));
-    PlatformBootManagerWaitCallbac= k (TimeoutRemain);
+  if (TimeoutRemain !=3D 0) {
+   = while (TimeoutRemain !=3D 0) {
+      DEBUG ((EFI_D_INFO,= "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) TimeoutRemain));
+   =   PlatformBootManagerWaitCallback (TimeoutRemain);

= &nbs= p;

-    BdsReadKeys (); //= BUGBUG: Only reading can signal HotkeyTriggered
-      &n= bsp;             //       &nbs= p; Can be removed after all keyboard drivers invoke callback in timer callb= ack.
+      BdsReadKeys (); // BUGBUG: Only reading can si= gnal HotkeyTriggered
+              &n= bsp;       //         Can be removed aft= er all keyboard drivers invoke callback in timer callback.

&n= bsp;

-    if (HotkeyTrigge= red !=3D NULL) {
-      Status =3D BdsWaitForSingleEvent (= HotkeyTriggered, EFI_TIMER_PERIOD_SECONDS (1));
-      if = (!EFI_ERROR (Status)) {
-        break;
<= div style=3D"margin: 0px; font-stretch: normal; font-size: 11px; line-heigh= t: normal; font-family: Menlo; color: rgb(57, 192, 38);" class=3D"">+  &nb= sp;   if (HotkeyTriggered !=3D NULL) {
+        = Status =3D BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PERIOD_SECONDS= (1));
+        if (!EFI_ERROR (Status)) {
+  =         break;
+        }<= /div>
<= span style=3D"font-variant-ligatures: no-common-ligatures" class=3D"">+&nbs= p;     } else {
+        gBS->Stall (100= 0000);
       }
-    } else {
<= div style=3D"margin: 0px; font-stretch: normal; font-size: 11px; line-heigh= t: normal; font-family: Menlo; color: rgb(202, 51, 35);" class=3D"">-  &nb= sp;   gBS->Stall (1000000);
-    }

<= span style=3D"font-variant-ligatures: no-common-ligatures" class=3D""> = ;

-    //
-   = // 0xffff means waiting forever
-    // BDS with no hotkey pro= vided and 0xffff as timeout will "hang" in the loop
-    //
-    if (TimeoutRemain !=3D 0xffff) {
-    &nbs= p; TimeoutRemain--;
+      //
+      // 0= xffff means waiting forever
+      // BDS with no hotkey p= rovided and 0xffff as timeout will "hang" in the loop
+    &nbs= p; //
+      if (TimeoutRemain !=3D 0xffff) {
=
+  &n= bsp;     TimeoutRemain--;
+      }
<= div style=3D"margin: 0px; font-stretch: normal; font-size: 11px; line-heigh= t: normal; font-family: Menlo; color: rgb(57, 192, 38);" class=3D"">+  &nb= sp; }
+    if (TimeoutRemain !=3D 0) {
+    &nbs= p; PlatformBootManagerWaitCallback (0);
     }
   }=
-  PlatformBootManagerWaitCallback (0);
   DEBUG ((EFI_D_IN= FO, "[Bds]Exit the waiting!\n"));
 }

 

Thanks,

Andrew Fish


Therefore, I cl= aim that the right fix is:
diff --git a/MdeMod= ulePkg/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-ou= t, and we have
+  // actually reached that, report 100% = completion to the platform.
+  //
+  = // Note that the (TimeoutRemain =3D=3D 0) condition excludes
= +  // PcdPlatformBootTimeOut=3D0xFFFF, and that's deliberate.
+  //
+  if (PcdGet16 (PcdPlatformBootTimeOu= t) !=3D 0 && TimeoutRemain =3D=3D 0) {
+   =  PlatformBootManagerWaitCallback (0);
+  }
  DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n"));
}


Thanks!
Laszlo


--Apple-Mail=_FA1659F4-CA68-4EA3-8DAE-1EEC4405F028--