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

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

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