From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web12.2373.1590601179467810461 for ; Wed, 27 May 2020 10:39:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=D9c5cyQQ; spf=pass (domain: nuviainc.com, ip: 209.85.128.67, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f67.google.com with SMTP id c71so259088wmd.5 for ; Wed, 27 May 2020 10:39:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Tez4PDrBjc8Y+AA3BO0OWpXtgrL8qFDHv5DtPV9vYEQ=; b=D9c5cyQQPju5W+NQOQX+f3flaGAkc+eC4UvuGruzr/SsgjlbSeGawmlliuFBRP+1TZ QCGknDXVzrHFdb7NeNgQMn9dOAUPXQs69pJVkeqCZkPyU/8szUdll2K1egcQ3kKLhcx3 zWvx0Fhj8y/7HK5dprVae9H8bGdOlHa3NZTzWapMMfSBryejCoFwm8+hx5d0DdYksx6r nUW49ciSUz3kVlBTJsL5y2OFmSoRlGUdj1RUeyONRbWoVB+8dK0rxQD6K+84amcPdEwj 7sBXrQAAQbymWcgjSzTzYcQPXQKi9bzU106xMteUjHN2WtFqTWth7Cgkptuyu6ZsiG0n PX3Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Tez4PDrBjc8Y+AA3BO0OWpXtgrL8qFDHv5DtPV9vYEQ=; b=KZpHUiur3qaN2kzpptMwXh6A/cL6wKLFbxY9Bu2f04VDNa6kqTlx0hPOZbMtwHLCsD kXIUg6P2MxVETYdHx8iIteaggspHD3lFheOQmm5dMOEqEnnXyKqvkUr830I+mDErInOL xduTXp7tibQiHeVMcfG+B7ThQq+IN3tIitLZrKsVR5QUiHKGtX1cWJKPGO0ttWwv4pKE AtrLDJlwKxmrhCyDjwYuTTBWRTivrvtsMfSAPSdGV9eKx/Nqmyiht0LOChgobOwdtOio W4OvHDU9VocIxrUgpzjKFJ4ygpa+6D2Np7OwYAjlwv/nt3F1+DwjHCF73OGwaW+PS3Bv XwYw== X-Gm-Message-State: AOAM533JHHLFKaadvUjea3KF+DHeqUS95szWbHB7wUdOI84UuZli9vUB 9b8s6tBMU+V34UY9dkwlFqpXHA== X-Google-Smtp-Source: ABdhPJwnAbQhUKTW4rIXlgyUCAGIOQe30shgogrbcd3KEumTNJbaIE4cA5Lg/PCd5R5laQlRmCTgoQ== X-Received: by 2002:a1c:6243:: with SMTP id w64mr3251273wmb.162.1590601177908; Wed, 27 May 2020 10:39:37 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id z25sm3582166wmf.10.2020.05.27.10.39.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 May 2020 10:39:37 -0700 (PDT) Date: Wed, 27 May 2020 18:39:35 +0100 From: "Leif Lindholm" To: Laszlo Ersek Cc: devel@edk2.groups.io, ard.biesheuvel@arm.com, jon@solid-run.com Subject: Re: [edk2-devel] [PATCH 2/5] ArmPkg/PlatformBootManagerLib: fall back to the UiApp on boot failure Message-ID: <20200527173935.GZ1923@vanye> References: <20200526161359.4810-1-ard.biesheuvel@arm.com> <20200526161359.4810-3-ard.biesheuvel@arm.com> <20200526212453.GT1923@vanye> <89a08f27-cda1-a144-bccc-80aecc7751f0@redhat.com> MIME-Version: 1.0 In-Reply-To: <89a08f27-cda1-a144-bccc-80aecc7751f0@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, May 27, 2020 at 17:34:13 +0200, Laszlo Ersek wrote: > Hi Leif, > > On 05/26/20 23:24, Leif Lindholm wrote: > > On Tue, May 26, 2020 at 18:13:56 +0200, Ard Biesheuvel wrote: > >> As a last resort, drop into the UiApp application when no active boot > >> options could be started. Doing so will connect all devices, and so > >> it will allow the user to enter the Boot Manager submenu and pick a > >> network or removable disk option. With the right UiApp library added > >> in, the UiApp also gives access to the UEFI Shell. > >> > >> Signed-off-by: Ard Biesheuvel > >> --- > >> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 +++++++++++++++- > >> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > >> index 23c925bbdb9c..f91f7cd09ca1 100644 > >> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > >> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c > >> @@ -830,5 +830,19 @@ PlatformBootManagerUnableToBoot ( > >> VOID > >> ) > >> { > >> - return; > >> + EFI_STATUS Status; > >> + EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu; > >> + > >> + // > >> + // BootManagerMenu doesn't contain the correct information when return status > >> + // is EFI_NOT_FOUND. > >> + // > >> + Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu); > >> + if (EFI_ERROR (Status)) { > > > > Nitpick: comment explicitly mentions EFI_NOT_FOUND, but code checks > > for any EFI_ERROR match. Since there are various other error codes > > that could be returned, change the comment to "when return status is > > not EFI_SUCCESS"? > > I agree the (likely) original code (see commit 5f66615bb504, > "OvmfPkg/PlatformBds: Implement PlatformBootManagerUnableToBoot", > 2018-07-27) is a bit confusing. > > Namely, both the comment and the subsequent error check are correct. The > problem is that there is not much connection between the comment and the > subsequent check. In other words, the comment does not *explain* the > check. > > The EfiBootManagerGetBootManagerMenu() function in > "MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c" is documented like > this: > > > /** > > Return the boot option corresponding to the Boot Manager Menu. > > It may automatically create one if the boot option hasn't been created yet. > > > > @param BootOption Return the Boot Manager Menu. > > > > @retval EFI_SUCCESS The Boot Manager Menu is successfully returned. > > @retval EFI_NOT_FOUND The Boot Manager Menu cannot be found. > > @retval others Return status of gRT->SetVariable (). BootOption still points > > to the Boot Manager Menu even the Status is not EFI_SUCCESS > > and EFI_NOT_FOUND. > > **/ > > So the comment change you're proposing wouldn't be technically correct, > I believe. > > I think at best we should drop the comment altogether. If > EfiBootManagerGetBootManagerMenu() fails due to EFI_NOT_FOUND, then > "BootManagerMenu" is indeterminate, so we need to bail. If > EfiBootManagerGetBootManagerMenu() fails with something else, then > "BootManagerMenu" is set, but we *still* need to bail (as much as I > understand from the EfiBootManagerGetBootManagerMenu() documentation). > And that seems to mean we should simply not highlight EFI_NOT_FOUND with > a comment. Ah, I see what you're saying. Yeah, that works. / Leif