From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web11.2582.1570811597860262928 for ; Fri, 11 Oct 2019 09:33:17 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 67E9681DFF; Fri, 11 Oct 2019 16:33:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-177.rdu2.redhat.com [10.10.120.177]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5CBEF19C58; Fri, 11 Oct 2019 16:33:16 +0000 (UTC) Subject: Re: [edk2-devel] OVMF is crashing for me in master To: Andrew Fish , devel@edk2.groups.io Cc: liming.gao@intel.com, Pete Batard References: <4A89E2EF3DFEDB4C8BFDE51014F606A14E515A05@SHSMSX104.ccr.corp.intel.com> <0411bb4e-7d90-df7e-8fa0-5072275f9c62@redhat.com> <65D5B022-0049-4BFA-AB15-B12B4D8F1E13@apple.com> From: "Laszlo Ersek" Message-ID: <1027706d-204d-8329-a505-6969d930e2a6@redhat.com> Date: Fri, 11 Oct 2019 18:33:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <65D5B022-0049-4BFA-AB15-B12B4D8F1E13@apple.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 11 Oct 2019 16:33:17 +0000 (UTC) Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit 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