From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 B87752095DBA8 for ; Sun, 6 Aug 2017 05:14:13 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 272EF1555A; Sun, 6 Aug 2017 12:16:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 272EF1555A Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-32.phx2.redhat.com [10.3.116.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id 185EC65E94; Sun, 6 Aug 2017 12:16:26 +0000 (UTC) From: Laszlo Ersek To: edk2-devel-01 Cc: Dhiru Kholia , Jordan Justen Date: Sun, 6 Aug 2017 14:16:22 +0200 Message-Id: <20170806121622.30494-2-lersek@redhat.com> In-Reply-To: <20170806121622.30494-1-lersek@redhat.com> References: <20170806121622.30494-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Sun, 06 Aug 2017 12:16:28 +0000 (UTC) Subject: [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: Sun, 06 Aug 2017 12:14:13 -0000 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=MvLU=w_LjRLXserO3NmsgHvaYE0aUCCWzg@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/AcpiPlatformDxe/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 transfered. @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 input 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 == 0) { + ReleaseS3Context (S3Context); + return EFI_SUCCESS; + } + Status = QemuFwCfgS3CallWhenBootScriptReady (AppendFwCfgBootScript, S3Context, sizeof (SCRATCH_BUFFER)); return (EFI_STATUS)Status; -- 2.13.1.3.g8be5a757fa67