From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F10D7209603ED for ; Tue, 15 May 2018 09:12:04 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4A12B4022931; Tue, 15 May 2018 16:12:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-147.rdu2.redhat.com [10.10.120.147]) by smtp.corp.redhat.com (Postfix) with ESMTP id 86C5463F52; Tue, 15 May 2018 16:12:01 +0000 (UTC) To: =?UTF-8?Q?Marvin_H=c3=a4user?= , "edk2-devel@lists.01.org" Cc: "gsomlo@gmail.com" , "ard.biesheuvel@linaro.org" , "ruiyu.ni@intel.com" , "eric.dong@intel.com" , "star.zeng@intel.com" , "jordan.l.justen@intel.com" References: <12b0e557-4f3b-3766-1e52-c069c02b692e@redhat.com> <9d7aa3e0-d342-ab5a-f54b-2a853a5fcf57@redhat.com> From: Laszlo Ersek Message-ID: <2aaccb36-5b8b-6f63-a9bb-7f3e7910ae2f@redhat.com> Date: Tue, 15 May 2018 18:12:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 15 May 2018 16:12:03 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 15 May 2018 16:12:03 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: Proposition of a BmEnumerateBootOptions() hook. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 May 2018 16:12:05 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 05/15/18 17:38, Marvin Häuser wrote: >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Tuesday, May 15, 2018 3:53 PM >> To: Marvin Häuser ; edk2- >> devel@lists.01.org >> Cc: eric.dong@intel.com; star.zeng@intel.com; jordan.l.justen@intel.com; >> ard.biesheuvel@linaro.org; ruiyu.ni@intel.com; Gabriel L. Somlo (GMail) >> ; Gerd Hoffmann >> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook. >> >> On 05/15/18 15:02, Marvin Häuser wrote: >>> 3.2: I think adding them as volatile variables is the better approach, >>> except for platform-specific code, which should be capable of adding >>> NV options. The reasoning behind this is that usually the found >>> volumes are removable volumes (such as USB Flash Drives) and will be >>> cleaned away soon again anyway. "Extra wishes" for boot options should >>> be added via the UEFI Setup menu or UEFI Shell. >> >> I can't really comment on this -- the fact that Boot#### options are non- >> volatile comes from the UEFI spec. I'm not immediately sure what I think of >> their suggested volatility, but this is likely something for the USWG to discuss. > > I think I might have been unclear here, I was solely referring to the Boot options created by the enumeration process and not Boot#### in general. > I wouldn't know of anything restricting all Boot#### variables to be NV, do you happen to have a quote? See "3.3 Globally Defined Variables" and "Table 10. Global Variables" in the UEFI-2.7 spec: [...] The variables with an attribute of NV are nonvolatile. [...] [...] Variable Name Attribute Description ------------- --------- ----------- [...] Boot#### NV, BS, RT [...] [...] >> Second, just because a boot option fails, it should not be removed. For >> example, a netboot option could fail, or a USB drive might not be connected >> (temporarily). I don't think that's grounds enough for summarily removing a >> boot option, in shared reference code. > > Oh, I was just talking about the possibility, because the current code does remove options under certain conditions. > However, for USB devices as you have mentioned, I do see this good practice so long as the option is volatile. > There is no point in exposing a boot option to a removable device that is not attached if not to prevent unnecessary Flash cycles as far as I can think. > Do you happen to have worries about a scenario I did not consider? Sure; it's a simple scenario: the user might want the system to automatically boot off of a USB drive whenever they connect it, before powering on or rebooting the system, rather than boot from the hard drive. (For USB devices, "USB WWID" and "USB Class" device path nodes are defined by the spec, so the user isn't even expected to plug the drive into the same USB port, for the drive to be found uniquely.) >>> Primary partition: The so-called "Startup Volume" unfortunately is a >>> bit trickier. For it, a practically invalid Boot Option is added, >>> which is an expanded device path to the volume to be booted, however >>> without having a File Device Path Node appended. >> >> This doesn't immediately seem invalid -- if memory serves, you can have >> \EFI\BOOT\BOOT.efi on fixed media as well, and if a boot option only >> names the HD() in question, that default boot path will be launched off of it. > > I think for a Boot#### option, it is always invalid, I disagree. I'll have to defer to Ray here, but in my experience, edk2 practice is that if you have a boot option whose devpath ends in a HD() node, then an UEFI image on that partition under the filepath \EFI\BOOT\BOOT.efi will be tried for booting. As far as I can see, this is neither mandated nor forbidden by the spec. Anyway, for Ray to answer authoritatively. > because the default path is defined to be used in the "scanning phase" once no Boot#### variable could be found. > Also, unfortunately the path is also stored via bless and definitely never the UEFI standard one, so supporting this vendor-specific scenario definitely requires special handling. > Do you have any opinion on the proposed second hook? Personally I remain unconvinced that the second hook is needed (to be invoked from core BdsDxe or UefiBootManagerLib code). But, that's just my opinion :) In general I find that library APIs (especially a *set* of library APIs) are very hard to design (because we can't see the future). So, "organic growth" works better in practice (even if it leads to "less well organized" sets of APIs). This is to say that, if you can propose a hook and immediately demonstrate that it saves code for, and/or simplifies, multiple platforms, then the new API might be justified. (Personally, I usually argue for "three call sites" as a minimum. Two might be enough if the code extracted is particularly baroque, or undergoes frequent changes.) >> I have a more general comment in the end: I doubt I could legally test an OSX >> guest on non-Apple hardware, so I won't try (and I'll most likely not buy or >> otherwise procure OSX, let alone Apple hardware, just for this). That means, >> if you post patches, my main focus will be on keeping the current behavior >> regression-free, and (secondarily) the implementation preferably simple & >> separated. > > Thank you that you are ready to merge changes you cannot even really test. I definitely intend to regression test any such changes :) OTOH, in any larger project, any given maintainer is surely unable to integration-test *all* features in the subsystem that they co-maintain. What's important is that each major feature has at least one maintainer that can integration-test it (possibly with the help of an automated test suite). For example, I totally cannot test Xen code in OvmfPkg or ArmVirtPkg, but that's why we have several permanent Reviewers from the Xen project, for OvmfPkg and ArmVirtPkg. Thanks, Laszlo