From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-x241.google.com (mail-pf0-x241.google.com [IPv6:2607:f8b0:400e:c00::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CA10F2095D8C5 for ; Sun, 6 Aug 2017 06:55:20 -0700 (PDT) Received: by mail-pf0-x241.google.com with SMTP id h75so6549628pfh.5 for ; Sun, 06 Aug 2017 06:57:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=zKiiTcK9NMa584hA8HdGsNscTS07z0EfycdI9l1Wzu0=; b=HlBcosfcSB3VLTp1YlTj+mxzDVCZ/aoTtz9cFxg8A0nxsTvqv0YTF6GW272BuvU+BX 24pFcEbcFKEKpV3Ds2bZPCj0o96Jhy650fT5fnb07d22o0gm2YXCx0eHEW3k7J+tVHCW MQcrwVTqQznXJ+SJcD7ps+BoxORKedWlnhCwX9pDqZo4Uqlol0OYc18p5yfSUBzqf1HS SW6ofaQBrVbwxzeI75brrcTaXoD/9B1ZRjfSPWplUNVPwbNhUppxn3N9czzYuZXU+3Gq sJpg+6zEGi7FeUT+3a0wjxEQXQ7ui/N3mczCPocONWvMsL9UzGIIeRlxez6uetSKagIc rrkA== 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=zKiiTcK9NMa584hA8HdGsNscTS07z0EfycdI9l1Wzu0=; b=VADulEhTxm4QCaRZSvUMA8wv/lu6KQTO2t39m5yQdieqnvKdkaFkESAJ8WKEyqSywE 5A0T6IMqlhUo4hV9x6ioWMZna9RczesJ0G23kc5BFLFtFYYrb2UJLLC0E/jGKqhrqHMT zVekVavL5x3tI84THIefHVkMAhtSVddlQPMrWETD/U+5QBk4KcrhHY5Zus4DeM9CWJO9 CuDT1XMeEJr7cAErpH4rwWoIgnz2ffBPsUCaJKUoyK0EpTL1rifUtUwiw70t5CNqZd7s Kr9sCzECYFEl0XIBXRPZw2aq0r4yvvogaHihaQI7QgPN69usYt7NYzo5hg46t7x2eQzB kUJw== X-Gm-Message-State: AIVw110X5M8j5/fDs/oWvwwLLiqhpfzDIBM4q2BlAxes6H5zdj1Q7uA/ K9XbMGvnGVDZ9A== X-Received: by 10.98.79.151 with SMTP id f23mr8593851pfj.57.1502027855345; Sun, 06 Aug 2017 06:57:35 -0700 (PDT) Received: from localhost ([59.94.211.53]) by smtp.gmail.com with ESMTPSA id w66sm12461839pfi.63.2017.08.06.06.57.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 06 Aug 2017 06:57:34 -0700 (PDT) Date: Sun, 6 Aug 2017 19:27:31 +0530 From: Dhiru Kholia To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Message-ID: <20170806135731.GA19415@lonestar> References: <20170806121622.30494-1-lersek@redhat.com> <20170806121622.30494-2-lersek@redhat.com> MIME-Version: 1.0 In-Reply-To: <20170806121622.30494-2-lersek@redhat.com> User-Agent: Mutt/1.8.3 (2017-05-23) 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: Sun, 06 Aug 2017 13:55:21 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Aug 06, 2017 at 02:16:22PM +0200, 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=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; Tested-by: Dhiru Kholia I can now successfully boot macOS 10.12.5 + Clover combination with "pc-q35-2.4" machine type with this patch applied on top of commit 1fceaddb12. Thanks, Dhiru