On 01/14/15 12:35, Olivier Martin wrote: > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:[email protected]] >> Sent: 13 January 2015 10:23 >> To: [email protected] >> Subject: [edk2] [PATCH v2 1/3] OvmfPkg: QemuBootOrderLib: expose QEMU's >> "-boot menu=on[, splash-time=N]" >> >> The QEMU command line option >> >> -boot menu=on >> >> is meant to have the guest firmware wait for a firmware-specific >> interval >> for the user to enter the boot menu. During the wait, the user can opt >> to >> enter the boot menu, or interrupt the wait and proceed to booting at >> once. >> If the wait interval elapses, the firmware should boot as it normally >> would. >> >> The QEMU command line option >> >> -boot menu=on,splash-time=N >> >> means the same, except the firmware should wait for cca. N milliseconds >> instead of a firmware-specific interval. >> >> We can approximate this behavior quite well for edk2's virtual >> platforms >> because the Intel BDS front page already supports a progress bar, with >> semantics similar to the above. Let's distill the fw_cfg bits >> underlying >> "-boot menu=on,splash-time=N" for the BDS policies, in the form of a >> timeout value they can pass to Intel's PlatformBdsEnterFrontPage(). >> >> If the boot menu is not requested, we return >> "gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPlatformBootTimeOut", >> which >> is what the virtual platforms use right now. >> >> If the boot menu is requested without specifying the timeout, we return >> the same PCD, unless it would cause us to skip the boot menu at once. >> In >> the latter case, we return 3 seconds (as an approximation of the 2500 >> ms >> SeaBIOS default.) >> >> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1170507 >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> >> Notes: >> v2: >> - distinguish the possible cases in more detail in the commit >> message >> and in code comments [Jordan] >> - refine logic for "-boot menu=on" (no splash-time=N) [Jordan] >> - rewrite GetFrontPageTimeoutFromQemu() to simplify nesting of >> "if"s >> >> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf | 3 +++ >> OvmfPkg/Include/Library/QemuBootOrderLib.h | 12 >> ++++++++++++ >> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c | 50 >> ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 65 insertions(+) >> >> diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf >> b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf >> index 6289e6a..e7be0f3 100644 >> --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf >> +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf >> @@ -57,3 +57,6 @@ >> [FeaturePcd] >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation >> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation >> + >> +[Pcd] >> + gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPlatformBootTimeOut >> diff --git a/OvmfPkg/Include/Library/QemuBootOrderLib.h >> b/OvmfPkg/Include/Library/QemuBootOrderLib.h >> index 12cda6a..80d02c8 100644 >> --- a/OvmfPkg/Include/Library/QemuBootOrderLib.h >> +++ b/OvmfPkg/Include/Library/QemuBootOrderLib.h >> @@ -54,4 +54,16 @@ SetBootOrderFromQemu ( >> IN CONST LIST_ENTRY *BootOptionList >> ); >> >> + >> +/** >> + Calculate the number of seconds we should be showing the FrontPage >> progress >> + bar for. >> + >> + @return The TimeoutDefault argument for >> PlatformBdsEnterFrontPage(). >> +**/ >> +UINT16 >> +GetFrontPageTimeoutFromQemu ( >> + VOID >> + ); >> + >> #endif >> diff --git a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c >> b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c >> index bc2fb56..e63ad26 100644 >> --- a/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c >> +++ b/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c >> @@ -1571,3 +1571,53 @@ ErrorFreeFwCfg: >> >> return Status; >> } >> + >> + >> +/** >> + Calculate the number of seconds we should be showing the FrontPage >> progress >> + bar for. >> + >> + @return The TimeoutDefault argument for >> PlatformBdsEnterFrontPage(). >> +**/ >> +UINT16 >> +GetFrontPageTimeoutFromQemu ( >> + VOID >> + ) >> +{ >> + FIRMWARE_CONFIG_ITEM BootMenuWaitItem; >> + UINTN BootMenuWaitSize; >> + >> + QemuFwCfgSelectItem (QemuFwCfgItemBootMenu); >> + if (QemuFwCfgRead16 () == 0) { >> + // >> + // The user specified "-boot menu=off", or didn't specify "-boot >> + // menu=(on|off)" at all. Return the platform default. >> + // >> + return PcdGet16 (PcdPlatformBootTimeOut); >> + } > > Should it not return 0?
No, not constant 0. > You said this PcdPlatformBootTimeOut is set to 0 for qemu in an earlier > email. I said that PcdPlatformBootTimeOut was set to 0 in the OvmfPkg DSC files. That's a difference from ArmVirtualizationQemu.dsc. In OVMF, Jordan wanted to disable the progress bar by default, hence the 0 value for the PCD. The justification is that we should boot as quickly as possible, and that the normal workings of the firmware provide enough time for the user to press a key on the TianoCore splash screen anyway. Intel BDS was slightly modified so that it would pick up any pending keypress even with a timeout value of 0. So, in OVMF, there's no progress bar, the user can press space while the splash screen is shown, and then the boot menu is entered (or the options are tried at once). For ArmVirtualizationQemu.dsc, this is different. First, we have no GOP, and no splash screen. Second, the ARM BDS has always offered a 3 second timeout for the user to break into the ARM BDS boot manager. Due to these reasons, PcdPlatformBootTimeOut is 3 for ArmVirtualizationQemu.dsc -- this way the "progress bar" is always shown by Intel BDS (in the form of simple countdown messages on the serial console), and the ARM BDS tradition is preserved. Hence, when the user doesn't specify "-boot menu=ZZZ" at all, the above code preserves each platform's prior behavior. > As far as I can see there is no way to differ "-boot menu=off" or no "-boot > menu=off". Correct. That's a qemu quirk here. The fw_cfg item that we're talking about is *not* an fw_cfg "file" -- that is, we can't look for it in the fw_cfg directory to see if it is present or not, and only check its contents if it is present. The item here is a predefined, direct *key* (the directory associates keys with filenames), and we can't query the *existence* of that key, only its value. So, value 0 means "-boot menu is explicitly turned off, or -boot menu is absent, or fw_cfg is fully unavailable" -- these can't be told apart. > > - "-boot menu=off" is clearly Timeout=0 > - no "-boot menu=off" means default behaviour. And the current default > behaviour is Timeout=0 for qEmu. Correct. However, due to the above aliasing, I have only two choices: - return constant zero, and then the *absence* of "-boot menu=anything" will be handled incorrectly (because we won't return the platform default where the platform default is different from zero -- in other words, we'll skip the menu at once even if the platform defaults to a countdown, despite the user not specifying -boot menu=anything at all) - return the platform default, and then the explicit "-boot menu=off" will be handled incorrectly (because it won't force off the countdown on platforms where the platform default is different from zero -- in other words, a countdown will be presented despite the user saying -boot menu=off). I chose option #2 here, because the full absence of "-boot menu=anything" is very common (and the platform default should work), and because "-boot menu=off" is very uncommon use (and a short countdown is not a big problem). > Reading this code might be confusing if we do not know > PcdPlatformBootTimeOut is set to 0. According to the above, I explicitly wanted to return a value different from 0 for ArmVirtualizationQemu.dsc, where the PCD says 3. *All* of * -boot menu=off * -boot menu=... absent * fw_cfg unavailable will unavoidably end up having the same treatment, so I chose "defer to the platform default" for that one common treatment. > That's why I am suggesting to explicitly return 0. > > > Feel free to ignore my comment if I miss something. I do not want to block > this patch if my comment is not relevant. Your comment is relevant. The root problem is that value 0 for the QemuFwCfgItemBootMenu fw_cfg key means several situations, and we can't avoid erring in at least one of those situations. I tried to pick the least annoying mapping (based on traditional qemu command line usage). "Force the menu off" is not something qemu users expect or say; they just assume the countdown is off by default, unless you specify -boot menu=on. If you set the edk2 platform's boot timeout PCD to a nonzero value, then that qemu user assumption is *slightly* violated, but not too annoyingly. If, in the same scenario I returned constant zero, despite a nonzero platform PCD, then (I think) that would *gravely* breach the definition of "PcdPlatformBootTimeOut". In any case, we can decide to restrict "PcdPlatformBootTimeOut" for the virt platforms the way you're suggesting. "PcdPlatformBootTimeOut" would then only have a consequence for "-boot menu=on" (without splash-time=N). ... After reading this, do you still think we should return constant 0 in the first branch? If so, I'm willing to change it. From an end-user perspective, you're definitely right. (For OVMF that's no observable change, because the PCD is 0 for OVMF anyway.) Thanks, Laszlo ------------------------------------------------------------------------------ New Year. New Location. New Benefits. New Data Center in Ashburn, VA. GigeNET is offering a free month of service with a new server in Ashburn. Choose from 2 high performing configs, both with 100TB of bandwidth. Higher redundancy.Lower latency.Increased capacity.Completely compliant. http://p.sf.net/sfu/gigenet _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
