Re: [PATCH v2 1/1] ArmPkg/Library: prevent endless reboot loop with emulated NV varstore

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 4 Feb 2021 at 15:17, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 02/04/21 15:06, Leif Lindholm wrote:
> > If no valid boot options were found, PlatformBootManagerLib refreshes a
> > set of sane default options and then reboots. However, if there is in
> > fact no persistent varstore, the same thing happens again on next boot,
> > and we end up in an endlessly rebooting loop.
> >
> > So when PcdEmuVariableNvModeEnable is TRUE, skip the reboot step and
> > enter the setup menu instead.
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >
> > Changes in v2:
> > - Fix indentation.
> > - Add missing space in commit message.
> >
> >  ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  1 +
> >  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 12 ++++++++----
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > index 2f726d117d7d..353d7a967b76 100644
> > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > @@ -55,6 +55,7 @@ [FeaturePcd]
> >    gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport
> >
> >  [FixedPcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> >    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
> >    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
> > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > index 9905cad22908..5ceb23d822e5 100644
> > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > @@ -848,11 +848,15 @@ PlatformBootManagerUnableToBoot (
> >    // If the number of configured boot options has changed, reboot
> >    // the system so the new boot options will be taken into account
> >    // while executing the ordinary BDS bootflow sequence.
> > +  // *Unless* persistent varstore is being emulated, since we would
> > +  // then end up in an endless reboot loop.
> >    //
> > -  if (NewBootOptionCount != OldBootOptionCount) {
> > -    DEBUG ((DEBUG_WARN, "%a: rebooting after refreshing all boot options\n",
> > -      __FUNCTION__));
> > -    gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> > +  if (!PcdGetBool (PcdEmuVariableNvModeEnable)) {
> > +    if (NewBootOptionCount != OldBootOptionCount) {
> > +      DEBUG ((DEBUG_WARN, "%a: rebooting after refreshing all boot options\n",
> > +        __FUNCTION__));
> > +      gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> > +    }
> >    }
> >
> >    Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> >
>
> At the level where I commented on v1 -- i.e., totally superficially --:
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> Ard should please review this patch for the logic change.
>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>