From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.8741.1590593662053862786 for ; Wed, 27 May 2020 08:34:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Bv2NMivs; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590593661; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tKcveQI4+wL3kpRl5Qgf3IsIvq3xTmg7c3wHlSpK4lY=; b=Bv2NMivswwvCxyiYoVQuRqy69F4rxk0qV5sPIuAs9TpoYxa/hSHYR0CkoPIDch2G+BVusv 6/L4drfzmY/520xRUpAZZos7+YaXn0zStHF50D6vhLjJ1KGOYoZDSoqVLLXwANKZzLvnb7 Ao+HHWdtcByvRU5ofStBigBcHCPD6jg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-284-w34p4OCmPry5trWaSH_8bA-1; Wed, 27 May 2020 11:34:17 -0400 X-MC-Unique: w34p4OCmPry5trWaSH_8bA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 400808064DD; Wed, 27 May 2020 15:34:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-77.ams2.redhat.com [10.36.113.77]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3749478B2C; Wed, 27 May 2020 15:34:14 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/5] ArmPkg/PlatformBootManagerLib: fall back to the UiApp on boot failure To: devel@edk2.groups.io, leif@nuviainc.com, ard.biesheuvel@arm.com Cc: jon@solid-run.com References: <20200526161359.4810-1-ard.biesheuvel@arm.com> <20200526161359.4810-3-ard.biesheuvel@arm.com> <20200526212453.GT1923@vanye> From: "Laszlo Ersek" Message-ID: <89a08f27-cda1-a144-bccc-80aecc7751f0@redhat.com> Date: Wed, 27 May 2020 17:34:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200526212453.GT1923@vanye> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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. Thanks Laszlo