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...@intel.com>
Sent: Thursday, February 9, 2023 10:42 PM
To: Kinney, Michael D <michael.d.kin...@intel.com>; devel@edk2.groups.io;
Jeshua Smith <jesh...@nvidia.com>; Demeter, Miki <miki.deme...@intel.com>;
Wang, Jian J <jian.j.w...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>;
Gao, Zhichao <zhichao....@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.kin...@intel.com>
> Sent: Friday, February 10, 2023 11:50 AM
> To: devel@edk2.groups.io; jesh...@nvidia.com; Demeter, Miki
> <miki.deme...@intel.com>; Ni, Ray <ray...@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>; Gao, Liming <gaolim...@byosoft.com.cn>; Gao,
> Zhichao <zhichao....@intel.com>
> Cc: Kinney, Michael D <michael.d.kin...@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.deme...@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.w...@intel.com; gaolim...@byosoft.com.cn;
> zhichao....@intel.com; ray...@intel.com; Jeshua Smith
> <jesh...@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 <jesh...@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
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100138): https://edk2.groups.io/g/devel/message/100138
Mute This Topic: https://groups.io/mt/96869869/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
