* [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
@ 2017-09-28 5:50 Ruiyu Ni
2017-09-28 5:59 ` Zeng, Star
2017-09-28 6:46 ` Wang, Sunny (HPS SW)
0 siblings, 2 replies; 7+ messages in thread
From: Ruiyu Ni @ 2017-09-28 5:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Star Zeng
Current implementation deletes the "BootNext" before calling
any PlatformBootManagerLib APIs, but if system resets in
PlatformBootManagerLib APIs, "BootNext" is not consumed but lost.
The patch defers the deletion of "BootNext" to before booting it.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35 ++++++++++++++++++--------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index ac5f9088dd..a6fe617b56 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -808,7 +808,8 @@ BdsEntry (
ASSERT_EFI_ERROR (Status);
//
- // Cache and remove the "BootNext" NV variable.
+ // Cache the "BootNext" NV variable before calling any PlatformBootManagerLib APIs
+ // This could avoid the "BootNext" set by PlatformBootManagerLib be consumed in this boot.
//
GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID **) &BootNext, &DataSize);
if (DataSize != sizeof (UINT16)) {
@@ -817,17 +818,6 @@ BdsEntry (
}
BootNext = NULL;
}
- Status = gRT->SetVariable (
- EFI_BOOT_NEXT_VARIABLE_NAME,
- &gEfiGlobalVariableGuid,
- 0,
- 0,
- NULL
- );
- //
- // Deleting NV variable shouldn't fail unless it doesn't exist.
- //
- ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
//
// Initialize the platform language variables
@@ -1052,10 +1042,25 @@ BdsEntry (
EfiBootManagerHotkeyBoot ();
- //
- // Boot to "BootNext"
- //
if (BootNext != NULL) {
+ //
+ // Delete "BootNext" NV variable before transferring control to it to prevent loops.
+ //
+ Status = gRT->SetVariable (
+ EFI_BOOT_NEXT_VARIABLE_NAME,
+ &gEfiGlobalVariableGuid,
+ 0,
+ 0,
+ NULL
+ );
+ //
+ // Deleting NV variable shouldn't fail unless it doesn't exist.
+ //
+ ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
+
+ //
+ // Boot to "BootNext"
+ //
UnicodeSPrint (BootNextVariableName, sizeof (BootNextVariableName), L"Boot%04x", *BootNext);
Status = EfiBootManagerVariableToLoadOption (BootNextVariableName, &LoadOption);
if (!EFI_ERROR (Status)) {
--
2.12.2.windows.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
2017-09-28 5:50 [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it Ruiyu Ni
@ 2017-09-28 5:59 ` Zeng, Star
2017-09-28 6:01 ` Ni, Ruiyu
2017-09-28 6:46 ` Wang, Sunny (HPS SW)
1 sibling, 1 reply; 7+ messages in thread
From: Zeng, Star @ 2017-09-28 5:59 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Dong, Eric, Zeng, Star
I am ok with the code logic change, but a little confused by the new comment. It seems not match with the commit log.
" This could avoid the "BootNext" set by PlatformBootManagerLib be consumed in this boot. "
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Thursday, September 28, 2017 1:50 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
Current implementation deletes the "BootNext" before calling any PlatformBootManagerLib APIs, but if system resets in PlatformBootManagerLib APIs, "BootNext" is not consumed but lost.
The patch defers the deletion of "BootNext" to before booting it.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35 ++++++++++++++++++--------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index ac5f9088dd..a6fe617b56 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -808,7 +808,8 @@ BdsEntry (
ASSERT_EFI_ERROR (Status);
//
- // Cache and remove the "BootNext" NV variable.
+ // Cache the "BootNext" NV variable before calling any
+ PlatformBootManagerLib APIs // This could avoid the "BootNext" set by PlatformBootManagerLib be consumed in this boot.
//
GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID **) &BootNext, &DataSize);
if (DataSize != sizeof (UINT16)) {
@@ -817,17 +818,6 @@ BdsEntry (
}
BootNext = NULL;
}
- Status = gRT->SetVariable (
- EFI_BOOT_NEXT_VARIABLE_NAME,
- &gEfiGlobalVariableGuid,
- 0,
- 0,
- NULL
- );
- //
- // Deleting NV variable shouldn't fail unless it doesn't exist.
- //
- ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
//
// Initialize the platform language variables @@ -1052,10 +1042,25 @@ BdsEntry (
EfiBootManagerHotkeyBoot ();
- //
- // Boot to "BootNext"
- //
if (BootNext != NULL) {
+ //
+ // Delete "BootNext" NV variable before transferring control to it to prevent loops.
+ //
+ Status = gRT->SetVariable (
+ EFI_BOOT_NEXT_VARIABLE_NAME,
+ &gEfiGlobalVariableGuid,
+ 0,
+ 0,
+ NULL
+ );
+ //
+ // Deleting NV variable shouldn't fail unless it doesn't exist.
+ //
+ ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
+
+ //
+ // Boot to "BootNext"
+ //
UnicodeSPrint (BootNextVariableName, sizeof (BootNextVariableName), L"Boot%04x", *BootNext);
Status = EfiBootManagerVariableToLoadOption (BootNextVariableName, &LoadOption);
if (!EFI_ERROR (Status)) {
--
2.12.2.windows.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
2017-09-28 5:59 ` Zeng, Star
@ 2017-09-28 6:01 ` Ni, Ruiyu
2017-09-28 6:13 ` Zeng, Star
0 siblings, 1 reply; 7+ messages in thread
From: Ni, Ruiyu @ 2017-09-28 6:01 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Dong, Eric
This could avoid the "BootNext" set by PlatformBootManagerLib be consumed in *this* boot.
If I add "*" around "this", is it more clear?
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 2:00 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until
> booting it
>
> I am ok with the code logic change, but a little confused by the new comment. It
> seems not match with the commit log.
>
> " This could avoid the "BootNext" set by PlatformBootManagerLib be consumed
> in this boot. "
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, September 28, 2017 1:50 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting
> it
>
> Current implementation deletes the "BootNext" before calling any
> PlatformBootManagerLib APIs, but if system resets in PlatformBootManagerLib
> APIs, "BootNext" is not consumed but lost.
>
> The patch defers the deletion of "BootNext" to before booting it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35 ++++++++++++++++++--------
> ------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> index ac5f9088dd..a6fe617b56 100644
> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -808,7 +808,8 @@ BdsEntry (
> ASSERT_EFI_ERROR (Status);
>
> //
> - // Cache and remove the "BootNext" NV variable.
> + // Cache the "BootNext" NV variable before calling any
> + PlatformBootManagerLib APIs // This could avoid the "BootNext" set by
> PlatformBootManagerLib be consumed in this boot.
> //
> GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID **)
> &BootNext, &DataSize);
> if (DataSize != sizeof (UINT16)) {
> @@ -817,17 +818,6 @@ BdsEntry (
> }
> BootNext = NULL;
> }
> - Status = gRT->SetVariable (
> - EFI_BOOT_NEXT_VARIABLE_NAME,
> - &gEfiGlobalVariableGuid,
> - 0,
> - 0,
> - NULL
> - );
> - //
> - // Deleting NV variable shouldn't fail unless it doesn't exist.
> - //
> - ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
>
> //
> // Initialize the platform language variables @@ -1052,10 +1042,25 @@
> BdsEntry (
>
> EfiBootManagerHotkeyBoot ();
>
> - //
> - // Boot to "BootNext"
> - //
> if (BootNext != NULL) {
> + //
> + // Delete "BootNext" NV variable before transferring control to it to
> prevent loops.
> + //
> + Status = gRT->SetVariable (
> + EFI_BOOT_NEXT_VARIABLE_NAME,
> + &gEfiGlobalVariableGuid,
> + 0,
> + 0,
> + NULL
> + );
> + //
> + // Deleting NV variable shouldn't fail unless it doesn't exist.
> + //
> + ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
> +
> + //
> + // Boot to "BootNext"
> + //
> UnicodeSPrint (BootNextVariableName, sizeof (BootNextVariableName),
> L"Boot%04x", *BootNext);
> Status = EfiBootManagerVariableToLoadOption (BootNextVariableName,
> &LoadOption);
> if (!EFI_ERROR (Status)) {
> --
> 2.12.2.windows.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
2017-09-28 6:01 ` Ni, Ruiyu
@ 2017-09-28 6:13 ` Zeng, Star
2017-09-28 6:18 ` Ni, Ruiyu
0 siblings, 1 reply; 7+ messages in thread
From: Zeng, Star @ 2017-09-28 6:13 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Dong, Eric, Zeng, Star
The commit log is saying the "BootNext" *DELETED* (before PlatformBootManagerLib) may be *LOST* if there is reset during PlatformBootManagerLib. I realized it.
The comment is saying to avoid the "BootNext" "SET" by PlatformBootManagerLib. Sorry I am not getting the point.
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Thursday, September 28, 2017 2:02 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>
Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
This could avoid the "BootNext" set by PlatformBootManagerLib be consumed in *this* boot.
If I add "*" around "this", is it more clear?
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 2:00 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext"
> until booting it
>
> I am ok with the code logic change, but a little confused by the new
> comment. It seems not match with the commit log.
>
> " This could avoid the "BootNext" set by PlatformBootManagerLib be
> consumed in this boot. "
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, September 28, 2017 1:50 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until
> booting it
>
> Current implementation deletes the "BootNext" before calling any
> PlatformBootManagerLib APIs, but if system resets in
> PlatformBootManagerLib APIs, "BootNext" is not consumed but lost.
>
> The patch defers the deletion of "BootNext" to before booting it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35
> ++++++++++++++++++--------
> ------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> index ac5f9088dd..a6fe617b56 100644
> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -808,7 +808,8 @@ BdsEntry (
> ASSERT_EFI_ERROR (Status);
>
> //
> - // Cache and remove the "BootNext" NV variable.
> + // Cache the "BootNext" NV variable before calling any
> + PlatformBootManagerLib APIs // This could avoid the "BootNext" set
> + by
> PlatformBootManagerLib be consumed in this boot.
> //
> GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID **)
> &BootNext, &DataSize);
> if (DataSize != sizeof (UINT16)) {
> @@ -817,17 +818,6 @@ BdsEntry (
> }
> BootNext = NULL;
> }
> - Status = gRT->SetVariable (
> - EFI_BOOT_NEXT_VARIABLE_NAME,
> - &gEfiGlobalVariableGuid,
> - 0,
> - 0,
> - NULL
> - );
> - //
> - // Deleting NV variable shouldn't fail unless it doesn't exist.
> - //
> - ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
>
> //
> // Initialize the platform language variables @@ -1052,10 +1042,25
> @@ BdsEntry (
>
> EfiBootManagerHotkeyBoot ();
>
> - //
> - // Boot to "BootNext"
> - //
> if (BootNext != NULL) {
> + //
> + // Delete "BootNext" NV variable before transferring control to
> + it to
> prevent loops.
> + //
> + Status = gRT->SetVariable (
> + EFI_BOOT_NEXT_VARIABLE_NAME,
> + &gEfiGlobalVariableGuid,
> + 0,
> + 0,
> + NULL
> + );
> + //
> + // Deleting NV variable shouldn't fail unless it doesn't exist.
> + //
> + ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
> +
> + //
> + // Boot to "BootNext"
> + //
> UnicodeSPrint (BootNextVariableName, sizeof
> (BootNextVariableName), L"Boot%04x", *BootNext);
> Status = EfiBootManagerVariableToLoadOption
> (BootNextVariableName, &LoadOption);
> if (!EFI_ERROR (Status)) {
> --
> 2.12.2.windows.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
2017-09-28 6:13 ` Zeng, Star
@ 2017-09-28 6:18 ` Ni, Ruiyu
2017-09-28 6:22 ` Zeng, Star
0 siblings, 1 reply; 7+ messages in thread
From: Ni, Ruiyu @ 2017-09-28 6:18 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Dong, Eric
I didn't change the position of code to cache "BootNext", because:
This could avoid the "BootNext" (set by PlatformBootManagerLib) be consumed in *this* boot.
Maybe this time it's more clear.
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 2:13 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until
> booting it
>
> The commit log is saying the "BootNext" *DELETED* (before
> PlatformBootManagerLib) may be *LOST* if there is reset during
> PlatformBootManagerLib. I realized it.
>
> The comment is saying to avoid the "BootNext" "SET" by
> PlatformBootManagerLib. Sorry I am not getting the point.
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, September 28, 2017 2:02 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until
> booting it
>
> This could avoid the "BootNext" set by PlatformBootManagerLib be consumed
> in *this* boot.
>
> If I add "*" around "this", is it more clear?
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Thursday, September 28, 2017 2:00 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext"
> > until booting it
> >
> > I am ok with the code logic change, but a little confused by the new
> > comment. It seems not match with the commit log.
> >
> > " This could avoid the "BootNext" set by PlatformBootManagerLib be
> > consumed in this boot. "
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Thursday, September 28, 2017 1:50 PM
> > To: edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until
> > booting it
> >
> > Current implementation deletes the "BootNext" before calling any
> > PlatformBootManagerLib APIs, but if system resets in
> > PlatformBootManagerLib APIs, "BootNext" is not consumed but lost.
> >
> > The patch defers the deletion of "BootNext" to before booting it.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > ---
> > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35
> > ++++++++++++++++++--------
> > ------
> > 1 file changed, 20 insertions(+), 15 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > index ac5f9088dd..a6fe617b56 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > @@ -808,7 +808,8 @@ BdsEntry (
> > ASSERT_EFI_ERROR (Status);
> >
> > //
> > - // Cache and remove the "BootNext" NV variable.
> > + // Cache the "BootNext" NV variable before calling any
> > + PlatformBootManagerLib APIs // This could avoid the "BootNext" set
> > + by
> > PlatformBootManagerLib be consumed in this boot.
> > //
> > GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID **)
> > &BootNext, &DataSize);
> > if (DataSize != sizeof (UINT16)) {
> > @@ -817,17 +818,6 @@ BdsEntry (
> > }
> > BootNext = NULL;
> > }
> > - Status = gRT->SetVariable (
> > - EFI_BOOT_NEXT_VARIABLE_NAME,
> > - &gEfiGlobalVariableGuid,
> > - 0,
> > - 0,
> > - NULL
> > - );
> > - //
> > - // Deleting NV variable shouldn't fail unless it doesn't exist.
> > - //
> > - ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
> >
> > //
> > // Initialize the platform language variables @@ -1052,10 +1042,25
> > @@ BdsEntry (
> >
> > EfiBootManagerHotkeyBoot ();
> >
> > - //
> > - // Boot to "BootNext"
> > - //
> > if (BootNext != NULL) {
> > + //
> > + // Delete "BootNext" NV variable before transferring control to
> > + it to
> > prevent loops.
> > + //
> > + Status = gRT->SetVariable (
> > + EFI_BOOT_NEXT_VARIABLE_NAME,
> > + &gEfiGlobalVariableGuid,
> > + 0,
> > + 0,
> > + NULL
> > + );
> > + //
> > + // Deleting NV variable shouldn't fail unless it doesn't exist.
> > + //
> > + ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
> > +
> > + //
> > + // Boot to "BootNext"
> > + //
> > UnicodeSPrint (BootNextVariableName, sizeof
> > (BootNextVariableName), L"Boot%04x", *BootNext);
> > Status = EfiBootManagerVariableToLoadOption
> > (BootNextVariableName, &LoadOption);
> > if (!EFI_ERROR (Status)) {
> > --
> > 2.12.2.windows.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
2017-09-28 6:18 ` Ni, Ruiyu
@ 2017-09-28 6:22 ` Zeng, Star
0 siblings, 0 replies; 7+ messages in thread
From: Zeng, Star @ 2017-09-28 6:22 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Dong, Eric, Zeng, Star
Ok, got it, tks. The new comment is showing why cache "BootNext" logic is still kept there, the commit log is showing the reason of the patch change. Make sense. :)
Reviewed-by: Star Zeng <star.zeng@intel.com>
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Thursday, September 28, 2017 2:18 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>
Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
I didn't change the position of code to cache "BootNext", because:
This could avoid the "BootNext" (set by PlatformBootManagerLib) be consumed in *this* boot.
Maybe this time it's more clear.
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 2:13 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext"
> until booting it
>
> The commit log is saying the "BootNext" *DELETED* (before
> PlatformBootManagerLib) may be *LOST* if there is reset during
> PlatformBootManagerLib. I realized it.
>
> The comment is saying to avoid the "BootNext" "SET" by
> PlatformBootManagerLib. Sorry I am not getting the point.
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, September 28, 2017 2:02 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext"
> until booting it
>
> This could avoid the "BootNext" set by PlatformBootManagerLib be
> consumed in *this* boot.
>
> If I add "*" around "this", is it more clear?
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Thursday, September 28, 2017 2:00 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext"
> > until booting it
> >
> > I am ok with the code logic change, but a little confused by the new
> > comment. It seems not match with the commit log.
> >
> > " This could avoid the "BootNext" set by PlatformBootManagerLib be
> > consumed in this boot. "
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Thursday, September 28, 2017 1:50 PM
> > To: edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until
> > booting it
> >
> > Current implementation deletes the "BootNext" before calling any
> > PlatformBootManagerLib APIs, but if system resets in
> > PlatformBootManagerLib APIs, "BootNext" is not consumed but lost.
> >
> > The patch defers the deletion of "BootNext" to before booting it.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > ---
> > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35
> > ++++++++++++++++++--------
> > ------
> > 1 file changed, 20 insertions(+), 15 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > index ac5f9088dd..a6fe617b56 100644
> > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > @@ -808,7 +808,8 @@ BdsEntry (
> > ASSERT_EFI_ERROR (Status);
> >
> > //
> > - // Cache and remove the "BootNext" NV variable.
> > + // Cache the "BootNext" NV variable before calling any
> > + PlatformBootManagerLib APIs // This could avoid the "BootNext"
> > + set by
> > PlatformBootManagerLib be consumed in this boot.
> > //
> > GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID **)
> > &BootNext, &DataSize);
> > if (DataSize != sizeof (UINT16)) { @@ -817,17 +818,6 @@ BdsEntry
> > (
> > }
> > BootNext = NULL;
> > }
> > - Status = gRT->SetVariable (
> > - EFI_BOOT_NEXT_VARIABLE_NAME,
> > - &gEfiGlobalVariableGuid,
> > - 0,
> > - 0,
> > - NULL
> > - );
> > - //
> > - // Deleting NV variable shouldn't fail unless it doesn't exist.
> > - //
> > - ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
> >
> > //
> > // Initialize the platform language variables @@ -1052,10
> > +1042,25 @@ BdsEntry (
> >
> > EfiBootManagerHotkeyBoot ();
> >
> > - //
> > - // Boot to "BootNext"
> > - //
> > if (BootNext != NULL) {
> > + //
> > + // Delete "BootNext" NV variable before transferring control
> > + to it to
> > prevent loops.
> > + //
> > + Status = gRT->SetVariable (
> > + EFI_BOOT_NEXT_VARIABLE_NAME,
> > + &gEfiGlobalVariableGuid,
> > + 0,
> > + 0,
> > + NULL
> > + );
> > + //
> > + // Deleting NV variable shouldn't fail unless it doesn't exist.
> > + //
> > + ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
> > +
> > + //
> > + // Boot to "BootNext"
> > + //
> > UnicodeSPrint (BootNextVariableName, sizeof
> > (BootNextVariableName), L"Boot%04x", *BootNext);
> > Status = EfiBootManagerVariableToLoadOption
> > (BootNextVariableName, &LoadOption);
> > if (!EFI_ERROR (Status)) {
> > --
> > 2.12.2.windows.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
2017-09-28 5:50 [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it Ruiyu Ni
2017-09-28 5:59 ` Zeng, Star
@ 2017-09-28 6:46 ` Wang, Sunny (HPS SW)
1 sibling, 0 replies; 7+ messages in thread
From: Wang, Sunny (HPS SW) @ 2017-09-28 6:46 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel@lists.01.org
Cc: Eric Dong, Star Zeng, Wang, Sunny (HPS SW)
The change Looks good to me.
Also, Good to see you adding the comment about why we need to cache BootNext before all the platform hook function calls. :)
Reviewed-by: Sunny Wang <sunnywang@hpe.com>
Regards,
Sunny Wang
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Thursday, September 28, 2017 1:50 PM
To: edk2-devel@lists.01.org
Cc: Eric Dong <eric.dong@intel.com>; Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it
Current implementation deletes the "BootNext" before calling any PlatformBootManagerLib APIs, but if system resets in PlatformBootManagerLib APIs, "BootNext" is not consumed but lost.
The patch defers the deletion of "BootNext" to before booting it.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 35 ++++++++++++++++++--------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index ac5f9088dd..a6fe617b56 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -808,7 +808,8 @@ BdsEntry (
ASSERT_EFI_ERROR (Status);
//
- // Cache and remove the "BootNext" NV variable.
+ // Cache the "BootNext" NV variable before calling any
+ PlatformBootManagerLib APIs // This could avoid the "BootNext" set by PlatformBootManagerLib be consumed in this boot.
//
GetEfiGlobalVariable2 (EFI_BOOT_NEXT_VARIABLE_NAME, (VOID **) &BootNext, &DataSize);
if (DataSize != sizeof (UINT16)) {
@@ -817,17 +818,6 @@ BdsEntry (
}
BootNext = NULL;
}
- Status = gRT->SetVariable (
- EFI_BOOT_NEXT_VARIABLE_NAME,
- &gEfiGlobalVariableGuid,
- 0,
- 0,
- NULL
- );
- //
- // Deleting NV variable shouldn't fail unless it doesn't exist.
- //
- ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
//
// Initialize the platform language variables @@ -1052,10 +1042,25 @@ BdsEntry (
EfiBootManagerHotkeyBoot ();
- //
- // Boot to "BootNext"
- //
if (BootNext != NULL) {
+ //
+ // Delete "BootNext" NV variable before transferring control to it to prevent loops.
+ //
+ Status = gRT->SetVariable (
+ EFI_BOOT_NEXT_VARIABLE_NAME,
+ &gEfiGlobalVariableGuid,
+ 0,
+ 0,
+ NULL
+ );
+ //
+ // Deleting NV variable shouldn't fail unless it doesn't exist.
+ //
+ ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_FOUND);
+
+ //
+ // Boot to "BootNext"
+ //
UnicodeSPrint (BootNextVariableName, sizeof (BootNextVariableName), L"Boot%04x", *BootNext);
Status = EfiBootManagerVariableToLoadOption (BootNextVariableName, &LoadOption);
if (!EFI_ERROR (Status)) {
--
2.12.2.windows.2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-09-28 6:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-28 5:50 [PATCH] MdeModulePkg/BdsDxe: Don't delete "BootNext" until booting it Ruiyu Ni
2017-09-28 5:59 ` Zeng, Star
2017-09-28 6:01 ` Ni, Ruiyu
2017-09-28 6:13 ` Zeng, Star
2017-09-28 6:18 ` Ni, Ruiyu
2017-09-28 6:22 ` Zeng, Star
2017-09-28 6:46 ` Wang, Sunny (HPS SW)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox