From 9e2a8e928995c3b1bb664b73fd59785055c6b5f6 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Sun, 6 Aug 2017 11:20:02 +0200 Subject: OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT 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 Tested-by: Dhiru Kholia Reviewed-by: Jordan Justen --- 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 a7d2f9e..1a188bf 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; -- cgit v1.1