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.4208.1570820859197012100 for ; Fri, 11 Oct 2019 12:07:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=ro6sX3hs; 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 x9BIq3te054761; Fri, 11 Oct 2019 12:07:36 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : content-type : mime-version : subject : from : in-reply-to : date : cc : content-transfer-encoding : message-id : references : to; s=20180706; bh=qfa4NS+3VH8dhAtt9GwTx3d3bWPchM7gpCZeoHVKY18=; b=ro6sX3hsJewG0mN2JSrLFZbeOcBAwA56QsE6J3e+AkFXKHbvsdQPpUWvbvNNFMl4cEBw azE1AkRPVR7h7JLqBzMEhleM00yHxELiyQOvoFa4NytICKqA9FtT6i61kqgz46dytv2e TnsRlJI0wOPmIQSj2Lm+uFUyBH8V4BWFOHhyM19pGadp4aYOwsm8YbPoG+7rubFYVj5N +mqDQNnv23DNd5uK+A2jNITFg4GQWy3I0UoOE9I43W4lb79VTrFqcMHG0OMT0/dUa3X5 a+3x9BOV5FxhzE6O8ZLbxnKzVJQoUqosz7YqvgVSJdBq+VptNd1sO3YmceoBccASaaQM aA== Received: from ma1-mtap-s01.corp.apple.com (ma1-mtap-s01.corp.apple.com [17.40.76.5]) by nwk-aaemail-lapp01.apple.com with ESMTP id 2vet4dcppq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Fri, 11 Oct 2019 12:07:36 -0700 Received: from nwk-mmpp-sz09.apple.com (nwk-mmpp-sz09.apple.com [17.128.115.80]) by ma1-mtap-s01.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PZ8006OB54LGFH0@ma1-mtap-s01.corp.apple.com>; Fri, 11 Oct 2019 12:07:35 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz09.apple.com by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PZ8000004ULPF00@nwk-mmpp-sz09.apple.com>; Fri, 11 Oct 2019 12:07:34 -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: f3276913-521b-48d0-b1cf-2fe743c63544 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: ab549336-1b93-45a3-b6bd-6f6103ead97d X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-11_10:,, signatures=0 Received: from [17.235.46.243] (unknown [17.235.46.243]) by nwk-mmpp-sz09.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PZ8007SD54KN590@nwk-mmpp-sz09.apple.com>; Fri, 11 Oct 2019 12:07:34 -0700 (PDT) Sender: afish@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 From: "Andrew Fish" In-reply-to: <1027706d-204d-8329-a505-6969d930e2a6@redhat.com> Date: Fri, 11 Oct 2019 12:07:31 -0700 Cc: liming.gao@intel.com, Pete Batard Message-id: References: <4A89E2EF3DFEDB4C8BFDE51014F606A14E515A05@SHSMSX104.ccr.corp.intel.com> <0411bb4e-7d90-df7e-8fa0-5072275f9c62@redhat.com> <65D5B022-0049-4BFA-AB15-B12B4D8F1E13@apple.com> <1027706d-204d-8329-a505-6969d930e2a6@redhat.com> To: devel@edk2.groups.io, lersek@redhat.com X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-10-11_10:,, signatures=0 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: quoted-printable > On Oct 11, 2019, at 9:33 AM, Laszlo Ersek wrote: >=20 > Hi Andrew, >=20 > On 10/11/19 15:13, Andrew Fish wrote: >=20 >> 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. >=20 > OK. >=20 >> 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 >> !=3D 0). >=20 > Let me paste your patch below, formatted with the > "--ignore-space-change" option (it hides changes due to simple > re-indentation of code): >=20 >> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Un= iversal/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; >>=20 >> DEBUG ((EFI_D_INFO, "[Bds]BdsWait ...Zzzzzzzzzzzz...\n")); >>=20 >> TimeoutRemain =3D PcdGet16 (PcdPlatformBootTimeOut); >> + if (TimeoutRemain !=3D 0) { >> while (TimeoutRemain !=3D 0) { >> DEBUG ((EFI_D_INFO, "[Bds]BdsWait(%d)..Zzzz...\n", (UINTN) Timeou= tRemain)); >> PlatformBootManagerWaitCallback (TimeoutRemain); >>=20 >> BdsReadKeys (); // BUGBUG: Only reading can signal HotkeyTriggere= d >> // Can be removed after all keyboard driv= ers invoke callback in timer callback. >>=20 >> if (HotkeyTriggered !=3D NULL) { >> Status =3D BdsWaitForSingleEvent (HotkeyTriggered, EFI_TIMER_PE= RIOD_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--; >> } >> } >> + if (TimeoutRemain !=3D 0) { >> PlatformBootManagerWaitCallback (0); >> + } >> + } >> DEBUG ((EFI_D_INFO, "[Bds]Exit the waiting!\n")); >> } >=20 > This would solve the (PcdPlatformBootTimeOut=3D=3D0) case well. >=20 > 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. >=20 > 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. >=20 > I think if we eliminate the *inner* "if" statement -- call > PlatformBootManagerWaitCallback (0) unconditionally --, and preserve the > outer "if" statement, it should work. >=20 Laszlo, Good point. I was trying to prevent 2 calls with 0 but now I notice that i= s not possible in the while loop. So I agree with your suggestion. I also l= ike Pete's idea of adding the error check in PlatformBootManagerWaitCallbac= k(). Thanks, Andrew Fish > Thanks > Laszlo >=20 >=20 >=20