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

["Ni, Ray" <ray.ni@intel.com>]

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