From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9904A21DF9678 for ; Mon, 7 Aug 2017 12:46:00 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Aug 2017 12:48:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,339,1498546800"; d="scan'208";a="137403770" Received: from ldtyree-mobl.amr.corp.intel.com (HELO localhost) ([10.255.79.10]) by fmsmga006.fm.intel.com with ESMTP; 07 Aug 2017 12:48:16 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <150213529590.25520.9142627387449032940@jljusten-skl> From: Jordan Justen In-Reply-To: <20170806121622.30494-2-lersek@redhat.com> References: <20170806121622.30494-1-lersek@redhat.com> <20170806121622.30494-2-lersek@redhat.com> User-Agent: alot/0.5.1 Date: Mon, 07 Aug 2017 12:48:16 -0700 Subject: Re: [PATCH 1/1] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Aug 2017 19:46:00 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jordan Justen On 2017-08-06 05:16:22, Laszlo Ersek wrote: > In commit 805762252733 ("OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script > with QemuFwCfgS3Lib", 2017-02-23), we replaced the explicit S3 boot script > manipulation in TransferS3ContextToBootScript() with a call to > QemuFwCfgS3CallWhenBootScriptReady(). (Passing AppendFwCfgBootScript() as > callback.) > = > QemuFwCfgS3CallWhenBootScriptReady() checks for fw_cfg DMA up-front, and > bails with RETURN_NOT_FOUND if fw_cfg DMA is missing. > = > (This is justified as the goal of QemuFwCfgS3Lib is to "enable[] driver > modules [...] to produce fw_cfg DMA operations that are to be replayed at > S3 resume time".) > = > In turn, if QemuFwCfgS3CallWhenBootScriptReady() fails, then > OvmfPkg/AcpiPlatformDxe rolls back any earlier linker/loader script > processing, and falls back to the built-in ACPI tables. > = > (This is also justified because failure to save WRITE_POINTER commands for > replaying at S3 resume implies failure to process the linker/loader script > comprehensively.) > = > Calling QemuFwCfgS3CallWhenBootScriptReady() from > TransferS3ContextToBootScript() *unconditionally* is wrong however. For > the case when the linker/loader script contains no WRITE_POINTER commands, > the call perpetuated an earlier side effect, and introduced another one: > = > (1) On machine types that provide fw_cfg DMA (i.e., 2.5+), > QemuFwCfgS3CallWhenBootScriptReady() would succeed, and allocate > workspace for the boot script opcodes in reserved memory. However, no > opcodes would actually be produced in the AppendFwCfgBootScript() > callback, due to lack of any WRITE_POINTER commands. > = > This waste of reserved memory had been introduced in earlier commit > df73df138d9d ("OvmfPkg/AcpiPlatformDxe: replay > QEMU_LOADER_WRITE_POINTER commands at S3", 2017-02-09). > = > (2) On machine types that lack fw_cfg DMA (i.e., 2.4 and earlier), > TransferS3ContextToBootScript() would now fail the linker/loader > script for no reason. > = > (Note that QEMU itself prevents adding devices that depend on > WRITE_POINTER if the machine type lacks fw_cfg DMA: > = > $ qemu-system-x86_64 -M pc-q35-2.4 -device vmgenid > = > qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write > support in fw_cfg, which this machine type does not provide) > = > Short-circuit an empty S3_CONTEXT in TransferS3ContextToBootScript() by > dropping S3_CONTEXT on the floor. This is compatible with the current > contract of the function as it constitutes a transfer of ownership. > = > Regression (2) was found and reported by Dhiru Kholia as an OSX guest boot > failure on the "pc-q35-2.4" machine type: > = > http://mid.mail-archive.com/CANO7a6x6EaWNZ8y=3DMvLU=3Dw_LjRLXserO3NmsgHva= YE0aUCCWzg@mail.gmail.com > = > Dhiru bisected the issue to commit 805762252733. > = > Cc: Dhiru Kholia > Cc: Jordan Justen > Fixes: df73df138d9d53f7f7570f4fe97a6cde941a2656 > Fixes: 805762252733bb67bc5157f0137c64e010724c77 > Reported-by: Dhiru Kholia > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/AcpiPlatformDxe/BootScript.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > = > diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformD= xe/BootScript.c > index a7d2f9e38f57..1a188bfd615c 100644 > --- a/OvmfPkg/AcpiPlatformDxe/BootScript.c > +++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c > @@ -249,7 +249,10 @@ FatalError: > because the ownership of S3Context has been tran= sfered. > = > @retval EFI_SUCCESS The translation of S3Context to ACPI S3 Boot Script > - opcodes has been successfully executed or queued. > + opcodes has been successfully executed or queued. = (This > + includes the case when S3Context was empty on inpu= t and > + no ACPI S3 Boot Script opcodes have been necessary= to > + produce.) > = > @return Error codes from underlying functions. > **/ > @@ -260,6 +263,11 @@ TransferS3ContextToBootScript ( > { > RETURN_STATUS Status; > = > + if (S3Context->Used =3D=3D 0) { > + ReleaseS3Context (S3Context); > + return EFI_SUCCESS; > + } > + > Status =3D QemuFwCfgS3CallWhenBootScriptReady (AppendFwCfgBootScript, > S3Context, sizeof (SCRATCH_BUFFER)); > return (EFI_STATUS)Status; > -- = > 2.13.1.3.g8be5a757fa67 >=20