3 files changed, 47 insertions(+), 21 deletions(-)
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|0x30001055
+ ## 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.PcdAllowBootNextFromPlatformBootManager
+ 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.PcdAllowBootNextFromPlatformBootManager
+ 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 (#99929): https://edk2.groups.io/g/devel/message/99929
Mute This Topic: https://groups.io/mt/96861530/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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|0x30001055
>
> + ## 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.PcdAllowBootNextFromPlatformBootManager
> + 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.PcdAllowBootNextFromPlatformBootManager
> + 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 (#99947): https://edk2.groups.io/g/devel/message/99947
Mute This Topic: https://groups.io/mt/96869869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99955): https://edk2.groups.io/g/devel/message/99955
Mute This Topic: https://groups.io/mt/96869869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100143): https://edk2.groups.io/g/devel/message/100143
Mute This Topic: https://groups.io/mt/96869869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Thanks for the reply.
The issue (and associated patch) is actually in BdsEntry.c, not PlatformBootManagerLib. The BdsEntry.c code is preventing our own PlatformBootManagerLib implementation from doing what it needs to do. We have copied and modified PlatformBootManagerLib for our platform as you suggest, but because BdsEntry.c is explicitly taking control of BootNext away from it, we're unable to do something we need to do on our platform (in this case responding to BMC's request via IPMI to temporarily modify the boot order of the upcoming boot device selection, similar to how the OS uses BootNext to request to temporarily modify the boot order of the upcoming boot device selection). My patch is adding a PCD to give that control back to PlatformBootManagerLib if the platform needs it, while still defaulting to keeping the restriction in place for platforms that don't want to allow their PlatformBootManagerLib to be able to use BootNext. To me it seems like the BdsEntry.c code is intentionally prohibiting something useful with no clear benefit. I understand that if PlatformBootManagerLib is allowed to modify BootNext it could potentially override a BootNext value that the OS had previously set, but it seems to me that if a platform is creating their own PlatformBootManagerLib and is writing BootNext in it, then it is the platform's responsibility to take that scenario into account and handle it in the best way for the platform.
We could fork BdsEntry.c to apply our patch, but it seems like the whole purpose of PlatformBootManagerLib is to allow the platform to do the customizations it needs in the library instead of needing to fork BdsEntry.c.
Does that make sense?
-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Monday, February 13, 2023 3:43 PM
To: Jeshua Smith <jeshuas@nvidia.com>; Ni, Ray <ray.ni@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>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] MdeModulePkg: allow PlatformBootManagerLib to use BootNext
External email: Use caution opening links or attachments
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
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100186): https://edk2.groups.io/g/devel/message/100186
Mute This Topic: https://groups.io/mt/96869869/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.