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

["Jeshua Smith" <jeshuas@nvidia.com>]

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
> >
> >
> >
> >
> >
> >
> >
> >
> > 
> >