Re: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Jeshua,

Some comments on the interpretation of 'next' in BootNext.

The UEFI Specification's main objective is an interface between the platform firmware and an operating system.

This is a 2-way communications path.  The firmware passes critical information to the OS required for the OS
to take over control of the platform from firmware and manage the platform from that point forward.  The UEFI
Specification also provides interfaces for the OS to pass information back to the firmware.  Some of these are
while the OS is running such as UEFI Runtime Services and ACPI methods.  Some other OS->FW communication mechanisms
are through the state of UEFI Variables and passing UEFI Capsules across reset/reboot.

BootNext is an example of an OS->FW communication path for the OS to set the BootNext policy for the OS to
perform some specific action by requesting the FW to perform a one-time alternate boot path after the next
system reset/reboot.  This is the intended use of BootNext within the UEFI Specification context.

The use of BootNext to select an alternate boot option between different FW components within the platform
firmware in the current FW boot is not defined by the UEFI Specification.  Any conventions in that area
are really and attribute of the firmware design/implementation.  In fact, firmware has to be very careful
if it modifies the value of BootNext because the current value could have been set by the OS and the firmware
must honor the OS request for BootNext.

The other element to be aware of is the first boot scenario where the platform firmware is booting for the
very first time and no UEFI variables exist.  This is a case where the default set of boot options are established
before additional ones are set by OS installation or by end users.  The firmware can set BootOrder and/or
BootNext to any values it wants because there is no OS->FW communications in this first boot scenario.

Another case to consider is when a UEFI Platform is used as an appliance where there are a fixed set of
boot options that can never be modified, and the platform will always force the same options with the
same priority every boot.

The challenge for edk2 is that these various uses cases (and there are many more) are difficult to support
in a single open-source implementation.  PlatformBootManagerLib is an implementation that tries to cover
common use cases, but this library is intended to be copied and modified if needed if a platform requires
behavior that is not covered by the common use cases.

Mike


> -----Original Message-----
> From: Jeshua Smith <jeshuas@nvidia.com>
> Sent: Monday, February 13, 2023 2:12 PM
> To: Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Demeter, Miki
> <miki.demeter@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext
> 
> TL;DR - The spec indicates BootNext and BootOrder are to be processed together at the point in time where the choice about boot
> device is being made. The current implementation allows PlatformBootManagerLib to freely control BootOrder, but explicitly takes
> control of BootNext away from PlatformBootManagerLib. This seems counter to the intent of them being processed and used
> together.
> 
> Details:
> The spec says "The BootNext variable is a single UINT16 that defines the Boot#### option that is to be tried first on the next
> boot. After the BootNext boot option is tried the normal BootOrder list is used." and "The BootCurrent variable is a single
> UINT16 that defines the Boot#### option that was selected on the current boot. The platform sets this variable before signaling
> EFI_EVENT_GROUP_READY_TO_BOOT."
> 
> It seems like the spec effectively says this process is followed:
> 1. Select a device for the upcoming boot, preferring BootNext and then the devices in BootOrder's order
> 2. Set BootCurrent to the selected device
> 3. Signal EFI_EVENT_GROUP_READY_TO_BOOT
> 4. Attempt to boot from the device specified by BootCurrent
> 
> Unfortunately, in English "next" has a nebulous meaning. If I say turn at the "next" stoplight, most people understand that I
> mean the light I'm approaching right now rather than meaning skipping "this" one and going to the one after. And if I say I'm
> going to do something "next" Saturday, most people consider that to mean the upcoming (aka "this") Saturday, rather than the one
> after 7 or more days have passed. Similarly, for the PlatformBootManagerLib code that is running before the boot device is
> selected (effectively Step 0 in the list I wrote above), I would expect the "next" boot to be the upcoming attempt to boot a
> device (step 4), rather than meaning skip the upcoming boot attempt (step 4) and apply it to the subsequent one (i.e. sometime
> after step 4 when you're starting the steps over).
> 
> Note: The current code wraps steps 2-4 up into the EfiBootManagerBoot($selected_device) call.
> 
> The current BdsEntry implementation limits the ability for code that runs before step 1 to control the boot device selection
> process (step 1) in these ways:
> - It allows PlatformBootManagerLib to rearrange the BootOrder list to whatever it wants in preparation for the upcoming boot
> attempt (step 4), but at the same time the implementation is also specifically blocking that same code from temporarily setting
> a BootNext to be used with that re-arranged list. In other words, the changes to the normal BootOrder list affect the upcoming
> use of the BootOrder list (step 1), but the changes to BootNext are being delayed until the use after that upcoming use (step
> sometime-in-the-future). This seems inconsistent since I would expect changes that are made at the same time by the same code to
> these paired variables to be applied at the same point in time (i.e. step 1), not split across separate uses of the BootOrder
> list.
> - If there happens to be an existing (cached) BootNext value when PlatformBootManagerLib is called (step 0), the changes
> PlatformBootManagerLib makes to BootOrder will be honored in step 4, but the changes it makes to BootNext will be silently
> deleted.
> 
> 
> As for the two options Ray suggests:
> - Changing BootOrder is "permanent". It seems likely that BootNext was created specifically to avoid needing to change BootOrder
> when you simply need a temporary change. It's not clear to me what purpose is served by delaying BootNext's effect to a boot
> after the upcoming one or by silently deleting it instead of honoring it when PlatformBootManagerLib makes the request.
> - Calling EfiBootManagerBoot() directly (effectively steps 2-4) from PlatformBootManagerLib (effectively step 0) results in
> skipping the following steps that BdsEntry does between PlatformBootManagerBeforeConsole() and the call to EfiBootManagerBoot().
> It seems undesirable to skip them or duplicate them in PlatformBootManagerLib:
> 	- Starting Hotkey Service
> 	- Processing Load Options
> 	- Connecting Consoles
> 	- PlatformBootManagerAfterConsole()
> 	- Evaluating PcdTestKeyUsed
> 	- Dumping LoadOptions
> 	- Launching Boot Manager Menu based on OsIndications
> 	- Launching PlatformRecovery based on OsIndications
> 	- Clearing OsIndications for the two above uses
> 	- Processing hotkeys
> 
> 
> As for Mike's question, Yes I think the TRUE behavior is the correct behavior and I would be happy to have it be that way always
> without a PCD (see my discussion above), but since it's possible that someone somewhere was relying on the existing behavior I
> didn't want to break them. I can't think of any advantage the existing (FALSE) behavior has, but that behavior does seem to be
> intentional based on Ray's response.
> 
> 
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, February 9, 2023 10:42 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; Jeshua Smith <jeshuas@nvidia.com>; Demeter, Miki
> <miki.demeter@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext
> 
> External email: Use caution opening links or attachments
> 
> 
> It's the intention to cache BootNext to avoid BootNext change from PlatformBootManagerLib taking affect in this boot.
> Per spec, BootNext selects the boot option of next boot.
> If PlatformBootManagerLib wants to change the boot option for this boot, either it can change the BootOrder, or it can use
> EfiBootManagerBoot() API to boot directly.
> 
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Friday, February 10, 2023 11:50 AM
> > To: devel@edk2.groups.io; jeshuas@nvidia.com; Demeter, Miki
> > <miki.demeter@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Gao,
> > Zhichao <zhichao.gao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow
> > PlatformBootManagerLib to use BootNext
> >
> > Hi Jeshua,
> >
> > I prefer to not add more PCDs if it can be avoided.
> >
> > Do you think the current behavior is a bug/gap and that the proposed
> > new behavior when the PCD is TRUE is the correct behavior?
> >
> > What would be the impact of not adding the PCD and just implementing
> > the new behavior?
> >
> > Mike
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Jeshua Smith via groups.io
> > > Sent: Thursday, February 9, 2023 12:01 PM
> > > To: Demeter, Miki <miki.demeter@intel.com>
> > > Cc: devel@edk2.groups.io
> > > Subject: FW: [edk2-devel] [PATCH] MdeModulePkg: allow
> > PlatformBootManagerLib to use BootNext
> > >
> > > Miki, this patch, as well as my queries on the mailing list about
> > > this topic
> > prior to the patch, hasn't received any response on
> > > the mailing list. One kind person did respond privately with some
> > information about my questions.
> > >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Jeshua Smith via groups.io
> > > Sent: Thursday, January 19, 2023 10:36 AM
> > > To: devel@edk2.groups.io
> > > Cc: jian.j.wang@intel.com; gaoliming@byosoft.com.cn;
> > zhichao.gao@intel.com; ray.ni@intel.com; Jeshua Smith
> > <jeshuas@nvidia.com>
> > > Subject: [edk2-devel] [PATCH] MdeModulePkg: allow
> > PlatformBootManagerLib to use BootNext
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > Currently BdsEntry caches BootNext before calling
> > PlatformBootManagerLib APIs, with the result that:
> > > - If BootNext is already set, a BootNext value written by the APIs
> > > will be
> > ignored and deleted, and the current boot will use
> > > the cached BootNext value.
> > > - If BootNext is not present, a BootNext value written by the APIs
> > > will have
> > no effect on the current boot, but will be used by
> > > the next boot.
> > >
> > > This patch adds PcdAllowBootNextFromPlatformBootManagerLib so that a
> > platform can enable PlatformBootManagerLib API calls to set
> > > BootNext to control the current boot.
> > > - If the PCD is FALSE (default), there is no change.
> > > - If the PCD is TRUE, then a BootNext value written by the
> > PlatformBootManagerLib APIs will affect the current boot.
> > >
> > > Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> > > ---
> > >  MdeModulePkg/MdeModulePkg.dec            |  7 +++++
> > >  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 27 ++++++++++---------
> > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 34
> > > ++++++++++++++++++------
> > >  3 files changed, 47 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index 9605c617b7..0e74131712
> > 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -1093,6 +1093,13 @@
> > >    # @Prompt Enable UEFI Stack Guard.
> > >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0
> > x30001055
> > >
> > > +  ## Indicates whether PlatformBootManagerLib code can set BootNext
> > for the current boot.
> > > +  #  If enabled, setting BootNext in PlatformBootManagerLib
> > > + controls the
> > current boot.<BR><BR>
> > > +  #   TRUE  - BootNext value from PlatformBootManagerLib will affect the
> > current boot.<BR>
> > > +  #   FALSE - BootNext value from PlatformBootManagerLib will affect the
> > subsequent boot (or be ignored if already set).<BR>
> > > +  # @Prompt Allow PlatformBootManagerLib to set BootNext for the
> > current boot.
> > > +
> > > +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdAllowBootNextFromPlatformBootM
> > anager
> > > + Lib|FALSE|BOOLEAN|0x30001056
> > > +
> > >  [PcdsFixedAtBuild, PcdsPatchableInModule]
> > >    ## Dynamic type PCD can be registered callback function for Pcd
> > > setting
> > action.
> > >    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
> > number of callback function diff --git
> > > a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > index 5bac635def..b7a3560f5f 100644
> > > --- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > @@ -85,19 +85,20 @@
> > >    gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate    ##
> > CONSUMES
> > >
> > >  [Pcd]
> > > -  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes            ##
> > CONSUMES
> > > -  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLang                 ##
> > SOMETIMES_CONSUMES
> > > -  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLangCodes
> > ## CONSUMES
> > > -  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang         ##
> > CONSUMES
> > > -  gEfiMdePkgTokenSpaceGuid.PcdHardwareErrorRecordLevel                ##
> > CONSUMES
> > > -  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut                     ##
> > CONSUMES
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor                    ##
> > CONSUMES
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision                  ##
> > CONSUMES
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand
> > ## CONSUMES
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable              ##
> > SOMETIMES_CONSUMES
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed                       ##
> > CONSUMES
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport              ##
> > CONSUMES
> > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport
> > ## CONSUMES
> > > +  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes
> > ## CONSUMES
> > > +  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLang                       ##
> > SOMETIMES_CONSUMES
> > > +  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLangCodes
> > ## CONSUMES
> > > +  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultPlatformLang
> > ## CONSUMES
> > > +  gEfiMdePkgTokenSpaceGuid.PcdHardwareErrorRecordLevel
> > ## CONSUMES
> > > +  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut                           ##
> > CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
> > ## CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareRevision
> > ## CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand
> > ## CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable
> > ## SOMETIMES_CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed                             ##
> > CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport
> > ## CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport
> > ## CONSUMES
> > > +
> > > +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdAllowBootNextFromPlatformBootM
> > anager
> > > + Lib ## CONSUMES
> > >
> > >  [Depex]
> > >    TRUE
> > > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > > index 766dde3aae..6450406cce 100644
> > > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > > @@ -787,15 +787,19 @@ BdsEntry (
> > >
> > >    //
> > >    // Cache the "BootNext" NV variable before calling any
> > PlatformBootManagerLib APIs
> > > -  // This could avoid the "BootNext" set by PlatformBootManagerLib
> > > be
> > consumed in this boot.
> > > -  //
> > > -  GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID
> > **)&BootNext, &DataSize);
> > > -  if (DataSize != sizeof (UINT16)) {
> > > -    if (BootNext != NULL) {
> > > -      FreePool (BootNext);
> > > -    }
> > > +  // if the Platform isn't allowed to override BootNext.
> > > +  // If "BootNext" was already set, a "BootNext" value set in
> > > + PlatformBootManagerLib APIs  // will be ignored; otherwise it will
> > > + not
> > take effect until the next boot.
> > > +  //
> > > +  if (!PcdGetBool (PcdAllowBootNextFromPlatformBootManagerLib)) {
> > > +    GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID
> > **)&BootNext, &DataSize);
> > > +    if (DataSize != sizeof (UINT16)) {
> > > +      if (BootNext != NULL) {
> > > +        FreePool (BootNext);
> > > +      }
> > >
> > > -    BootNext = NULL;
> > > +      BootNext = NULL;
> > > +    }
> > >    }
> > >
> > >    //
> > > @@ -1048,6 +1052,20 @@ BdsEntry (
> > >
> > >      EfiBootManagerHotkeyBoot ();
> > >
> > > +    //
> > > +    // If PlatformBootManagerLib APIs are allowed to override
> > > + BootNext,
> > read it just before use
> > > +    //
> > > +    if (PcdGetBool (PcdAllowBootNextFromPlatformBootManagerLib)) {
> > > +      GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID
> > **)&BootNext, &DataSize);
> > > +      if (DataSize != sizeof (UINT16)) {
> > > +        if (BootNext != NULL) {
> > > +          FreePool (BootNext);
> > > +        }
> > > +
> > > +        BootNext = NULL;
> > > +      }
> > > +    }
> > > +
> > >      if (BootNext != NULL) {
> > >        //
> > >        // Delete "BootNext" NV variable before transferring control
> > > to it to
> > prevent loops.
> > > --
> > > 2.17.1
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > 
> > >