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 <dhiru.kholia@gmail.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Fixes: df73df138d9d53f7f7570f4fe97a6cde941a2656
Fixes: 805762252733bb67bc5157f0137c64e010724c77
Reported-by: Dhiru Kholia <dhiru.kholia@gmail.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Dhiru Kholia <dhiru.kholia@gmail.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
index a7d2f9e38f5742f2b9894b5234a5705d39e56a30..1a188bfd615cd914720faefe6ba5475632caa338 100644 (file)
--- a/OvmfPkg/AcpiPlatformDxe/BootScript.c
+++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
@@ -249,7 +249,10 @@ FatalError:
                         because the ownership of S3Context has been transfered.\r
 \r
   @retval EFI_SUCCESS The translation of S3Context to ACPI S3 Boot Script\r
-                      opcodes has been successfully executed or queued.\r
+                      opcodes has been successfully executed or queued. (This\r
+                      includes the case when S3Context was empty on input and\r
+                      no ACPI S3 Boot Script opcodes have been necessary to\r
+                      produce.)\r
 \r
   @return             Error codes from underlying functions.\r
 **/\r
@@ -260,6 +263,11 @@ TransferS3ContextToBootScript (
 {\r
   RETURN_STATUS Status;\r
 \r
+  if (S3Context->Used == 0) {\r
+    ReleaseS3Context (S3Context);\r
+    return EFI_SUCCESS;\r
+  }\r
+\r
   Status = QemuFwCfgS3CallWhenBootScriptReady (AppendFwCfgBootScript,\r
              S3Context, sizeof (SCRATCH_BUFFER));\r
   return (EFI_STATUS)Status;\r