From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web08.9802.1612453755620939087 for ; Thu, 04 Feb 2021 07:49:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=ialVfNtD; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id 0E3FA64F5C for ; Thu, 4 Feb 2021 15:49:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1612453755; bh=SrBnt9cVgRMsgY8OqlfzGzVAlQ0J411DqToYEKxcPXk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ialVfNtDHoTCSbP8vVnDgGl2SvPvzip0d1bX78HPLzvTch5xjKYK9QKizTXQJXwoq J5FQatlW39ZVFfO4PTx2nsNnbvDa3TYrunnOtTWkU28z0GSnyrKitSu9VErIaYQaRJ w9RMdKBKPx7YnoU9GWtsgDnPPmzqOagibOkXLImfI4ljRIhBhAgCzO2facBbwOwED9 GNhdVE7ymagXhPo2iUioNxvxzlUZyUQZf0nxc4FrlaEbCugM9nbI6AhQNH5dqz70wz nzpT5MfHHd+xxeJm9vpr5hdGqNIdYvzPqRxMw1p0cLIhklF9HfHNUAz46kQ1gG6tIN 6IWtcRWHg/Qyg== Received: by mail-oi1-f176.google.com with SMTP id d20so4115715oiw.10 for ; Thu, 04 Feb 2021 07:49:15 -0800 (PST) X-Gm-Message-State: AOAM533lLTvXyQWg5eNx0NDqe5vKxNuJZSlF0y3h+rw4hj85W3yHEjH1 flIAUpgI/5m9vEefp93ABdn22+v1FfXAKZQz7A4= X-Google-Smtp-Source: ABdhPJy4nilz/U3ElNBM2SMwnXXmhrrSpzq9rVYp8kukrxZ0SYEJRRAdk1Vc4JQEmugzf38lv4j+boHutCFuDRSxp58= X-Received: by 2002:aca:110b:: with SMTP id 11mr29601oir.174.1612453754359; Thu, 04 Feb 2021 07:49:14 -0800 (PST) MIME-Version: 1.0 References: <20210204140601.9215-1-leif@nuviainc.com> <2f86afc5-eecb-8c63-2c40-01e380ab301f@redhat.com> In-Reply-To: <2f86afc5-eecb-8c63-2c40-01e380ab301f@redhat.com> From: "Ard Biesheuvel" Date: Thu, 4 Feb 2021 16:49:03 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 1/1] ArmPkg/Library: prevent endless reboot loop with emulated NV varstore To: Laszlo Ersek Cc: Leif Lindholm , devel@edk2.groups.io, Ard Biesheuvel Content-Type: text/plain; charset="UTF-8" On Thu, 4 Feb 2021 at 15:17, Laszlo Ersek 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 > > Cc: Laszlo Ersek > > Signed-off-by: Leif Lindholm > > --- > > > > 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 > > Ard should please review this patch for the logic change. > Reviewed-by: Ard Biesheuvel