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


Zhiguang Liu
 

Currently, load option is only sorted when setup is the first priority in b=
oot 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 priori=
ty

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

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

diff --git a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBds=
HookLib.c b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBdsH=
ookLib.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 ();=0D
}=0D
=0D
-=0D
-/**=0D
- The function is to consider the boot order which is not in our expectati=
on.=0D
- In the case that we need to re-sort the boot option.=0D
-=0D
- @retval TRUE Need to sort Boot Option.=0D
- @retval FALSE Don't need to sort Boot Option.=0D
-**/=0D
-BOOLEAN=0D
-IsNeedSortBootOption (=0D
- VOID=0D
- )=0D
-{=0D
- EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;=0D
- UINTN BootOptionCount;=0D
-=0D
- BootOptions =3D EfiBootManagerGetLoadOptions (&BootOptionCount, LoadOpti=
onTypeBoot);=0D
-=0D
- //=0D
- // If setup is the first priority in boot option, we need to sort boot o=
ption.=0D
- //=0D
- if ((BootOptionCount > 1) &&=0D
- (((StrnCmp (BootOptions->Description, L"Enter Setup", StrLen (L"Enter =
Setup"))) =3D=3D 0) ||=0D
- ((StrnCmp (BootOptions->Description, L"BootManagerMenuApp", StrLen (L"=
BootManagerMenuApp"))) =3D=3D 0))) {=0D
- return TRUE;=0D
- }=0D
-=0D
- return FALSE;=0D
-}=0D
-=0D
-=0D
/**=0D
Connects Root Bridge=0D
**/=0D
@@ -1383,7 +1352,5 @@ BdsAfterConsoleReadyBeforeBootOptionCallback (
=0D
EfiBootManagerRefreshAllBootOption ();=0D
=0D
- if (IsNeedSortBootOption()) {=0D
- EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot, CompareBootO=
ption);=0D
- }=0D
+ EfiBootManagerSortLoadOptionVariable (LoadOptionTypeBoot, CompareBootOpt=
ion);=0D
}=0D
--=20
2.30.0.windows.2


Ni, Ray
 

Zhiguang,

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

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@...>; Liming Gao <gaoliming@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Agyeman, Prince <prince.agyeman@...>
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@...>
Cc: Liming Gao <gaoliming@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Prince Agyeman <prince.agyeman@...>

Signed-off-by: Zhiguang Liu <zhiguang.liu@...>
---
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@...]
-=-=-=-=-=-=


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@...>
Cc: Dong, Eric <eric.dong@...>; Liming Gao
<gaoliming@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Agyeman, Prince
<prince.agyeman@...>
Subject: Re: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg:
Always sort load option

Zhiguang,

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

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@...>; Liming Gao
<gaoliming@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Agyeman, Prince
<prince.agyeman@...>
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@...>
Cc: Liming Gao <gaoliming@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Prince Agyeman <prince.agyeman@...>

Signed-off-by: Zhiguang Liu <zhiguang.liu@...>
---

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@...]
-=-=-=-=-=-=




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@...>
Sent: Tuesday, March 2, 2021 6:46 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@...>; Liu, Zhiguang <zhiguang.liu@...>
Cc: Dong, Eric <eric.dong@...>; Liming Gao <gaoliming@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Agyeman, Prince <prince.agyeman@...>
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@...>
Cc: Dong, Eric <eric.dong@...>; Liming Gao
<gaoliming@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Agyeman, Prince
<prince.agyeman@...>
Subject: Re: [edk2-devel] [PATCH] [edk2-platforms]Intel/BoardModulePkg:
Always sort load option

Zhiguang,

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

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@...>; Liming Gao
<gaoliming@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Agyeman, Prince
<prince.agyeman@...>
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@...>
Cc: Liming Gao <gaoliming@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Prince Agyeman <prince.agyeman@...>

Signed-off-by: Zhiguang Liu <zhiguang.liu@...>
---

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@...]
-=-=-=-=-=-=