public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Guo Heyi <heyi.guo@linaro.org>
To: Peter Jones <pjones@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>,
	"Huangming (Mark)" <huangming23@huawei.com>,
	ard.biesheuvel@linaro.org, edk2-devel@lists.01.org,
	zhangjinsong2@huawei.com, Ming Huang <heyi.guo@linaro.org>,
	mengfanrong@huawei.com, linaro-uefi@lists.linaro.org,
	guoheyi@huawei.com, waip23@126.com, wanghuiqiang@huawei.com
Subject: Re: [PATCH edk2-platforms v1 13/14] Hisilicon/Library: Add OsBootLib
Date: Mon, 26 Feb 2018 09:12:27 +0800	[thread overview]
Message-ID: <20180226011227.GE111715@SZX1000114654> (raw)
In-Reply-To: <20180207211655.eqw7khfklznw5bjm@redhat.com>

Hi Peter,

Thanks for your detailed explanation. I still have one question: 

On normal platforms, there are more boot options created by UEFI other than the
hard disk boot option created by OS installation, like PXE network boot, USB
stick boot, etc. If we use the PlatformRecovery#### method, will UEFI first
try all normal boot options like PXE and USB? If PXE boot environment is always
available, then the system may enter PXE OS installation again, which is not
what we expect.

Thanks and regards,

Gary (Heyi Guo)

On Wed, Feb 07, 2018 at 04:16:55PM -0500, 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.
> 
> -- 
>         Peter


  parent reply	other threads:[~2018-02-26  1:06 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-18 15:01 [PATCH edk2-platforms v1 00/14] Improve D0x platforms and bug fix Ming Huang
2018-01-18 15:01 ` [PATCH edk2-platforms v1 01/14] Hisilicon/D05: Add PPTT support Ming Huang
2018-01-20 10:16   ` Ard Biesheuvel
2018-01-22  9:16     ` Huangming (Mark)
2018-01-23  6:00     ` Huangming (Mark)
2018-01-22 13:53   ` Leif Lindholm
2018-01-22 14:15     ` Leif Lindholm
2018-01-24 13:49     ` graeme.gregory
2018-01-23 21:29   ` Jeremy Linton
2018-01-24  7:57     ` Huangming (Mark)
2018-01-25  5:56     ` Huangming (Mark)
2018-01-25 15:27       ` Jeremy Linton
2018-01-18 15:01 ` [PATCH edk2-platforms v1 02/14] Hisilicon D03/D05:Switch to Generic BDS driver Ming Huang
2018-01-20 10:27   ` Ard Biesheuvel
2018-01-22 18:38   ` Leif Lindholm
2018-01-23  6:03     ` Huangming (Mark)
2018-01-18 15:01 ` [PATCH edk2-platforms v1 03/14] Hisilicon D03/D05: Optimize the feature of BMC set boot option Ming Huang
2018-01-20 10:41   ` Ard Biesheuvel
2018-01-23  8:28     ` Huangming (Mark)
2018-01-23 10:28   ` Leif Lindholm
2018-01-23 10:51     ` Huangming (Mark)
2018-01-18 15:01 ` [PATCH edk2-platforms v1 04/14] Hisilicon D03/D05: Add capsule upgrade support Ming Huang
2018-01-20 10:50   ` Ard Biesheuvel
2018-01-23  8:53     ` Huangming (Mark)
2018-01-23  9:33       ` Ard Biesheuvel
2018-01-24 11:10     ` Huangming (Mark)
2018-01-24 11:21       ` Ard Biesheuvel
2018-01-25  0:53         ` Huangming (Mark)
2018-01-23 14:06   ` Leif Lindholm
2018-01-18 15:01 ` [PATCH edk2-platforms v1 05/14] Hisilicon D03/D05: Open SasPlatform source code Ming Huang
2018-01-20 10:57   ` Ard Biesheuvel
2018-01-23 11:01     ` Huangming (Mark)
2018-01-23 14:04   ` Leif Lindholm
2018-01-18 15:01 ` [PATCH edk2-platforms v1 06/14] Hisilicon D03/D05: Open SnpPlatform " Ming Huang
2018-01-20 11:00   ` Ard Biesheuvel
2018-01-23 11:01     ` Huangming (Mark)
2018-01-23 14:07   ` Leif Lindholm
2018-01-24 12:31     ` Huangming (Mark)
2018-01-24 13:47       ` Leif Lindholm
2018-01-18 15:01 ` [PATCH edk2-platforms v1 07/14] Hisilicon/Smbios: modify type 4 Ming Huang
2018-01-20 11:01   ` Ard Biesheuvel
2018-01-23 14:15   ` Leif Lindholm
2018-01-18 15:01 ` [PATCH edk2-platforms v1 08/14] Hisilicon/PCIe: Disable PCIe ASPM Ming Huang
2018-01-20 11:04   ` Ard Biesheuvel
2018-01-18 15:01 ` [PATCH edk2-platforms v1 09/14] Hisilicon/D05: Replace SP805Watchdog by WatchdogTimer driver Ming Huang
2018-01-20 11:05   ` Ard Biesheuvel
2018-01-23 14:21   ` Leif Lindholm
2018-01-18 15:01 ` [PATCH edk2-platforms v1 10/14] Hisilicon/D03: " Ming Huang
2018-01-20 11:05   ` Ard Biesheuvel
2018-01-23 14:21   ` Leif Lindholm
2018-01-18 15:01 ` [PATCH edk2-platforms v1 11/14] Hisilicon/D05/ACPI: Add ITS PXM Ming Huang
2018-01-20 11:06   ` Ard Biesheuvel
2018-01-18 15:01 ` [PATCH edk2-platforms v1 12/14] Hisilicon/D05/ACPI: Add Pcie, HNS and SAS PXM Ming Huang
2018-01-20 11:08   ` Ard Biesheuvel
2018-01-18 15:01 ` [PATCH edk2-platforms v1 13/14] Hisilicon/Library: Add OsBootLib Ming Huang
2018-01-20 11:11   ` Ard Biesheuvel
2018-01-23 10:23   ` Leif Lindholm
2018-01-27  1:47     ` Huangming (Mark)
2018-01-27 10:37       ` Ard Biesheuvel
2018-01-29  8:55         ` Huangming (Mark)
2018-01-29 10:19           ` Ard Biesheuvel
2018-01-29 11:16       ` Leif Lindholm
2018-02-07 21:16         ` Peter Jones
2018-02-11  6:03           ` Huangming (Mark)
2018-02-26  1:12           ` Guo Heyi [this message]
2018-01-18 15:01 ` [PATCH edk2-platforms v1 14/14] Hisilicon D03/D05: Update firmware version to 18.02 Ming Huang
2018-01-20 11:11   ` Ard Biesheuvel
2018-01-23 10:18   ` Leif Lindholm
2018-01-24  1:17     ` Huangming (Mark)
2018-01-24  7:54       ` Leif Lindholm
2018-01-22 13:26 ` [PATCH edk2-platforms v1 00/14] Improve D0x platforms and bug fix Leif Lindholm
2018-01-23 14:24 ` Leif Lindholm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180226011227.GE111715@SZX1000114654 \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox