[PATCH] [edk2-platforms]Intel/BoardModulePkg: Always sort load option

[PATCH] [edk2-platforms]Intel/BoardModulePkg: Always sort load option

[Zhiguang Liu]

Currently, load option is only sorted when setup is the first priority in boot option.
This condition is not needed because the below reasons:
1. Setup option may have different string name depending on platform side.
   It shouldn't be hardcoded here.
2. Always sorting meets the needs that setup should not be the first priority

Cc: Eric Dong <eric.dong@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c | 35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c
index d7612fb80a..60acf48dd6 100644
--- a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c
+++ b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c
@@ -992,37 +992,6 @@ ConnectSequence (
   EfiBootManagerConnectAll ();
 }
 
-
-/**
-  The function is to consider the boot order which is not in our expectation.
-  In the case that we need to re-sort the boot option.
-
-  @retval  TRUE         Need to sort Boot Option.
-  @retval  FALSE        Don't need to sort Boot Option.
-**/
-BOOLEAN
-IsNeedSortBootOption (
-  VOID
-  )
-{
-  EFI_BOOT_MANAGER_LOAD_OPTION  *BootOptions;
-  UINTN                         BootOptionCount;
-
-  BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, LoadOptionTypeBoot);
-
-  //
-  // If setup is the first priority in boot option, we need to sort boot option.
-  //
-  if ((BootOptionCount > 1) &&
-    (((StrnCmp (BootOptions->Description, L"Enter Setup", StrLen (L"Enter Setup"))) == 0) ||
-    ((StrnCmp (BootOptions->Description, L"BootManagerMenuApp", StrLen (L"BootManagerMenuApp"))) == 0))) {
-    return TRUE;
-  }
-
-  return FALSE;
-}
-
-
 /**
   Connects Root Bridge
  **/
@@ -1383,7 +1352,5 @@ BdsAfterConsoleReadyBeforeBootOptionCallback (
 
   EfiBootManagerRefreshAllBootOption ();
 
-  if (IsNeedSortBootOption()) {
-    EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot, CompareBootOption);
-  }
+  EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot, CompareBootOption);
 }
-- 
2.30.0.windows.2

Re: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg: Always sort load option

[Ni, Ray]

Zhiguang,

Reviewed-by: Ray Ni <ray.ni@intel.com>

I think you can add a third reason in commit message:

3. Below change in UefiBootManagerLib puts setup in the end
  MdeModulePkg/UefiBootManagerLib: Put BootMenu at the end of BootOrder
  SHA-1: 7f34681c488aee2563eaa2afcc6a2c8aa7c5b912

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhiguang Liu
> Sent: Tuesday, March 2, 2021 5:04 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>
> Subject: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg: Always sort load option
> 
> Currently, load option is only sorted when setup is the first priority in boot option.
> This condition is not needed because the below reasons:
> 1. Setup option may have different string name depending on platform side.
>    It shouldn't be hardcoded here.
> 2. Always sorting meets the needs that setup should not be the first priority
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Prince Agyeman <prince.agyeman@intel.com>
> 
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c | 35 +----------------------------------
>  1 file changed, 1 insertion(+), 34 deletions(-)
> 
> diff --git a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c
> b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c
> index d7612fb80a..60acf48dd6 100644
> --- a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c
> +++ b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.c
> @@ -992,37 +992,6 @@ ConnectSequence (
>    EfiBootManagerConnectAll ();
> 
>  }
> 
> 
> 
> -
> 
> -/**
> 
> -  The function is to consider the boot order which is not in our expectation.
> 
> -  In the case that we need to re-sort the boot option.
> 
> -
> 
> -  @retval  TRUE         Need to sort Boot Option.
> 
> -  @retval  FALSE        Don't need to sort Boot Option.
> 
> -**/
> 
> -BOOLEAN
> 
> -IsNeedSortBootOption (
> 
> -  VOID
> 
> -  )
> 
> -{
> 
> -  EFI_BOOT_MANAGER_LOAD_OPTION  *BootOptions;
> 
> -  UINTN                         BootOptionCount;
> 
> -
> 
> -  BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, LoadOptionTypeBoot);
> 
> -
> 
> -  //
> 
> -  // If setup is the first priority in boot option, we need to sort boot option.
> 
> -  //
> 
> -  if ((BootOptionCount > 1) &&
> 
> -    (((StrnCmp (BootOptions->Description, L"Enter Setup", StrLen (L"Enter Setup"))) == 0) ||
> 
> -    ((StrnCmp (BootOptions->Description, L"BootManagerMenuApp", StrLen (L"BootManagerMenuApp"))) == 0))) {
> 
> -    return TRUE;
> 
> -  }
> 
> -
> 
> -  return FALSE;
> 
> -}
> 
> -
> 
> -
> 
>  /**
> 
>    Connects Root Bridge
> 
>   **/
> 
> @@ -1383,7 +1352,5 @@ BdsAfterConsoleReadyBeforeBootOptionCallback (
> 
> 
>    EfiBootManagerRefreshAllBootOption ();
> 
> 
> 
> -  if (IsNeedSortBootOption()) {
> 
> -    EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot, CompareBootOption);
> 
> -  }
> 
> +  EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot, CompareBootOption);
> 
>  }
> 
> --
> 2.30.0.windows.2
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#72329): https://edk2.groups.io/g/devel/message/72329
> Mute This Topic: https://groups.io/mt/81021303/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
> 

Re: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg: Always sort load option

[Gao, Zhichao]

Hi Ray,

I just think of that if we always do the sort, it may cause the changed boot order (by the user of the platform) resort again. That's unexpected.

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, March 2, 2021 5:12 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> <prince.agyeman@intel.com>
> Subject: Re: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg:
> Always sort load option
> 
> Zhiguang,
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> I think you can add a third reason in commit message:
> 
> 3. Below change in UefiBootManagerLib puts setup in the end
>   MdeModulePkg/UefiBootManagerLib: Put BootMenu at the end of BootOrder
>   SHA-1: 7f34681c488aee2563eaa2afcc6a2c8aa7c5b912
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Zhiguang Liu
> > Sent: Tuesday, March 2, 2021 5:04 PM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> > <prince.agyeman@intel.com>
> > Subject: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg:
> > Always sort load option
> >
> > Currently, load option is only sorted when setup is the first priority in boot
> option.
> > This condition is not needed because the below reasons:
> > 1. Setup option may have different string name depending on platform side.
> >    It shouldn't be hardcoded here.
> > 2. Always sorting meets the needs that setup should not be the first
> > priority
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Prince Agyeman <prince.agyeman@intel.com>
> >
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> >
> > Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.
> > c | 35 +----------------------------------
> >  1 file changed, 1 insertion(+), 34 deletions(-)
> >
> > diff --git
> > a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLi
> > b.c
> > b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLi
> > b.c
> > index d7612fb80a..60acf48dd6 100644
> > ---
> > a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLi
> > b.c
> > +++
> b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHo
> > +++ okLib.c
> > @@ -992,37 +992,6 @@ ConnectSequence (
> >    EfiBootManagerConnectAll ();
> >
> >  }
> >
> >
> >
> > -
> >
> > -/**
> >
> > -  The function is to consider the boot order which is not in our expectation.
> >
> > -  In the case that we need to re-sort the boot option.
> >
> > -
> >
> > -  @retval  TRUE         Need to sort Boot Option.
> >
> > -  @retval  FALSE        Don't need to sort Boot Option.
> >
> > -**/
> >
> > -BOOLEAN
> >
> > -IsNeedSortBootOption (
> >
> > -  VOID
> >
> > -  )
> >
> > -{
> >
> > -  EFI_BOOT_MANAGER_LOAD_OPTION  *BootOptions;
> >
> > -  UINTN                         BootOptionCount;
> >
> > -
> >
> > -  BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount,
> > LoadOptionTypeBoot);
> >
> > -
> >
> > -  //
> >
> > -  // If setup is the first priority in boot option, we need to sort boot option.
> >
> > -  //
> >
> > -  if ((BootOptionCount > 1) &&
> >
> > -    (((StrnCmp (BootOptions->Description, L"Enter Setup", StrLen (L"Enter
> Setup"))) == 0) ||
> >
> > -    ((StrnCmp (BootOptions->Description, L"BootManagerMenuApp", StrLen
> (L"BootManagerMenuApp"))) == 0))) {
> >
> > -    return TRUE;
> >
> > -  }
> >
> > -
> >
> > -  return FALSE;
> >
> > -}
> >
> > -
> >
> > -
> >
> >  /**
> >
> >    Connects Root Bridge
> >
> >   **/
> >
> > @@ -1383,7 +1352,5 @@ BdsAfterConsoleReadyBeforeBootOptionCallback (
> >
> >
> >    EfiBootManagerRefreshAllBootOption ();
> >
> >
> >
> > -  if (IsNeedSortBootOption()) {
> >
> > -    EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot,
> CompareBootOption);
> >
> > -  }
> >
> > +  EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot,
> > + CompareBootOption);
> >
> >  }
> >
> > --
> > 2.30.0.windows.2
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#72329):
> > https://edk2.groups.io/g/devel/message/72329
> > Mute This Topic: https://groups.io/mt/81021303/1712937
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> > -=-=-=-=-=-=
> >
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg: Always sort load option

[Ni, Ray]

Good catch.
BdsAfterConsoleReadyBeforeBootOptionCallback() in BoardModulePkg is not implemented properly.
It should only do the boot option sort either:
1. in the first boot after flashing the firmware, or
2. in BOOT_WITH_FULL_CONFIGURATION boot path if the platform PEI can correctly changes the boot mode to other boot modes when no configuration changes.

Thanks,
Ray

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, March 2, 2021 6:46 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>
> Subject: RE: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg: Always sort load option
> 
> Hi Ray,
> 
> I just think of that if we always do the sort, it may cause the changed boot order (by the user of the platform) resort again.
> That's unexpected.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> > Sent: Tuesday, March 2, 2021 5:12 PM
> > To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> > <prince.agyeman@intel.com>
> > Subject: Re: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg:
> > Always sort load option
> >
> > Zhiguang,
> >
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> >
> > I think you can add a third reason in commit message:
> >
> > 3. Below change in UefiBootManagerLib puts setup in the end
> >   MdeModulePkg/UefiBootManagerLib: Put BootMenu at the end of BootOrder
> >   SHA-1: 7f34681c488aee2563eaa2afcc6a2c8aa7c5b912
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Zhiguang Liu
> > > Sent: Tuesday, March 2, 2021 5:04 PM
> > > To: devel@edk2.groups.io
> > > Cc: Dong, Eric <eric.dong@intel.com>; Liming Gao
> > > <gaoliming@byosoft.com.cn>; Desimone, Nathaniel L
> > > <nathaniel.l.desimone@intel.com>; Agyeman, Prince
> > > <prince.agyeman@intel.com>
> > > Subject: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg:
> > > Always sort load option
> > >
> > > Currently, load option is only sorted when setup is the first priority in boot
> > option.
> > > This condition is not needed because the below reasons:
> > > 1. Setup option may have different string name depending on platform side.
> > >    It shouldn't be hardcoded here.
> > > 2. Always sorting meets the needs that setup should not be the first
> > > priority
> > >
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > > Cc: Prince Agyeman <prince.agyeman@intel.com>
> > >
> > > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > > ---
> > >
> > > Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLib.
> > > c | 35 +----------------------------------
> > >  1 file changed, 1 insertion(+), 34 deletions(-)
> > >
> > > diff --git
> > > a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLi
> > > b.c
> > > b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLi
> > > b.c
> > > index d7612fb80a..60acf48dd6 100644
> > > ---
> > > a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHookLi
> > > b.c
> > > +++
> > b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsHo
> > > +++ okLib.c
> > > @@ -992,37 +992,6 @@ ConnectSequence (
> > >    EfiBootManagerConnectAll ();
> > >
> > >  }
> > >
> > >
> > >
> > > -
> > >
> > > -/**
> > >
> > > -  The function is to consider the boot order which is not in our expectation.
> > >
> > > -  In the case that we need to re-sort the boot option.
> > >
> > > -
> > >
> > > -  @retval  TRUE         Need to sort Boot Option.
> > >
> > > -  @retval  FALSE        Don't need to sort Boot Option.
> > >
> > > -**/
> > >
> > > -BOOLEAN
> > >
> > > -IsNeedSortBootOption (
> > >
> > > -  VOID
> > >
> > > -  )
> > >
> > > -{
> > >
> > > -  EFI_BOOT_MANAGER_LOAD_OPTION  *BootOptions;
> > >
> > > -  UINTN                         BootOptionCount;
> > >
> > > -
> > >
> > > -  BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount,
> > > LoadOptionTypeBoot);
> > >
> > > -
> > >
> > > -  //
> > >
> > > -  // If setup is the first priority in boot option, we need to sort boot option.
> > >
> > > -  //
> > >
> > > -  if ((BootOptionCount > 1) &&
> > >
> > > -    (((StrnCmp (BootOptions->Description, L"Enter Setup", StrLen (L"Enter
> > Setup"))) == 0) ||
> > >
> > > -    ((StrnCmp (BootOptions->Description, L"BootManagerMenuApp", StrLen
> > (L"BootManagerMenuApp"))) == 0))) {
> > >
> > > -    return TRUE;
> > >
> > > -  }
> > >
> > > -
> > >
> > > -  return FALSE;
> > >
> > > -}
> > >
> > > -
> > >
> > > -
> > >
> > >  /**
> > >
> > >    Connects Root Bridge
> > >
> > >   **/
> > >
> > > @@ -1383,7 +1352,5 @@ BdsAfterConsoleReadyBeforeBootOptionCallback (
> > >
> > >
> > >    EfiBootManagerRefreshAllBootOption ();
> > >
> > >
> > >
> > > -  if (IsNeedSortBootOption()) {
> > >
> > > -    EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot,
> > CompareBootOption);
> > >
> > > -  }
> > >
> > > +  EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot,
> > > + CompareBootOption);
> > >
> > >  }
> > >
> > > --
> > > 2.30.0.windows.2
> > >
> > >
> > >
> > > -=-=-=-=-=-=
> > > Groups.io Links: You receive all messages sent to this group.
> > > View/Reply Online (#72329):
> > > https://edk2.groups.io/g/devel/message/72329
> > > Mute This Topic: https://groups.io/mt/81021303/1712937
> > > Group Owner: devel+owner@edk2.groups.io
> > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> > > -=-=-=-=-=-=
> > >
> >
> >
> >
> > 
> >