Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: Put BootMenu at the end of BootOrder

[Wang, Sunny (HPS SW)]

Yeah, the problem is that always adding the Boot Menu to the top of BootOrder 
causes confusion to the users who manipulate the Boot Order under OS. Also, we 
can't find any reason why the Boot Menu needs to be always added to the top of 
the Boot Order even if we check the change history and emails. 
 
Reviewed-by: Sunny Wang 


-Original Message-
From: Li, Walon  
Sent: Friday, February 19, 2021 5:40 PM
To: devel@edk2.groups.io
Cc: Li, Walon ; Wang, Sunny (HPS SW) ; 
ler...@redhat.com; ray...@intel.com; hao.a...@intel.com; 
gaolim...@byosoft.com.cn
Subject: [PATCH v2] MdeModulePkg/UefiBootManagerLib: Put BootMenu at the end of 
BootOrder

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3135

When Boot Menu does not exist in the BootOrder, BmRegisterBootManagerMenu will 
create one into list. However, it should be put at the "end" of BootOrder 
instead of "start" of BootOrder. Replace 0 by -1 to adjust order of load 
options.

Signed-off-by: Walon Li 
---
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c 
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index aff620ad52..6cc34d29c0 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -3,7 +3,7 @@
  Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. Copyright (c) 
2011 - 2020, Intel Corporation. All rights reserved.-(C) Copyright 
2015-2016 Hewlett Packard Enterprise Development LP+(C) Copyright 2015-2021 
Hewlett Packard Enterprise Development LP SPDX-License-Identifier: 
BSD-2-Clause-Patent  **/@@ -2505,7 +2505,7 @@ BmRegisterBootManagerMenu (
 EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); ); -  
return EfiBootManagerAddLoadOptionVariable (BootOption, 0);+  return 
EfiBootManagerAddLoadOptionVariable (BootOption, (UINTN) -1); }  /**-- 
2.23.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71931): https://edk2.groups.io/g/devel/message/71931
Mute This Topic: https://groups.io/mt/80751724/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery

[Wang, Sunny (HPS SW)]

Hi all, 

How about we signal ReadyToBoot ONLY for the default platform recovery option?  
The default platform recovery option here means the one created by the code 
below in BdsEntry(). 
  Status = EfiBootManagerInitializeLoadOption (
 ,
 LoadOptionNumberUnassigned,
 LoadOptionTypePlatformRecovery,
 LOAD_OPTION_ACTIVE,
 L"Default PlatformRecovery",
 FilePath,
 NULL,
 0
 );

In other words, we just need to slightly update Pete's patch as the following 
(adding the code below to EfiBootManagerProcessLoadOption()): 

+   if ((LoadOption->OptionType == LoadOptionTypePlatformRecovery) &&
  StrCmp (LoadOption ->Description, L"Default PlatformRecovery")) {
+//
+// Signal the EVT_SIGNAL_READY_TO_BOOT event when we are about to load and 
execute the boot option.
+//
+EfiSignalEventReadyToBoot ();

+//
+// Report Status Code to indicate ReadyToBoot was signalled  
+//  
+REPORT_STATUS_CODE (EFI_PROGRESS_CODE, (EFI_SOFTWARE_DXE_BS_DRIVER | 
EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
+  }

I think the existing platforms that have their platform-specific 
PlatformRecovery option may also do either of the following things to make the 
system have no chance to load the default platform recovery option because they 
do have a better way to recover the boot options:
1. Make their PlatformRecovery option have higher priority than the default 
platform recovery option (has a lower number () than the default platform 
recovery option) 
2. Remove the default platform recovery option. 
Therefore, if we only signal ReadyToBoot for the default platform recovery 
option, this may not affect the existing platforms because the code may never 
be run on these platforms. 





If the solution above doesn't work, I think the suggestion (Solution 2: adding 
a new application as a PlatformRecovery) I mentioned, in the beginning, can 
be re-considered. The suggestion (solution 2) is based on the thoughts below: 
 1.  I think that processing/evaluating the Boot can be interpreted as 
the code after the comment " 6. Load EFI boot option to ImageHandle" in 
EfiBootManagerBoot() because these code are similar to the code in 
EfiBootManagerProcessLoadOption(). Based on this, I think our current 
implementation is compliant with the description below in the UEFI spec. Of 
course, we can improve our implementation by moving the code for 
processing/evaluating the Boot from EfiBootManagerBoot() to 
EfiBootManagerProcessLoadOption() and make EfiBootManagerBoot() call 
EfiBootManagerProcessLoadOption(). 
  “After all SysPrep variables have been launched and exited, 
the platform shall notify EFI_EVENT_GROUP_READY_TO_BOOT event group and begin 
to evaluate Boot variables with Attributes set to LOAD_OPTION_CATEGORY_BOOT 
according to the order defined by BootOrder. The FilePathList of variables 
marked LOAD_OPTION_CATEGORY_BOOT shall not be evaluated prior to the completion 
of EFI_EVENT_GROUP_READY_TO_BOOT event group processing."
 2. Moreover, it looks like we want to process PlatformRecovery option 
in the same way as Boot (do more things like setting BootCurrent for 
PlatformRecovery). If so, I would still prefer to do what I suggest in the 
beginning to create a new application as a new PlatformRecovery option for 
generating and launching a boot option for the bootable image that is found by 
using a short-form File Path Media Device Path so that we won't run into other 
difficulties. At least, I already saw the difficulty of no connection between 
BootCurrent variable and PlatformRecovery variable. Of course, this 
application can be implemented without platform specific stuff, so it can be 
commonly used by all platforms that need to load a boot image discovered by 
using short-form File Path Media Device Path. 




Regards,
Sunny Wang

-Original Message-
From: Laszlo Ersek  
Sent: Wednesday, February 17, 2021 10:26 PM
To: Pete Batard ; Leif Lindholm ; 
devel@edk2.groups.io
Cc: Samer El-Haj-Mahmoud ; Andrei Warkentin 
(awarken...@vmware.com) ; Wang, Sunny (HPS SW) 
; zhichao@intel.com; ray...@intel.com; Ard Biesheuvel 
; Andrew Fish ; Michael D Kinney 
; Jian J Wang ; Hao A Wu 

Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2][PATCH 1/1] 
MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery

On 02/17/21 13:18, Pete Batard wrote:
> Hi Leif,
> 
> Thanks for trying to resurrect this issue.
> 
> At this stage, and despite some initial pushback in the bugzilla 
> ticket, I believe we can all agree with the consensus that 
> UefiBootManagerLib is not in fact specs-compliant and therefore needs 
> to be fixed, one way or another, to make it specs-compliant.
> 
> My take on this is that, rather than propose a new patch

Re: [edk2-devel] [PATCH] UefiPayloadPkg/PlatformBootManager: Connect console after EndOfDxe

[Wang, Sunny (HPS SW)]

Sorry for the delay.
Thanks for clarifying and further checking this, Dong.
I think some platforms in https://github.com/tianocore/edk2-platforms would do 
this before EndOfDxe. 
Yeah, I agree with Dong. If UefiPayloadPkg doesn't support trusted console and 
may not support it in the future, I also think it's fine to have this change. 

Reviewed-by: Sunny Wang < sunnyw...@hpe.com>

Regards,
Sunny Wang

-Original Message-
From: Dong, Guo  
Sent: Saturday, February 6, 2021 7:16 AM
To: Patrick Rudolph ; Wang, Sunny (HPS SW) 

Cc: devel@edk2.groups.io; Park, Aiden ; You, Benjamin 
; philipp.deppenwi...@9elements.com; Ma, Maurice 

Subject: RE: [edk2-devel] [PATCH] UefiPayloadPkg/PlatformBootManager: Connect 
console after EndOfDxe


Trusted console is required for TCG Physical Presence and only trusted console 
could be connected before EndOfDxe. Since TCG Physical Presence is not enabled 
yet in the UefiPayloadPkg, I think it is ok to have this change. 

Reviewed-by: Guo Dong 

> -Original Message-
> From: Patrick Rudolph 
> Sent: Wednesday, February 3, 2021 3:26 AM
> To: Wang, Sunny (HPS SW) 
> Cc: devel@edk2.groups.io; Park, Aiden ; You, 
> Benjamin ; philipp.deppenwi...@9elements.com; 
> Ma, Maurice ; Dong, Guo 
> Subject: Re: [edk2-devel] [PATCH] UefiPayloadPkg/PlatformBootManager:
> Connect console after EndOfDxe
> 
> Hi Sunny,
> none of the other packages are doing this before EndOfDxe. And there's 
> no point in having trusted console as earlier as possible, as nothing 
> is displayed in PlatformBootManagerBeforeConsole().
> Please explain your use case. I don't see one here.
> 
> Kind Regards,
> Patrick Rudolph
> 
> On Wed, Feb 3, 2021 at 10:32 AM Wang, Sunny (HPS SW) 
>  wrote:
> >
> > Hi Patrick,
> >
> > I'm not familiar with UefiPayloadPkg. However, since we may want to 
> > enable
> the trusted console as earlier as possible, you may still need to keep 
> the
> PlatformConsoleInit() call at the beginning of
> PlatformBootManagerBeforeConsole() to support the platform that has 
> trusted/on-board Consoles.
> >
> > Regards,
> > Sunny Wang
> >
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of 
> > Patrick
> Rudolph
> > Sent: Tuesday, February 2, 2021 4:34 PM
> > To: devel@edk2.groups.io
> > Cc: aiden.p...@intel.com; benjamin@intel.com;
> philipp.deppenwi...@9elements.com; maurice...@intel.com; 
> guo.d...@intel.com
> > Subject: [edk2-devel] [PATCH] UefiPayloadPkg/PlatformBootManager:
> Connect console after EndOfDxe
> >
> > Currently the console is connected before EndOfDxe causing 
> > OptionsROMs to
> be loaded, but their drivers aren't used and thus no GOP is installed.
> >
> > To make use of 3rdparty OptionROMs connect the console after EndOfDxe.
> >
> > Tested on Intel CFL board using Nvidia Quadro GPU.
> >
> > Signed-off-by: Patrick Rudolph 
> > ---
> >  UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c 
> > |
> 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> > index c5c6af0abc..7fa3a048b7 100644
> > ---
> a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> > +++
> b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.
> > +++ c
> > @@ -157,8 +157,6 @@ PlatformBootManagerBeforeConsole (
> >EFI_INPUT_KEYDown;
> >EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
> >
> > -  PlatformConsoleInit ();
> > -
> >//
> >// Register ENTER as CONTINUE key
> >//
> > @@ -192,6 +190,8 @@ PlatformBootManagerBeforeConsole (
> >// Dispatch deferred images after EndOfDxe event and ReadyToLock
> installation.
> >//
> >EfiBootManagerDispatchDeferredImages ();
> > +
> > +  PlatformConsoleInit ();
> >  }
> >
> >  /**
> > --
> > 2.26.2
> >
> >
> >
> > 
> >
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71495): https://edk2.groups.io/g/devel/message/71495
Mute This Topic: https://groups.io/mt/80310284/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiPayloadPkg/PlatformBootManager: Connect console after EndOfDxe

[Wang, Sunny (HPS SW)]

Hi Patrick,

I'm not familiar with UefiPayloadPkg. However, since we may want to enable the 
trusted console as earlier as possible, you may still need to keep the 
PlatformConsoleInit() call at the beginning of 
PlatformBootManagerBeforeConsole() to support the platform that has 
trusted/on-board Consoles.   

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Patrick Rudolph
Sent: Tuesday, February 2, 2021 4:34 PM
To: devel@edk2.groups.io
Cc: aiden.p...@intel.com; benjamin@intel.com; 
philipp.deppenwi...@9elements.com; maurice...@intel.com; guo.d...@intel.com
Subject: [edk2-devel] [PATCH] UefiPayloadPkg/PlatformBootManager: Connect 
console after EndOfDxe

Currently the console is connected before EndOfDxe causing OptionsROMs to be 
loaded, but their drivers aren't used and thus no GOP is installed.

To make use of 3rdparty OptionROMs connect the console after EndOfDxe.

Tested on Intel CFL board using Nvidia Quadro GPU.

Signed-off-by: Patrick Rudolph 
---
 UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git 
a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c 
b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
index c5c6af0abc..7fa3a048b7 100644
--- a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
+++ b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.
+++ c
@@ -157,8 +157,6 @@ PlatformBootManagerBeforeConsole (
   EFI_INPUT_KEYDown;
   EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
 
-  PlatformConsoleInit ();
-
   //
   // Register ENTER as CONTINUE key
   //
@@ -192,6 +190,8 @@ PlatformBootManagerBeforeConsole (
   // Dispatch deferred images after EndOfDxe event and ReadyToLock 
installation.
   //
   EfiBootManagerDispatchDeferredImages ();
+
+  PlatformConsoleInit ();
 }
 
 /**
--
2.26.2








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71103): https://edk2.groups.io/g/devel/message/71103
Mute This Topic: https://groups.io/mt/80310284/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Open Design Meeting to discuss storage/configuration purge design

[Wang, Sunny (HPS SW)]

Thanks, Ray.

Hi All,
If you're working on option cards' UEFI driver (especially storage card UEFI 
driver), welcome to join the meeting and provide your valuable thoughts.

Regards,
Sunny Wang

From: Ni, Ray 
Sent: Thursday, September 17, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Wang, Sunny (HPS SW) ; Rothman, Michael A 
; Bi, Dandan ; Wu, Hao A 
; Dong, Eric 
Subject: Open Design Meeting to discuss storage/configuration purge design

All,
There will be one topic in this week's open design meeting discussing about 
storage/configuration purge design. (In case you think the meeting is cancelled 
as usually.)
Sunny will present and I invited Rothman Michael to help review the proposal. 
Michael is the expert of storage and HII.

Dandan, Hao and Eric, since you are the HII/Storage module maintainers. That 
would be great if you could join to provide the valuable thoughts.

NOTE:
There is one minor change: we need to use Webex for the meeting. I suggest you 
download Webex (www.webex.com/download<http://www.webex.com/download>) and try 
running it before the meeting.

Thanks,
Ray


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65361): https://edk2.groups.io/g/devel/message/65361
Mute This Topic: https://groups.io/mt/76905826/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg: Upon BootOption failure, Destroy RamDisk memory before RSC.

[Wang, Sunny (HPS SW)]

Looks good to me. Not sure if there was a reason to call BmDestroyRamDisk 
before ReportStatusCode. 
By the way, it is an interesting case that there is a custom boot option or 
application that needs the memory that was occupied by the RAM disk. It looks 
to me like the custom boot option or application would like to create the other 
RAM disk with allocating large memory for recovering the failed boot option. 

Reviewed-by: Sunny Wang 

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
KrishnadasX Veliyathuparambil Prakashan
Sent: Friday, June 19, 2020 10:40 AM
To: devel@edk2.groups.io
Cc: Gao, Zhichao ; Ni, Ray 
Subject: [edk2-devel] [PATCH] MdeModulePkg: Upon BootOption failure, Destroy 
RamDisk memory before RSC.

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2818 

For better memory management, re-ordered the DestroyRamDisk and 
ReportStatusCode calls inside the EfiBootManagerBoot() function.

This will help to clean the unused memory before reporting the failure status, 
so that OEMs can use RSC Listener to launch custom boot option or application 
for recovering the failed hard drive.

This change will help to ensure that the allocated pool of memory for the 
failed boot option is freed before executing OEM's RSC listener callback to 
handle every boot option failure.

Signed-off-by: KrishnadasX Veliyathuparambil Prakashan 

Cc: "Gao, Zhichao" 
Cc: "Ni, Ray" 
---
 .../Library/UefiBootManagerLib/BmBoot.c   | 28 ++-
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c 
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 540d169ec1..aff620ad52 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) 2019, NVIDIA CORPORATION. All rights reserved.
-Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.
+Copyright (c) 2011 - 2020, Intel Corporation. All rights reserved.
 (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -1903,17 +1903,17 @@ EfiBootManagerBoot (
 gBS->UnloadImage (ImageHandle);
   }
   //
-  // Report Status Code with the failure status to indicate that the 
failure to load boot option
-  //
-  BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);
-  BootOption->Status = Status;
-  //
   // Destroy the RAM disk
   //
   if (RamDiskDevicePath != NULL) {
 BmDestroyRamDisk (RamDiskDevicePath);
 FreePool (RamDiskDevicePath);
   }
+  //
+  // Report Status Code with the failure status to indicate that the 
failure to load boot option
+  //
+  BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);
+  BootOption->Status = Status;
   return;
 }
   }
@@ -1982,13 +1982,6 @@ EfiBootManagerBoot (
   Status = gBS->StartImage (ImageHandle, >ExitDataSize, 
>ExitData);
   DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Image Return Status = %r\n", Status));
   BootOption->Status = Status;
-  if (EFI_ERROR (Status)) {
-//
-// Report Status Code with the failure status to indicate that boot failure
-//
-BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);
-  }
-  PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
 
   //
   // Destroy the RAM disk
@@ -1998,6 +1991,15 @@ EfiBootManagerBoot (
 FreePool (RamDiskDevicePath);
   }
 
+  if (EFI_ERROR (Status)) {
+//
+// Report Status Code with the failure status to indicate that boot failure
+//
+BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);
+ }  PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) 
+ OptionNumber);
+
+
   //
   // Clear the Watchdog Timer after the image returns
   //
--
2.27.0.windows.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61554): https://edk2.groups.io/g/devel/message/61554
Mute This Topic: https://groups.io/mt/74978785/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery

[Wang, Sunny (HPS SW)]

Thanks for checking my comments, Pete. 

> Or is the "one more" the issue, meaning that it would get signaled more than 
> once?
[Sunny] Yeah, it would get signaled more than once if the PlatformRecovery 
option (a UEFI application) calls EfiBootManagerBoot() to launch the recovered 
boot option inside of the application.  

> I don't mind trying an alternative approach, but I don't understand how what 
> you describe would help. Can you please be more specific about what you have 
> in mind?
[Sunny] Sure. I added more information below. If it is still not clear enough, 
feel free to let me know.
 1. Create a UEFI application with the code to signal ReadyToBoot and pick 
/efi/boot/bootaa64.efi from either SD or USB and run it.
 2. Then, call EfiBootManagerInitializeLoadOption like the following in a 
DXE driver or other places before "Default PlatformRecovery" registration:  
  Status = EfiBootManagerInitializeLoadOption (
 ,
 0, 
-> 0 is the OptionNumber to let application be load before " 
Default PlatformRecovery" option
 LoadOptionTypePlatformRecovery,
 LOAD_OPTION_ACTIVE,
 L"Application for recovering boot options",
 FilePath,  
  -> FilePath is the Application's device path,
 NULL,
 0
 );


> My reasoning is that, if PlatformRecovery can execute a regular 
> bootloader like /efi/boot/boot.efi from installation media, then it 
> should go through the same kind of initialization that happens for a regular 
> boot option, and that should include signaling the ReadyToBoot event. 
[Sunny] Thanks for clarifying this, and Sorry about that I missed your cover 
letter for this patch.  I was just thinking that we may not really need to make 
this behavior change in both EDK II code and UEFI specification for solving the 
problem specific to the case that OS is loaded by "Default PlatformRecovery" 
option, and I'm also not sure if it is worth making this change to affect some 
of the system or BIOS vendors who have implemented their PlatformRecovery 
option. If the alternative approach I mentioned works for you, I think that 
would be an easier solution.

Regards,
Sunny Wang

-Original Message-
From: Pete Batard [mailto:p...@akeo.ie] 
Sent: Wednesday, June 17, 2020 6:59 PM
To: Wang, Sunny (HPS SW) ; devel@edk2.groups.io
Cc: zhichao@intel.com; ray...@intel.com; ard.biesheu...@arm.com; 
l...@nuviainc.com
Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: 
Signal ReadyToBoot on platform recovery

Hi Sunny, thanks for looking into this.

On 2020.06.17 09:16, Wang, Sunny (HPS SW) wrote:
> Hi Pete.
> 
> Since the EfiBootManagerProcessLoadOption is called by ProcessLoadOptions as 
> well, your change would also cause some unexpected behavior like:
> 1. Signal one more ReadyToBoot for the PlatformRecovery option which is an 
> application that calls EfiBootManagerBoot() to launch its recovered boot 
> option.

I'm not sure I understand how this part is unwanted.

The point of this patch is to ensure that ReadyToBoot is signalled for the 
PlatformRecovery option, so isn't what you describe above exactly what we want?

Or is the "one more" the issue, meaning that it would get signalled more than 
once?


> 2. Signal ReadyToBoot for SysPrep or Driver that is not really a 
> "boot" option.

Yes, I've been wondering about that, because BdsEntry.c's ProcessLoadOptions(), 
which calls EfiBootManagerProcessLoadOption(),
mentions that it will load will load and start every Driver, SysPrep or 
PlatformRecovery. But the comment about the while() loop in 
EfiBootManagerProcessLoadOption() only mentions PlatformRecovery.

If needed, I guess we could amend the patch to detect the type of option and 
only signal ReadyToBoot for PlatformRecovery.

> To solve your problem, creating a PlatformRecovery option with the smallest 
> option number and using it instead of default one (with short-form File Path 
> Media Device Path) looks like a simpler solution.

I don't mind trying an alternative approach, but I don't understand how what 
you describe would help. Can you please be more specific about what you have in 
mind?

Our main issue here is that we must have ReadyToBoot signalled so that the 
ReadyToBoot() function callback from EmbeddedPkg/Drivers/ConsolePrefDxe gets 
executed in order for the boot loader invoked from PlatformRecovery  to use 
a properly initialized graphical console. So I'm not sure I quite get how 
switching from one PlatformRecovery option to another would improve things.

If it helps, here is what we currently default to, in terms of boot o

Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery

[Wang, Sunny (HPS SW)]

Hi Pete. 

Since the EfiBootManagerProcessLoadOption is called by ProcessLoadOptions as 
well, your change would also cause some unexpected behavior like:
1. Signal one more ReadyToBoot for the PlatformRecovery option which is an 
application that calls EfiBootManagerBoot() to launch its recovered boot option.
2. Signal ReadyToBoot for SysPrep or Driver that is not really a "boot" 
option. 

To solve your problem, creating a PlatformRecovery option with the smallest 
option number and using it instead of default one (with short-form File Path 
Media Device Path) looks like a simpler solution.

By the way, I also checked the UEFI specification. It looks making sense to 
only signal ReadyToBoot for boot option (Boot). Therefore, your change may 
also require specification change.

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Pete 
Batard
Sent: Tuesday, June 16, 2020 5:56 PM
To: devel@edk2.groups.io
Cc: zhichao@intel.com; ray...@intel.com; ard.biesheu...@arm.com; 
l...@nuviainc.com
Subject: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal 
ReadyToBoot on platform recovery

Currently, the ReadyToBoot event is only signaled when a formal Boot Manager 
option is executed (in BmBoot.c -> EfiBootManagerBoot ()).

However, with the introduction of Platform Recovery in UEFI 2.5, which may lead 
to the execution of a boot loader that has similar requirements to a regular 
one, yet is not launched as a Boot Manager option, it also becomes necessary to 
signal ReadyToBoot when a Platform Recovery boot loader runs.

Especially, this can be critical to ensuring that the graphical console is 
actually usable during platform recovery, as some platforms do rely on the 
ConsolePrefDxe driver, which only performs console initialization after 
ReadyToBoot is triggered.

This patch fixes that behaviour by calling EfiSignalEventReadyToBoot () in 
EfiBootManagerProcessLoadOption (), which is the function that sets up the 
platform recovery boot process.

Signed-off-by: Pete Batard 
---
 MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c 
b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
index 89372b3b97b8..117f1f5b124c 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
@@ -1376,6 +1376,15 @@ EfiBootManagerProcessLoadOption (
 return EFI_SUCCESS;
   }
 
+  //
+  // Signal the EVT_SIGNAL_READY_TO_BOOT event when we are about to load and 
execute the boot option.
+  //
+  EfiSignalEventReadyToBoot ();
+  //
+  // Report Status Code to indicate ReadyToBoot was signalled  //
+ REPORT_STATUS_CODE (EFI_PROGRESS_CODE, (EFI_SOFTWARE_DXE_BS_DRIVER | 
+ EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
+
   //
   // Load and start the load option.
   //
--
2.21.0.windows.1





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61389): https://edk2.groups.io/g/devel/message/61389
Mute This Topic: https://groups.io/mt/74912987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] TianoCore Community Design Meeting Minutes - Feb 21, 2020

[Wang, Sunny (HPS SW)]

Hi Mike, Sean, and Ray, 

A BIG thanks to you guys. It was really good to have the EDK2 design meetings 
to talk to you guys. I got a lot of valuable feedback from you guys. The 
following are what I got from today's meeting and the follow-up I will do. Feel 
free to let me know anything I missed or misunderstood. 
1. Use SystemPrep and lock them to protect the boot priority of the 
target boot options. This is really a good idea. I will further check this. 
2. Partially lock - No real use case at this moment. The only real use case 
is BootOrder. Therefore, I may not work on this until we find a real case.
3. For protecting other critical variables, locking them won't cause any 
problem with OS, so let's use variable lock (variable policy) for them.
4. For recovering other critical variables, we can have a platform code to 
check and recover them at a specific timing before locking them. If you guys 
think adding the recovery mechanism into the variable policy is a good idea, 
let me know. I will look into this.
5. As for the new variable attribute bit, we don't need it because of no 
use case at this moment either. OS can check SystemPrep instead and show 
the message to users.

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Wang, 
Sunny (HPS SW)
Sent: Wednesday, March 4, 2020 3:53 PM
To: devel@edk2.groups.io; ray...@intel.com; Sean Brogan 
; Kinney, Michael D 
Cc: Wei, Kent (HPS SW) ; Spottswood, Jason 
; Wang, Sunny (HPS SW) 
Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - Feb 21, 
2020

Sure!

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Wednesday, March 4, 2020 3:26 PM
To: devel@edk2.groups.io; Wang, Sunny (HPS SW) ; Sean Brogan 
; Kinney, Michael D 
Cc: Wei, Kent (HPS SW) ; Spottswood, Jason 

Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - Feb 21, 
2020

Sunny,
Let's discuss in this week's meeting to see whether the below enhancement 
proposal can be aligned first.

Thanks,
Ray

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Wang, 
> Sunny (HPS SW)
> Sent: Wednesday, March 4, 2020 12:12 PM
> To: devel@edk2.groups.io; Ni, Ray ; Sean Brogan 
> ; Kinney, Michael D 
> 
> Cc: Wang, Sunny (HPS SW) ; Wei, Kent (HPS SW) 
> ; Spottswood, Jason 
> Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - 
> Feb 21, 2020
> 
> Sorry for not making any progress since last meeting.
> Sure! I will work on enhancing the variable policy to support partial 
> protection and recovery. However, the update will be late because I need to 
> first deal with other urgent stuff.
> By the way, thanks for giving a lot of valuable comments at our 
> offline discussion, Ray.  :)
> 
> Regards,
> Sunny Wang
> 
> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Ni, Ray
> Sent: Wednesday, March 4, 2020 11:46 AM
> To: Sean Brogan ; Kinney, Michael D 
> ; Wang, Sunny (HPS SW) 
> Cc: Ni, Ray ; devel@edk2.groups.io
> Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - 
> Feb 21, 2020
> 
> Variable policy works well on protecting a whole variable.
> But the BootOrder in Sunny's case may require a partial protection, 
> which means portion of the variable buffer needs to be read-only.
> Today's variable policy proposal doesn't take this into consideration.
> If we could enhance variable policy to support partial protection, 
> @Sunny can you please check whether it can meet your requirement?
> 
> The enhancement I think of is: Introduce two fields to the policy structure 
> Offset and Length.
> Offset (-1) indicates a whole variable protection.
> Offset (>= 0) indicates a partial variable protection and the protection 
> range starts from Offset with Length bytes.
> 
> This enhancement is also useful when some policy fields inside a big policy 
> structure needs to be read-only.
> Protecting multiple discontinuous ranges of  a variable can be 
> achieved by adding multiple policy entries with different Offset/Length.
> 
> 
> Thanks,
> Ray
> 
> > -Original Message-
> > From: annou...@edk2.groups.io  On Behalf Of 
> > Ni, Ray
> > Sent: Friday, February 21, 2020 5:17 PM
> > To: annou...@edk2.groups.io
> > Subject: [edk2-announce] TianoCore Community Design Meeting Minutes
> > - Feb 21, 2020
> >
> > OPEN:
> >   Today's meeting is using Zoom because of the long latency using BlueJeans.
> >   The URL to join meeting is changed. Make sure to check 
> > https://edk2.groups.io/g/devel/calendarfor details.
> >   We will try Zoom for next meeting as w

Re: [edk2-devel] TianoCore Community Design Meeting Minutes - Feb 21, 2020

[Wang, Sunny (HPS SW)]

Sure!

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Wednesday, March 4, 2020 3:26 PM
To: devel@edk2.groups.io; Wang, Sunny (HPS SW) ; Sean Brogan 
; Kinney, Michael D 
Cc: Wei, Kent (HPS SW) ; Spottswood, Jason 

Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - Feb 21, 
2020

Sunny,
Let's discuss in this week's meeting to see whether the below enhancement 
proposal can be aligned first.

Thanks,
Ray

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Wang, 
> Sunny (HPS SW)
> Sent: Wednesday, March 4, 2020 12:12 PM
> To: devel@edk2.groups.io; Ni, Ray ; Sean Brogan 
> ; Kinney, Michael D 
> 
> Cc: Wang, Sunny (HPS SW) ; Wei, Kent (HPS SW) 
> ; Spottswood, Jason 
> Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - 
> Feb 21, 2020
> 
> Sorry for not making any progress since last meeting.
> Sure! I will work on enhancing the variable policy to support partial 
> protection and recovery. However, the update will be late because I need to 
> first deal with other urgent stuff.
> By the way, thanks for giving a lot of valuable comments at our 
> offline discussion, Ray.  :)
> 
> Regards,
> Sunny Wang
> 
> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Ni, Ray
> Sent: Wednesday, March 4, 2020 11:46 AM
> To: Sean Brogan ; Kinney, Michael D 
> ; Wang, Sunny (HPS SW) 
> Cc: Ni, Ray ; devel@edk2.groups.io
> Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - 
> Feb 21, 2020
> 
> Variable policy works well on protecting a whole variable.
> But the BootOrder in Sunny's case may require a partial protection, 
> which means portion of the variable buffer needs to be read-only.
> Today's variable policy proposal doesn't take this into consideration.
> If we could enhance variable policy to support partial protection, 
> @Sunny can you please check whether it can meet your requirement?
> 
> The enhancement I think of is: Introduce two fields to the policy structure 
> Offset and Length.
> Offset (-1) indicates a whole variable protection.
> Offset (>= 0) indicates a partial variable protection and the protection 
> range starts from Offset with Length bytes.
> 
> This enhancement is also useful when some policy fields inside a big policy 
> structure needs to be read-only.
> Protecting multiple discontinuous ranges of  a variable can be 
> achieved by adding multiple policy entries with different Offset/Length.
> 
> 
> Thanks,
> Ray
> 
> > -Original Message-
> > From: annou...@edk2.groups.io  On Behalf Of 
> > Ni, Ray
> > Sent: Friday, February 21, 2020 5:17 PM
> > To: annou...@edk2.groups.io
> > Subject: [edk2-announce] TianoCore Community Design Meeting Minutes 
> > - Feb 21, 2020
> >
> > OPEN:
> >   Today's meeting is using Zoom because of the long latency using BlueJeans.
> >   The URL to join meeting is changed. Make sure to check 
> > https://edk2.groups.io/g/devel/calendar   for details.
> >   We will try Zoom for next meeting as well. If everything is good, we will 
> > continue to use Zoom.
> >
> > 1. Platform Libraries for Supporting UEFI Variable Resiliency (HPE)
> > Presenter: Sunny Wang
> > Slides:
> > INVALID URI REMOVED
> > devel_files_Designs_2020_0221_Platform-2520Libraries-2520for-2520Sup
> > po 
> > rting-2520UEFI-2520Variable-25=DwIFAg=C5b8zRQO1miGmBeVZ2LFWg=Z
> > 9c 
> > LgEMdGZCI1_R0bW6KqOAGzCXLJUR24f8N3205AYw=U5okqrn3H585vxU4GwALnIBwi
> > 0s 0dQ0hYIDuFj2z-4Y=oG0quXKBZu3XD7Drm04CsF445C8kfOGOJGzeqACJxAA=
> > 20Resiliency.pdf
> >
> > Problem: Support UEFI variable resiliency to compliant to security 
> > related guidelines and requirements. #page 2
> >
> > Locking BootOrder causes issues in OSes which is not acceptable.
> > EDKII is lack of interfaces for adding platform variable protection.
> > Today's presentation is to propose a solution.
> > Basic rule of how variable resiliency manages BootOrder changes: 
> > #5-#6
> > - Put down untrusted changes
> > - Keep trusted changes
> >
> > @Mike: Where is the reference data stored?
> > @Sunny: In BMC.
> >
> > 
> > @Mike: Would like to see a small enhancement in variable policy protocol 
> > proposed by Microsoft to meet your case.
> > @Sunny: I checked the variable policy proposal by Microsoft. Using that 
> > might be complicated.
> > @Sean: We (Microsoft) have looked this. Variable hook in DXE phase 
> > not in SMM is a security hole. Don't like the way of managing 
> &g

Re: [edk2-devel] TianoCore Community Design Meeting Minutes - Feb 21, 2020

[Wang, Sunny (HPS SW)]

Sorry for not making any progress since last meeting.
Sure! I will work on enhancing the variable policy to support partial 
protection and recovery. However, the update will be late because I need to 
first deal with other urgent stuff.
By the way, thanks for giving a lot of valuable comments at our offline 
discussion, Ray.  :)

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Wednesday, March 4, 2020 11:46 AM
To: Sean Brogan ; Kinney, Michael D 
; Wang, Sunny (HPS SW) 
Cc: Ni, Ray ; devel@edk2.groups.io
Subject: Re: [edk2-devel] TianoCore Community Design Meeting Minutes - Feb 21, 
2020

Variable policy works well on protecting a whole variable.
But the BootOrder in Sunny's case may require a partial protection, which means 
portion of the variable buffer needs to be read-only.
Today's variable policy proposal doesn't take this into consideration.
If we could enhance variable policy to support partial protection, @Sunny can 
you please check whether it can meet your requirement?

The enhancement I think of is: Introduce two fields to the policy structure 
Offset and Length.
Offset (-1) indicates a whole variable protection.
Offset (>= 0) indicates a partial variable protection and the protection range 
starts from Offset with Length bytes.

This enhancement is also useful when some policy fields inside a big policy 
structure needs to be read-only.
Protecting multiple discontinuous ranges of  a variable can be achieved by 
adding multiple policy entries with different Offset/Length.


Thanks,
Ray

> -Original Message-
> From: annou...@edk2.groups.io  On Behalf Of 
> Ni, Ray
> Sent: Friday, February 21, 2020 5:17 PM
> To: annou...@edk2.groups.io
> Subject: [edk2-announce] TianoCore Community Design Meeting Minutes - 
> Feb 21, 2020
> 
> OPEN:
>   Today's meeting is using Zoom because of the long latency using BlueJeans.
>   The URL to join meeting is changed. Make sure to check 
> https://edk2.groups.io/g/devel/calendar  for details.
>   We will try Zoom for next meeting as well. If everything is good, we will 
> continue to use Zoom.
> 
> 1. Platform Libraries for Supporting UEFI Variable Resiliency (HPE)
> Presenter: Sunny Wang
> Slides:
> INVALID URI REMOVED
> devel_files_Designs_2020_0221_Platform-2520Libraries-2520for-2520Suppo
> rting-2520UEFI-2520Variable-25=DwIFAg=C5b8zRQO1miGmBeVZ2LFWg=Z9c
> LgEMdGZCI1_R0bW6KqOAGzCXLJUR24f8N3205AYw=U5okqrn3H585vxU4GwALnIBwi0s
> 0dQ0hYIDuFj2z-4Y=oG0quXKBZu3XD7Drm04CsF445C8kfOGOJGzeqACJxAA=
> 20Resiliency.pdf
> 
> Problem: Support UEFI variable resiliency to compliant to security 
> related guidelines and requirements. #page 2
> 
> Locking BootOrder causes issues in OSes which is not acceptable.
> EDKII is lack of interfaces for adding platform variable protection.
> Today's presentation is to propose a solution.
> Basic rule of how variable resiliency manages BootOrder changes: #5-#6
> - Put down untrusted changes
> - Keep trusted changes
> 
> @Mike: Where is the reference data stored?
> @Sunny: In BMC.
> 
> 
> @Mike: Would like to see a small enhancement in variable policy protocol 
> proposed by Microsoft to meet your case.
> @Sunny: I checked the variable policy proposal by Microsoft. Using that might 
> be complicated.
> @Sean: We (Microsoft) have looked this. Variable hook in DXE phase not 
> in SMM is a security hole. Don't like the way of managing BootOrder by 
> allowing OS to change BootOrder and reverting. Boot may contain critical 
> data for OS and reverting that may cause troubles.
> @Sunny: I cannot think of solutions for OS runtime change.
> 
> 
> @Mike: I would break the big problem to 3 smaller ones:
>1. variable data corruption
> It requires a way to detect corruption and recovery.
>2. critical platform variables
> It usually requires a lock mechanism and variable policy proposal is 
> more general for this protection.
>3. UEFI variables with multiple producers
> How to protect them could be a topic for USWG.
> @Sean: The scope of the problem discussed in this presentation is 
> huge. Can a platform module run at a different point of time to manage the 
> variable storage instead of using hook way?
> @Sunny: BootOrder is just one of the variables that need protection.
> 
> 
> @Mike: Using a separate platform module might be better because it 
> will also check the variables not changed by firmware.
> @Sean: PEI modules may access the wrong data modified by untrusted entity.
> @Ray: Is the protection based on not just the variable GUID/name, but also 
> who requests the change?
> @Sunny: Yes. Following sides (#page 10+) will talk about protection from 
> non-trusted entity.
> @Ray: Let's move to email 

Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol

[Wang, Sunny (HPS SW)]

Hi Ashish, 

I was busy with some other stuff and forgot to reply to you. Sorry about that.
I was just thinking about adding more hooks would be easier for us to add 
platform code. However, it looks like your preference is to have only one hook. 
I'm also fine with this. 
Moreover, your solutions look workable to me. I will give them a try. Thanks! 

Regards,
Sunny Wang

-Original Message-
From: Ashish Singhal [mailto:ashishsin...@nvidia.com] 
Sent: Tuesday, December 24, 2019 12:48 PM
To: Wang, Sunny (HPS SW) ; Ni, Ray ; 
devel@edk2.groups.io; Wang, Jian J ; Wu, Hao A 
; Gao, Zhichao ; Kinney, Michael D 
; 'Andrew Fish (af...@apple.com)' 
Cc: Spottswood, Jason 
Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol

Hi Sunny,

For the 3 use cases you suggested, please let me know if you think I am wrong.

1. RefreshAllBootOptions can refresh boot options which are auto created by 
BmEnumerateBootOptions as well as NV boot options in the variable store. 
Platform implementation of RefreshAllBootOptions can have calls to platform 
specific other libraries/drivers that create more boot options.
2. In EfiBootManagerRefreshAllBootOption, BmEnumerateBootOptions is the only 
function that populates boot options and then validates/invalidates them as 
well as NV boot options. RefreshAllBootOptions can modify static-informational 
data or configuration data from the boot options created by 
BmEnumerateBootOptions as well as in NV store.
3. Solution for third use case can be derived by using a PCD which can be 
defaulted to tell code to call EfiBootManagerRefreshAllBootOption every time 
but can be overridden by platform to not call it from anywhere except BDS.

Thanks
Ashish

-Original Message-
From: Wang, Sunny (HPS SW) 
Sent: Monday, December 23, 2019 9:38 PM
To: Ni, Ray ; devel@edk2.groups.io; Ashish Singhal 
; Wang, Jian J ; Wu, Hao A 
; Gao, Zhichao ; Kinney, Michael D 
; 'Andrew Fish (af...@apple.com)' 
Cc: Spottswood, Jason ; Wang, Sunny (HPS SW) 

Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol

External email: Use caution opening links or attachments


Thanks for checking this, Ray.

Platform may want to:
 1. Refresh the static boot options (that are not created by 
BmEnumerateBootOptions) without a reboot.
 2. Update some other static-informational data or configuration data right 
after calling EfiBootManagerRefreshAllBootOption.
 3. Always skip calling EfiBootManagerRefreshAllBootOption for the cases like 
BOOT_ASSUMING_NO_CONFIGURATION_CHANGES.

I know these actions can be done by adding code to other places, but using 
hooks in EfiBootManagerRefreshAllBootOption would be an easier solution for the 
platform. We won't need to take care of all the 
EfiBootManagerRefreshAllBootOption callers. Therefore, If we don't have a 
concern about adding more hooks and want to give the platform more flexibility, 
we could add two more hooks (1 and 3) in the future to have three hooks as 
below:
  1. BeginOfRefreshAllBootOptions
  2. RefreshAllBootOptions or RefreshEnumeratedBootOptions
  3. EndOfRefreshAllBootOption

By the way, the current change looks good enough to me. In case Ashish or 
others are in urgent need of this code change, we can discuss my comments later 
in a separated email.

Regards,
Sunny Wang

-Original Message-
From: Ni, Ray [mailto:ray...@intel.com]
Sent: Tuesday, December 24, 2019 10:40 AM
To: Wang, Sunny (HPS SW) ; devel@edk2.groups.io; 
ashishsin...@nvidia.com; Wang, Jian J ; Wu, Hao A 
; Gao, Zhichao ; Kinney, Michael D 
; 'Andrew Fish (af...@apple.com)' 
Cc: Spottswood, Jason 
Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol



> -Original Message-
> From: Wang, Sunny (HPS SW) 
> Sent: Friday, December 20, 2019 7:29 PM
> To: devel@edk2.groups.io; ashishsin...@nvidia.com; Ni, Ray 
> ; Wang, Jian J ; Wu, Hao A 
> ; Gao, Zhichao ; Kinney, 
> Michael D ; 'Andrew Fish (af...@apple.com)'
> 
> Cc: Spottswood, Jason ; Wang, Sunny (HPS
> SW) 
> Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform 
> Boot Manager Protocol
>
> Good point. The way you used is more robust. It can cover a mistake in 
> function's error handling. Thanks for clarifying this, Ashish.
>
> In addition, the other naming suggestion just comes to mind. How about 
> we rename the function to a more generic one (based on location) like 
> AfterEnumerateBootOptions or a more specific one like 
> RefreshEnumeratedBootOptions? In the future, we may add the other hook 
> function in the EfiBootManagerRefreshAllBootOption to deal with the 
> boot options that are not created by BmEnumerateBootOptions. In this 
> case (two hook functions in EfiBootManagerRefreshAllBootOption), the 
> original function name "RefreshAllBootOptions" may cause some confusion.

Sunny,
What else feasi

Re: [edk2-devel] [PATCH v7] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol

[Wang, Sunny (HPS SW)]

Looks good enough to me. Thanks for addressing my comments, Ashish.
Reviewed-by: Sunny Wang 

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ashish 
Singhal
Sent: Tuesday, December 24, 2019 10:58 AM
To: devel@edk2.groups.io; ray...@intel.com; jian.j.w...@intel.com; 
hao.a...@intel.com; zhichao@intel.com; michael.d.kin...@intel.com; 
af...@apple.com
Cc: Ashish Singhal 
Subject: [edk2-devel] [PATCH v7] MdeModulePkg: Add EDK2 Platform Boot Manager 
Protocol

Add edk2 platform boot manager protocol which would have platform specific 
refreshes to the auto enumerated as well as NV boot options for the platform.

Signed-off-by: Ashish Singhal 
---
 .../Include/Protocol/PlatformBootManager.h | 82 ++
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c   | 41 +--
 .../Library/UefiBootManagerLib/InternalBm.h|  2 +
 .../UefiBootManagerLib/UefiBootManagerLib.inf  |  2 +
 MdeModulePkg/MdeModulePkg.dec  |  4 ++
 5 files changed, 124 insertions(+), 7 deletions(-)  create mode 100644 
MdeModulePkg/Include/Protocol/PlatformBootManager.h

diff --git a/MdeModulePkg/Include/Protocol/PlatformBootManager.h 
b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
new file mode 100644
index 000..26b9ce4
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
@@ -0,0 +1,82 @@
+/** @file
+
+  Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
+#define __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
+
+#include 
+
+//
+// Platform Boot Manager Protocol GUID value // #define 
+EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_GUID \
+{ \
+  0xaa17add4, 0x756c, 0x460d, { 0x94, 0xb8, 0x43, 0x88, 0xd7, 0xfb, 0x3e, 
0x59 } \
+}
+
+//
+// Protocol interface structure
+//
+typedef struct _EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL 
+EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL;
+
+//
+// Revision The revision to which the protocol interface adheres.
+//  All future revisions must be backwards compatible.
+//  If a future version is not back wards compatible it is not the 
same GUID.
+//
+#define EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION 0x0001
+
+//
+// Function Prototypes
+//
+
+/*
+  This function allows platform to refresh all boot options specific to 
+the platform. Within
+  this function, platform can make modifications to the auto enumerated 
+platform boot options
+  as well as NV boot options.
+
+  @param[in const] BootOptions An array of auto enumerated 
platform boot options.
+   This array will be freed by caller 
upon successful
+   exit of this function and output 
array would be used.
+
+  @param[in const] BootOptionsCountThe number of elements in 
BootOptions.
+
+  @param[out]  UpdatedBootOptions  An array of boot options that have 
been customized
+   for the platform on top of input 
boot options. This
+   array would be allocated by 
REFRESH_ALL_BOOT_OPTIONS
+   and would be freed by caller after 
consuming it.
+
+  @param[out]  UpdatedBootOptionsCount The number of elements in 
UpdatedBootOptions.
+
+
+  @retval EFI_SUCCESS  Platform refresh to input 
BootOptions and
+   BootCount have been done.
+
+  @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+
+  @retval EFI_INVALID_PARAMETERInput is not correct.
+
+  @retval EFI_UNSUPPORTED  Platform specific overrides are not 
supported.
+*/
+typedef
+EFI_STATUS
+(EFIAPI *PLATFORM_BOOT_MANAGER_REFRESH_ALL_BOOT_OPTIONS) (
+  IN  CONST EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions,
+  IN  CONST UINTNBootOptionsCount,
+  OUT   EFI_BOOT_MANAGER_LOAD_OPTION **UpdatedBootOptions,
+  OUT   UINTN*UpdatedBootOptionsCount
+  );
+
+struct _EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL {
+  UINT64 Revision;
+  PLATFORM_BOOT_MANAGER_REFRESH_ALL_BOOT_OPTIONS RefreshAllBootOptions; 
+};
+
+extern EFI_GUID gEdkiiPlatformBootManagerProtocolGuid;
+
+#endif /* __PLATFORM_BOOT_MANAGER_PROTOCOL_H__ */
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c 
b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 760d764..62c5b2dc 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -1,6 +1,7 @@
 /** @file
   Library functions which relates with booting.
 
+Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
 Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.
 (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP
 

Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol

[Wang, Sunny (HPS SW)]

Thanks for checking this, Ray. 

Platform may want to: 
 1. Refresh the static boot options (that are not created by 
BmEnumerateBootOptions) without a reboot. 
 2. Update some other static-informational data or configuration data right 
after calling EfiBootManagerRefreshAllBootOption. 
 3. Always skip calling EfiBootManagerRefreshAllBootOption for the cases like 
BOOT_ASSUMING_NO_CONFIGURATION_CHANGES.

I know these actions can be done by adding code to other places, but using 
hooks in EfiBootManagerRefreshAllBootOption would be an easier solution for the 
platform. We won't need to take care of all the 
EfiBootManagerRefreshAllBootOption callers. Therefore, If we don't have a 
concern about adding more hooks and want to give the platform more flexibility, 
we could add two more hooks (1 and 3) in the future to have three hooks as 
below: 
  1. BeginOfRefreshAllBootOptions
  2. RefreshAllBootOptions or RefreshEnumeratedBootOptions
  3. EndOfRefreshAllBootOption

By the way, the current change looks good enough to me. In case Ashish or 
others are in urgent need of this code change, we can discuss my comments later 
in a separated email.

Regards,
Sunny Wang

-Original Message-
From: Ni, Ray [mailto:ray...@intel.com] 
Sent: Tuesday, December 24, 2019 10:40 AM
To: Wang, Sunny (HPS SW) ; devel@edk2.groups.io; 
ashishsin...@nvidia.com; Wang, Jian J ; Wu, Hao A 
; Gao, Zhichao ; Kinney, Michael D 
; 'Andrew Fish (af...@apple.com)' 
Cc: Spottswood, Jason 
Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol



> -Original Message-
> From: Wang, Sunny (HPS SW) 
> Sent: Friday, December 20, 2019 7:29 PM
> To: devel@edk2.groups.io; ashishsin...@nvidia.com; Ni, Ray 
> ; Wang, Jian J ; Wu, Hao A 
> ; Gao, Zhichao ; Kinney, 
> Michael D ; 'Andrew Fish (af...@apple.com)'
> 
> Cc: Spottswood, Jason ; Wang, Sunny (HPS
> SW) 
> Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform 
> Boot Manager Protocol
> 
> Good point. The way you used is more robust. It can cover a mistake in 
> function's error handling. Thanks for clarifying this, Ashish.
> 
> In addition, the other naming suggestion just comes to mind. How about 
> we rename the function to a more generic one (based on location) like 
> AfterEnumerateBootOptions or a more specific one like 
> RefreshEnumeratedBootOptions? In the future, we may add the other hook 
> function in the EfiBootManagerRefreshAllBootOption to deal with the 
> boot options that are not created by BmEnumerateBootOptions. In this 
> case (two hook functions in EfiBootManagerRefreshAllBootOption), the 
> original function name "RefreshAllBootOptions" may cause some confusion.

Sunny,
What else feasibility do you think platform may require in future but this 
RefreshAllBootOptions cannot support?

Thanks,
Ray 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52530): https://edk2.groups.io/g/devel/message/52530
Mute This Topic: https://groups.io/mt/68802855/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol

[Wang, Sunny (HPS SW)]

Good point. The way you used is more robust. It can cover a mistake in 
function's error handling. Thanks for clarifying this, Ashish.

In addition, the other naming suggestion just comes to mind. How about we 
rename the function to a more generic one (based on location) like 
AfterEnumerateBootOptions or a more specific one like 
RefreshEnumeratedBootOptions? In the future, we may add the other hook function 
in the EfiBootManagerRefreshAllBootOption to deal with the boot options that 
are not created by BmEnumerateBootOptions. In this case (two hook functions in 
EfiBootManagerRefreshAllBootOption), the original function name 
"RefreshAllBootOptions" may cause some confusion.

Regards,
Sunny Wang
-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ashish 
Singhal
Sent: Friday, December 20, 2019 12:38 AM
To: Wang, Sunny (HPS SW) ; devel@edk2.groups.io; 
ray...@intel.com; Wang, Jian J ; Wu, Hao A 
; Gao, Zhichao ; Kinney, Michael D 
; 'Andrew Fish (af...@apple.com)' 
Cc: Spottswood, Jason 
Subject: Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol

Hi Sunny,

I thought about keeping the arguments the way you suggested but then decided 
against it so that the auto enumerated boot options list is not tampered with 
in case the function implementation hits an error in between. This way, if the 
function returns an error, we can use the list we passed in and continue boot.

I am OK with any suggested name changes.

Thanks
Ashish

-Original Message-----
From: Wang, Sunny (HPS SW) 
Sent: Thursday, December 19, 2019 7:23 AM
To: devel@edk2.groups.io; ray...@intel.com; Wang, Jian J 
; Wu, Hao A ; Gao, Zhichao 
; Kinney, Michael D ; 
'Andrew Fish (af...@apple.com)' 
Cc: Ashish Singhal ; Wang, Sunny (HPS SW) 
; Spottswood, Jason 
Subject: RE: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol

External email: Use caution opening links or attachments


1. Is it a proper solution to the problem?
Yes, it already solved my concern discussed in the other email.
2. Is the new protocol/function name proper?
Yes, but I'm not good at naming. We may need others' feedback. :) 3. Are 
the parameters in the function proper?
How about we only have two parameters as below to simplify this code change?
typedef
EFI_STATUS
(EFIAPI *REFRESH_ALL_BOOT_OPTIONS) (
IN  OUT EFI_BOOT_MANAGER_LOAD_OPTION **BootOptions,
IN  OUT UINTN   
*BootOptionsCount,
);

In addition, I have one more suggestion about the structure name inline.

Besides these two comments, everything else looks good to me.

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Thursday, December 19, 2019 9:59 AM
To: Wang, Jian J ; Wu, Hao A ; Gao, 
Zhichao ; Kinney, Michael D 
; 'Andrew Fish (af...@apple.com)' 
Cc: devel@edk2.groups.io; Ashish Singhal 
Subject: Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol

All,
The new EDKII Platform Boot Manager protocol provides a platform hook to solve 
below problem.
Can you please review and think about:
1. Is it a proper solution to the problem?
2. Is the new protocol/function name proper?
3. Are the parameters in the function proper?


**Problem:
   Platform requires certain BlockIo/SimpleFileSystem/LoadFile 
instances don't cause Boot created. It's a need of platform customization.

**Details:
   Boot for BlockIo/SimpleFileSystem/LoadFile are created by 
API EfiBootMangerRefreshAllBootOptions(). There are 2 places that call this API:
1.  Platform Boot Manager calls the API (usually in the full configuration 
boot path)
2.  UiApp calls the API when entering "Boot Manager" page and "Boot 
Maintenance Manager" page.

Platform can change Platform Boot Manager library to remove the unneeded 
Boot in case #1.
But platform has no way to remove the Boot created in case #2 .

Thanks,
Ray

> -Original Message-
> From: Ashish Singhal 
> Sent: Thursday, December 19, 2019 3:11 AM
> To: devel@edk2.groups.io; Wang, Jian J ; Wu, 
> Hao A ; Gao, Zhichao ; Ni, 
> Ray 
> Cc: Ashish Singhal 
> Subject: [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager 
> Protocol
>
> Add edk2 platform boot manager protocol which would have platform 
> specific refreshes to the auto enumerated as well as NV boot options 
> for the platform.
>
> Signed-off-by: Ashish Singhal 
> ---
>  .../Include/Protocol/PlatformBootManager.h | 82
> ++
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c   | 41 +--
>  .../Library/UefiBootManagerLib/InternalBm.h|  2 +
>  .../UefiBootManagerLib/UefiBootManagerLib.inf  |  2 +
>  MdeModulePkg/MdeModulePkg.dec  |  4 ++
>  5 f

Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager Protocol

[Wang, Sunny (HPS SW)]

1. Is it a proper solution to the problem?
Yes, it already solved my concern discussed in the other email.
2. Is the new protocol/function name proper?
Yes, but I'm not good at naming. We may need others' feedback. :)
3. Are the parameters in the function proper?
How about we only have two parameters as below to simplify this code 
change? 
typedef
EFI_STATUS
(EFIAPI *REFRESH_ALL_BOOT_OPTIONS) (
IN  OUT EFI_BOOT_MANAGER_LOAD_OPTION **BootOptions,
IN  OUT UINTN   
*BootOptionsCount,
);

In addition, I have one more suggestion about the structure name inline. 

Besides these two comments, everything else looks good to me. 

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni, Ray
Sent: Thursday, December 19, 2019 9:59 AM
To: Wang, Jian J ; Wu, Hao A ; Gao, 
Zhichao ; Kinney, Michael D 
; 'Andrew Fish (af...@apple.com)' 
Cc: devel@edk2.groups.io; Ashish Singhal 
Subject: Re: [edk2-devel] [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot 
Manager Protocol

All,
The new EDKII Platform Boot Manager protocol provides a platform hook to solve 
below problem.
Can you please review and think about:
1. Is it a proper solution to the problem?
2. Is the new protocol/function name proper?
3. Are the parameters in the function proper?


**Problem:
   Platform requires certain BlockIo/SimpleFileSystem/LoadFile 
instances don't cause Boot created. It's a need of platform customization.

**Details:
   Boot for BlockIo/SimpleFileSystem/LoadFile are created by 
API EfiBootMangerRefreshAllBootOptions(). There are 2 places that call this API:
1.  Platform Boot Manager calls the API (usually in the full configuration 
boot path)
2.  UiApp calls the API when entering "Boot Manager" page and "Boot 
Maintenance Manager" page.

Platform can change Platform Boot Manager library to remove the unneeded 
Boot in case #1.
But platform has no way to remove the Boot created in case #2 .

Thanks,
Ray

> -Original Message-
> From: Ashish Singhal 
> Sent: Thursday, December 19, 2019 3:11 AM
> To: devel@edk2.groups.io; Wang, Jian J ; Wu, 
> Hao A ; Gao, Zhichao ; Ni, 
> Ray 
> Cc: Ashish Singhal 
> Subject: [PATCH v4] MdeModulePkg: Add EDK2 Platform Boot Manager 
> Protocol
> 
> Add edk2 platform boot manager protocol which would have platform 
> specific refreshes to the auto enumerated as well as NV boot options 
> for the platform.
> 
> Signed-off-by: Ashish Singhal 
> ---
>  .../Include/Protocol/PlatformBootManager.h | 82
> ++
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c   | 41 +--
>  .../Library/UefiBootManagerLib/InternalBm.h|  2 +
>  .../UefiBootManagerLib/UefiBootManagerLib.inf  |  2 +
>  MdeModulePkg/MdeModulePkg.dec  |  4 ++
>  5 files changed, 124 insertions(+), 7 deletions(-)  create mode 
> 100644 MdeModulePkg/Include/Protocol/PlatformBootManager.h
> 
> diff --git a/MdeModulePkg/Include/Protocol/PlatformBootManager.h
> b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
> new file mode 100644
> index 000..ec32215
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
> @@ -0,0 +1,82 @@
> +/** @file
> +
> +  Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +#define __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +
> +#include 
> +
> +//
> +// Platform Boot Manager Protocol GUID value // #define 
> +EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_GUID \
> +{ \
> +  0xaa17add4, 0x756c, 0x460d, { 0x94, 0xb8, 0x43, 0x88, 0xd7, 
> +0xfb, 0x3e,
> 0x59 } \
> +}
> +
> +//
> +// Protocol interface structure
> +//
> +typedef struct _PLATFORM_BOOT_MANAGER_PROTOCOL
> PLATFORM_BOOT_MANAGER_PROTOCOL;

For being consistent with other EDKII protocols like 
EDKII_PLATFORM_LOGO_PROTOCOL , how about we update it to the following? 
typedef struct _EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL 
EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL;

> +
> +//
> +// Revision The revision to which the protocol interface adheres.
> +//  All future revisions must be backwards compatible.
> +//  If a future version is not back wards compatible it is not the 
> same
> GUID.
> +//
> +#define EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION
> 0x0001
> +
> +//
> +// Function Prototypes
> +//
> +
> +/*
> +  This function allows platform to refresh all boot options specific 
> +to the
> platform. Within
> +  this function, platform can make modifications to the auto 
> + enumerated
> platform boot options
> +  as well as NV boot options.
> +
> +  @param[in const] BootOptions An array of auto enumerated
> platform boot options.
> +   This array will be freed by 
> caller upon successful
> +   

Re: [edk2-discuss] [edk2-devel] [PATCH] Support skipping automatic BM enumeration

[Wang, Sunny (HPS SW)]

Sorry for the delay. Somehow I didn't catch the follow-up email. Thanks for 
checking my comments, Ray and Ashish. 
Yeah, agree. 2.b is better. I will review the code change. 

Regards,
Sunny Wang

-Original Message-
From: disc...@edk2.groups.io [mailto:disc...@edk2.groups.io] On Behalf Of 
Ashish Singhal
Sent: Wednesday, December 18, 2019 4:16 AM
To: Jeff Brasen ; Ni, Ray ; 
devel@edk2.groups.io; Laszlo Ersek ; af...@apple.com; 
disc...@edk2.groups.io
Cc: Wang, Jian J ; Wu, Hao A ; Gao, 
Zhichao ; Kinney, Michael D 
Subject: Re: [edk2-discuss] [edk2-devel] [PATCH] Support skipping automatic BM 
enumeration

I have submitted a patch based on 2.b as suggested by Ray. I am open to making 
changes in the structure of the protocol functions as well as the verbal 
description. Please let me know what you all think about it.

Thanks
Ashish

From: Jeff Brasen 
Sent: Thursday, December 12, 2019 10:52 AM
To: Ni, Ray ; devel@edk2.groups.io ; 
Laszlo Ersek ; af...@apple.com ; 
disc...@edk2.groups.io 
Cc: Ashish Singhal ; Wang, Jian J 
; Wu, Hao A ; Gao, Zhichao 
; Kinney, Michael D 
Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration

Thanks for the summary Ray, for the problem summary only thing I would add 
would be that platform also wants to create/modify boot options when full 
enumeration is requested as well.

For solutions I prefer option 2 as we don't have to put the same logic 
everywhere of how to modify the default enumerated list. And if we do that 2b 
makes more sense as then we don't have to modify all of the existing platforms.

I see two things the platform would need to do.

  1.  Update list created in BmEnumerateBootOptions
  2.  Delete any no longer valid platform created boot options


Thanks,

Jeff


From: Ni, Ray 
Sent: Wednesday, December 11, 2019 7:00 AM
To: Jeff Brasen ; devel@edk2.groups.io 
; Laszlo Ersek ; af...@apple.com 
; disc...@edk2.groups.io 
Cc: Ashish Singhal ; Wang, Jian J 
; Wu, Hao A ; Gao, Zhichao 
; Kinney, Michael D 
Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumeration

External email: Use caution opening links or attachments


Jeff,

Tom from AMD booked the meeting for SEV discussion months ago. I am afraid 
there is no time for this discussion.

Let's try to resolve it in mails.



Firstly, let me rephase the problem and your proposed solutions here 
(subjective + verb + objective). Sunny's input is also included. Hope Mike K 
and others can provide inputs.

Personally, I agree with 2.b. It helps us to gradually migrate the 
PlatformBootManagerLib to PlatformBootManager protocol. Protocol with Revision 
field helps to reduce the impact to old platforms with new APIs added.



**Problem:

   Platform requires certain BlockIo/SimpleFileSystem/LoadFile 
instances don't cause Boot created. It's a need of platform customization.



**Details:

   Boot for BlockIo/SimpleFileSystem/LoadFile are created by 
API EfiBootMangerRefreshAllBootOptions(). There are 2 places that call this API:

  1.  Platform Boot Manager calls the API (usually in the full configuration 
boot path)
  2.  UiApp calls the API when entering "Boot Manager" page and "Boot 
Maintenance Manager" page.



Platform can change Platform Boot Manager to remove the unneeded Boot in 
case #1.

But platform has no way to remove the Boot created in case #2 .



**Potential solutions:

  1.  Update UiApp
 *   Define a new PCD and a new event group.

If PCD is TRUE, UiApp signals the event. Event callback creates the Boot. 
Otherwise, EfiBootManagerRefreshAllBootOptions() is called.

 *   Add a new PlatformBootManagerLib API (implemented by platform).

UiApp calls the new API instead of EfiBootManagerRefreshAllBootOption. (need to 
coordinate rollout with updates to all platforms).

 *   Add a new protocol (implemented by platform).

UiApp calls the new protocol if it exists otherwise calls 
EfiBootManagerRefreshAllBootOption.



  1.  Update EfiBootManagerRefreshAllBootOptions()
 *   Add a new library class (implemented by platform).

EfiBootManagerRefreshAllBootOption() calls the new library class.

 *   Add a new PlatformBootManager protocol (implemented by platform).

EfiBootManagerRefreshAllBootOption() calls the new protocol if it exists.



Thanks,

Ray



From: Jeff Brasen 
Sent: Wednesday, December 11, 2019 4:46 AM
To: Ni, Ray ; devel@edk2.groups.io; Laszlo Ersek 
; af...@apple.com; disc...@edk2.groups.io
Cc: Ashish Singhal ; Wang, Jian J 
; Wu, Hao A ; Gao, Zhichao 
; Kinney, Michael D 
Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration



Can we discuss this at the design meeting this week (12/12)?



Thanks,

Jeff



From: Jeff Brasen
Sent: Thursday, November 14, 2019 10:04 AM
To: Ni, Ray mailto:ray...@intel.com>>; 
devel@edk2.groups.io 

Re: [edk2-discuss] [edk2-devel] [PATCH] Support skipping automatic BM enumeration

[Wang, Sunny (HPS SW)]

Hi all, 

Since I can't attend the design meeting (EMEA), I would like to add some use 
cases and a suggestion for your guys' reference before this topic being 
discussed:

We had similar needs (use cases) as Ashish like the following, so making this 
code change would be definitely good for system vendors like us. 
- Disabling a specific boot option like a PXE boot option for a 
specific NIC port.
- Hiding a partition that is used for special purpose rather than 
booting.   

If we don't have a strong reason to prevent creating a platform hook function 
(solution 5), I will prefer to use it because it is the simplest solution and 
can be easily extended and maintained. Also, solution 5 is our current 
implementation. We can even contribute our implementation to EDK2 if you guys 
need it. Moreover, if we finally choose solution 5, I will prefer to create a 
new platform library instead of adding a new function into the 
PlatformBootManagerLib, and let this new platform library only be called by 
UefiBootManagerLib. This would avoid a potential circular dependency problem.

For solutions 1, 2, 3, and 4, if I understand them correctly, they all require 
making changes in the EfiBootManagerRefreshAllBootOption callers. UiApp may not 
be the only application to call EfiBootManagerRefreshAllBootOption, so this may 
cause additional maintenance effort. However, if we still need to choose a 
solution other than solution 5, can we make the change directly in 
EfiBootManagerRefreshAllBootOption instead of UiApp? 

By the way, if we still need to discuss this further at a meeting, I will be at 
the other design meeting (APAC).

Regards,
Sunny Wang

-Original Message-
From: disc...@edk2.groups.io [mailto:disc...@edk2.groups.io] On Behalf Of Jeff 
Brasen
Sent: Wednesday, December 11, 2019 4:46 AM
To: Ni, Ray ; devel@edk2.groups.io; Laszlo Ersek 
; af...@apple.com; disc...@edk2.groups.io
Cc: Ashish Singhal ; Wang, Jian J 
; Wu, Hao A ; Gao, Zhichao 
; Kinney, Michael D 
Subject: Re: [edk2-discuss] [edk2-devel] [PATCH] Support skipping automatic BM 
enumeration

Can we discuss this at the design meeting this week (12/12)?


Thanks,

Jeff


From: Jeff Brasen
Sent: Thursday, November 14, 2019 10:04 AM
To: Ni, Ray ; devel@edk2.groups.io ; 
Laszlo Ersek ; af...@apple.com 
Cc: Ashish Singhal ; Wang, Jian J 
; Wu, Hao A ; Gao, Zhichao 
; Kinney, Michael D 
Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumeration


Yes, I think that would be good.



Summarizing everything in this thread



Problem: Platform needs to customize the boot options, this can be done for 
normal boot but the UiApp calls EfiBootManagerRefreshAllBootOption in a couple 
places.



Potential solutions:

1 - Define new PCD and event group if PCD is set true then signal event instead 
of calling EfiBootManagerRefreshAllBootOption in UiApp

2 - Add new function to boot manager library and replace call to 
EfiBootManagerRefreshAllBootOption in UiApp (need to coordinate rollout with 
updates to all platform.

3 - Add new protocol with new function, if supported call this otherwise call 
EfiBootManagerRefreshAllBootOption as is done now

4 - For 2/3 use  generic function so we don't need new APIs for future expansion

5 - Update EfiBootManagerRefreshAllBootOption to call platform specific 
function.



Thanks,
Jeff





From: Ni, Ray 
Sent: Wednesday, November 13, 2019 7:09 PM
To: devel@edk2.groups.io; Jeff Brasen ; Laszlo Ersek 
; af...@apple.com
Cc: Ashish Singhal ; Wang, Jian J 
; Wu, Hao A ; Gao, Zhichao 
; Kinney, Michael D 
Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumeration



Jeff,

I think it's a good topic that we could discuss in the open design meeting.

Are you ok to present the problem you have and discuss the potential solutions 
in that meeting?

https://github.com/tianocore/tianocore.github.io/wiki/Design-Meeting



Thanks,

Ray



From: devel@edk2.groups.io 
mailto:devel@edk2.groups.io>> On Behalf Of Jeff Brasen
Sent: Thursday, November 14, 2019 2:43 AM
To: Ni, Ray mailto:ray...@intel.com>>; 
devel@edk2.groups.io; Laszlo Ersek 
mailto:ler...@redhat.com>>; 
af...@apple.com
Cc: Ashish Singhal mailto:ashishsin...@nvidia.com>>; 
Wang, Jian J mailto:jian.j.w...@intel.com>>; Wu, Hao A 
mailto:hao.a...@intel.com>>; Gao, Zhichao 
mailto:zhichao@intel.com>>; Kinney, Michael D 
mailto:michael.d.kin...@intel.com>>
Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration



Thinking about this more I think we could do this with a PCD and a new group 
event without having to define any new function interfaces.



We could change UiApp and BootManagerMenuApp (and any others that are relevant) 
from



EfiBootManagerRefreshAllBootOption ();



to



if (FeaturePcdGet (PcdEventBasedRefreshAllBootOptionSupport) {

  EFI_EVENT Event;

  gBS->CreateEventEx ( 

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set RuntimeServicesSupported variable

[Wang, Sunny (HPS SW)]

Thanks for the clarification, Ray.

Regards,
Sunny Wang

-Original Message-
From: Ni, Ray [mailto:ray...@intel.com] 
Sent: Thursday, November 21, 2019 7:51 PM
To: devel@edk2.groups.io; Wang, Sunny (HPS SW) ; Gao, 
Zhichao ; Jeff Brasen ; 
edk2-de...@lists.01.org
Cc: Gao, Liming ; Kinney, Michael D 
; Wu, Hao A ; Spottswood, Jason 

Subject: RE: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
RuntimeServicesSupported variable
Importance: High

Sunny,
I am fine to put it to dynamic section if there are real requirements.
I thought if the platform doesn't support runtime services, it always doesn't 
support.

Thanks,
Ray

> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Wang, 
> Sunny (HPS SW)
> Sent: Thursday, November 21, 2019 7:27 PM
> To: devel@edk2.groups.io; Gao, Zhichao ; Ni, 
> Ray ; Jeff Brasen ; 
> edk2-de...@lists.01.org
> Cc: Gao, Liming ; Kinney, Michael D 
> ; Wu, Hao A ; 
> Spottswood, Jason ; Wang, Sunny (HPS SW) 
> 
> Subject: Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
> RuntimeServicesSupported variable
> 
> Hi Ray,
> 
> May I know why we need to put this PCD to [PcdsFixedAtBuild, 
> PcdsPatchableInModule] section only? If the reason is the security 
> concern, Locking the variable (value of PCD) at the EndOfDxe should be secure 
> enough. For the platforms that want to make it more secure (don't want the 
> PCD to be modified), they can override the PCD type in their .dsc file.
> I can imagine that there are still some use cases that need to modify 
> the PCD during boot. Can we put this PCD in [PcdsFixedAtBuild, 
> PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] to make it more flexible?
> 
> Regards,
> Sunny Wang
> 
> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Gao, Zhichao
> Sent: Thursday, November 21, 2019 2:12 PM
> To: Ni, Ray ; Jeff Brasen ; 
> edk2-de...@lists.01.org; devel@edk2.groups.io
> Cc: Gao, Liming ; Kinney, Michael D 
> ; Wu, Hao A 
> Subject: Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
> RuntimeServicesSupported variable
> 
> Agree with Ray, and we should update the uni file at the same time when add 
> the new pcd.
> 
> Thanks,
> Zhichao
> 
> > -Original Message-
> > From: Ni, Ray 
> > Sent: Thursday, November 21, 2019 11:13 AM
> > To: Jeff Brasen ; edk2-de...@lists.01.org; 
> > devel@edk2.groups.io
> > Cc: Gao, Liming ; Kinney, Michael D 
> > ; Wu, Hao A ; Gao, 
> > Zhichao 
> > Subject: RE: [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
> > RuntimeServicesSupported variable
> >
> > Jeff,
> > I suggest you add the PCD definition to MdePkg.dec because this PCD 
> > just maps to the spec defined variable RuntimeServicesSupported.
> >
> > And can you put this PCD to [PcdsFixedAtBuild, 
> > PcdsPatchableInModule] section only?
> >
> > Thanks,
> > Ray
> >
> > > -Original Message-
> > > From: Jeff Brasen 
> > > Sent: Saturday, November 16, 2019 1:43 AM
> > > To: edk2-de...@lists.01.org; devel@edk2.groups.io
> > > Cc: Jeff Brasen ; Gao, Liming 
> > > ; Kinney, Michael D 
> > > ; Wu, Hao A ; Ni, 
> > > Ray ; Gao, Zhichao 
> > > Subject: [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
> > > RuntimeServicesSupported variable
> > >
> > > Add support for initializing and setting the UEFI 2.8 global 
> > > variable RuntimeServicesSupported based on the value of a PCD.
> > >
> > > Signed-off-by: Jeff Brasen 
> > > ---
> > >  MdeModulePkg/MdeModulePkg.dec| 18 
> > >  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 + 
> > > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35
> > > +++-
> > >  3 files changed, 53 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/MdeModulePkg.dec 
> > > b/MdeModulePkg/MdeModulePkg.dec index 41b9e70..a1767e4 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -2003,6 +2003,24 @@
> > ># @Prompt Capsule On Disk relocation device path.
> > >
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdCodRelocationDevPath|{0xFF}|VOI
> > > D*|0x002f
> > >
> > > +  ## Bitmask of supported runtime services  #  BIT0  - 
> > > + GetTime #
> > > + BIT1  - SetTime  #  BIT2  - GetWakeupTime  #  BIT3  - 
> > > + SetWakeupTime #
> > > + BIT4  - GetVariable  #  BIT5  - GetNextVariableName  #  BIT6  - 
> > > + SetVariable  #  BIT7  - SetVirtualAddressMap  #  BIT8  - 

Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set RuntimeServicesSupported variable

[Wang, Sunny (HPS SW)]

Hi Ray, 

May I know why we need to put this PCD to [PcdsFixedAtBuild, 
PcdsPatchableInModule] section only? If the reason is the security concern, 
Locking the variable (value of PCD) at the EndOfDxe should be secure enough. 
For the platforms that want to make it more secure (don't want the PCD to be 
modified), they can override the PCD type in their .dsc file. 
I can imagine that there are still some use cases that need to modify the PCD 
during boot. Can we put this PCD in [PcdsFixedAtBuild, PcdsPatchableInModule, 
PcdsDynamic, PcdsDynamicEx] to make it more flexible? 

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, 
Zhichao
Sent: Thursday, November 21, 2019 2:12 PM
To: Ni, Ray ; Jeff Brasen ; 
edk2-de...@lists.01.org; devel@edk2.groups.io
Cc: Gao, Liming ; Kinney, Michael D 
; Wu, Hao A 
Subject: Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
RuntimeServicesSupported variable

Agree with Ray, and we should update the uni file at the same time when add the 
new pcd.

Thanks,
Zhichao

> -Original Message-
> From: Ni, Ray 
> Sent: Thursday, November 21, 2019 11:13 AM
> To: Jeff Brasen ; edk2-de...@lists.01.org; 
> devel@edk2.groups.io
> Cc: Gao, Liming ; Kinney, Michael D 
> ; Wu, Hao A ; Gao, 
> Zhichao 
> Subject: RE: [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
> RuntimeServicesSupported variable
> 
> Jeff,
> I suggest you add the PCD definition to MdePkg.dec because this PCD 
> just maps to the spec defined variable RuntimeServicesSupported.
> 
> And can you put this PCD to [PcdsFixedAtBuild, PcdsPatchableInModule] 
> section only?
> 
> Thanks,
> Ray
> 
> > -Original Message-
> > From: Jeff Brasen 
> > Sent: Saturday, November 16, 2019 1:43 AM
> > To: edk2-de...@lists.01.org; devel@edk2.groups.io
> > Cc: Jeff Brasen ; Gao, Liming 
> > ; Kinney, Michael D 
> > ; Wu, Hao A ; Ni, 
> > Ray ; Gao, Zhichao 
> > Subject: [PATCH 3/3] MdeModulePkg/BdsDxe: Set 
> > RuntimeServicesSupported variable
> >
> > Add support for initializing and setting the UEFI 2.8 global 
> > variable RuntimeServicesSupported based on the value of a PCD.
> >
> > Signed-off-by: Jeff Brasen 
> > ---
> >  MdeModulePkg/MdeModulePkg.dec| 18 
> >  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 + 
> > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35
> > +++-
> >  3 files changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec 
> > b/MdeModulePkg/MdeModulePkg.dec index 41b9e70..a1767e4 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -2003,6 +2003,24 @@
> ># @Prompt Capsule On Disk relocation device path.
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdCodRelocationDevPath|{0xFF}|VOI
> > D*|0x002f
> >
> > +  ## Bitmask of supported runtime services  #  BIT0  - GetTime
> > + #
> > + BIT1  - SetTime  #  BIT2  - GetWakeupTime  #  BIT3  - 
> > + SetWakeupTime #
> > + BIT4  - GetVariable  #  BIT5  - GetNextVariableName  #  BIT6  - 
> > + SetVariable  #  BIT7  - SetVirtualAddressMap  #  BIT8  - 
> > + ConvertPointer  #  BIT9  - GetNextHighMonotonicCount  #  BIT10 - 
> > + ResetSystem  #  BIT11 - UpdateCapsule  #  BIT12 - 
> > + QueryCapsuleCapabilites  #  BIT13 - QueryVariableInfo  # @Prompt 
> > + Supported Runtime services bitmask.
> > +
> > +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdRuntimeServicesSupported|0x3FFF
> > |UINT
> > + 16|0x0030
> > +
> >  [PcdsPatchableInModule]
> >## Specify memory size with page number for PEI code when
> >#  Loading Module at Fixed Address feature is enabled.
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > index 9310b4d..e4ba9be 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > @@ -97,6 +97,7 @@
> >gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed   ##
> > CONSUMES
> >gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleOnDiskSupport  ##
> > CONSUMES
> >gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport   ##
> > CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRuntimeServicesSupported
> > ## CONSUMES
> >
> >  [Depex]
> >TRUE
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > index d387dbe..16bc593 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > @@ -40,7 +40,8 @@ CHAR16  *mReadOnlyVariables[] = {
> >EFI_LANG_CODES_VARIABLE_NAME,
> >EFI_BOOT_OPTION_SUPPORT_VARIABLE_NAME,
> >EFI_HW_ERR_REC_SUPPORT_VARIABLE_NAME,
> > -  EFI_OS_INDICATIONS_SUPPORT_VARIABLE_NAME
> > +  EFI_OS_INDICATIONS_SUPPORT_VARIABLE_NAME,
> > +  EFI_RUNTIME_SERVICES_SUPPORTED_VARIABLE_NAME
> >};
> >
> >  CHAR16 *mBdsLoadOptionName[] = {
> > @@ -626,6 +627,33 @@ BdsFormalizeOSIndicationVariable (
> >
> >  

Re: [edk2-devel] Use a pcd to control PlatformRecovery

[Wang, Sunny (HPS SW)]

Sorry for the delay and thanks for checking my question, Ray.

Since the OS recovery and platform recovery options were added to UEFI 
specification at the same time, I thought we will also implement it and push 
the code regardless of the OS support. 

No, I didn't see a real need of using the OS recovery option. If my memory 
serves me well, I have only seen a need of using "One-Time" OS recovery, but it 
can be done by creating, adjusting, and removing a boot option in boot order. 
However, I can still imagine that the OS recovery option is needed for the use 
of "persistent" OS recovery. Therefore, if we don't have any concern about 
adding OS recovery option support, I think it is still better to push the code 
because the UEFI specification mentioned it. Add Peter. He may know more 
information about this.

By the way, I think if no one works on the task below, there will be no OS or 
OS tool supporting it. :)
   - https://github.com/rhboot/efibootmgr/issues/41


Regards,
Sunny Wang

-Original Message-
From: Ni, Ray [mailto:ray...@intel.com] 
Sent: Wednesday, October 16, 2019 4:43 PM
To: Wang, Sunny (HPS SW) ; devel@edk2.groups.io; Gao, 
Zhichao 
Cc: Wang, Jian J ; Wu, Hao A ; Zeng, 
Star ; Gao, Liming ; Sean Brogan 
; Michael Turner ; 
Bret Barkelew ; Li, Walon ; Wei, 
Kent (HPS SW) ; Spottswood, Jason 
Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
Importance: High

Sunny,
For the OS recovery question, I had the code change in 
https://github.com/niruiyu/edk2/tree/bds/osrecovery.

Due to there is no OS support for OS recovery yet, the code was not pushed.

Do you see any needs of the OS recovery?

Thanks,
Ray

> -Original Message-----
> From: Wang, Sunny (HPS SW) 
> Sent: Wednesday, October 16, 2019 3:42 PM
> To: devel@edk2.groups.io; Gao, Zhichao ; Ni, 
> Ray 
> Cc: Wang, Jian J ; Wu, Hao A 
> ; Zeng, Star ; Gao, Liming 
> ; Sean Brogan ; 
> Michael Turner ; Bret Barkelew 
> ; Li, Walon ; Wei, Kent 
> (HPS SW) ; Spottswood, Jason 
> ; Wang, Sunny (HPS SW) 
> Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> 
> Thanks for checking this, Zhichao and Ray.
> I just sent a patch to fix it with the subject " [edk2-devel] [PATCH]
> MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of 
> OsIndications"
> 
> Regards,
> Sunny Wang
> 
> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Gao, Zhichao
> Sent: Wednesday, October 16, 2019 1:57 PM
> To: Wang, Sunny (HPS SW) ; devel@edk2.groups.io; 
> Ni, Ray 
> Cc: Wang, Jian J ; Wu, Hao A 
> ; Zeng, Star ; Gao, Liming 
> ; Sean Brogan ; 
> Michael Turner ; Bret Barkelew 
> ; Li, Walon ; Wei, Kent 
> (HPS SW) 
> Subject: Re: [edk2-devel] Use a pcd to control PlatformRecovery
> Importance: High
> 
> 
> > -Original Message-
> > From: Gao, Zhichao
> > Sent: Wednesday, October 16, 2019 10:09 AM
> > To: 'Wang, Sunny (HPS SW)' ;
> devel@edk2.groups.io;
> > Ni, Ray 
> > Cc: Wang, Jian J ; Wu, Hao A 
> > ; Zeng, Star ; Gao, Liming 
> > ; Sean Brogan ; 
> > Michael Turner ; Bret Barkelew 
> > ; Li, Walon ; Wei,
> Kent
> > (HPS SW) 
> > Subject: RE: Use a pcd to control PlatformRecovery
> >
> > First of all, the patch didn't aim to change the other part of the 
> > boot flow except PlatformRecovery.
> >
> > Local variable PlatformRecovery is controlled by OsIndications 
> > variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is
> set,
> > the firmware should try to platform specific recovery. But that 
> > doesn't mean the platform must support the specific recovery. i.e.
> > local PlatformRecovery is controlling the boot flow and the Pcd just 
> > indicated whether the platform support recovery or not.
> > So, on my opinion, I don't agree with your change.
> 
> After discuss with Sunny and Ray, and refer to the spec section 3.4.1 
> and 3.4.2, the OsRecovery and PlatformRecovery should always be 
> operated regardless of the value of OsIndication variable if fail to 
> boot the BootOrder. I am wrong. We should change to use the 
> PcdPlatformRecoverySupport to control the PlatformRecovery. Please 
> help to send a patch to fix it. Thanks a lot.
> 
> >
> > Default Platform Recovery refer to the short file path to boot the OS.
> > If the firmware supports platform recovery, then *short file path* 
> > option would be one part of the PlatformRecovery#### in case there 
> > are no other boot options. If the firmware doesn't support platform 
> > recovery, we still need this default boot thru a short file path and 
> > it should not depend on the PlatformRecovery variable for the
> compatibili

Re: [edk2-devel] [PATCH] MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of OsIndications

[Wang, Sunny (HPS SW)]

Thanks for catching this, Laszlo. 
Thanks for the suggestion and information, Zhichao. Next time, I will 
definitely take care of this in the first place. 

Hi Jian and Hao, 
I forgot to add both of you in Cc. Sorry about that. I will send you guys a new 
patch with the updated commit message below. If you guys need me to resend an 
email to edk2-devel for the new patch, feel free to let me know.

===
MdeModulePkg/BdsDxe: Fix PlatformRecovery issue

For now, PlatformRecovery doesn't work if OsIndications variable 
doesn't exist, which is wrong.  
According to the UEFI specification section 3.4.1 and 3.4.2, if 
processing of BootOrder does not result in success, the OsRecovery
and PlatformRecovery options should still be processed regardless of
the existence of the OsIndications variable.  
Therefore, update the code to check PcdPlatformRecoverySupport instead 
of the value of OsIndications variable (PlatformRecovery) to fix
this issue.

Cc: Jian J Wang 
Cc: Hao Wu 
Cc: Ray Ni 
Cc: Zhichao Gao 
Cc: Walon Li 
Signed-off-by: Sunny Wang 
=

Regards,
Sunny Wang

-Original Message-
From: Gao, Zhichao [mailto:zhichao@intel.com] 
Sent: Wednesday, October 16, 2019 4:36 PM
To: devel@edk2.groups.io; ler...@redhat.com; Wang, Sunny (HPS SW) 

Cc: Ni, Ray ; Li, Walon 
Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/BdsDxe: Make PlatformRecovery 
work regardless of OsIndications
Importance: High

MdeModulePkg/BdsDxe: Do PlatformRecovery regardless of OsIndications

According to the UEFI specification section 3.4.1 and 3.4.2, the OsRecovery and 
PlatformRecovery options should still be processed regardless of the value of 
OsIndications variable if processing of BootOrder does not result in success.
Therefore, update the code to check PcdPlatformRecoverySupport instead of the 
value of OsIndications variable (PlatformRecovery).

I suggest to use the above title because of the length limitation. And I also 
break the commit message into serval lines.
Here is the development process for edk open source link: 
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process.
 That would help you to send a patch in a basic correct format.
With the commit massage issue addressed, Reviewed-by: Zhichao Gao 


Thanks,
Zhichao

> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Laszlo Ersek
> Sent: Wednesday, October 16, 2019 4:02 PM
> To: devel@edk2.groups.io; sunnyw...@hpe.com
> Cc: Ni, Ray ; Gao, Zhichao ; 
> Walon Li 
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/BdsDxe: Make 
> PlatformRecovery work regardless of OsIndications
> 
> On 10/16/19 09:40, Wang, Sunny (HPS SW) wrote:
> > According to the UEFI specification section 3.4.1 and 3.4.2, the 
> > OsRecovery
> and PlatformRecovery options should still be processed regardless of 
> the value of OsIndications variable if processing of BootOrder does 
> not result in success. Therefore, update the code to check 
> PcdPlatformRecoverySupport instead of the value of OsIndications variable 
> (PlatformRecovery).
> 
> Please wrap the commit message to 74 characters.
> 
> (Can be done on push, if the maintainer accepts the patch.)
> 
> Thanks
> Laszlo
> 
> >
> > Cc: Ray Ni 
> > Cc: Zhichao Gao 
> > Cc: Walon Li 
> > Signed-off-by: Sunny Wang 
> > ---
> >  MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > index d6ec31118c..d387dbe7ac 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > @@ -6,7 +6,7 @@
> >to enter BDS phase.
> >
> >  Copyright (c) 2004 - 2019, Intel Corporation. All rights 
> > reserved.
> > -(C) Copyright 2016 Hewlett Packard Enterprise Development LP
> > +(C) Copyright 2016-2019 Hewlett Packard Enterprise Development 
> > +LP
> >  (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -1069,7 +1069,7 @@ BdsEntry (
> >}
> >
> >if (!BootSuccess) {
> > -if (PlatformRecovery) {
> > +if (PcdGetBool (PcdPlatformRecoverySupport)) {
> >LoadOptions = EfiBootManagerGetLoadOptions (,
> LoadOptionTypePlatformRecovery);
> >ProcessLoadOptions (LoadOptions, LoadOptionCount);
> >EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> >
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49086): https://edk2.groups.io/g/devel/message/49086
Mute This Topic: https://groups.io/mt/34557852/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Use a pcd to control PlatformRecovery

[Wang, Sunny (HPS SW)]

Thanks for checking this, Zhichao and Ray. 
I just sent a patch to fix it with the subject " [edk2-devel] [PATCH] 
MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of OsIndications"

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, 
Zhichao
Sent: Wednesday, October 16, 2019 1:57 PM
To: Wang, Sunny (HPS SW) ; devel@edk2.groups.io; Ni, Ray 

Cc: Wang, Jian J ; Wu, Hao A ; Zeng, 
Star ; Gao, Liming ; Sean Brogan 
; Michael Turner ; 
Bret Barkelew ; Li, Walon ; Wei, 
Kent (HPS SW) 
Subject: Re: [edk2-devel] Use a pcd to control PlatformRecovery
Importance: High


> -Original Message-
> From: Gao, Zhichao
> Sent: Wednesday, October 16, 2019 10:09 AM
> To: 'Wang, Sunny (HPS SW)' ; devel@edk2.groups.io; 
> Ni, Ray 
> Cc: Wang, Jian J ; Wu, Hao A 
> ; Zeng, Star ; Gao, Liming 
> ; Sean Brogan ; 
> Michael Turner ; Bret Barkelew 
> ; Li, Walon ; Wei, Kent 
> (HPS SW) 
> Subject: RE: Use a pcd to control PlatformRecovery
> 
> First of all, the patch didn't aim to change the other part of the 
> boot flow except PlatformRecovery.
> 
> Local variable PlatformRecovery is controlled by OsIndications 
> variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is set, 
> the firmware should try to platform specific recovery. But that 
> doesn't mean the platform must support the specific recovery. i.e. 
> local PlatformRecovery is controlling the boot flow and the Pcd just 
> indicated whether the platform support recovery or not.
> So, on my opinion, I don't agree with your change.

After discuss with Sunny and Ray, and refer to the spec section 3.4.1 and 
3.4.2, the OsRecovery and PlatformRecovery should always be operated regardless 
of the value of OsIndication variable if fail to boot the BootOrder. I am 
wrong. We should change to use the PcdPlatformRecoverySupport to control the 
PlatformRecovery. Please help to send a patch to fix it. Thanks a lot.

> 
> Default Platform Recovery refer to the short file path to boot the OS. 
> If the firmware supports platform recovery, then *short file path* 
> option would be one part of the PlatformRecovery in case there are 
> no other boot options. If the firmware doesn't support platform 
> recovery, we still need this default boot thru a short file path and 
> it should not depend on the PlatformRecovery variable for the 
> compatibility thinking.
> 
> Thanks,
> Zhichao
> 
> > -Original Message-
> > From: Wang, Sunny (HPS SW) [mailto:sunnyw...@hpe.com]
> > Sent: Tuesday, October 15, 2019 4:53 PM
> > To: devel@edk2.groups.io; Gao, Zhichao ; Ni, 
> > Ray 
> > Cc: Wang, Jian J ; Wu, Hao A 
> > ; Zeng, Star ; Gao, Liming 
> > ; Sean Brogan ; 
> > Michael Turner ; Bret Barkelew 
> > ; Li, Walon ; Wei,
> Kent
> > (HPS SW) ; Wang, Sunny (HPS SW)
> 
> > Subject: Use a pcd to control PlatformRecovery
> >
> > Hi Zhichao and Ray,
> >
> > I have some questions about this code change. Sorry for being late 
> > to bring my questions here.
> >
> > For now, the code block for iterating the PlatformRecovery 
> > variables is controlled by OsIndications variable. However, it looks 
> > to me like that the PlatformRecovery should still be attempted 
> > for the case of that processing of BootOrder does NOT result in 
> > success (according to section 3.4 in UEFI 2.8). In other words, I 
> > think we should check PCD "PcdPlatformRecoverySupport" instead of 
> > Local variable
> "PlatformRecovery"
> > (from OsIndications variable) like the code below. What do you guys 
> > think? If you need a meeting or short talk to discuss this, feel 
> > free to let me
> know.
> >
> >   if (!BootSuccess) {
> > -   if (PlatformRecovery) {
> > +  if (PcdGetBool (PcdPlatformRecoverySupport)) {
> >   LoadOptions = EfiBootManagerGetLoadOptions (, 
> > LoadOptionTypePlatformRecovery);
> >   ProcessLoadOptions (LoadOptions, LoadOptionCount);
> >   EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > } else {
> >   //
> >   // When platform recovery is not enabled, still boot to 
> > platform default file path.
> >   //
> >   EfiBootManagerProcessLoadOption ();
> > }
> >
> >
> > In addition, it looks like EDK2 don't have code to process 
> > OsRecovery variables. Do we need to create a Bug on TianoCore
> Bugzilla system?

OsRecovery doesn't have an implement yet, we should co-work with the OS 
vendor to define the operation. For now, there is no requirement.

> >
> > Moreover, I saw th

[edk2-devel] [PATCH] MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of OsIndications

[Wang, Sunny (HPS SW)]

According to the UEFI specification section 3.4.1 and 3.4.2, the OsRecovery and 
PlatformRecovery options should still be processed regardless of the value of 
OsIndications variable if processing of BootOrder does not result in success. 
Therefore, update the code to check PcdPlatformRecoverySupport instead of the 
value of OsIndications variable (PlatformRecovery).

Cc: Ray Ni 
Cc: Zhichao Gao 
Cc: Walon Li 
Signed-off-by: Sunny Wang 
---
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index d6ec31118c..d387dbe7ac 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -6,7 +6,7 @@
   to enter BDS phase.
 
 Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.
-(C) Copyright 2016 Hewlett Packard Enterprise Development LP
+(C) Copyright 2016-2019 Hewlett Packard Enterprise Development LP
 (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -1069,7 +1069,7 @@ BdsEntry (
   }
 
   if (!BootSuccess) {
-if (PlatformRecovery) {
+if (PcdGetBool (PcdPlatformRecoverySupport)) {
   LoadOptions = EfiBootManagerGetLoadOptions (, 
LoadOptionTypePlatformRecovery);
   ProcessLoadOptions (LoadOptions, LoadOptionCount);
   EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49061): https://edk2.groups.io/g/devel/message/49061
Mute This Topic: https://groups.io/mt/34557852/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Use a pcd to control PlatformRecovery

[Wang, Sunny (HPS SW)]

Hi Zhichao and Ray, 

I have some questions about this code change. Sorry for being late to bring my 
questions here.

For now, the code block for iterating the PlatformRecovery variables is 
controlled by OsIndications variable. However, it looks to me like that the 
PlatformRecovery should still be attempted for the case of that processing 
of BootOrder does NOT result in success (according to section 3.4 in UEFI 2.8). 
In other words, I think we should check PCD "PcdPlatformRecoverySupport" 
instead of Local variable "PlatformRecovery" (from OsIndications variable) like 
the code below. What do you guys think? If you need a meeting or short talk to 
discuss this, feel free to let me know. 

  if (!BootSuccess) {
-   if (PlatformRecovery) {
+  if (PcdGetBool (PcdPlatformRecoverySupport)) {
  LoadOptions = EfiBootManagerGetLoadOptions (, 
LoadOptionTypePlatformRecovery);
  ProcessLoadOptions (LoadOptions, LoadOptionCount);
  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
} else {
  //
  // When platform recovery is not enabled, still boot to platform default 
file path.
  //
  EfiBootManagerProcessLoadOption ();
}
 

In addition, it looks like EDK2 don't have code to process OsRecovery 
variables. Do we need to create a Bug on TianoCore Bugzilla system? 

Moreover, I saw that both of you had a discussion about "Default 
PlatformRecovery", but I can't figure out the connection between the discussion 
and the final code change. Isn't the "Default PlatformRecovery" part of the 
Platform Recovery feature? At this moment, we don't have OS recovery support, 
so I think that NO platform recovery support can be identified as NO boot 
option recovery support. For this case, shouldn't we use PCD 
"PcdPlatformRecoverySupport" to control "Default PlatformRecovery" as well? For 
the next step, I think we need to get further clarification from USWG to either 
not tie "Default Boot Behavior" with PlatformRecovery or update the description 
to the following: 
- If system firmware supports Platform recovery as described in Section 
3.4.2, system firmware must include a PlatformRecovery variable specifying 
a short-form File Path Media Device Path

Regards,
Sunny Wang

-Original Message-
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, 
Zhichao
Sent: Thursday, June 20, 2019 11:44 AM
To: devel@edk2.groups.io
Cc: Jian J Wang ; Hao Wu ; Ray Ni 
; Star Zeng ; Liming Gao 
; Sean Brogan ; Michael Turner 
; Bret Barkelew 
Subject: [edk2-devel] [PATCH v5 2/2] MdeModulePkg/BdsDxe: Use a pcd to control 
PlatformRecovery

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678 

Use the PcdPlatformRecoverySupport to control the function of platform recovery 
in BDS.
First, set the variable's ("OsIndicationsSupported") 
EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit base on the pcd.
It would affect the variable "OsIndications".
While the platform does not support the platform recovery, it is inappropriate 
to set a PlatformRecovery variable. So skip setting the variable. But it 
should remain the behavior of booting from a default file path (such as 
\EFI\BOOT\BOOTX64.EFI) to be compatible with the previous version UEFI spec.

Add memory check before build platform default boot option. If fail to allocate 
memory for the defualt boot file path, put the system into dead loop to 
indicate it is unable to boot.

Cc: Jian J Wang 
Cc: Hao Wu 
Cc: Ray Ni 
Cc: Star Zeng 
Cc: Liming Gao 
Cc: Sean Brogan 
Cc: Michael Turner 
Cc: Bret Barkelew 
Signed-off-by: Zhichao Gao 
---
 MdeModulePkg/Universal/BdsDxe/BdsDxe.inf |  3 +-  
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 71 +++-
 2 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf 
b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
index 6913389d34..7f94ca17df 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -5,7 +5,7 @@
 #  gEfiBdsArchProtocolGuid. After DxeCore finish dispatching, DxeCore will 
invoke Entry  #  interface of protocol gEfiBdsArchProtocolGuid, then BDS phase 
is entered.
 #
-#  Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
+#  Copyright (c) 2008 - 2019, Intel Corporation. All rights 
+reserved.
 #  SPDX-License-Identifier: BSD-2-Clause-Patent  #  ## @@ -95,6 +95,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand  ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable  ## 
SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed   ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport   ## 
CONSUMES
 
 [Depex]
   TRUE
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 9d312bd982..4f3168b62a 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++