From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=45.249.212.191; helo=huawei.com; envelope-from=huangming23@huawei.com; receiver=edk2-devel@lists.01.org Received: from huawei.com (szxga05-in.huawei.com [45.249.212.191]) (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 9FA24222DE14B for ; Sat, 10 Feb 2018 21:58:08 -0800 (PST) Received: from DGGEMS406-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 753EDC1FD0C18; Sun, 11 Feb 2018 14:03:38 +0800 (CST) Received: from [127.0.0.1] (10.61.17.224) by DGGEMS406-HUB.china.huawei.com (10.3.19.206) with Microsoft SMTP Server id 14.3.361.1; Sun, 11 Feb 2018 14:03:33 +0800 To: Peter Jones , Leif Lindholm References: <1516287703-35516-1-git-send-email-huangming23@huawei.com> <1516287703-35516-14-git-send-email-huangming23@huawei.com> <20180123102320.7rloxdthhq2njbvu@bivouac.eciton.net> <2ced3021-fd0e-7551-c476-4468c915f82e@huawei.com> <20180129111621.qbtejpasxw6ttdnl@bivouac.eciton.net> <20180207211655.eqw7khfklznw5bjm@redhat.com> CC: , , , Ming Huang , , , , , From: "Huangming (Mark)" Message-ID: <3132e5bf-8edf-c320-1fbd-e843c4a7b61e@huawei.com> Date: Sun, 11 Feb 2018 14:03:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20180207211655.eqw7khfklznw5bjm@redhat.com> X-Originating-IP: [10.61.17.224] X-CFilter-Loop: Reflected Subject: Re: [PATCH edk2-platforms v1 13/14] Hisilicon/Library: Add OsBootLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 11 Feb 2018 05:58:09 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Thank you for your detailed reply, I will think about your suggestions. On 2018/2/8 5:16, Peter Jones wrote: > Coming late to the party because Leif called my attention to this > thread... > > On Mon, Jan 29, 2018 at 11:16:21AM +0000, Leif Lindholm wrote: >> This type of system behaviour has been seen multiple times to break >> installations in the real world. > > I can't agree more; that's why there's a pile of language in 2.6 and > later that says how to do a better job of this. > >> Note: my main objections here are really with regards to: >> 1) the expectation that variable store is erased on fw update >> 2) automatically rewriting boot variables >> >> If (1) was resolved, then I could potentially see a use for a >> last-ditch fallback option (but even then, I don't think it should be >> enabled by default). > > I certainly agree resolving #1 is the thing to do here, but there is is > actually useful functionality to have in general on the fallback path - > though I don't think this patch implements it in a correct or preferred > way. In particular, it would be better to follow the advice in 3.1.1 of > the UEFI 2.7 spec, where we say that while yes, the firmware is allowed > to do boot order maintenance, it shouldn't remove anything or change the > order itself except in the most dire of circumstances. In particular: > > | The firmware should not, under normal operation, automatically remove > | any correctly formed Boot#### variable currently referenced by the > | BootOrder or BootNext variables. Such removal should be limited to > | scenarios where the firmware is guided by direct user interaction. > > The right thing to do here is to publish some PlatformRecovery#### > variables as specified in 3.4.2, which can be a static list that's the > same every time you boot up, and when there's a failure here BootNext > and BootOrder have been exhausted, proceed according to 3.4.1 and 3.4.2, > which I'll attempt to summarize below. > > The PlatformRecovery#### variables here should have device paths that > are something like: > > PlatformRecovery0000: File(\EFI\BOOT\BOOTAA64.EFI) > PlatformRecovery0001: > File(\EFI\centos\grubaa64.efi)/EndInstance/File(\EFI\debian\grubaa64.efi)/EndInstance/File(\EFI\GRUB2\GRUBAA64.EFI)/EndInstance/File(\EFI\Microsoft\Boot\bootmgfw.efi)/EndInstance/File(\EFI\redhat\grub.efi)/EndInstance/... etc .../EndEntire > > The EFI_OS_INDICATIONS_START_OS_RECOVERY and > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bits should be set in > OsIndicationsSupported. > > (As an aside, I don't think we've explicitly said that multi-instance > device paths do or don't work in boot variables, so that may require > some work, or if you have a stronger preference about order you could > just make each one its own PlatformRecovery#### variable. For what it's > worth, I don't think that the list in this patch is great - at the least > the arch suffixes should be generated according to what the build target > is, and what you've got currently doesn't correctly match several > shipping OSes - many of which *do* provide a file at > \EFI\BOOT\BOOT${ARCH}.EFI which will fix the BootOrder for you.) > > If you supply those static variables, then in the normal boot path, if > you've exhausted BootNext and all of BootOrder, continue according to > chapter 3.4.2 and then 3.4.3 of the spec. That basically says there's a > list of things to try as if they're Boot#### options: > > - If OsRecoveryOrder exists, it's a list of GUIDs under which there may > be OsRecovery#### variables. The GUIDs are processed in the order > they're listed, and the OsRecovery#### variables under each GUID are > processed in hexadecimal numerical order. > - PlatformRecovery#### variables are processed in hexadecimal numerical > order if OsRecovery variables are exhausted without successfully > booting anything. > > For everything in that list, the variables basically get treated exactly > like Boot#### variables. For each #### variable: > - parse the variable like you'd parse Boot####, and use the normal > discovery method to iterate across any files that match it. > - if so, see if there's a Boot#### that matches that (it isn't > necessarily in BootOrder); if not create one. > - Try to boot it like any other boot entry: set BootCurrent to the > Boot#### number, do LoadImage() and StartImage(), etc. There's no > modification of BootOrder here. > - If the binary there can't be found, loaded, or returns an > error, continue iterating the normal way to see if there are more > matching files, and if there aren't, then proceed to the next > variable. > > If you exhaust this list, it's time for an error message and a > menu or something ;) > > There are some important characteristics here that we need to maintain: > 1) We haven't really talked enough about when it's really okay to > Boot#### entries, because in general removing them is undoing > something that was done on purpose (even if that purpose has been > obviated now.) A pretty good rule is: don't remove Boot#### variables > unless there are dire circumstances, like you've nearly completely > run out of flash. The user or the OS set these for a reason. If you > absolutely must prune them, start with the ones that aren't in > BootOrder or BootNext, and then proceed to the high-numbered ones > that are duplicates of other ones. And even then, only remove until > you're under some known safe storage threshold. > 2) Don't change BootOrder ever. The user set that for a reason. If you > absolutely have to change it, append to the end. But you really > don't need to change it unless the user told you to. > 3) Don't probe the disk except to try to match a boot entry you're > attempting to boot from. If you need to read a partition table to > match an HD(), that's fine. If you need to read a disk to match a > File(), that's fine. But only when you're trying to boot those > device paths and they require iterating the disks. All probing the > disk accomplishes in other cases is slowing things down and powering > up devices unnecessarily. > -- Best Regards, Ming