From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id A4023941D67 for ; Thu, 26 Oct 2023 16:19:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=3bkpzBZpFkgq5cqRCS76C2TsxByyh2Z06SbRgY3Cjc8=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698337153; v=1; b=MxfR3mcwmgpvhWLvmtj31B2qIFWBwF9Vd//yD3u2bRYISQNUeCukPk9rg+7xnb/pTvcVxP4G n2jfj3RSYew8XTvJ5TYEZpchQUMHgqdBiNXMPTnvoUqCCkJl7fhlqwpZqRJ9jO8dYL4jAJ7JdsL jlkoTNX5ODCmX2TtyQqLmlT4= X-Received: by 127.0.0.2 with SMTP id Hty5YY7687511xIywc2HisWQ; Thu, 26 Oct 2023 09:19:13 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.75180.1698337152698901302 for ; Thu, 26 Oct 2023 09:19:12 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-235-nWQpGRWIOEWONiaDuSI9Qw-1; Thu, 26 Oct 2023 12:19:06 -0400 X-MC-Unique: nWQpGRWIOEWONiaDuSI9Qw-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 335E8810BCF; Thu, 26 Oct 2023 16:19:06 +0000 (UTC) X-Received: from [10.39.192.119] (unknown [10.39.192.119]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9B5151C060AE; Thu, 26 Oct 2023 16:19:04 +0000 (UTC) Message-ID: <38bf5698-07be-3421-ad0b-5dd028e9155b@redhat.com> Date: Thu, 26 Oct 2023 18:19:03 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery To: devel@edk2.groups.io, ngompa13@gmail.com Cc: Neal Gompa , Pete Batard , =?UTF-8?Q?Daniel_P_=2e_Berrang=c3=a9?= , Gerd Hoffmann , Samer El-Haj-Mahmoud References: <20231026135324.15914-1-ngompa@fedoraproject.org> From: "Laszlo Ersek" In-Reply-To: <20231026135324.15914-1-ngompa@fedoraproject.org> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 9SK2ucdiOzung2fpc6TczO3xx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=MxfR3mcw; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 10/26/23 15:53, Neal Gompa wrote: > From: Neal Gompa > > Currently, the ReadyToBoot event is only signaled when a formal Boot > Manager option is executed (in BmBoot.c -> EfiBootManagerBoot ()). > > However, the introduction of Platform Recovery in UEFI 2.5 makes it > necessary to signal ReadyToBoot when a Platform Recovery boot loader > runs because otherwise it may lead to the execution of a boot loader > that has similar requirements to a regular one that is not launched > as a Boot Manager option. > > This is especially critical to ensuring that the graphical console > is actually usable during platform recovery, as some platforms do > rely on the ConsolePrefDxe driver, which only performs console > initialization after ReadyToBoot is triggered. > > This patch fixes that behavior by calling EfiSignalEventReadyToBoot () > in EfiBootManagerProcessLoadOption (), which is the function that sets > up the platform recovery boot process. > > The expected behavior has been clarified in the UEFI 2.10 specification > to explicitly indicate this behavior is required for correct operation. > > This is a rebased version of the patch originally written by Pete Batard. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2831 > > Cc: Pete Batard > Cc: Daniel P. Berrangé > Cc: Gerd Hoffmann > Cc: Samer El-Haj-Mahmoud > > Co-authored-by: Pete Batard > Signed-off-by: Neal Gompa > --- > MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c > index 2087f0b91d..31ed608817 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c > @@ -1416,6 +1416,15 @@ EfiBootManagerProcessLoadOption ( > return EFI_SUCCESS; > } > > + // > + // Signal the EVT_SIGNAL_READY_TO_BOOT event when we are about to load and execute the boot option. > + // > + EfiSignalEventReadyToBoot (); > + // > + // Report Status Code to indicate ReadyToBoot was signalled > + // > + REPORT_STATUS_CODE (EFI_PROGRESS_CODE, (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT)); > + > // > // Load and start the load option. > // While the patch does the right thing for the latest UEFI spec language, it does a bit too much. The spec says (v2.10), under 7.1.2 "EFI_BOOT_SERVICES.CreateEventEx()": ----------- EFI_EVENT_GROUP_READY_TO_BOOT This event group is notified by the system right before notifying EFI_EVENT_GROUP_AFTER_READY_TO_BOOT event group when the Boot Manager is about to load and execute a boot option or a platform or OS recovery option. The event group presents the last chance to modify device or system configuration prior to passing control to a boot option. ----------- NB "boot option", or "platform or OS recovery option". However, EfiBootManagerProcessLoadOption() is also used for launching Driver#### and SysPrep#### options. EfiBootManagerProcessLoadOption() has two call sites: (1) in ProcessLoadOptions() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c] (2) near the end of BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c] Call site (2) bears the comment "When platform recovery is not enabled, still boot to platform default file path", so I figure signaling ready-to-boot at that point is fine. Call site (1) requires further investigation. Namely, ProcessLoadOptions() itself is called from three locations, all inside BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]: (1.1) under comment "Execute Driver Options", for "LoadOptionTypeDriver" type load options (1.2) under comment "Execute SysPrep####", for "LoadOptionTypeSysPrep" type load options (1.3) for "LoadOptionTypePlatformRecovery" type load options. I figure the intended extension is for case (1.3), but the patch, as proposed, will also affect (1.1) and (1.2); that is, when Driver#### and SysPrep#### options are processes. That's beyond what the spec says, in my opinion. Now, EfiBootManagerProcessLoadOption() takes a pointer to an EFI_BOOT_MANAGER_LOAD_OPTION structure. This structure has a field called OptionType (of type EFI_BOOT_MANAGER_LOAD_OPTION_TYPE). You might want to restrict the signaling and the status code reporting to when LoadOption->OptionType is either LoadOptionTypeBoot or LoadOptionTypePlatformRecovery (i.e., exclude LoadOptionTypeDriver and LoadOptionTypeSysPrep). ( I've made an attempt to check the above-noted four call paths (i.e., (1.1) through (1.3), and (2)), and I *think* the OptionType field is populated on all of them; thus, the check I recommend should be safe. Call paths (1.1) through (1.3) call EfiBootManagerGetLoadOptions(), which sets the OptionType field in each returned structure according to the input parameter "LoadOptionType" (note the tricky call to EfiBootManagerVariableToLoadOption()). And (2) relies on "PlatformDefaultBootOption", which is created in BdsEntry() with LoadOptionTypePlatformRecovery. ) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110126): https://edk2.groups.io/g/devel/message/110126 Mute This Topic: https://groups.io/mt/102200076/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-