Re: [edk2] [PATCH v2] MdeModulePkg: Skip registering BootManagerMenu if its FFS is not found

[Wang, Sunny (HPS SW)]

Just a update. After offline discussing with Ray, I will send patch v3 based on 
some of his comments listed below and some further updates coming from our 
offline discussion.

Regards,
Sunny Wang

-Original Message-
From: Ni, Ruiyu [mailto:ruiyu...@intel.com] 
Sent: Wednesday, June 29, 2016 10:26 AM
To: Wang, Sunny (HPS SW) <sunnyw...@hpe.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH v2] MdeModulePkg: Skip registering BootManagerMenu 
if its FFS is not found
Importance: High

Sunny,
I have 7 minor comments. Please check below.

Regards,
Ray

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>Sunny Wang
>Sent: Tuesday, June 28, 2016 10:27 AM
>To: edk2-devel@lists.01.org
>Cc: Ni, Ruiyu <ruiyu...@intel.com>
>Subject: [edk2] [PATCH v2] MdeModulePkg: Skip registering 
>BootManagerMenu if its FFS is not found
>
>This is a enhancement to support the case when platform firmware doesn’t 
>support Boot Manager Menu.
>For now, if BootManagerMenu FFS can not be retrieved from FV, BDS core 
>code will still register a boot option for it. Then, this 
>non-functional boot option will still be booted by user's request (like 
>HotKey or Exit from shell) to cause additional boot time and error 
>status code reported. Therefore, it would be good to skip BootManagerMenu boot 
>option registration and then return error status and Invalid BootOption data 
>for this case so that the BootManagerBoot() or other consumers can directly 
>return without doing anything.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Sunny Wang <sunnyw...@hpe.com>
>---
> MdeModulePkg/Include/Library/UefiBootManagerLib.h  |  7 ++--
> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c   | 39 ++
> MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c |  9 +++--
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c   | 29 +++-
> 4 files changed, 64 insertions(+), 20 deletions(-)
>
>diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h 
>b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
>index 0fdb23d..649af9a 100644
>--- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
>+++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
>@@ -418,12 +418,13 @@ EfiBootManagerBoot (
>   );
>
> /**
>-  Return the Boot Manager Menu.
>-
>+  Return the boot option corresponding to the Boot Manager Menu.
>+  It may automatically create one if the boot option hasn't been created yet.
>+
>   @param BootOptionReturn the Boot Manager Menu.
>
>   @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
>-  @retval EFI_NOT_FOUND The Boot Manager Menu is not found.
>+  @retval StatusReturn status of either gRT->SetVariable () or 
>GetSectionFromFv().
> **/

1. How about we change the return status as following:
  @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
  @retval EFI_NOT_FOUND The Boot Manager Menu cannot be found.
  @retval others Return status of gRT->SetVariable (). BootOption still 
points
to the Boot Manager Menu even the Status is not 
EFI_SUCCESS
and EFI_NOT_FOUND.

> EFI_STATUS
> EFIAPI
>diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>index d016517..17e416a 100644
>--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>@@ -2,7 +2,7 @@
>   Library functions which relates with booting.
>
> Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
>-(C) Copyright 2015 Hewlett Packard Enterprise Development LP
>+(C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP
> This program and the accompanying materials  are licensed and made 
>available under the terms and conditions of the BSD License  which 
>accompanies this distribution.  The full text of the license may be 
>found at @@ -2159,16 +2159,19 @@ EfiBootManagerRefreshAllBootOption (  
>}
>
> /**
>-  This function is called to create the boot option for the Boot Manager Menu.
>+  This function is called to get or create the boot option for the Boot 
>Manager Menu.
>
>   The Boot Manager Menu is shown after successfully booting a boot option.
>   Assume the BootManagerMenuFile is in the same FV as the module links to 
> this library.
>
>-  @param  BootOptionReturn the boot option of the Boot Manager Menu
>+  @param  BootOptionReturn the boot option of the Boot Manager Menu.
>+If BootManagerMenu fails to be set as a Boot 
>variables, BootOption
>+still points to the Boot Manager Menu.
>+ 

Re: [edk2] [PATCH v2] MdeModulePkg: Skip registering BootManagerMenu if its FFS is not found

[Ni, Ruiyu]

Sunny,
I have 7 minor comments. Please check below.

Regards,
Ray

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Sunny 
>Wang
>Sent: Tuesday, June 28, 2016 10:27 AM
>To: edk2-devel@lists.01.org
>Cc: Ni, Ruiyu <ruiyu...@intel.com>
>Subject: [edk2] [PATCH v2] MdeModulePkg: Skip registering BootManagerMenu if 
>its FFS is not found
>
>This is a enhancement to support the case when platform firmware doesn’t 
>support Boot Manager Menu.
>For now, if BootManagerMenu FFS can not be retrieved from FV, BDS core code 
>will still register a boot option for it. Then, this
>non-functional boot option will still be booted by user's request (like HotKey 
>or Exit from shell) to cause additional boot time
>and error status code reported. Therefore, it would be good to skip 
>BootManagerMenu boot option registration and then
>return error status and Invalid BootOption data for this case so that the 
>BootManagerBoot() or other consumers can directly
>return without doing anything.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Sunny Wang <sunnyw...@hpe.com>
>---
> MdeModulePkg/Include/Library/UefiBootManagerLib.h  |  7 ++--
> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c   | 39 ++
> MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c |  9 +++--
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c   | 29 +++-
> 4 files changed, 64 insertions(+), 20 deletions(-)
>
>diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h 
>b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
>index 0fdb23d..649af9a 100644
>--- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
>+++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
>@@ -418,12 +418,13 @@ EfiBootManagerBoot (
>   );
>
> /**
>-  Return the Boot Manager Menu.
>-
>+  Return the boot option corresponding to the Boot Manager Menu.
>+  It may automatically create one if the boot option hasn't been created yet.
>+
>   @param BootOptionReturn the Boot Manager Menu.
>
>   @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
>-  @retval EFI_NOT_FOUND The Boot Manager Menu is not found.
>+  @retval StatusReturn status of either gRT->SetVariable () or 
>GetSectionFromFv().
> **/

1. How about we change the return status as following:
  @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
  @retval EFI_NOT_FOUND The Boot Manager Menu cannot be found.
  @retval others Return status of gRT->SetVariable (). BootOption still 
points
to the Boot Manager Menu even the Status is not 
EFI_SUCCESS
and EFI_NOT_FOUND.

> EFI_STATUS
> EFIAPI
>diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>index d016517..17e416a 100644
>--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>@@ -2,7 +2,7 @@
>   Library functions which relates with booting.
>
> Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
>-(C) Copyright 2015 Hewlett Packard Enterprise Development LP
>+(C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD 
> License
> which accompanies this distribution.  The full text of the license may be 
> found at
>@@ -2159,16 +2159,19 @@ EfiBootManagerRefreshAllBootOption (
> }
>
> /**
>-  This function is called to create the boot option for the Boot Manager Menu.
>+  This function is called to get or create the boot option for the Boot 
>Manager Menu.
>
>   The Boot Manager Menu is shown after successfully booting a boot option.
>   Assume the BootManagerMenuFile is in the same FV as the module links to 
> this library.
>
>-  @param  BootOptionReturn the boot option of the Boot Manager Menu
>+  @param  BootOptionReturn the boot option of the Boot Manager Menu.
>+If BootManagerMenu fails to be set as a Boot 
>variables, BootOption
>+still points to the Boot Manager Menu.
>+If BootManagerMenu FFS section can not be retrieved, 
>BootOption will
>+be contain invalid data to prevent other function 
>from using it.
>
>   @retval EFI_SUCCESS   Successfully register the Boot Manager Menu.
>-  @retval StatusReturn status of gRT->SetVariable (). BootOption 
>still points
>-to the Boot Manager Menu even the Status is not 
>EFI_SUCCESS.
>+  @retval StatusReturn status of ei

[edk2] [PATCH v2] MdeModulePkg: Skip registering BootManagerMenu if its FFS is not found

[Sunny Wang]

This is a enhancement to support the case when platform firmware doesn’t 
support Boot Manager Menu.
For now, if BootManagerMenu FFS can not be retrieved from FV, BDS core code 
will still register a boot option for it. Then, this non-functional boot option 
will still be booted by user's request (like HotKey or Exit from shell) to 
cause additional boot time and error status code reported. Therefore, it would 
be good to skip BootManagerMenu boot option registration and then return error 
status and Invalid BootOption data for this case so that the BootManagerBoot() 
or other consumers can directly return without doing anything.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Sunny Wang 
---
 MdeModulePkg/Include/Library/UefiBootManagerLib.h  |  7 ++--
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c   | 39 ++
 MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c |  9 +++--
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c   | 29 +++-
 4 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h 
b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
index 0fdb23d..649af9a 100644
--- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
+++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
@@ -418,12 +418,13 @@ EfiBootManagerBoot (
   );
 
 /**
-  Return the Boot Manager Menu.
- 
+  Return the boot option corresponding to the Boot Manager Menu.
+  It may automatically create one if the boot option hasn't been created yet.
+
   @param BootOptionReturn the Boot Manager Menu.
 
   @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
-  @retval EFI_NOT_FOUND The Boot Manager Menu is not found.
+  @retval StatusReturn status of either gRT->SetVariable () or 
GetSectionFromFv().
 **/
 EFI_STATUS
 EFIAPI
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c 
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index d016517..17e416a 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -2,7 +2,7 @@
   Library functions which relates with booting.
 
 Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
-(C) Copyright 2015 Hewlett Packard Enterprise Development LP
+(C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -2159,16 +2159,19 @@ EfiBootManagerRefreshAllBootOption (
 }
 
 /**
-  This function is called to create the boot option for the Boot Manager Menu.
+  This function is called to get or create the boot option for the Boot 
Manager Menu.
 
   The Boot Manager Menu is shown after successfully booting a boot option.
   Assume the BootManagerMenuFile is in the same FV as the module links to this 
library.
 
-  @param  BootOptionReturn the boot option of the Boot Manager Menu
+  @param  BootOptionReturn the boot option of the Boot Manager Menu.
+If BootManagerMenu fails to be set as a Boot 
variables, BootOption
+still points to the Boot Manager Menu.
+If BootManagerMenu FFS section can not be retrieved, 
BootOption will 
+be contain invalid data to prevent other function from 
using it.
 
   @retval EFI_SUCCESS   Successfully register the Boot Manager Menu.
-  @retval StatusReturn status of gRT->SetVariable (). BootOption still 
points
-to the Boot Manager Menu even the Status is not 
EFI_SUCCESS.
+  @retval StatusReturn status of either gRT->SetVariable() or 
GetSectionFromFv().
 **/
 EFI_STATUS
 BmRegisterBootManagerMenu (
@@ -2181,9 +2184,32 @@ BmRegisterBootManagerMenu (
   EFI_DEVICE_PATH_PROTOCOL   *DevicePath;
   EFI_LOADED_IMAGE_PROTOCOL  *LoadedImage;
   MEDIA_FW_VOL_FILEPATH_DEVICE_PATH  FileNode;
+  VOID   *Data;
+  UINTN  DataSize;
 
   Status = GetSectionFromFv (
  PcdGetPtr (PcdBootManagerMenuFile),
+ EFI_SECTION_PE32,
+ 0,
+ (VOID **) ,
+ 
+ );
+  if (Data != NULL) {
+FreePool (Data);
+  }
+  if (EFI_ERROR (Status)) {
+DEBUG ((EFI_D_ERROR, "[Bds]BootManagerMenu FFS section can not be found, 
skip its boot option registeration\n"));
+ZeroMem (BootOption, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION));
+BootOption->OptionNumber = LoadOptionNumberUnassigned;
+BootOption->OptionType   = LoadOptionTypeMax;
+return Status;
+  }
+
+  //
+  // Get BootManagerMenu application's description from EFI User Interface 
Section.
+  //
+  Status = GetSectionFromFv (
+ PcdGetPtr (PcdBootManagerMenuFile),