[PATCH 0/5] OvmfPkg: support QEMU_LOADER_WRITE_POINTER

[PATCH 0/5] OvmfPkg: support QEMU_LOADER_WRITE_POINTER

[Laszlo Ersek]

This patch series adds support for the new WRITE_POINTER command of
QEMU's ACPI linker/loader.

The command's first use case is QEMU's implementation of the "Microsoft
Hyper-V Generation Counter" device (a.k.a. "VMGENID"). The latest
version of the QEMU patch series (v7) can be found at:

  [Qemu-devel] [PATCH v7 0/8] Add support for VM Generation ID
  msgid <cover.1487224954.git.ben@skyportsystems.com>
  http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03456.html

The VMGENID device is publicly specified by Microsoft. Please find the
reference, and how QEMU implements the specification in cooperation with
guest firmware (OVMF and SeaBIOS), at:

  [Qemu-devel] [PATCH v7 2/8] docs: VM Generation ID device description
  msgid <6e7af53f114f4875fbb952fb047d9d91bba2634f.1487224954.git.ben@skyportsystems.com>
  http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03459.html

The WRITE_POINTER command is a building block for this.

While minuscule tweaks are expected in v8 of the QEMU series, we have
finally stabilized the QEMU<->firmware interface, over a long and taxing
series of emails and patch set versions. I've successfully tested basic
VMGENID functionality (initial setup, guest memory contents, ACPI S3
suspend/resume, device status) with RHEL-7 and Windows Server 2012 R2
guests.

This series can be fetched from <https://github.com/lersek/edk2.git>,
branch "write_pointer".

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359

Thanks
Laszlo

Laszlo Ersek (5):
  OvmfPkg/AcpiPlatformDxe: prepare for QEMU_LOADER_WRITE_POINTER
    definitions
  OvmfPkg/AcpiPlatformDxe: add QEMU_LOADER_WRITE_POINTER definitions
  OvmfPkg/AcpiPlatformDxe: rewrap license block in "QemuFwCfgAcpi.c"
  OvmfPkg/AcpiPlatformDxe: implement the QEMU_LOADER_WRITE_POINTER
    command
  OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at
    S3

 OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf          |   2 +
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf |   2 +
 OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h               |  27 ++
 OvmfPkg/AcpiPlatformDxe/QemuLoader.h                 |  36 +-
 OvmfPkg/AcpiPlatformDxe/BootScript.c                 | 414 ++++++++++++++++++++
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c              | 245 +++++++++++-
 6 files changed, 711 insertions(+), 15 deletions(-)
 create mode 100644 OvmfPkg/AcpiPlatformDxe/BootScript.c

-- 
2.9.3

[PATCH 1/5] OvmfPkg/AcpiPlatformDxe: prepare for QEMU_LOADER_WRITE_POINTER definitions

[Laszlo Ersek]

No functional changes in this patch, just prepare the grounds with some
reformatting (trailing comma after the last enumeration constant,
horizontal whitespace insertion) so that the next patch can be cleaner.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuLoader.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
index 84dec06422d6..b29944378d76 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
+++ b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
@@ -25,11 +25,11 @@
 #define QEMU_LOADER_FNAME_SIZE QEMU_FW_CFG_FNAME_SIZE
 
 typedef enum {
   QemuLoaderCmdAllocate = 1,
   QemuLoaderCmdAddPointer,
-  QemuLoaderCmdAddChecksum
+  QemuLoaderCmdAddChecksum,
 } QEMU_LOADER_COMMAND_TYPE;
 
 typedef enum {
   QemuLoaderAllocHigh = 1,
   QemuLoaderAllocFSeg
@@ -73,14 +73,14 @@ typedef struct {
 } QEMU_LOADER_ADD_CHECKSUM;
 
 typedef struct {
   UINT32 Type;                             // QEMU_LOADER_COMMAND_TYPE values
   union {
-    QEMU_LOADER_ALLOCATE     Allocate;
-    QEMU_LOADER_ADD_POINTER  AddPointer;
-    QEMU_LOADER_ADD_CHECKSUM AddChecksum;
-    UINT8                    Padding[124];
+    QEMU_LOADER_ALLOCATE      Allocate;
+    QEMU_LOADER_ADD_POINTER   AddPointer;
+    QEMU_LOADER_ADD_CHECKSUM  AddChecksum;
+    UINT8                     Padding[124];
   } Command;
 } QEMU_LOADER_ENTRY;
 #pragma pack ()
 
 #endif
-- 
2.9.3

[PATCH 2/5] OvmfPkg/AcpiPlatformDxe: add QEMU_LOADER_WRITE_POINTER definitions

[Laszlo Ersek]

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuLoader.h | 26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
index b29944378d76..437776d86d9a 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
+++ b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
@@ -26,10 +26,11 @@
 
 typedef enum {
   QemuLoaderCmdAllocate = 1,
   QemuLoaderCmdAddPointer,
   QemuLoaderCmdAddChecksum,
+  QemuLoaderCmdWritePointer,
 } QEMU_LOADER_COMMAND_TYPE;
 
 typedef enum {
   QemuLoaderAllocHigh = 1,
   QemuLoaderAllocFSeg
@@ -70,16 +71,41 @@ typedef struct {
   UINT32 ResultOffset;
   UINT32 Start;
   UINT32 Length;
 } QEMU_LOADER_ADD_CHECKSUM;
 
+//
+// QemuLoaderCmdWritePointer: the bytes at
+// [PointerOffset..PointerOffset+PointerSize) in the writeable fw_cfg file
+// PointerFile are to receive the absolute address of PointeeFile, as allocated
+// and downloaded by the firmware, incremented by the value of PointeeOffset.
+// Store the sum of (a) the base address of where PointeeFile's contents have
+// been placed (when QemuLoaderCmdAllocate has been executed for PointeeFile)
+// and (b) PointeeOffset, to this portion of PointerFile.
+//
+// This command is similar to QemuLoaderCmdAddPointer; the difference is that
+// the "pointer to patch" does not exist in guest-physical address space, only
+// in "fw_cfg file space". In addition, the "pointer to patch" is not
+// initialized by QEMU in-place with a possibly nonzero offset value: the
+// relative offset into PointeeFile comes from the explicit PointeeOffset
+// field.
+//
+typedef struct {
+  UINT8  PointerFile[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
+  UINT8  PointeeFile[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
+  UINT32 PointerOffset;
+  UINT32 PointeeOffset;
+  UINT8  PointerSize;                         // one of 1, 2, 4, 8
+} QEMU_LOADER_WRITE_POINTER;
+
 typedef struct {
   UINT32 Type;                             // QEMU_LOADER_COMMAND_TYPE values
   union {
     QEMU_LOADER_ALLOCATE      Allocate;
     QEMU_LOADER_ADD_POINTER   AddPointer;
     QEMU_LOADER_ADD_CHECKSUM  AddChecksum;
+    QEMU_LOADER_WRITE_POINTER WritePointer;
     UINT8                     Padding[124];
   } Command;
 } QEMU_LOADER_ENTRY;
 #pragma pack ()
 
-- 
2.9.3

[PATCH 3/5] OvmfPkg/AcpiPlatformDxe: rewrap license block in "QemuFwCfgAcpi.c"

[Laszlo Ersek]

The longest line is currently 84 characters long.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index deaf14c6b0dc..404589cad0b7 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -2,17 +2,17 @@
   OVMF ACPI support using QEMU's fw-cfg interface
 
   Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.<BR>
   Copyright (C) 2012-2014, Red Hat, Inc.
 
-  This program and the accompanying materials
-  are licensed and made available under the terms and conditions of the BSD License
-  which accompanies this distribution.  The full text of the license may be found at
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution.  The full text of the license may be found at
   http://opensource.org/licenses/bsd-license.php
 
-  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 **/
 
 #include "AcpiPlatform.h"
 #include "QemuLoader.h"
-- 
2.9.3

[PATCH 4/5] OvmfPkg/AcpiPlatformDxe: implement the QEMU_LOADER_WRITE_POINTER command

[Laszlo Ersek]

The QEMU_LOADER_WRITE_POINTER command instructs the firmware to write the
address of a field within a previously allocated/downloaded fw_cfg blob
into another (writeable) fw_cfg file at a specific offset.

Put differently, QEMU_LOADER_WRITE_POINTER propagates, to QEMU, the
address that QEMU_LOADER_ALLOCATE placed the designated fw_cfg blob at, as
adjusted for the given field inside the allocated blob.

The implementation is similar to that of QEMU_LOADER_ADD_POINTER. Since
here we "patch" a pointer object in "fw_cfg file space", not guest memory
space, we utilize the QemuFwCfgSkipBytes() and QemuFwCfgWriteBytes() APIs
completed in commit range 465663e9f128..7fcb73541299.

An interesting aspect is that QEMU_LOADER_WRITE_POINTER creates a
host-level reference to a guest memory location. Therefore, if we fail to
process the linker/loader script for any reason, we have to clear out
those references first, before we release the guest memory allocations in
response to the error.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 171 +++++++++++++++++++-
 1 file changed, 168 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 404589cad0b7..de827c2df204 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -350,10 +350,147 @@ ProcessCmdAddChecksum (
     AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length));
   return EFI_SUCCESS;
 }
 
 
+/**
+  Process a QEMU_LOADER_WRITE_POINTER command.
+
+  @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
+
+  @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
+                            structures created thus far.
+
+  @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
+                              WritePointer. Or, the WritePointer command
+                              references a file unknown to Tracker or the
+                              fw_cfg directory. Or, the pointer object to
+                              rewrite has invalid location, size, or initial
+                              relative value. Or, the pointer value to store
+                              does not fit in the given pointer size.
+
+  @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
+                              file has been written.
+**/
+STATIC
+EFI_STATUS
+ProcessCmdWritePointer (
+  IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
+  IN     CONST ORDERED_COLLECTION        *Tracker
+  )
+{
+  RETURN_STATUS            Status;
+  FIRMWARE_CONFIG_ITEM     PointerItem;
+  UINTN                    PointerItemSize;
+  ORDERED_COLLECTION_ENTRY *PointeeEntry;
+  BLOB                     *PointeeBlob;
+  UINT64                   PointerValue;
+
+  if (WritePointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' ||
+      WritePointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
+    DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
+             &PointerItem, &PointerItemSize);
+  PointeeEntry = OrderedCollectionFind (Tracker, WritePointer->PointeeFile);
+  if (RETURN_ERROR (Status) || PointeeEntry == NULL) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: invalid fw_cfg file or blob reference \"%a\" / \"%a\"\n",
+      __FUNCTION__, WritePointer->PointerFile, WritePointer->PointeeFile));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  if ((WritePointer->PointerSize != 1 && WritePointer->PointerSize != 2 &&
+       WritePointer->PointerSize != 4 && WritePointer->PointerSize != 8) ||
+      (PointerItemSize < WritePointer->PointerSize) ||
+      (PointerItemSize - WritePointer->PointerSize <
+       WritePointer->PointerOffset)) {
+    DEBUG ((DEBUG_ERROR, "%a: invalid pointer location or size in \"%a\"\n",
+      __FUNCTION__, WritePointer->PointerFile));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  PointeeBlob = OrderedCollectionUserStruct (PointeeEntry);
+  PointerValue = WritePointer->PointeeOffset;
+  if (PointerValue >= PointeeBlob->Size) {
+    DEBUG ((DEBUG_ERROR, "%a: invalid PointeeOffset\n", __FUNCTION__));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  //
+  // The memory allocation system ensures that the address of the byte past the
+  // last byte of any allocated object is expressible (no wraparound).
+  //
+  ASSERT ((UINTN)PointeeBlob->Base <= MAX_ADDRESS - PointeeBlob->Size);
+
+  PointerValue += (UINT64)(UINTN)PointeeBlob->Base;
+  if (RShiftU64 (
+        RShiftU64 (PointerValue, WritePointer->PointerSize * 8 - 1), 1) != 0) {
+    DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
+      __FUNCTION__, WritePointer->PointerFile));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  QemuFwCfgSelectItem (PointerItem);
+  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
+  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
+
+  //
+  // Because QEMU has now learned PointeeBlob->Base, we must mark PointeeBlob
+  // as unreleasable, for the case when the whole linker/loader script is
+  // handled successfully.
+  //
+  PointeeBlob->HostsOnlyTableData = FALSE;
+
+  DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=\"%a\" PointeeFile=\"%a\" "
+    "PointerOffset=0x%x PointeeOffset=0x%x PointerSize=%d\n", __FUNCTION__,
+    WritePointer->PointerFile, WritePointer->PointeeFile,
+    WritePointer->PointerOffset, WritePointer->PointeeOffset,
+    WritePointer->PointerSize));
+  return EFI_SUCCESS;
+}
+
+
+/**
+  Undo a QEMU_LOADER_WRITE_POINTER command.
+
+  This function revokes (zeroes out) a guest memory reference communicated to
+  QEMU earlier. The caller is responsible for invoking this function only on
+  such QEMU_LOADER_WRITE_POINTER commands that have been successfully processed
+  by ProcessCmdWritePointer().
+
+  @param[in] WritePointer  The QEMU_LOADER_WRITE_POINTER command to undo.
+**/
+STATIC
+VOID
+UndoCmdWritePointer (
+  IN CONST QEMU_LOADER_WRITE_POINTER *WritePointer
+  )
+{
+  RETURN_STATUS        Status;
+  FIRMWARE_CONFIG_ITEM PointerItem;
+  UINTN                PointerItemSize;
+  UINT64               PointerValue;
+
+  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
+             &PointerItem, &PointerItemSize);
+  ASSERT_RETURN_ERROR (Status);
+
+  PointerValue = 0;
+  QemuFwCfgSelectItem (PointerItem);
+  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
+  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
+
+  DEBUG ((DEBUG_VERBOSE,
+    "%a: PointerFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__,
+    WritePointer->PointerFile, WritePointer->PointerOffset,
+    WritePointer->PointerSize));
+}
+
+
 //
 // We'll be saving the keys of installed tables so that we can roll them back
 // in case of failure. 128 tables should be enough for anyone (TM).
 //
 #define INSTALLED_TABLES_MAX 128
@@ -559,10 +696,11 @@ InstallQemuFwCfgTables (
   EFI_STATUS               Status;
   FIRMWARE_CONFIG_ITEM     FwCfgItem;
   UINTN                    FwCfgSize;
   QEMU_LOADER_ENTRY        *LoaderStart;
   CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
+  CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
   ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
   UINTN                    OriginalPciAttributesCount;
   ORDERED_COLLECTION       *Tracker;
   UINTN                    *InstalledKey;
   INT32                    Installed;
@@ -595,10 +733,15 @@ InstallQemuFwCfgTables (
   }
 
   //
   // first pass: process the commands
   //
+  // "WritePointerSubsetEnd" points one past the last successful
+  // QEMU_LOADER_WRITE_POINTER command. Now when we're about to start the first
+  // pass, no such command has been encountered yet.
+  //
+  WritePointerSubsetEnd = LoaderStart;
   for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
     switch (LoaderEntry->Type) {
     case QemuLoaderCmdAllocate:
       Status = ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tracker);
       break;
@@ -611,25 +754,33 @@ InstallQemuFwCfgTables (
     case QemuLoaderCmdAddChecksum:
       Status = ProcessCmdAddChecksum (&LoaderEntry->Command.AddChecksum,
                  Tracker);
       break;
 
+    case QemuLoaderCmdWritePointer:
+        Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
+                   Tracker);
+        if (!EFI_ERROR (Status)) {
+          WritePointerSubsetEnd = LoaderEntry + 1;
+        }
+        break;
+
     default:
       DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n",
         __FUNCTION__, LoaderEntry->Type));
       break;
     }
 
     if (EFI_ERROR (Status)) {
-      goto FreeTracker;
+      goto RollbackWritePointersAndFreeTracker;
     }
   }
 
   InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
   if (InstalledKey == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
-    goto FreeTracker;
+    goto RollbackWritePointersAndFreeTracker;
   }
 
   //
   // second pass: identify and install ACPI tables
   //
@@ -656,11 +807,25 @@ InstallQemuFwCfgTables (
     DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
   }
 
   FreePool (InstalledKey);
 
-FreeTracker:
+RollbackWritePointersAndFreeTracker:
+  //
+  // In case of failure, revoke any allocation addresses that were communicated
+  // to QEMU previously, before we release all the blobs.
+  //
+  if (EFI_ERROR (Status)) {
+    LoaderEntry = WritePointerSubsetEnd;
+    while (LoaderEntry > LoaderStart) {
+      --LoaderEntry;
+      if (LoaderEntry->Type == QemuLoaderCmdWritePointer) {
+        UndoCmdWritePointer (&LoaderEntry->Command.WritePointer);
+      }
+    }
+  }
+
   //
   // Tear down the tracker infrastructure. Each fw_cfg blob will be left in
   // place only if we're exiting with success and the blob hosts data that is
   // not directly part of some ACPI table.
   //
-- 
2.9.3

Re: [PATCH 4/5] OvmfPkg/AcpiPlatformDxe: implement the QEMU_LOADER_WRITE_POINTER command

[Jordan Justen]

On 2017-02-16 12:41:36, Laszlo Ersek wrote:
> The QEMU_LOADER_WRITE_POINTER command instructs the firmware to write the
> address of a field within a previously allocated/downloaded fw_cfg blob
> into another (writeable) fw_cfg file at a specific offset.
> 
> Put differently, QEMU_LOADER_WRITE_POINTER propagates, to QEMU, the
> address that QEMU_LOADER_ALLOCATE placed the designated fw_cfg blob at, as
> adjusted for the given field inside the allocated blob.
> 
> The implementation is similar to that of QEMU_LOADER_ADD_POINTER. Since
> here we "patch" a pointer object in "fw_cfg file space", not guest memory
> space, we utilize the QemuFwCfgSkipBytes() and QemuFwCfgWriteBytes() APIs
> completed in commit range 465663e9f128..7fcb73541299.
> 
> An interesting aspect is that QEMU_LOADER_WRITE_POINTER creates a
> host-level reference to a guest memory location. Therefore, if we fail to
> process the linker/loader script for any reason, we have to clear out
> those references first, before we release the guest memory allocations in
> response to the error.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 171 +++++++++++++++++++-
>  1 file changed, 168 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 404589cad0b7..de827c2df204 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -350,10 +350,147 @@ ProcessCmdAddChecksum (
>      AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length));
>    return EFI_SUCCESS;
>  }
>  
>  
> +/**
> +  Process a QEMU_LOADER_WRITE_POINTER command.
> +
> +  @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
> +
> +  @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
> +                            structures created thus far.
> +
> +  @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
> +                              WritePointer. Or, the WritePointer command
> +                              references a file unknown to Tracker or the
> +                              fw_cfg directory. Or, the pointer object to
> +                              rewrite has invalid location, size, or initial
> +                              relative value. Or, the pointer value to store
> +                              does not fit in the given pointer size.
> +
> +  @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
> +                              file has been written.
> +**/
> +STATIC
> +EFI_STATUS
> +ProcessCmdWritePointer (
> +  IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
> +  IN     CONST ORDERED_COLLECTION        *Tracker
> +  )
> +{
> +  RETURN_STATUS            Status;
> +  FIRMWARE_CONFIG_ITEM     PointerItem;
> +  UINTN                    PointerItemSize;
> +  ORDERED_COLLECTION_ENTRY *PointeeEntry;
> +  BLOB                     *PointeeBlob;
> +  UINT64                   PointerValue;
> +
> +  if (WritePointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' ||
> +      WritePointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
> +    DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
> +             &PointerItem, &PointerItemSize);
> +  PointeeEntry = OrderedCollectionFind (Tracker, WritePointer->PointeeFile);
> +  if (RETURN_ERROR (Status) || PointeeEntry == NULL) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: invalid fw_cfg file or blob reference \"%a\" / \"%a\"\n",
> +      __FUNCTION__, WritePointer->PointerFile, WritePointer->PointeeFile));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  if ((WritePointer->PointerSize != 1 && WritePointer->PointerSize != 2 &&
> +       WritePointer->PointerSize != 4 && WritePointer->PointerSize != 8) ||
> +      (PointerItemSize < WritePointer->PointerSize) ||
> +      (PointerItemSize - WritePointer->PointerSize <
> +       WritePointer->PointerOffset)) {
> +    DEBUG ((DEBUG_ERROR, "%a: invalid pointer location or size in \"%a\"\n",
> +      __FUNCTION__, WritePointer->PointerFile));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  PointeeBlob = OrderedCollectionUserStruct (PointeeEntry);
> +  PointerValue = WritePointer->PointeeOffset;
> +  if (PointerValue >= PointeeBlob->Size) {
> +    DEBUG ((DEBUG_ERROR, "%a: invalid PointeeOffset\n", __FUNCTION__));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  //
> +  // The memory allocation system ensures that the address of the byte past the
> +  // last byte of any allocated object is expressible (no wraparound).
> +  //
> +  ASSERT ((UINTN)PointeeBlob->Base <= MAX_ADDRESS - PointeeBlob->Size);
> +
> +  PointerValue += (UINT64)(UINTN)PointeeBlob->Base;
> +  if (RShiftU64 (
> +        RShiftU64 (PointerValue, WritePointer->PointerSize * 8 - 1), 1) != 0) {

What do you think of this instead?

  if (WritePointer->PointerSize < 8 &&
      RShiftU64 (PointerValue, WritePointer->PointerSize * 8) != 0) {

Patches 1-4 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

> +    DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
> +      __FUNCTION__, WritePointer->PointerFile));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  QemuFwCfgSelectItem (PointerItem);
> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
> +
> +  //
> +  // Because QEMU has now learned PointeeBlob->Base, we must mark PointeeBlob
> +  // as unreleasable, for the case when the whole linker/loader script is
> +  // handled successfully.
> +  //
> +  PointeeBlob->HostsOnlyTableData = FALSE;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=\"%a\" PointeeFile=\"%a\" "
> +    "PointerOffset=0x%x PointeeOffset=0x%x PointerSize=%d\n", __FUNCTION__,
> +    WritePointer->PointerFile, WritePointer->PointeeFile,
> +    WritePointer->PointerOffset, WritePointer->PointeeOffset,
> +    WritePointer->PointerSize));
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Undo a QEMU_LOADER_WRITE_POINTER command.
> +
> +  This function revokes (zeroes out) a guest memory reference communicated to
> +  QEMU earlier. The caller is responsible for invoking this function only on
> +  such QEMU_LOADER_WRITE_POINTER commands that have been successfully processed
> +  by ProcessCmdWritePointer().
> +
> +  @param[in] WritePointer  The QEMU_LOADER_WRITE_POINTER command to undo.
> +**/
> +STATIC
> +VOID
> +UndoCmdWritePointer (
> +  IN CONST QEMU_LOADER_WRITE_POINTER *WritePointer
> +  )
> +{
> +  RETURN_STATUS        Status;
> +  FIRMWARE_CONFIG_ITEM PointerItem;
> +  UINTN                PointerItemSize;
> +  UINT64               PointerValue;
> +
> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
> +             &PointerItem, &PointerItemSize);
> +  ASSERT_RETURN_ERROR (Status);
> +
> +  PointerValue = 0;
> +  QemuFwCfgSelectItem (PointerItem);
> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
> +
> +  DEBUG ((DEBUG_VERBOSE,
> +    "%a: PointerFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__,
> +    WritePointer->PointerFile, WritePointer->PointerOffset,
> +    WritePointer->PointerSize));
> +}
> +
> +
>  //
>  // We'll be saving the keys of installed tables so that we can roll them back
>  // in case of failure. 128 tables should be enough for anyone (TM).
>  //
>  #define INSTALLED_TABLES_MAX 128
> @@ -559,10 +696,11 @@ InstallQemuFwCfgTables (
>    EFI_STATUS               Status;
>    FIRMWARE_CONFIG_ITEM     FwCfgItem;
>    UINTN                    FwCfgSize;
>    QEMU_LOADER_ENTRY        *LoaderStart;
>    CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
> +  CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
>    ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
>    UINTN                    OriginalPciAttributesCount;
>    ORDERED_COLLECTION       *Tracker;
>    UINTN                    *InstalledKey;
>    INT32                    Installed;
> @@ -595,10 +733,15 @@ InstallQemuFwCfgTables (
>    }
>  
>    //
>    // first pass: process the commands
>    //
> +  // "WritePointerSubsetEnd" points one past the last successful
> +  // QEMU_LOADER_WRITE_POINTER command. Now when we're about to start the first
> +  // pass, no such command has been encountered yet.
> +  //
> +  WritePointerSubsetEnd = LoaderStart;
>    for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>      switch (LoaderEntry->Type) {
>      case QemuLoaderCmdAllocate:
>        Status = ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tracker);
>        break;
> @@ -611,25 +754,33 @@ InstallQemuFwCfgTables (
>      case QemuLoaderCmdAddChecksum:
>        Status = ProcessCmdAddChecksum (&LoaderEntry->Command.AddChecksum,
>                   Tracker);
>        break;
>  
> +    case QemuLoaderCmdWritePointer:
> +        Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
> +                   Tracker);
> +        if (!EFI_ERROR (Status)) {
> +          WritePointerSubsetEnd = LoaderEntry + 1;
> +        }
> +        break;
> +
>      default:
>        DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n",
>          __FUNCTION__, LoaderEntry->Type));
>        break;
>      }
>  
>      if (EFI_ERROR (Status)) {
> -      goto FreeTracker;
> +      goto RollbackWritePointersAndFreeTracker;
>      }
>    }
>  
>    InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
>    if (InstalledKey == NULL) {
>      Status = EFI_OUT_OF_RESOURCES;
> -    goto FreeTracker;
> +    goto RollbackWritePointersAndFreeTracker;
>    }
>  
>    //
>    // second pass: identify and install ACPI tables
>    //
> @@ -656,11 +807,25 @@ InstallQemuFwCfgTables (
>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>    }
>  
>    FreePool (InstalledKey);
>  
> -FreeTracker:
> +RollbackWritePointersAndFreeTracker:
> +  //
> +  // In case of failure, revoke any allocation addresses that were communicated
> +  // to QEMU previously, before we release all the blobs.
> +  //
> +  if (EFI_ERROR (Status)) {
> +    LoaderEntry = WritePointerSubsetEnd;
> +    while (LoaderEntry > LoaderStart) {
> +      --LoaderEntry;
> +      if (LoaderEntry->Type == QemuLoaderCmdWritePointer) {
> +        UndoCmdWritePointer (&LoaderEntry->Command.WritePointer);
> +      }
> +    }
> +  }
> +
>    //
>    // Tear down the tracker infrastructure. Each fw_cfg blob will be left in
>    // place only if we're exiting with success and the blob hosts data that is
>    // not directly part of some ACPI table.
>    //
> -- 
> 2.9.3
> 
> 

Re: [PATCH 4/5] OvmfPkg/AcpiPlatformDxe: implement the QEMU_LOADER_WRITE_POINTER command

[Laszlo Ersek]

On 02/17/17 20:34, Jordan Justen wrote:
> On 2017-02-16 12:41:36, Laszlo Ersek wrote:
>> The QEMU_LOADER_WRITE_POINTER command instructs the firmware to write the
>> address of a field within a previously allocated/downloaded fw_cfg blob
>> into another (writeable) fw_cfg file at a specific offset.
>>
>> Put differently, QEMU_LOADER_WRITE_POINTER propagates, to QEMU, the
>> address that QEMU_LOADER_ALLOCATE placed the designated fw_cfg blob at, as
>> adjusted for the given field inside the allocated blob.
>>
>> The implementation is similar to that of QEMU_LOADER_ADD_POINTER. Since
>> here we "patch" a pointer object in "fw_cfg file space", not guest memory
>> space, we utilize the QemuFwCfgSkipBytes() and QemuFwCfgWriteBytes() APIs
>> completed in commit range 465663e9f128..7fcb73541299.
>>
>> An interesting aspect is that QEMU_LOADER_WRITE_POINTER creates a
>> host-level reference to a guest memory location. Therefore, if we fail to
>> process the linker/loader script for any reason, we have to clear out
>> those references first, before we release the guest memory allocations in
>> response to the error.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 171 +++++++++++++++++++-
>>  1 file changed, 168 insertions(+), 3 deletions(-)
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> index 404589cad0b7..de827c2df204 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> @@ -350,10 +350,147 @@ ProcessCmdAddChecksum (
>>      AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length));
>>    return EFI_SUCCESS;
>>  }
>>  
>>  
>> +/**
>> +  Process a QEMU_LOADER_WRITE_POINTER command.
>> +
>> +  @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
>> +
>> +  @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
>> +                            structures created thus far.
>> +
>> +  @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
>> +                              WritePointer. Or, the WritePointer command
>> +                              references a file unknown to Tracker or the
>> +                              fw_cfg directory. Or, the pointer object to
>> +                              rewrite has invalid location, size, or initial
>> +                              relative value. Or, the pointer value to store
>> +                              does not fit in the given pointer size.
>> +
>> +  @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
>> +                              file has been written.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +ProcessCmdWritePointer (
>> +  IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
>> +  IN     CONST ORDERED_COLLECTION        *Tracker
>> +  )
>> +{
>> +  RETURN_STATUS            Status;
>> +  FIRMWARE_CONFIG_ITEM     PointerItem;
>> +  UINTN                    PointerItemSize;
>> +  ORDERED_COLLECTION_ENTRY *PointeeEntry;
>> +  BLOB                     *PointeeBlob;
>> +  UINT64                   PointerValue;
>> +
>> +  if (WritePointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' ||
>> +      WritePointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
>> +    DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
>> +             &PointerItem, &PointerItemSize);
>> +  PointeeEntry = OrderedCollectionFind (Tracker, WritePointer->PointeeFile);
>> +  if (RETURN_ERROR (Status) || PointeeEntry == NULL) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: invalid fw_cfg file or blob reference \"%a\" / \"%a\"\n",
>> +      __FUNCTION__, WritePointer->PointerFile, WritePointer->PointeeFile));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  if ((WritePointer->PointerSize != 1 && WritePointer->PointerSize != 2 &&
>> +       WritePointer->PointerSize != 4 && WritePointer->PointerSize != 8) ||
>> +      (PointerItemSize < WritePointer->PointerSize) ||
>> +      (PointerItemSize - WritePointer->PointerSize <
>> +       WritePointer->PointerOffset)) {
>> +    DEBUG ((DEBUG_ERROR, "%a: invalid pointer location or size in \"%a\"\n",
>> +      __FUNCTION__, WritePointer->PointerFile));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  PointeeBlob = OrderedCollectionUserStruct (PointeeEntry);
>> +  PointerValue = WritePointer->PointeeOffset;
>> +  if (PointerValue >= PointeeBlob->Size) {
>> +    DEBUG ((DEBUG_ERROR, "%a: invalid PointeeOffset\n", __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  //
>> +  // The memory allocation system ensures that the address of the byte past the
>> +  // last byte of any allocated object is expressible (no wraparound).
>> +  //
>> +  ASSERT ((UINTN)PointeeBlob->Base <= MAX_ADDRESS - PointeeBlob->Size);
>> +
>> +  PointerValue += (UINT64)(UINTN)PointeeBlob->Base;
>> +  if (RShiftU64 (
>> +        RShiftU64 (PointerValue, WritePointer->PointerSize * 8 - 1), 1) != 0) {
> 
> What do you think of this instead?
> 
>   if (WritePointer->PointerSize < 8 &&
>       RShiftU64 (PointerValue, WritePointer->PointerSize * 8) != 0) {

Entirely valid, but then I should update ProcessCmdAddPointer()
similarly :) Also, I'll feel less smart about myself! ;)

If that's OK with you, I'll fix it up later. (I'll collect these warts
into another BZ, as long as they aren't critical.)

BTW, today I had a successful end-to-end (VM migration) test with Ben's
QEMU v8 series, and this one for OVMF; that is, VMGENID worked "in
action" with a Windows Server 2012 R2 guest. In the guest, I used a
small Windows application from a colleague to wait for the VMGENID to
change (and to report / query it).

> 
> Patches 1-4 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thank you very much!
Laszlo

> 
>> +    DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
>> +      __FUNCTION__, WritePointer->PointerFile));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  QemuFwCfgSelectItem (PointerItem);
>> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
>> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
>> +
>> +  //
>> +  // Because QEMU has now learned PointeeBlob->Base, we must mark PointeeBlob
>> +  // as unreleasable, for the case when the whole linker/loader script is
>> +  // handled successfully.
>> +  //
>> +  PointeeBlob->HostsOnlyTableData = FALSE;
>> +
>> +  DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=\"%a\" PointeeFile=\"%a\" "
>> +    "PointerOffset=0x%x PointeeOffset=0x%x PointerSize=%d\n", __FUNCTION__,
>> +    WritePointer->PointerFile, WritePointer->PointeeFile,
>> +    WritePointer->PointerOffset, WritePointer->PointeeOffset,
>> +    WritePointer->PointerSize));
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +
>> +/**
>> +  Undo a QEMU_LOADER_WRITE_POINTER command.
>> +
>> +  This function revokes (zeroes out) a guest memory reference communicated to
>> +  QEMU earlier. The caller is responsible for invoking this function only on
>> +  such QEMU_LOADER_WRITE_POINTER commands that have been successfully processed
>> +  by ProcessCmdWritePointer().
>> +
>> +  @param[in] WritePointer  The QEMU_LOADER_WRITE_POINTER command to undo.
>> +**/
>> +STATIC
>> +VOID
>> +UndoCmdWritePointer (
>> +  IN CONST QEMU_LOADER_WRITE_POINTER *WritePointer
>> +  )
>> +{
>> +  RETURN_STATUS        Status;
>> +  FIRMWARE_CONFIG_ITEM PointerItem;
>> +  UINTN                PointerItemSize;
>> +  UINT64               PointerValue;
>> +
>> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
>> +             &PointerItem, &PointerItemSize);
>> +  ASSERT_RETURN_ERROR (Status);
>> +
>> +  PointerValue = 0;
>> +  QemuFwCfgSelectItem (PointerItem);
>> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
>> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
>> +
>> +  DEBUG ((DEBUG_VERBOSE,
>> +    "%a: PointerFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__,
>> +    WritePointer->PointerFile, WritePointer->PointerOffset,
>> +    WritePointer->PointerSize));
>> +}
>> +
>> +
>>  //
>>  // We'll be saving the keys of installed tables so that we can roll them back
>>  // in case of failure. 128 tables should be enough for anyone (TM).
>>  //
>>  #define INSTALLED_TABLES_MAX 128
>> @@ -559,10 +696,11 @@ InstallQemuFwCfgTables (
>>    EFI_STATUS               Status;
>>    FIRMWARE_CONFIG_ITEM     FwCfgItem;
>>    UINTN                    FwCfgSize;
>>    QEMU_LOADER_ENTRY        *LoaderStart;
>>    CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
>> +  CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
>>    ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
>>    UINTN                    OriginalPciAttributesCount;
>>    ORDERED_COLLECTION       *Tracker;
>>    UINTN                    *InstalledKey;
>>    INT32                    Installed;
>> @@ -595,10 +733,15 @@ InstallQemuFwCfgTables (
>>    }
>>  
>>    //
>>    // first pass: process the commands
>>    //
>> +  // "WritePointerSubsetEnd" points one past the last successful
>> +  // QEMU_LOADER_WRITE_POINTER command. Now when we're about to start the first
>> +  // pass, no such command has been encountered yet.
>> +  //
>> +  WritePointerSubsetEnd = LoaderStart;
>>    for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>>      switch (LoaderEntry->Type) {
>>      case QemuLoaderCmdAllocate:
>>        Status = ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tracker);
>>        break;
>> @@ -611,25 +754,33 @@ InstallQemuFwCfgTables (
>>      case QemuLoaderCmdAddChecksum:
>>        Status = ProcessCmdAddChecksum (&LoaderEntry->Command.AddChecksum,
>>                   Tracker);
>>        break;
>>  
>> +    case QemuLoaderCmdWritePointer:
>> +        Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
>> +                   Tracker);
>> +        if (!EFI_ERROR (Status)) {
>> +          WritePointerSubsetEnd = LoaderEntry + 1;
>> +        }
>> +        break;
>> +
>>      default:
>>        DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n",
>>          __FUNCTION__, LoaderEntry->Type));
>>        break;
>>      }
>>  
>>      if (EFI_ERROR (Status)) {
>> -      goto FreeTracker;
>> +      goto RollbackWritePointersAndFreeTracker;
>>      }
>>    }
>>  
>>    InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
>>    if (InstalledKey == NULL) {
>>      Status = EFI_OUT_OF_RESOURCES;
>> -    goto FreeTracker;
>> +    goto RollbackWritePointersAndFreeTracker;
>>    }
>>  
>>    //
>>    // second pass: identify and install ACPI tables
>>    //
>> @@ -656,11 +807,25 @@ InstallQemuFwCfgTables (
>>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>>    }
>>  
>>    FreePool (InstalledKey);
>>  
>> -FreeTracker:
>> +RollbackWritePointersAndFreeTracker:
>> +  //
>> +  // In case of failure, revoke any allocation addresses that were communicated
>> +  // to QEMU previously, before we release all the blobs.
>> +  //
>> +  if (EFI_ERROR (Status)) {
>> +    LoaderEntry = WritePointerSubsetEnd;
>> +    while (LoaderEntry > LoaderStart) {
>> +      --LoaderEntry;
>> +      if (LoaderEntry->Type == QemuLoaderCmdWritePointer) {
>> +        UndoCmdWritePointer (&LoaderEntry->Command.WritePointer);
>> +      }
>> +    }
>> +  }
>> +
>>    //
>>    // Tear down the tracker infrastructure. Each fw_cfg blob will be left in
>>    // place only if we're exiting with success and the blob hosts data that is
>>    // not directly part of some ACPI table.
>>    //
>> -- 
>> 2.9.3
>>
>>

[PATCH 5/5] OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at S3

[Laszlo Ersek]

Ultimately, each QEMU_LOADER_WRITE_POINTER command creates a guest memory
reference in some QEMU device. When the virtual machine is reset, the
device willfully forgets the guest address, since the guest memory is
wholly invalidated during platform reset.

... Unless the reset is part of S3 resume. Then the guest memory is
preserved intact, and the firmware must reprogram those devices with the
original guest memory allocation addresses.

This patch accumulates the fw_cfg select, skip and write operations of
ProcessCmdWritePointer() in a validated / condensed form, and turns them
into an ACPI S3 Boot Script fragment at the very end of
InstallQemuFwCfgTables().

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf          |   2 +
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf |   2 +
 OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h               |  27 ++
 OvmfPkg/AcpiPlatformDxe/BootScript.c                 | 414 ++++++++++++++++++++
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c              |  70 +++-
 5 files changed, 510 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
index 654d3a03905d..bb5f14e0fc7a 100644
--- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
+++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
@@ -31,10 +31,11 @@ [Sources]
   Qemu.c
   QemuFwCfgAcpi.c
   Xen.c
   EntryPoint.c
   PciDecoding.c
+  BootScript.c
 
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   OvmfPkg/OvmfPkg.dec
@@ -57,10 +58,11 @@ [LibraryClasses]
   OrderedCollectionLib
 
 [Protocols]
   gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
   gEfiPciIoProtocolGuid                         # PROTOCOL SOMETIMES_CONSUMED
+  gEfiS3SaveStateProtocolGuid                   # PROTOCOL SOMETIMES_CONSUMED
 
 [Guids]
   gEfiXenInfoGuid
   gRootBridgesConnectedEventGroupGuid
 
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
index d99f2d5a95c7..e550ff5a4714 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
@@ -29,10 +29,11 @@ [Defines]
 [Sources]
   QemuFwCfgAcpiPlatform.c
   QemuFwCfgAcpi.c
   EntryPoint.c
   PciDecoding.c
+  BootScript.c
 
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   OvmfPkg/OvmfPkg.dec
@@ -47,10 +48,11 @@ [LibraryClasses]
   UefiDriverEntryPoint
 
 [Protocols]
   gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
   gEfiPciIoProtocolGuid                         # PROTOCOL SOMETIMES_CONSUMED
+  gEfiS3SaveStateProtocolGuid                   # PROTOCOL SOMETIMES_CONSUMED
 
 [Guids]
   gRootBridgesConnectedEventGroupGuid
 
 [Pcd]
diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
index 08dd7f8f7dd7..0f035a0d5751 100644
--- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
+++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
@@ -31,10 +31,12 @@
 typedef struct {
   EFI_PCI_IO_PROTOCOL *PciIo;
   UINT64              PciAttributes;
 } ORIGINAL_ATTRIBUTES;
 
+typedef struct S3_CONTEXT S3_CONTEXT;
+
 EFI_STATUS
 EFIAPI
 InstallAcpiTable (
   IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol,
   IN   VOID                          *AcpiTableBuffer,
@@ -89,7 +91,32 @@ VOID
 RestorePciDecoding (
   IN ORIGINAL_ATTRIBUTES *OriginalAttributes,
   IN UINTN               Count
   );
 
+EFI_STATUS
+AllocateS3Context (
+  OUT S3_CONTEXT **S3Context,
+  IN  UINTN      WritePointerCount
+  );
+
+VOID
+ReleaseS3Context (
+  IN S3_CONTEXT *S3Context
+  );
+
+EFI_STATUS
+SaveCondensedWritePointerToS3Context (
+  IN OUT S3_CONTEXT *S3Context,
+  IN     UINT16     PointerItem,
+  IN     UINT8      PointerSize,
+  IN     UINT32     PointerOffset,
+  IN     UINT64     PointerValue
+  );
+
+EFI_STATUS
+TransferS3ContextToBootScript (
+  IN CONST S3_CONTEXT *S3Context
+  );
+
 #endif
 
diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
new file mode 100644
index 000000000000..b7a7f270f223
--- /dev/null
+++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
@@ -0,0 +1,414 @@
+/** @file
+  Append an ACPI S3 Boot Script fragment from the QEMU_LOADER_WRITE_POINTER
+  commands of QEMU's fully processed table linker/loader script.
+
+  Copyright (C) 2017, Red Hat, Inc.
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#include <Library/MemoryAllocationLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Protocol/S3SaveState.h>
+
+#include "AcpiPlatform.h"
+
+
+//
+// Condensed structure for capturing the fw_cfg operations -- select, skip,
+// write -- inherent in executing a QEMU_LOADER_WRITE_POINTER command.
+//
+typedef struct {
+  UINT16 PointerItem;   // resolved from QEMU_LOADER_WRITE_POINTER.PointerFile
+  UINT8  PointerSize;   // copied as-is from QEMU_LOADER_WRITE_POINTER
+  UINT32 PointerOffset; // copied as-is from QEMU_LOADER_WRITE_POINTER
+  UINT64 PointerValue;  // resolved from QEMU_LOADER_WRITE_POINTER.PointeeFile
+} CONDENSED_WRITE_POINTER;
+
+
+//
+// Context structure to accumulate CONDENSED_WRITE_POINTER objects from
+// QEMU_LOADER_WRITE_POINTER commands.
+//
+// Any pointers in this structure own the pointed-to objects; that is, when the
+// context structure is released, all pointed-to objects must be released too.
+//
+struct S3_CONTEXT {
+  CONDENSED_WRITE_POINTER *WritePointers; // one array element per processed
+                                          //   QEMU_LOADER_WRITE_POINTER
+                                          //   command
+  UINTN                   Allocated;      // number of elements allocated for
+                                          //   WritePointers
+  UINTN                   Used;           // number of elements populated in
+                                          //   WritePointers
+};
+
+
+//
+// Scratch buffer, allocated in EfiReservedMemoryType type memory, for the ACPI
+// S3 Boot Script opcodes to work on. We use the buffer to compose and to
+// replay several fw_cfg select+skip and write operations, using the DMA access
+// method. The fw_cfg operations will implement the actions dictated by
+// CONDENSED_WRITE_POINTER objects.
+//
+#pragma pack (1)
+typedef struct {
+  FW_CFG_DMA_ACCESS Access;       // filled in from
+                                  //   CONDENSED_WRITE_POINTER.PointerItem,
+                                  //   CONDENSED_WRITE_POINTER.PointerSize,
+                                  //   CONDENSED_WRITE_POINTER.PointerOffset
+  UINT64            PointerValue; // filled in from
+                                  //   CONDENSED_WRITE_POINTER.PointerValue
+} SCRATCH_BUFFER;
+#pragma pack ()
+
+
+/**
+  Allocate an S3_CONTEXT object.
+
+  @param[out] S3Context         The allocated S3_CONTEXT object is returned
+                                through this parameter.
+
+  @param[in] WritePointerCount  Number of CONDENSED_WRITE_POINTER elements to
+                                allocate room for. WritePointerCount must be
+                                positive.
+
+  @retval EFI_SUCCESS            Allocation successful.
+
+  @retval EFI_OUT_OF_RESOURCES   Out of memory.
+
+  @retval EFI_INVALID_PARAMETER  WritePointerCount is zero.
+**/
+EFI_STATUS
+AllocateS3Context (
+  OUT S3_CONTEXT **S3Context,
+  IN  UINTN      WritePointerCount
+  )
+{
+  EFI_STATUS Status;
+  S3_CONTEXT *Context;
+
+  if (WritePointerCount == 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Context = AllocateZeroPool (sizeof *Context);
+  if (Context == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  Context->WritePointers = AllocatePool (WritePointerCount *
+                             sizeof *Context->WritePointers);
+  if (Context->WritePointers == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto FreeContext;
+  }
+
+  Context->Allocated = WritePointerCount;
+  *S3Context = Context;
+  return EFI_SUCCESS;
+
+FreeContext:
+  FreePool (Context);
+
+  return Status;
+}
+
+
+/**
+  Release an S3_CONTEXT object.
+
+  @param[in] S3Context  The object to release.
+**/
+VOID
+ReleaseS3Context (
+  IN S3_CONTEXT *S3Context
+  )
+{
+  FreePool (S3Context->WritePointers);
+  FreePool (S3Context);
+}
+
+
+/**
+  Save the information necessary to replicate a QEMU_LOADER_WRITE_POINTER
+  command during S3 resume, in condensed format.
+
+  This function is to be called from ProcessCmdWritePointer(), after all the
+  sanity checks have passed, and before the fw_cfg operations are performed.
+
+  @param[in,out] S3Context  The S3_CONTEXT object into which the caller wants
+                            to save the information that was derived from
+                            QEMU_LOADER_WRITE_POINTER.
+
+  @param[in] PointerItem    The FIRMWARE_CONFIG_ITEM that
+                            QEMU_LOADER_WRITE_POINTER.PointerFile was resolved
+                            to, expressed as a UINT16 value.
+
+  @param[in] PointerSize    Copied directly from
+                            QEMU_LOADER_WRITE_POINTER.PointerSize.
+
+  @param[in] PointerOffset  Copied directly from
+                            QEMU_LOADER_WRITE_POINTER.PointerOffset.
+
+  @param[in] PointerValue   The base address of the allocated / downloaded
+                            fw_cfg blob that is identified by
+                            QEMU_LOADER_WRITE_POINTER.PointeeFile.
+
+  @retval EFI_SUCCESS           The information derived from
+                                QEMU_LOADER_WRITE_POINTER has been successfully
+                                absorbed into S3Context.
+
+  @retval EFI_OUT_OF_RESOURCES  No room available in S3Context.
+**/
+EFI_STATUS
+SaveCondensedWritePointerToS3Context (
+  IN OUT S3_CONTEXT *S3Context,
+  IN     UINT16     PointerItem,
+  IN     UINT8      PointerSize,
+  IN     UINT32     PointerOffset,
+  IN     UINT64     PointerValue
+  )
+{
+  CONDENSED_WRITE_POINTER *Condensed;
+
+  if (S3Context->Used == S3Context->Allocated) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+  Condensed = S3Context->WritePointers + S3Context->Used;
+  Condensed->PointerItem   = PointerItem;
+  Condensed->PointerSize   = PointerSize;
+  Condensed->PointerOffset = PointerOffset;
+  Condensed->PointerValue  = PointerValue;
+  DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n",
+    __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue,
+    (UINT64)S3Context->Used));
+  ++S3Context->Used;
+  return EFI_SUCCESS;
+}
+
+
+/**
+  Translate and append the information from an S3_CONTEXT object to the ACPI S3
+  Boot Script.
+
+  The effects of a successful call to this function cannot be undone.
+
+  @param[in] S3Context  The S3_CONTEXT object to translate to ACPI S3 Boot
+                        Script opcodes.
+
+  @retval EFI_OUT_OF_RESOURCES  Out of memory.
+
+  @retval EFI_SUCCESS           The translation of S3Context to ACPI S3 Boot
+                                Script opcodes has been successful.
+
+  @return                       Error codes from underlying functions.
+**/
+EFI_STATUS
+TransferS3ContextToBootScript (
+  IN CONST S3_CONTEXT *S3Context
+  )
+{
+  EFI_STATUS                 Status;
+  EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState;
+  SCRATCH_BUFFER             *ScratchBuffer;
+  FW_CFG_DMA_ACCESS          *Access;
+  UINT64                     BigEndianAddressOfAccess;
+  UINT32                     ControlPollData;
+  UINT32                     ControlPollMask;
+  UINTN                      Index;
+
+  //
+  // If the following protocol lookup fails, it shall not happen due to an
+  // unexpected DXE driver dispatch order.
+  //
+  // Namely, this function is only invoked on QEMU. Therefore it is only
+  // reached after Platform BDS signals gRootBridgesConnectedEventGroupGuid
+  // (see OnRootBridgesConnected() in "EntryPoint.c"). Hence, because
+  // TransferS3ContextToBootScript() is invoked in BDS, all DXE drivers,
+  // including S3SaveStateDxe (producing EFI_S3_SAVE_STATE_PROTOCOL), have been
+  // dispatched by the time we get here. (S3SaveStateDxe is not expected to
+  // have any stricter-than-TRUE DEPEX -- not a DEPEX that gets unblocked only
+  // within BDS anyway.)
+  //
+  // Reaching this function also depends on QemuFwCfgS3Enabled(). That implies
+  // S3SaveStateDxe has not exited immediately due to S3 being disabled. Thus
+  // EFI_S3_SAVE_STATE_PROTOCOL can only be missing for genuinely unforeseeable
+  // reasons.
+  //
+  Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
+                  NULL /* Registration */, (VOID **)&S3SaveState);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a: LocateProtocol(): %r\n", __FUNCTION__, Status));
+    return Status;
+  }
+
+  ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer);
+  if (ScratchBuffer == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  //
+  // Set up helper variables that we'll use identically for all
+  // CONDENSED_WRITE_POINTER elements.
+  //
+  Access = &ScratchBuffer->Access;
+  BigEndianAddressOfAccess = SwapBytes64 ((UINTN)Access);
+  ControlPollData = 0;
+  ControlPollMask = MAX_UINT32;
+
+  //
+  // For each CONDENSED_WRITE_POINTER, we need six ACPI S3 Boot Script opcodes:
+  // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
+  //     the writeable fw_cfg file PointerFile (through PointerItem), and skips
+  //     to PointerOffset in it,
+  // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
+  // (3) wait for the select+skip to finish,
+  // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
+  //     PointerValue (base address of the allocated / downloaded PointeeFile),
+  //     of size PointerSize, into the fw_cfg file selected in (1), at the
+  //     offset sought to in (1),
+  // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
+  // (6) wait for the write to finish.
+  //
+  // EFI_S3_SAVE_STATE_PROTOCOL does not allow rolling back opcode additions,
+  // therefore we treat any failure here as fatal.
+  //
+  for (Index = 0; Index < S3Context->Used; ++Index) {
+    CONST CONDENSED_WRITE_POINTER *Condensed;
+
+    Condensed = &S3Context->WritePointers[Index];
+
+    //
+    // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
+    //     the writeable fw_cfg file PointerFile (through PointerItem), and
+    //     skips to PointerOffset in it,
+    //
+    Access->Control = SwapBytes32 ((UINT32)Condensed->PointerItem << 16 |
+                        FW_CFG_DMA_CTL_SELECT | FW_CFG_DMA_CTL_SKIP);
+    Access->Length = SwapBytes32 (Condensed->PointerOffset);
+    Access->Address = 0;
+    Status = S3SaveState->Write (
+                            S3SaveState,                      // This
+                            EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
+                            EfiBootScriptWidthUint8,          // Width
+                            (UINT64)(UINTN)Access,            // Address
+                            sizeof *Access,                   // Count
+                            Access                            // Buffer
+                            );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 1: %r\n", __FUNCTION__,
+        (UINT64)Index, Status));
+      goto FatalError;
+    }
+
+    //
+    // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
+    //
+    Status = S3SaveState->Write (
+                            S3SaveState,                     // This
+                            EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
+                            EfiBootScriptWidthUint32,        // Width
+                            (UINT64)0x514,                   // Address
+                            (UINTN)2,                        // Count
+                            &BigEndianAddressOfAccess        // Buffer
+                            );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 2: %r\n", __FUNCTION__,
+        (UINT64)Index, Status));
+      goto FatalError;
+    }
+
+    //
+    // (3) wait for the select+skip to finish,
+    //
+    Status = S3SaveState->Write (
+                            S3SaveState,                     // This
+                            EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
+                            EfiBootScriptWidthUint32,        // Width
+                            (UINT64)(UINTN)&Access->Control, // Address
+                            &ControlPollData,                // Data
+                            &ControlPollMask,                // DataMask
+                            MAX_UINT64                       // Delay
+                            );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 3: %r\n", __FUNCTION__,
+        (UINT64)Index, Status));
+      goto FatalError;
+    }
+
+    //
+    // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
+    //     PointerValue (base address of the allocated / downloaded
+    //     PointeeFile), of size PointerSize, into the fw_cfg file selected in
+    //     (1), at the offset sought to in (1),
+    //
+    Access->Control = SwapBytes32 (FW_CFG_DMA_CTL_WRITE);
+    Access->Length = SwapBytes32 (Condensed->PointerSize);
+    Access->Address = SwapBytes64 ((UINTN)&ScratchBuffer->PointerValue);
+    ScratchBuffer->PointerValue = Condensed->PointerValue;
+    Status = S3SaveState->Write (
+                            S3SaveState,                      // This
+                            EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
+                            EfiBootScriptWidthUint8,          // Width
+                            (UINT64)(UINTN)ScratchBuffer,     // Address
+                            sizeof *ScratchBuffer,            // Count
+                            ScratchBuffer                     // Buffer
+                            );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 4: %r\n", __FUNCTION__,
+        (UINT64)Index, Status));
+      goto FatalError;
+    }
+
+    //
+    // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
+    //
+    Status = S3SaveState->Write (
+                            S3SaveState,                     // This
+                            EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
+                            EfiBootScriptWidthUint32,        // Width
+                            (UINT64)0x514,                   // Address
+                            (UINTN)2,                        // Count
+                            &BigEndianAddressOfAccess        // Buffer
+                            );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 5: %r\n", __FUNCTION__,
+        (UINT64)Index, Status));
+      goto FatalError;
+    }
+
+    //
+    // (6) wait for the write to finish.
+    //
+    Status = S3SaveState->Write (
+                            S3SaveState,                     // This
+                            EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
+                            EfiBootScriptWidthUint32,        // Width
+                            (UINT64)(UINTN)&Access->Control, // Address
+                            &ControlPollData,                // Data
+                            &ControlPollMask,                // DataMask
+                            MAX_UINT64                       // Delay
+                            );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 6: %r\n", __FUNCTION__,
+        (UINT64)Index, Status));
+      goto FatalError;
+    }
+  }
+
+  DEBUG ((DEBUG_VERBOSE, "%a: boot script fragment saved, ScratchBuffer=%p\n",
+    __FUNCTION__, (VOID *)ScratchBuffer));
+  return EFI_SUCCESS;
+
+FatalError:
+  ASSERT (FALSE);
+  CpuDeadLoop ();
+  return Status;
+}
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index de827c2df204..eadd690bef4e 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -358,26 +358,39 @@ ProcessCmdAddChecksum (
   @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
 
   @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
                             structures created thus far.
 
+  @param[in,out] S3Context  The S3_CONTEXT object capturing the fw_cfg actions
+                            of successfully processed QEMU_LOADER_WRITE_POINTER
+                            commands, to be replayed at S3 resume. S3Context
+                            may be NULL if S3 is disabled.
+
   @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
                               WritePointer. Or, the WritePointer command
                               references a file unknown to Tracker or the
                               fw_cfg directory. Or, the pointer object to
                               rewrite has invalid location, size, or initial
                               relative value. Or, the pointer value to store
                               does not fit in the given pointer size.
 
   @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
-                              file has been written.
+                              file has been written. If S3Context is not NULL,
+                              then WritePointer has been condensed into
+                              S3Context.
+
+  @return                     Error codes propagated from
+                              SaveCondensedWritePointerToS3Context(). The
+                              pointer object inside the writeable fw_cfg file
+                              has not been written.
 **/
 STATIC
 EFI_STATUS
 ProcessCmdWritePointer (
   IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
-  IN     CONST ORDERED_COLLECTION        *Tracker
+  IN     CONST ORDERED_COLLECTION        *Tracker,
+  IN OUT       S3_CONTEXT                *S3Context OPTIONAL
   )
 {
   RETURN_STATUS            Status;
   FIRMWARE_CONFIG_ITEM     PointerItem;
   UINTN                    PointerItemSize;
@@ -430,10 +443,29 @@ ProcessCmdWritePointer (
     DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
       __FUNCTION__, WritePointer->PointerFile));
     return EFI_PROTOCOL_ERROR;
   }
 
+  //
+  // If S3 is enabled, we have to capture the below fw_cfg actions in condensed
+  // form, to be replayed during S3 resume.
+  //
+  if (S3Context != NULL) {
+    EFI_STATUS SaveStatus;
+
+    SaveStatus = SaveCondensedWritePointerToS3Context (
+                   S3Context,
+                   (UINT16)PointerItem,
+                   WritePointer->PointerSize,
+                   WritePointer->PointerOffset,
+                   PointerValue
+                   );
+    if (EFI_ERROR (SaveStatus)) {
+      return SaveStatus;
+    }
+  }
+
   QemuFwCfgSelectItem (PointerItem);
   QemuFwCfgSkipBytes (WritePointer->PointerOffset);
   QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
 
   //
@@ -699,10 +731,11 @@ InstallQemuFwCfgTables (
   QEMU_LOADER_ENTRY        *LoaderStart;
   CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
   CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
   ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
   UINTN                    OriginalPciAttributesCount;
+  S3_CONTEXT               *S3Context;
   ORDERED_COLLECTION       *Tracker;
   UINTN                    *InstalledKey;
   INT32                    Installed;
   ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
 
@@ -724,14 +757,26 @@ InstallQemuFwCfgTables (
   QemuFwCfgSelectItem (FwCfgItem);
   QemuFwCfgReadBytes (FwCfgSize, LoaderStart);
   RestorePciDecoding (OriginalPciAttributes, OriginalPciAttributesCount);
   LoaderEnd = LoaderStart + FwCfgSize / sizeof *LoaderEntry;
 
+  S3Context = NULL;
+  if (QemuFwCfgS3Enabled ()) {
+    //
+    // Size the allocation pessimistically, assuming that all commands in the
+    // script are QEMU_LOADER_WRITE_POINTER commands.
+    //
+    Status = AllocateS3Context (&S3Context, LoaderEnd - LoaderStart);
+    if (EFI_ERROR (Status)) {
+      goto FreeLoader;
+    }
+  }
+
   Tracker = OrderedCollectionInit (BlobCompare, BlobKeyCompare);
   if (Tracker == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
-    goto FreeLoader;
+    goto FreeS3Context;
   }
 
   //
   // first pass: process the commands
   //
@@ -756,11 +801,11 @@ InstallQemuFwCfgTables (
                  Tracker);
       break;
 
     case QemuLoaderCmdWritePointer:
         Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
-                   Tracker);
+                   Tracker, S3Context);
         if (!EFI_ERROR (Status)) {
           WritePointerSubsetEnd = LoaderEntry + 1;
         }
         break;
 
@@ -788,15 +833,25 @@ InstallQemuFwCfgTables (
   for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
     if (LoaderEntry->Type == QemuLoaderCmdAddPointer) {
       Status = Process2ndPassCmdAddPointer (&LoaderEntry->Command.AddPointer,
                  Tracker, AcpiProtocol, InstalledKey, &Installed);
       if (EFI_ERROR (Status)) {
-        break;
+        goto UninstallAcpiTables;
       }
     }
   }
 
+  //
+  // Translating the condensed QEMU_LOADER_WRITE_POINTER commands to ACPI S3
+  // Boot Script opcodes has to be the last operation in this function, because
+  // if it succeeds, it cannot be undone.
+  //
+  if (S3Context != NULL) {
+    Status = TransferS3ContextToBootScript (S3Context);
+  }
+
+UninstallAcpiTables:
   if (EFI_ERROR (Status)) {
     //
     // roll back partial installation
     //
     while (Installed > 0) {
@@ -845,10 +900,15 @@ RollbackWritePointersAndFreeTracker:
     }
     FreePool (Blob);
   }
   OrderedCollectionUninit (Tracker);
 
+FreeS3Context:
+  if (S3Context != NULL) {
+    ReleaseS3Context (S3Context);
+  }
+
 FreeLoader:
   FreePool (LoaderStart);
 
   return Status;
 }
-- 
2.9.3

Re: [PATCH 5/5] OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at S3

[Jordan Justen]

On 2017-02-16 12:41:37, Laszlo Ersek wrote:
> Ultimately, each QEMU_LOADER_WRITE_POINTER command creates a guest memory
> reference in some QEMU device. When the virtual machine is reset, the
> device willfully forgets the guest address, since the guest memory is
> wholly invalidated during platform reset.
> 
> ... Unless the reset is part of S3 resume. Then the guest memory is
> preserved intact, and the firmware must reprogram those devices with the
> original guest memory allocation addresses.
> 
> This patch accumulates the fw_cfg select, skip and write operations of
> ProcessCmdWritePointer() in a validated / condensed form, and turns them
> into an ACPI S3 Boot Script fragment at the very end of
> InstallQemuFwCfgTables().
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf          |   2 +
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf |   2 +
>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h               |  27 ++
>  OvmfPkg/AcpiPlatformDxe/BootScript.c                 | 414 ++++++++++++++++++++
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c              |  70 +++-
>  5 files changed, 510 insertions(+), 5 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> index 654d3a03905d..bb5f14e0fc7a 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> @@ -31,10 +31,11 @@ [Sources]
>    Qemu.c
>    QemuFwCfgAcpi.c
>    Xen.c
>    EntryPoint.c
>    PciDecoding.c
> +  BootScript.c
>  
>  [Packages]
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    OvmfPkg/OvmfPkg.dec
> @@ -57,10 +58,11 @@ [LibraryClasses]
>    OrderedCollectionLib
>  
>  [Protocols]
>    gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
>    gEfiPciIoProtocolGuid                         # PROTOCOL SOMETIMES_CONSUMED
> +  gEfiS3SaveStateProtocolGuid                   # PROTOCOL SOMETIMES_CONSUMED
>  
>  [Guids]
>    gEfiXenInfoGuid
>    gRootBridgesConnectedEventGroupGuid
>  
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> index d99f2d5a95c7..e550ff5a4714 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> @@ -29,10 +29,11 @@ [Defines]
>  [Sources]
>    QemuFwCfgAcpiPlatform.c
>    QemuFwCfgAcpi.c
>    EntryPoint.c
>    PciDecoding.c
> +  BootScript.c
>  
>  [Packages]
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    OvmfPkg/OvmfPkg.dec
> @@ -47,10 +48,11 @@ [LibraryClasses]
>    UefiDriverEntryPoint
>  
>  [Protocols]
>    gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
>    gEfiPciIoProtocolGuid                         # PROTOCOL SOMETIMES_CONSUMED
> +  gEfiS3SaveStateProtocolGuid                   # PROTOCOL SOMETIMES_CONSUMED
>  
>  [Guids]
>    gRootBridgesConnectedEventGroupGuid
>  
>  [Pcd]
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> index 08dd7f8f7dd7..0f035a0d5751 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> @@ -31,10 +31,12 @@
>  typedef struct {
>    EFI_PCI_IO_PROTOCOL *PciIo;
>    UINT64              PciAttributes;
>  } ORIGINAL_ATTRIBUTES;
>  
> +typedef struct S3_CONTEXT S3_CONTEXT;
> +
>  EFI_STATUS
>  EFIAPI
>  InstallAcpiTable (
>    IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol,
>    IN   VOID                          *AcpiTableBuffer,
> @@ -89,7 +91,32 @@ VOID
>  RestorePciDecoding (
>    IN ORIGINAL_ATTRIBUTES *OriginalAttributes,
>    IN UINTN               Count
>    );
>  
> +EFI_STATUS
> +AllocateS3Context (
> +  OUT S3_CONTEXT **S3Context,
> +  IN  UINTN      WritePointerCount
> +  );
> +
> +VOID
> +ReleaseS3Context (
> +  IN S3_CONTEXT *S3Context
> +  );
> +
> +EFI_STATUS
> +SaveCondensedWritePointerToS3Context (
> +  IN OUT S3_CONTEXT *S3Context,
> +  IN     UINT16     PointerItem,
> +  IN     UINT8      PointerSize,
> +  IN     UINT32     PointerOffset,
> +  IN     UINT64     PointerValue
> +  );
> +
> +EFI_STATUS
> +TransferS3ContextToBootScript (
> +  IN CONST S3_CONTEXT *S3Context
> +  );
> +
>  #endif
>  
> diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
> new file mode 100644
> index 000000000000..b7a7f270f223
> --- /dev/null
> +++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
> @@ -0,0 +1,414 @@
> +/** @file
> +  Append an ACPI S3 Boot Script fragment from the QEMU_LOADER_WRITE_POINTER
> +  commands of QEMU's fully processed table linker/loader script.
> +
> +  Copyright (C) 2017, Red Hat, Inc.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +**/
> +
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +#include <Protocol/S3SaveState.h>
> +
> +#include "AcpiPlatform.h"
> +
> +
> +//
> +// Condensed structure for capturing the fw_cfg operations -- select, skip,
> +// write -- inherent in executing a QEMU_LOADER_WRITE_POINTER command.
> +//
> +typedef struct {
> +  UINT16 PointerItem;   // resolved from QEMU_LOADER_WRITE_POINTER.PointerFile
> +  UINT8  PointerSize;   // copied as-is from QEMU_LOADER_WRITE_POINTER
> +  UINT32 PointerOffset; // copied as-is from QEMU_LOADER_WRITE_POINTER
> +  UINT64 PointerValue;  // resolved from QEMU_LOADER_WRITE_POINTER.PointeeFile
> +} CONDENSED_WRITE_POINTER;
> +
> +
> +//
> +// Context structure to accumulate CONDENSED_WRITE_POINTER objects from
> +// QEMU_LOADER_WRITE_POINTER commands.
> +//
> +// Any pointers in this structure own the pointed-to objects; that is, when the
> +// context structure is released, all pointed-to objects must be released too.
> +//
> +struct S3_CONTEXT {
> +  CONDENSED_WRITE_POINTER *WritePointers; // one array element per processed
> +                                          //   QEMU_LOADER_WRITE_POINTER
> +                                          //   command
> +  UINTN                   Allocated;      // number of elements allocated for
> +                                          //   WritePointers
> +  UINTN                   Used;           // number of elements populated in
> +                                          //   WritePointers
> +};
> +
> +
> +//
> +// Scratch buffer, allocated in EfiReservedMemoryType type memory, for the ACPI
> +// S3 Boot Script opcodes to work on. We use the buffer to compose and to
> +// replay several fw_cfg select+skip and write operations, using the DMA access
> +// method. The fw_cfg operations will implement the actions dictated by
> +// CONDENSED_WRITE_POINTER objects.
> +//
> +#pragma pack (1)
> +typedef struct {
> +  FW_CFG_DMA_ACCESS Access;       // filled in from
> +                                  //   CONDENSED_WRITE_POINTER.PointerItem,
> +                                  //   CONDENSED_WRITE_POINTER.PointerSize,
> +                                  //   CONDENSED_WRITE_POINTER.PointerOffset
> +  UINT64            PointerValue; // filled in from
> +                                  //   CONDENSED_WRITE_POINTER.PointerValue
> +} SCRATCH_BUFFER;
> +#pragma pack ()
> +
> +
> +/**
> +  Allocate an S3_CONTEXT object.
> +
> +  @param[out] S3Context         The allocated S3_CONTEXT object is returned
> +                                through this parameter.
> +
> +  @param[in] WritePointerCount  Number of CONDENSED_WRITE_POINTER elements to
> +                                allocate room for. WritePointerCount must be
> +                                positive.
> +
> +  @retval EFI_SUCCESS            Allocation successful.
> +
> +  @retval EFI_OUT_OF_RESOURCES   Out of memory.
> +
> +  @retval EFI_INVALID_PARAMETER  WritePointerCount is zero.
> +**/
> +EFI_STATUS
> +AllocateS3Context (
> +  OUT S3_CONTEXT **S3Context,
> +  IN  UINTN      WritePointerCount
> +  )
> +{
> +  EFI_STATUS Status;
> +  S3_CONTEXT *Context;
> +
> +  if (WritePointerCount == 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Context = AllocateZeroPool (sizeof *Context);
> +  if (Context == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Context->WritePointers = AllocatePool (WritePointerCount *
> +                             sizeof *Context->WritePointers);
> +  if (Context->WritePointers == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeContext;
> +  }
> +
> +  Context->Allocated = WritePointerCount;
> +  *S3Context = Context;
> +  return EFI_SUCCESS;
> +
> +FreeContext:
> +  FreePool (Context);
> +
> +  return Status;
> +}
> +
> +
> +/**
> +  Release an S3_CONTEXT object.
> +
> +  @param[in] S3Context  The object to release.
> +**/
> +VOID
> +ReleaseS3Context (
> +  IN S3_CONTEXT *S3Context
> +  )
> +{
> +  FreePool (S3Context->WritePointers);
> +  FreePool (S3Context);
> +}
> +
> +
> +/**
> +  Save the information necessary to replicate a QEMU_LOADER_WRITE_POINTER
> +  command during S3 resume, in condensed format.
> +
> +  This function is to be called from ProcessCmdWritePointer(), after all the
> +  sanity checks have passed, and before the fw_cfg operations are performed.
> +
> +  @param[in,out] S3Context  The S3_CONTEXT object into which the caller wants
> +                            to save the information that was derived from
> +                            QEMU_LOADER_WRITE_POINTER.
> +
> +  @param[in] PointerItem    The FIRMWARE_CONFIG_ITEM that
> +                            QEMU_LOADER_WRITE_POINTER.PointerFile was resolved
> +                            to, expressed as a UINT16 value.
> +
> +  @param[in] PointerSize    Copied directly from
> +                            QEMU_LOADER_WRITE_POINTER.PointerSize.
> +
> +  @param[in] PointerOffset  Copied directly from
> +                            QEMU_LOADER_WRITE_POINTER.PointerOffset.
> +
> +  @param[in] PointerValue   The base address of the allocated / downloaded
> +                            fw_cfg blob that is identified by
> +                            QEMU_LOADER_WRITE_POINTER.PointeeFile.
> +
> +  @retval EFI_SUCCESS           The information derived from
> +                                QEMU_LOADER_WRITE_POINTER has been successfully
> +                                absorbed into S3Context.
> +
> +  @retval EFI_OUT_OF_RESOURCES  No room available in S3Context.
> +**/
> +EFI_STATUS
> +SaveCondensedWritePointerToS3Context (
> +  IN OUT S3_CONTEXT *S3Context,
> +  IN     UINT16     PointerItem,
> +  IN     UINT8      PointerSize,
> +  IN     UINT32     PointerOffset,
> +  IN     UINT64     PointerValue
> +  )
> +{
> +  CONDENSED_WRITE_POINTER *Condensed;
> +
> +  if (S3Context->Used == S3Context->Allocated) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +  Condensed = S3Context->WritePointers + S3Context->Used;
> +  Condensed->PointerItem   = PointerItem;
> +  Condensed->PointerSize   = PointerSize;
> +  Condensed->PointerOffset = PointerOffset;
> +  Condensed->PointerValue  = PointerValue;
> +  DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n",
> +    __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue,
> +    (UINT64)S3Context->Used));
> +  ++S3Context->Used;
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Translate and append the information from an S3_CONTEXT object to the ACPI S3
> +  Boot Script.
> +
> +  The effects of a successful call to this function cannot be undone.
> +
> +  @param[in] S3Context  The S3_CONTEXT object to translate to ACPI S3 Boot
> +                        Script opcodes.
> +
> +  @retval EFI_OUT_OF_RESOURCES  Out of memory.
> +
> +  @retval EFI_SUCCESS           The translation of S3Context to ACPI S3 Boot
> +                                Script opcodes has been successful.
> +
> +  @return                       Error codes from underlying functions.
> +**/
> +EFI_STATUS
> +TransferS3ContextToBootScript (
> +  IN CONST S3_CONTEXT *S3Context
> +  )
> +{
> +  EFI_STATUS                 Status;
> +  EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState;
> +  SCRATCH_BUFFER             *ScratchBuffer;
> +  FW_CFG_DMA_ACCESS          *Access;
> +  UINT64                     BigEndianAddressOfAccess;
> +  UINT32                     ControlPollData;
> +  UINT32                     ControlPollMask;
> +  UINTN                      Index;
> +
> +  //
> +  // If the following protocol lookup fails, it shall not happen due to an
> +  // unexpected DXE driver dispatch order.
> +  //
> +  // Namely, this function is only invoked on QEMU. Therefore it is only
> +  // reached after Platform BDS signals gRootBridgesConnectedEventGroupGuid
> +  // (see OnRootBridgesConnected() in "EntryPoint.c"). Hence, because
> +  // TransferS3ContextToBootScript() is invoked in BDS, all DXE drivers,
> +  // including S3SaveStateDxe (producing EFI_S3_SAVE_STATE_PROTOCOL), have been
> +  // dispatched by the time we get here. (S3SaveStateDxe is not expected to
> +  // have any stricter-than-TRUE DEPEX -- not a DEPEX that gets unblocked only
> +  // within BDS anyway.)
> +  //
> +  // Reaching this function also depends on QemuFwCfgS3Enabled(). That implies
> +  // S3SaveStateDxe has not exited immediately due to S3 being disabled. Thus
> +  // EFI_S3_SAVE_STATE_PROTOCOL can only be missing for genuinely unforeseeable
> +  // reasons.
> +  //
> +  Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
> +                  NULL /* Registration */, (VOID **)&S3SaveState);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: LocateProtocol(): %r\n", __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer);
> +  if (ScratchBuffer == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Set up helper variables that we'll use identically for all
> +  // CONDENSED_WRITE_POINTER elements.
> +  //
> +  Access = &ScratchBuffer->Access;
> +  BigEndianAddressOfAccess = SwapBytes64 ((UINTN)Access);
> +  ControlPollData = 0;
> +  ControlPollMask = MAX_UINT32;
> +
> +  //
> +  // For each CONDENSED_WRITE_POINTER, we need six ACPI S3 Boot Script opcodes:
> +  // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
> +  //     the writeable fw_cfg file PointerFile (through PointerItem), and skips
> +  //     to PointerOffset in it,
> +  // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
> +  // (3) wait for the select+skip to finish,
> +  // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
> +  //     PointerValue (base address of the allocated / downloaded PointeeFile),
> +  //     of size PointerSize, into the fw_cfg file selected in (1), at the
> +  //     offset sought to in (1),
> +  // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
> +  // (6) wait for the write to finish.
> +  //
> +  // EFI_S3_SAVE_STATE_PROTOCOL does not allow rolling back opcode additions,
> +  // therefore we treat any failure here as fatal.
> +  //
> +  for (Index = 0; Index < S3Context->Used; ++Index) {
> +    CONST CONDENSED_WRITE_POINTER *Condensed;
> +
> +    Condensed = &S3Context->WritePointers[Index];
> +
> +    //
> +    // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
> +    //     the writeable fw_cfg file PointerFile (through PointerItem), and
> +    //     skips to PointerOffset in it,
> +    //
> +    Access->Control = SwapBytes32 ((UINT32)Condensed->PointerItem << 16 |
> +                        FW_CFG_DMA_CTL_SELECT | FW_CFG_DMA_CTL_SKIP);
> +    Access->Length = SwapBytes32 (Condensed->PointerOffset);
> +    Access->Address = 0;
> +    Status = S3SaveState->Write (
> +                            S3SaveState,                      // This
> +                            EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
> +                            EfiBootScriptWidthUint8,          // Width
> +                            (UINT64)(UINTN)Access,            // Address
> +                            sizeof *Access,                   // Count
> +                            Access                            // Buffer
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 1: %r\n", __FUNCTION__,
> +        (UINT64)Index, Status));
> +      goto FatalError;
> +    }
> +
> +    //
> +    // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
> +    //
> +    Status = S3SaveState->Write (
> +                            S3SaveState,                     // This
> +                            EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
> +                            EfiBootScriptWidthUint32,        // Width
> +                            (UINT64)0x514,                   // Address

It's unfortunate that the boot script makes us add fw-cfg low level
details into a module besides QemuFwCfgLib.

We probably should add OvmfPkg/Include/IndustryStandard/QemuFwCfg.h to
define the IA32/X64 I/O ports used by fw-cfg. Maybe move some other
items from QemuFwCfgLib.h too.

This is using the DMA access, right? Does something prevent adding
this boot script entry when the DMA interface is not supported?

-Jordan

> +                            (UINTN)2,                        // Count
> +                            &BigEndianAddressOfAccess        // Buffer
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 2: %r\n", __FUNCTION__,
> +        (UINT64)Index, Status));
> +      goto FatalError;
> +    }
> +
> +    //
> +    // (3) wait for the select+skip to finish,
> +    //
> +    Status = S3SaveState->Write (
> +                            S3SaveState,                     // This
> +                            EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
> +                            EfiBootScriptWidthUint32,        // Width
> +                            (UINT64)(UINTN)&Access->Control, // Address
> +                            &ControlPollData,                // Data
> +                            &ControlPollMask,                // DataMask
> +                            MAX_UINT64                       // Delay
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 3: %r\n", __FUNCTION__,
> +        (UINT64)Index, Status));
> +      goto FatalError;
> +    }
> +
> +    //
> +    // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
> +    //     PointerValue (base address of the allocated / downloaded
> +    //     PointeeFile), of size PointerSize, into the fw_cfg file selected in
> +    //     (1), at the offset sought to in (1),
> +    //
> +    Access->Control = SwapBytes32 (FW_CFG_DMA_CTL_WRITE);
> +    Access->Length = SwapBytes32 (Condensed->PointerSize);
> +    Access->Address = SwapBytes64 ((UINTN)&ScratchBuffer->PointerValue);
> +    ScratchBuffer->PointerValue = Condensed->PointerValue;
> +    Status = S3SaveState->Write (
> +                            S3SaveState,                      // This
> +                            EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
> +                            EfiBootScriptWidthUint8,          // Width
> +                            (UINT64)(UINTN)ScratchBuffer,     // Address
> +                            sizeof *ScratchBuffer,            // Count
> +                            ScratchBuffer                     // Buffer
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 4: %r\n", __FUNCTION__,
> +        (UINT64)Index, Status));
> +      goto FatalError;
> +    }
> +
> +    //
> +    // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
> +    //
> +    Status = S3SaveState->Write (
> +                            S3SaveState,                     // This
> +                            EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
> +                            EfiBootScriptWidthUint32,        // Width
> +                            (UINT64)0x514,                   // Address
> +                            (UINTN)2,                        // Count
> +                            &BigEndianAddressOfAccess        // Buffer
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 5: %r\n", __FUNCTION__,
> +        (UINT64)Index, Status));
> +      goto FatalError;
> +    }
> +
> +    //
> +    // (6) wait for the write to finish.
> +    //
> +    Status = S3SaveState->Write (
> +                            S3SaveState,                     // This
> +                            EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
> +                            EfiBootScriptWidthUint32,        // Width
> +                            (UINT64)(UINTN)&Access->Control, // Address
> +                            &ControlPollData,                // Data
> +                            &ControlPollMask,                // DataMask
> +                            MAX_UINT64                       // Delay
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 6: %r\n", __FUNCTION__,
> +        (UINT64)Index, Status));
> +      goto FatalError;
> +    }
> +  }
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: boot script fragment saved, ScratchBuffer=%p\n",
> +    __FUNCTION__, (VOID *)ScratchBuffer));
> +  return EFI_SUCCESS;
> +
> +FatalError:
> +  ASSERT (FALSE);
> +  CpuDeadLoop ();
> +  return Status;
> +}
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index de827c2df204..eadd690bef4e 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -358,26 +358,39 @@ ProcessCmdAddChecksum (
>    @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
>  
>    @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
>                              structures created thus far.
>  
> +  @param[in,out] S3Context  The S3_CONTEXT object capturing the fw_cfg actions
> +                            of successfully processed QEMU_LOADER_WRITE_POINTER
> +                            commands, to be replayed at S3 resume. S3Context
> +                            may be NULL if S3 is disabled.
> +
>    @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
>                                WritePointer. Or, the WritePointer command
>                                references a file unknown to Tracker or the
>                                fw_cfg directory. Or, the pointer object to
>                                rewrite has invalid location, size, or initial
>                                relative value. Or, the pointer value to store
>                                does not fit in the given pointer size.
>  
>    @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
> -                              file has been written.
> +                              file has been written. If S3Context is not NULL,
> +                              then WritePointer has been condensed into
> +                              S3Context.
> +
> +  @return                     Error codes propagated from
> +                              SaveCondensedWritePointerToS3Context(). The
> +                              pointer object inside the writeable fw_cfg file
> +                              has not been written.
>  **/
>  STATIC
>  EFI_STATUS
>  ProcessCmdWritePointer (
>    IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
> -  IN     CONST ORDERED_COLLECTION        *Tracker
> +  IN     CONST ORDERED_COLLECTION        *Tracker,
> +  IN OUT       S3_CONTEXT                *S3Context OPTIONAL
>    )
>  {
>    RETURN_STATUS            Status;
>    FIRMWARE_CONFIG_ITEM     PointerItem;
>    UINTN                    PointerItemSize;
> @@ -430,10 +443,29 @@ ProcessCmdWritePointer (
>      DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
>        __FUNCTION__, WritePointer->PointerFile));
>      return EFI_PROTOCOL_ERROR;
>    }
>  
> +  //
> +  // If S3 is enabled, we have to capture the below fw_cfg actions in condensed
> +  // form, to be replayed during S3 resume.
> +  //
> +  if (S3Context != NULL) {
> +    EFI_STATUS SaveStatus;
> +
> +    SaveStatus = SaveCondensedWritePointerToS3Context (
> +                   S3Context,
> +                   (UINT16)PointerItem,
> +                   WritePointer->PointerSize,
> +                   WritePointer->PointerOffset,
> +                   PointerValue
> +                   );
> +    if (EFI_ERROR (SaveStatus)) {
> +      return SaveStatus;
> +    }
> +  }
> +
>    QemuFwCfgSelectItem (PointerItem);
>    QemuFwCfgSkipBytes (WritePointer->PointerOffset);
>    QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
>  
>    //
> @@ -699,10 +731,11 @@ InstallQemuFwCfgTables (
>    QEMU_LOADER_ENTRY        *LoaderStart;
>    CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
>    CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
>    ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
>    UINTN                    OriginalPciAttributesCount;
> +  S3_CONTEXT               *S3Context;
>    ORDERED_COLLECTION       *Tracker;
>    UINTN                    *InstalledKey;
>    INT32                    Installed;
>    ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
>  
> @@ -724,14 +757,26 @@ InstallQemuFwCfgTables (
>    QemuFwCfgSelectItem (FwCfgItem);
>    QemuFwCfgReadBytes (FwCfgSize, LoaderStart);
>    RestorePciDecoding (OriginalPciAttributes, OriginalPciAttributesCount);
>    LoaderEnd = LoaderStart + FwCfgSize / sizeof *LoaderEntry;
>  
> +  S3Context = NULL;
> +  if (QemuFwCfgS3Enabled ()) {
> +    //
> +    // Size the allocation pessimistically, assuming that all commands in the
> +    // script are QEMU_LOADER_WRITE_POINTER commands.
> +    //
> +    Status = AllocateS3Context (&S3Context, LoaderEnd - LoaderStart);
> +    if (EFI_ERROR (Status)) {
> +      goto FreeLoader;
> +    }
> +  }
> +
>    Tracker = OrderedCollectionInit (BlobCompare, BlobKeyCompare);
>    if (Tracker == NULL) {
>      Status = EFI_OUT_OF_RESOURCES;
> -    goto FreeLoader;
> +    goto FreeS3Context;
>    }
>  
>    //
>    // first pass: process the commands
>    //
> @@ -756,11 +801,11 @@ InstallQemuFwCfgTables (
>                   Tracker);
>        break;
>  
>      case QemuLoaderCmdWritePointer:
>          Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
> -                   Tracker);
> +                   Tracker, S3Context);
>          if (!EFI_ERROR (Status)) {
>            WritePointerSubsetEnd = LoaderEntry + 1;
>          }
>          break;
>  
> @@ -788,15 +833,25 @@ InstallQemuFwCfgTables (
>    for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>      if (LoaderEntry->Type == QemuLoaderCmdAddPointer) {
>        Status = Process2ndPassCmdAddPointer (&LoaderEntry->Command.AddPointer,
>                   Tracker, AcpiProtocol, InstalledKey, &Installed);
>        if (EFI_ERROR (Status)) {
> -        break;
> +        goto UninstallAcpiTables;
>        }
>      }
>    }
>  
> +  //
> +  // Translating the condensed QEMU_LOADER_WRITE_POINTER commands to ACPI S3
> +  // Boot Script opcodes has to be the last operation in this function, because
> +  // if it succeeds, it cannot be undone.
> +  //
> +  if (S3Context != NULL) {
> +    Status = TransferS3ContextToBootScript (S3Context);
> +  }
> +
> +UninstallAcpiTables:
>    if (EFI_ERROR (Status)) {
>      //
>      // roll back partial installation
>      //
>      while (Installed > 0) {
> @@ -845,10 +900,15 @@ RollbackWritePointersAndFreeTracker:
>      }
>      FreePool (Blob);
>    }
>    OrderedCollectionUninit (Tracker);
>  
> +FreeS3Context:
> +  if (S3Context != NULL) {
> +    ReleaseS3Context (S3Context);
> +  }
> +
>  FreeLoader:
>    FreePool (LoaderStart);
>  
>    return Status;
>  }
> -- 
> 2.9.3
> 

Re: [PATCH 5/5] OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at S3

[Laszlo Ersek]

On 02/17/17 22:25, Jordan Justen wrote:
> On 2017-02-16 12:41:37, Laszlo Ersek wrote:
>> Ultimately, each QEMU_LOADER_WRITE_POINTER command creates a guest memory
>> reference in some QEMU device. When the virtual machine is reset, the
>> device willfully forgets the guest address, since the guest memory is
>> wholly invalidated during platform reset.
>>
>> ... Unless the reset is part of S3 resume. Then the guest memory is
>> preserved intact, and the firmware must reprogram those devices with the
>> original guest memory allocation addresses.
>>
>> This patch accumulates the fw_cfg select, skip and write operations of
>> ProcessCmdWritePointer() in a validated / condensed form, and turns them
>> into an ACPI S3 Boot Script fragment at the very end of
>> InstallQemuFwCfgTables().
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf          |   2 +
>>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf |   2 +
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h               |  27 ++
>>  OvmfPkg/AcpiPlatformDxe/BootScript.c                 | 414 ++++++++++++++++++++
>>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c              |  70 +++-
>>  5 files changed, 510 insertions(+), 5 deletions(-)
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
>> index 654d3a03905d..bb5f14e0fc7a 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
>> @@ -31,10 +31,11 @@ [Sources]
>>    Qemu.c
>>    QemuFwCfgAcpi.c
>>    Xen.c
>>    EntryPoint.c
>>    PciDecoding.c
>> +  BootScript.c
>>  
>>  [Packages]
>>    MdePkg/MdePkg.dec
>>    MdeModulePkg/MdeModulePkg.dec
>>    OvmfPkg/OvmfPkg.dec
>> @@ -57,10 +58,11 @@ [LibraryClasses]
>>    OrderedCollectionLib
>>  
>>  [Protocols]
>>    gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
>>    gEfiPciIoProtocolGuid                         # PROTOCOL SOMETIMES_CONSUMED
>> +  gEfiS3SaveStateProtocolGuid                   # PROTOCOL SOMETIMES_CONSUMED
>>  
>>  [Guids]
>>    gEfiXenInfoGuid
>>    gRootBridgesConnectedEventGroupGuid
>>  
>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
>> index d99f2d5a95c7..e550ff5a4714 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
>> @@ -29,10 +29,11 @@ [Defines]
>>  [Sources]
>>    QemuFwCfgAcpiPlatform.c
>>    QemuFwCfgAcpi.c
>>    EntryPoint.c
>>    PciDecoding.c
>> +  BootScript.c
>>  
>>  [Packages]
>>    MdePkg/MdePkg.dec
>>    MdeModulePkg/MdeModulePkg.dec
>>    OvmfPkg/OvmfPkg.dec
>> @@ -47,10 +48,11 @@ [LibraryClasses]
>>    UefiDriverEntryPoint
>>  
>>  [Protocols]
>>    gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
>>    gEfiPciIoProtocolGuid                         # PROTOCOL SOMETIMES_CONSUMED
>> +  gEfiS3SaveStateProtocolGuid                   # PROTOCOL SOMETIMES_CONSUMED
>>  
>>  [Guids]
>>    gRootBridgesConnectedEventGroupGuid
>>  
>>  [Pcd]
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> index 08dd7f8f7dd7..0f035a0d5751 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> @@ -31,10 +31,12 @@
>>  typedef struct {
>>    EFI_PCI_IO_PROTOCOL *PciIo;
>>    UINT64              PciAttributes;
>>  } ORIGINAL_ATTRIBUTES;
>>  
>> +typedef struct S3_CONTEXT S3_CONTEXT;
>> +
>>  EFI_STATUS
>>  EFIAPI
>>  InstallAcpiTable (
>>    IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol,
>>    IN   VOID                          *AcpiTableBuffer,
>> @@ -89,7 +91,32 @@ VOID
>>  RestorePciDecoding (
>>    IN ORIGINAL_ATTRIBUTES *OriginalAttributes,
>>    IN UINTN               Count
>>    );
>>  
>> +EFI_STATUS
>> +AllocateS3Context (
>> +  OUT S3_CONTEXT **S3Context,
>> +  IN  UINTN      WritePointerCount
>> +  );
>> +
>> +VOID
>> +ReleaseS3Context (
>> +  IN S3_CONTEXT *S3Context
>> +  );
>> +
>> +EFI_STATUS
>> +SaveCondensedWritePointerToS3Context (
>> +  IN OUT S3_CONTEXT *S3Context,
>> +  IN     UINT16     PointerItem,
>> +  IN     UINT8      PointerSize,
>> +  IN     UINT32     PointerOffset,
>> +  IN     UINT64     PointerValue
>> +  );
>> +
>> +EFI_STATUS
>> +TransferS3ContextToBootScript (
>> +  IN CONST S3_CONTEXT *S3Context
>> +  );
>> +
>>  #endif
>>  
>> diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
>> new file mode 100644
>> index 000000000000..b7a7f270f223
>> --- /dev/null
>> +++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
>> @@ -0,0 +1,414 @@
>> +/** @file
>> +  Append an ACPI S3 Boot Script fragment from the QEMU_LOADER_WRITE_POINTER
>> +  commands of QEMU's fully processed table linker/loader script.
>> +
>> +  Copyright (C) 2017, Red Hat, Inc.
>> +
>> +  This program and the accompanying materials are licensed and made available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +**/
>> +
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/QemuFwCfgLib.h>
>> +#include <Protocol/S3SaveState.h>
>> +
>> +#include "AcpiPlatform.h"
>> +
>> +
>> +//
>> +// Condensed structure for capturing the fw_cfg operations -- select, skip,
>> +// write -- inherent in executing a QEMU_LOADER_WRITE_POINTER command.
>> +//
>> +typedef struct {
>> +  UINT16 PointerItem;   // resolved from QEMU_LOADER_WRITE_POINTER.PointerFile
>> +  UINT8  PointerSize;   // copied as-is from QEMU_LOADER_WRITE_POINTER
>> +  UINT32 PointerOffset; // copied as-is from QEMU_LOADER_WRITE_POINTER
>> +  UINT64 PointerValue;  // resolved from QEMU_LOADER_WRITE_POINTER.PointeeFile
>> +} CONDENSED_WRITE_POINTER;
>> +
>> +
>> +//
>> +// Context structure to accumulate CONDENSED_WRITE_POINTER objects from
>> +// QEMU_LOADER_WRITE_POINTER commands.
>> +//
>> +// Any pointers in this structure own the pointed-to objects; that is, when the
>> +// context structure is released, all pointed-to objects must be released too.
>> +//
>> +struct S3_CONTEXT {
>> +  CONDENSED_WRITE_POINTER *WritePointers; // one array element per processed
>> +                                          //   QEMU_LOADER_WRITE_POINTER
>> +                                          //   command
>> +  UINTN                   Allocated;      // number of elements allocated for
>> +                                          //   WritePointers
>> +  UINTN                   Used;           // number of elements populated in
>> +                                          //   WritePointers
>> +};
>> +
>> +
>> +//
>> +// Scratch buffer, allocated in EfiReservedMemoryType type memory, for the ACPI
>> +// S3 Boot Script opcodes to work on. We use the buffer to compose and to
>> +// replay several fw_cfg select+skip and write operations, using the DMA access
>> +// method. The fw_cfg operations will implement the actions dictated by
>> +// CONDENSED_WRITE_POINTER objects.
>> +//
>> +#pragma pack (1)
>> +typedef struct {
>> +  FW_CFG_DMA_ACCESS Access;       // filled in from
>> +                                  //   CONDENSED_WRITE_POINTER.PointerItem,
>> +                                  //   CONDENSED_WRITE_POINTER.PointerSize,
>> +                                  //   CONDENSED_WRITE_POINTER.PointerOffset
>> +  UINT64            PointerValue; // filled in from
>> +                                  //   CONDENSED_WRITE_POINTER.PointerValue
>> +} SCRATCH_BUFFER;
>> +#pragma pack ()
>> +
>> +
>> +/**
>> +  Allocate an S3_CONTEXT object.
>> +
>> +  @param[out] S3Context         The allocated S3_CONTEXT object is returned
>> +                                through this parameter.
>> +
>> +  @param[in] WritePointerCount  Number of CONDENSED_WRITE_POINTER elements to
>> +                                allocate room for. WritePointerCount must be
>> +                                positive.
>> +
>> +  @retval EFI_SUCCESS            Allocation successful.
>> +
>> +  @retval EFI_OUT_OF_RESOURCES   Out of memory.
>> +
>> +  @retval EFI_INVALID_PARAMETER  WritePointerCount is zero.
>> +**/
>> +EFI_STATUS
>> +AllocateS3Context (
>> +  OUT S3_CONTEXT **S3Context,
>> +  IN  UINTN      WritePointerCount
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +  S3_CONTEXT *Context;
>> +
>> +  if (WritePointerCount == 0) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  Context = AllocateZeroPool (sizeof *Context);
>> +  if (Context == NULL) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  Context->WritePointers = AllocatePool (WritePointerCount *
>> +                             sizeof *Context->WritePointers);
>> +  if (Context->WritePointers == NULL) {
>> +    Status = EFI_OUT_OF_RESOURCES;
>> +    goto FreeContext;
>> +  }
>> +
>> +  Context->Allocated = WritePointerCount;
>> +  *S3Context = Context;
>> +  return EFI_SUCCESS;
>> +
>> +FreeContext:
>> +  FreePool (Context);
>> +
>> +  return Status;
>> +}
>> +
>> +
>> +/**
>> +  Release an S3_CONTEXT object.
>> +
>> +  @param[in] S3Context  The object to release.
>> +**/
>> +VOID
>> +ReleaseS3Context (
>> +  IN S3_CONTEXT *S3Context
>> +  )
>> +{
>> +  FreePool (S3Context->WritePointers);
>> +  FreePool (S3Context);
>> +}
>> +
>> +
>> +/**
>> +  Save the information necessary to replicate a QEMU_LOADER_WRITE_POINTER
>> +  command during S3 resume, in condensed format.
>> +
>> +  This function is to be called from ProcessCmdWritePointer(), after all the
>> +  sanity checks have passed, and before the fw_cfg operations are performed.
>> +
>> +  @param[in,out] S3Context  The S3_CONTEXT object into which the caller wants
>> +                            to save the information that was derived from
>> +                            QEMU_LOADER_WRITE_POINTER.
>> +
>> +  @param[in] PointerItem    The FIRMWARE_CONFIG_ITEM that
>> +                            QEMU_LOADER_WRITE_POINTER.PointerFile was resolved
>> +                            to, expressed as a UINT16 value.
>> +
>> +  @param[in] PointerSize    Copied directly from
>> +                            QEMU_LOADER_WRITE_POINTER.PointerSize.
>> +
>> +  @param[in] PointerOffset  Copied directly from
>> +                            QEMU_LOADER_WRITE_POINTER.PointerOffset.
>> +
>> +  @param[in] PointerValue   The base address of the allocated / downloaded
>> +                            fw_cfg blob that is identified by
>> +                            QEMU_LOADER_WRITE_POINTER.PointeeFile.
>> +
>> +  @retval EFI_SUCCESS           The information derived from
>> +                                QEMU_LOADER_WRITE_POINTER has been successfully
>> +                                absorbed into S3Context.
>> +
>> +  @retval EFI_OUT_OF_RESOURCES  No room available in S3Context.
>> +**/
>> +EFI_STATUS
>> +SaveCondensedWritePointerToS3Context (
>> +  IN OUT S3_CONTEXT *S3Context,
>> +  IN     UINT16     PointerItem,
>> +  IN     UINT8      PointerSize,
>> +  IN     UINT32     PointerOffset,
>> +  IN     UINT64     PointerValue
>> +  )
>> +{
>> +  CONDENSED_WRITE_POINTER *Condensed;
>> +
>> +  if (S3Context->Used == S3Context->Allocated) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +  Condensed = S3Context->WritePointers + S3Context->Used;
>> +  Condensed->PointerItem   = PointerItem;
>> +  Condensed->PointerSize   = PointerSize;
>> +  Condensed->PointerOffset = PointerOffset;
>> +  Condensed->PointerValue  = PointerValue;
>> +  DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n",
>> +    __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue,
>> +    (UINT64)S3Context->Used));
>> +  ++S3Context->Used;
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +
>> +/**
>> +  Translate and append the information from an S3_CONTEXT object to the ACPI S3
>> +  Boot Script.
>> +
>> +  The effects of a successful call to this function cannot be undone.
>> +
>> +  @param[in] S3Context  The S3_CONTEXT object to translate to ACPI S3 Boot
>> +                        Script opcodes.
>> +
>> +  @retval EFI_OUT_OF_RESOURCES  Out of memory.
>> +
>> +  @retval EFI_SUCCESS           The translation of S3Context to ACPI S3 Boot
>> +                                Script opcodes has been successful.
>> +
>> +  @return                       Error codes from underlying functions.
>> +**/
>> +EFI_STATUS
>> +TransferS3ContextToBootScript (
>> +  IN CONST S3_CONTEXT *S3Context
>> +  )
>> +{
>> +  EFI_STATUS                 Status;
>> +  EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState;
>> +  SCRATCH_BUFFER             *ScratchBuffer;
>> +  FW_CFG_DMA_ACCESS          *Access;
>> +  UINT64                     BigEndianAddressOfAccess;
>> +  UINT32                     ControlPollData;
>> +  UINT32                     ControlPollMask;
>> +  UINTN                      Index;
>> +
>> +  //
>> +  // If the following protocol lookup fails, it shall not happen due to an
>> +  // unexpected DXE driver dispatch order.
>> +  //
>> +  // Namely, this function is only invoked on QEMU. Therefore it is only
>> +  // reached after Platform BDS signals gRootBridgesConnectedEventGroupGuid
>> +  // (see OnRootBridgesConnected() in "EntryPoint.c"). Hence, because
>> +  // TransferS3ContextToBootScript() is invoked in BDS, all DXE drivers,
>> +  // including S3SaveStateDxe (producing EFI_S3_SAVE_STATE_PROTOCOL), have been
>> +  // dispatched by the time we get here. (S3SaveStateDxe is not expected to
>> +  // have any stricter-than-TRUE DEPEX -- not a DEPEX that gets unblocked only
>> +  // within BDS anyway.)
>> +  //
>> +  // Reaching this function also depends on QemuFwCfgS3Enabled(). That implies
>> +  // S3SaveStateDxe has not exited immediately due to S3 being disabled. Thus
>> +  // EFI_S3_SAVE_STATE_PROTOCOL can only be missing for genuinely unforeseeable
>> +  // reasons.
>> +  //
>> +  Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
>> +                  NULL /* Registration */, (VOID **)&S3SaveState);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR, "%a: LocateProtocol(): %r\n", __FUNCTION__, Status));
>> +    return Status;
>> +  }
>> +
>> +  ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer);
>> +  if (ScratchBuffer == NULL) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  //
>> +  // Set up helper variables that we'll use identically for all
>> +  // CONDENSED_WRITE_POINTER elements.
>> +  //
>> +  Access = &ScratchBuffer->Access;
>> +  BigEndianAddressOfAccess = SwapBytes64 ((UINTN)Access);
>> +  ControlPollData = 0;
>> +  ControlPollMask = MAX_UINT32;
>> +
>> +  //
>> +  // For each CONDENSED_WRITE_POINTER, we need six ACPI S3 Boot Script opcodes:
>> +  // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
>> +  //     the writeable fw_cfg file PointerFile (through PointerItem), and skips
>> +  //     to PointerOffset in it,
>> +  // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
>> +  // (3) wait for the select+skip to finish,
>> +  // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
>> +  //     PointerValue (base address of the allocated / downloaded PointeeFile),
>> +  //     of size PointerSize, into the fw_cfg file selected in (1), at the
>> +  //     offset sought to in (1),
>> +  // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
>> +  // (6) wait for the write to finish.
>> +  //
>> +  // EFI_S3_SAVE_STATE_PROTOCOL does not allow rolling back opcode additions,
>> +  // therefore we treat any failure here as fatal.
>> +  //
>> +  for (Index = 0; Index < S3Context->Used; ++Index) {
>> +    CONST CONDENSED_WRITE_POINTER *Condensed;
>> +
>> +    Condensed = &S3Context->WritePointers[Index];
>> +
>> +    //
>> +    // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
>> +    //     the writeable fw_cfg file PointerFile (through PointerItem), and
>> +    //     skips to PointerOffset in it,
>> +    //
>> +    Access->Control = SwapBytes32 ((UINT32)Condensed->PointerItem << 16 |
>> +                        FW_CFG_DMA_CTL_SELECT | FW_CFG_DMA_CTL_SKIP);
>> +    Access->Length = SwapBytes32 (Condensed->PointerOffset);
>> +    Access->Address = 0;
>> +    Status = S3SaveState->Write (
>> +                            S3SaveState,                      // This
>> +                            EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
>> +                            EfiBootScriptWidthUint8,          // Width
>> +                            (UINT64)(UINTN)Access,            // Address
>> +                            sizeof *Access,                   // Count
>> +                            Access                            // Buffer
>> +                            );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 1: %r\n", __FUNCTION__,
>> +        (UINT64)Index, Status));
>> +      goto FatalError;
>> +    }
>> +
>> +    //
>> +    // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
>> +    //
>> +    Status = S3SaveState->Write (
>> +                            S3SaveState,                     // This
>> +                            EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
>> +                            EfiBootScriptWidthUint32,        // Width
>> +                            (UINT64)0x514,                   // Address
> 
> It's unfortunate that the boot script makes us add fw-cfg low level
> details into a module besides QemuFwCfgLib.

I agree. I dislike it too.

I was thinking about adding APIs to QemuFwCfgLib that would "format"
operations into a boot script rather than performing them at once, but I
couldn't really find a good abstraction where the interface wasn't
similarly baroque.

Also, it's severely complicated by the fact that these functions would
not only depend on being in DXE phase, but even on the presence of the
protocol (which is a dynamic question, based on QEMU config).

May not be impossible, but it looked like a task at least as big as this
series itself.

I vaguely recall some IoLib instance (???) that would perform an
operation and immediately capture it in a bootscript too. Unfortunately,
that's not flexible enough; in the present case for example, we need to
do a bunch of IO first, roll it all back if anything fails, and modify
the boot script only after everything else succeeds.

> 
> We probably should add OvmfPkg/Include/IndustryStandard/QemuFwCfg.h to
> define the IA32/X64 I/O ports used by fw-cfg. Maybe move some other
> items from QemuFwCfgLib.h too.

Yeah, the port numbers are the primary candidates (low hanging fruit).

> 
> This is using the DMA access, right?

Yes.

> Does something prevent adding
> this boot script entry when the DMA interface is not supported?

Well asked :)

So, yes, this is exactly one thing that I raised while reviewing the
QEMU work. In QEMU, the x86 machine types (i440fx and q35) are versioned
(along with a few others); that is, new QEMU releases preserve the old
machine types for compatibility reasons. The idea is that you can
upgrade QEMU, and the guest will not notice, as long as you stick with
the original machine type. This is important for both migration between
hosts (where the target host is allowed to run a higher QEMU release,
but it must be configured with the same machine type version), and for
keeping guest OSes in an unchanged hw environment across QEMU upgrades.

So, up to and including pc-i440fx-2.4 and pc-q35-2.4, the x86 machine
types don't support DMA for fw_cfg. But as it is right now, on the QEMU
command line, you can add the vmgenid device, even when selecting a
machine type that lacks DMA for fw_cfg. This will cause QEMU to generate
WRITE_POINTER commands that the firmware simply cannot carry out
successfully.

This is going to be addressed in followup QEMU patches (still before
QEMU 2.9 is frozen). The creation of the vmgenid device (and of all
other devices that might require WRITE_POINTER commands) will be
prevented if the machine type lacks DMA for fw_cfg.

(In more detail, the vmgenid device will grow a boolean compat property
that will be set to "off" in parallel with the existent "dma_enabled"
property of the "fw_cfg_io" and "fw_cfg_mem" device types, in
HW_COMPAT_2_4 [include/hw/compat.h], and the vmgenid device's realize()
method will report an error when this property is false.)

Thus, the mere presence of WRITE_POINTER commands in the linker/loader
script guarantees (... will guarantee, before QEMU 2.9 is released with
vmgenid / WRITE_POINTER) that fw_cfg DMA is supported.

BTW, a similar question can be asked about the broadcast SMI negotiation
-- that needs DMA for fw_cfg too. The answer is similar: broadcast SMI
negotiation is only available in pc-q35-2.9+ machine types, and all of
those provide DMA for fw_cfg. On earlier machine types (that might miss
DMA for fw_cfg), the fw_cfg files that implement the negotiation
interface are absent from the fw_cfg file directory, hence
SmmControl2Dxe will never get as far as calling QemuFwCfgWriteBytes() or
producing fw_cfg DMA opcodes for the boot script. (See
NegotiateSmiFeatures() and how its return value is used.)

Thus, this is something that I've specifically paid attention to, in
both the QEMU review and in the OVMF code.


I briefly considered a QemuFwCfgLib API that lets the caller inquire
about DMA enablement, but it just doesn't feel right, and (considering
the above) it's not even useful. Later I would like to abstract away
this S3 boot script munging, from both this driver and SmmControl2Dxe,
but the interface is a hard nut:

- SEC lib instance (exists only for x86): must return EFI_UNSUPPORTED
- PEI/DXE instance (exists only for x86): must be split into
  - PEI only: returns EFI_UNSUPPORTED
  - DXE only:
    - returns EFI_UNSUPPORTED if S3 is disabled
    - works if the protocol is available
    - what does it do if the protocol is not available *yet*?
      - can't use a DEPEX (whole client module won't load with S3
        disabled!)
      - register protocol notify in constructor? then what?
        - collect operations and write them to boot script at EndOfDxe?
          - that always turns out to be a bad idea, due to unspecified
            callback invocation order
- DXE only instance (exists for arm/aarch64 only): very different from
  the x86 impl already (uses chunked MMIO at the bottom, not REP IO),
  so unification with the above is hard

While writing this patch, I was racking my brain for hours about this. I
think I must have restarted this patch three or four times just because
of that.

Ultimately, it's the second driver where we use a meaningful boot script
fragment, and usually the third occurrence of a pattern is where the
generics start to emerge, and (hopefully) become a candidate for
factoring out. I dislike the repetitive and hard-to-read nature of the
opcode additions (hence the profuse comments above), but from the two
instances we can look at (SmmControl2Dxe and this patch), I couldn't
really abstract away a natural interface.

I think it is a relatively big task anyway, deserving its own BZ (not to
be confused with the also separate BZ that I proposed for residual /
cleanup patches on top of this series, should you recommend such.) If
you felt inclined to research / prototype that, that would be huge...

Thanks!
Laszlo

> 
> -Jordan
> 
>> +                            (UINTN)2,                        // Count
>> +                            &BigEndianAddressOfAccess        // Buffer
>> +                            );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 2: %r\n", __FUNCTION__,
>> +        (UINT64)Index, Status));
>> +      goto FatalError;
>> +    }
>> +
>> +    //
>> +    // (3) wait for the select+skip to finish,
>> +    //
>> +    Status = S3SaveState->Write (
>> +                            S3SaveState,                     // This
>> +                            EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
>> +                            EfiBootScriptWidthUint32,        // Width
>> +                            (UINT64)(UINTN)&Access->Control, // Address
>> +                            &ControlPollData,                // Data
>> +                            &ControlPollMask,                // DataMask
>> +                            MAX_UINT64                       // Delay
>> +                            );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 3: %r\n", __FUNCTION__,
>> +        (UINT64)Index, Status));
>> +      goto FatalError;
>> +    }
>> +
>> +    //
>> +    // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
>> +    //     PointerValue (base address of the allocated / downloaded
>> +    //     PointeeFile), of size PointerSize, into the fw_cfg file selected in
>> +    //     (1), at the offset sought to in (1),
>> +    //
>> +    Access->Control = SwapBytes32 (FW_CFG_DMA_CTL_WRITE);
>> +    Access->Length = SwapBytes32 (Condensed->PointerSize);
>> +    Access->Address = SwapBytes64 ((UINTN)&ScratchBuffer->PointerValue);
>> +    ScratchBuffer->PointerValue = Condensed->PointerValue;
>> +    Status = S3SaveState->Write (
>> +                            S3SaveState,                      // This
>> +                            EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
>> +                            EfiBootScriptWidthUint8,          // Width
>> +                            (UINT64)(UINTN)ScratchBuffer,     // Address
>> +                            sizeof *ScratchBuffer,            // Count
>> +                            ScratchBuffer                     // Buffer
>> +                            );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 4: %r\n", __FUNCTION__,
>> +        (UINT64)Index, Status));
>> +      goto FatalError;
>> +    }
>> +
>> +    //
>> +    // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
>> +    //
>> +    Status = S3SaveState->Write (
>> +                            S3SaveState,                     // This
>> +                            EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
>> +                            EfiBootScriptWidthUint32,        // Width
>> +                            (UINT64)0x514,                   // Address
>> +                            (UINTN)2,                        // Count
>> +                            &BigEndianAddressOfAccess        // Buffer
>> +                            );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 5: %r\n", __FUNCTION__,
>> +        (UINT64)Index, Status));
>> +      goto FatalError;
>> +    }
>> +
>> +    //
>> +    // (6) wait for the write to finish.
>> +    //
>> +    Status = S3SaveState->Write (
>> +                            S3SaveState,                     // This
>> +                            EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
>> +                            EfiBootScriptWidthUint32,        // Width
>> +                            (UINT64)(UINTN)&Access->Control, // Address
>> +                            &ControlPollData,                // Data
>> +                            &ControlPollMask,                // DataMask
>> +                            MAX_UINT64                       // Delay
>> +                            );
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 6: %r\n", __FUNCTION__,
>> +        (UINT64)Index, Status));
>> +      goto FatalError;
>> +    }
>> +  }
>> +
>> +  DEBUG ((DEBUG_VERBOSE, "%a: boot script fragment saved, ScratchBuffer=%p\n",
>> +    __FUNCTION__, (VOID *)ScratchBuffer));
>> +  return EFI_SUCCESS;
>> +
>> +FatalError:
>> +  ASSERT (FALSE);
>> +  CpuDeadLoop ();
>> +  return Status;
>> +}
>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> index de827c2df204..eadd690bef4e 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> @@ -358,26 +358,39 @@ ProcessCmdAddChecksum (
>>    @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
>>  
>>    @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
>>                              structures created thus far.
>>  
>> +  @param[in,out] S3Context  The S3_CONTEXT object capturing the fw_cfg actions
>> +                            of successfully processed QEMU_LOADER_WRITE_POINTER
>> +                            commands, to be replayed at S3 resume. S3Context
>> +                            may be NULL if S3 is disabled.
>> +
>>    @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
>>                                WritePointer. Or, the WritePointer command
>>                                references a file unknown to Tracker or the
>>                                fw_cfg directory. Or, the pointer object to
>>                                rewrite has invalid location, size, or initial
>>                                relative value. Or, the pointer value to store
>>                                does not fit in the given pointer size.
>>  
>>    @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
>> -                              file has been written.
>> +                              file has been written. If S3Context is not NULL,
>> +                              then WritePointer has been condensed into
>> +                              S3Context.
>> +
>> +  @return                     Error codes propagated from
>> +                              SaveCondensedWritePointerToS3Context(). The
>> +                              pointer object inside the writeable fw_cfg file
>> +                              has not been written.
>>  **/
>>  STATIC
>>  EFI_STATUS
>>  ProcessCmdWritePointer (
>>    IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
>> -  IN     CONST ORDERED_COLLECTION        *Tracker
>> +  IN     CONST ORDERED_COLLECTION        *Tracker,
>> +  IN OUT       S3_CONTEXT                *S3Context OPTIONAL
>>    )
>>  {
>>    RETURN_STATUS            Status;
>>    FIRMWARE_CONFIG_ITEM     PointerItem;
>>    UINTN                    PointerItemSize;
>> @@ -430,10 +443,29 @@ ProcessCmdWritePointer (
>>      DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
>>        __FUNCTION__, WritePointer->PointerFile));
>>      return EFI_PROTOCOL_ERROR;
>>    }
>>  
>> +  //
>> +  // If S3 is enabled, we have to capture the below fw_cfg actions in condensed
>> +  // form, to be replayed during S3 resume.
>> +  //
>> +  if (S3Context != NULL) {
>> +    EFI_STATUS SaveStatus;
>> +
>> +    SaveStatus = SaveCondensedWritePointerToS3Context (
>> +                   S3Context,
>> +                   (UINT16)PointerItem,
>> +                   WritePointer->PointerSize,
>> +                   WritePointer->PointerOffset,
>> +                   PointerValue
>> +                   );
>> +    if (EFI_ERROR (SaveStatus)) {
>> +      return SaveStatus;
>> +    }
>> +  }
>> +
>>    QemuFwCfgSelectItem (PointerItem);
>>    QemuFwCfgSkipBytes (WritePointer->PointerOffset);
>>    QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
>>  
>>    //
>> @@ -699,10 +731,11 @@ InstallQemuFwCfgTables (
>>    QEMU_LOADER_ENTRY        *LoaderStart;
>>    CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
>>    CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
>>    ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
>>    UINTN                    OriginalPciAttributesCount;
>> +  S3_CONTEXT               *S3Context;
>>    ORDERED_COLLECTION       *Tracker;
>>    UINTN                    *InstalledKey;
>>    INT32                    Installed;
>>    ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
>>  
>> @@ -724,14 +757,26 @@ InstallQemuFwCfgTables (
>>    QemuFwCfgSelectItem (FwCfgItem);
>>    QemuFwCfgReadBytes (FwCfgSize, LoaderStart);
>>    RestorePciDecoding (OriginalPciAttributes, OriginalPciAttributesCount);
>>    LoaderEnd = LoaderStart + FwCfgSize / sizeof *LoaderEntry;
>>  
>> +  S3Context = NULL;
>> +  if (QemuFwCfgS3Enabled ()) {
>> +    //
>> +    // Size the allocation pessimistically, assuming that all commands in the
>> +    // script are QEMU_LOADER_WRITE_POINTER commands.
>> +    //
>> +    Status = AllocateS3Context (&S3Context, LoaderEnd - LoaderStart);
>> +    if (EFI_ERROR (Status)) {
>> +      goto FreeLoader;
>> +    }
>> +  }
>> +
>>    Tracker = OrderedCollectionInit (BlobCompare, BlobKeyCompare);
>>    if (Tracker == NULL) {
>>      Status = EFI_OUT_OF_RESOURCES;
>> -    goto FreeLoader;
>> +    goto FreeS3Context;
>>    }
>>  
>>    //
>>    // first pass: process the commands
>>    //
>> @@ -756,11 +801,11 @@ InstallQemuFwCfgTables (
>>                   Tracker);
>>        break;
>>  
>>      case QemuLoaderCmdWritePointer:
>>          Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
>> -                   Tracker);
>> +                   Tracker, S3Context);
>>          if (!EFI_ERROR (Status)) {
>>            WritePointerSubsetEnd = LoaderEntry + 1;
>>          }
>>          break;
>>  
>> @@ -788,15 +833,25 @@ InstallQemuFwCfgTables (
>>    for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>>      if (LoaderEntry->Type == QemuLoaderCmdAddPointer) {
>>        Status = Process2ndPassCmdAddPointer (&LoaderEntry->Command.AddPointer,
>>                   Tracker, AcpiProtocol, InstalledKey, &Installed);
>>        if (EFI_ERROR (Status)) {
>> -        break;
>> +        goto UninstallAcpiTables;
>>        }
>>      }
>>    }
>>  
>> +  //
>> +  // Translating the condensed QEMU_LOADER_WRITE_POINTER commands to ACPI S3
>> +  // Boot Script opcodes has to be the last operation in this function, because
>> +  // if it succeeds, it cannot be undone.
>> +  //
>> +  if (S3Context != NULL) {
>> +    Status = TransferS3ContextToBootScript (S3Context);
>> +  }
>> +
>> +UninstallAcpiTables:
>>    if (EFI_ERROR (Status)) {
>>      //
>>      // roll back partial installation
>>      //
>>      while (Installed > 0) {
>> @@ -845,10 +900,15 @@ RollbackWritePointersAndFreeTracker:
>>      }
>>      FreePool (Blob);
>>    }
>>    OrderedCollectionUninit (Tracker);
>>  
>> +FreeS3Context:
>> +  if (S3Context != NULL) {
>> +    ReleaseS3Context (S3Context);
>> +  }
>> +
>>  FreeLoader:
>>    FreePool (LoaderStart);
>>  
>>    return Status;
>>  }
>> -- 
>> 2.9.3
>>

Re: [PATCH 5/5] OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at S3

[Laszlo Ersek]

On 02/17/17 23:41, Laszlo Ersek wrote:

[snip]

> I briefly considered a QemuFwCfgLib API that lets the caller inquire
> about DMA enablement, but it just doesn't feel right, and (considering
> the above) it's not even useful. Later I would like to abstract away
> this S3 boot script munging, from both this driver and SmmControl2Dxe,
> but the interface is a hard nut:
> 
> - SEC lib instance (exists only for x86): must return EFI_UNSUPPORTED
> - PEI/DXE instance (exists only for x86): must be split into
>   - PEI only: returns EFI_UNSUPPORTED
>   - DXE only:
>     - returns EFI_UNSUPPORTED if S3 is disabled
>     - works if the protocol is available
>     - what does it do if the protocol is not available *yet*?
>       - can't use a DEPEX (whole client module won't load with S3
>         disabled!)
>       - register protocol notify in constructor? then what?
>         - collect operations and write them to boot script at EndOfDxe?
>           - that always turns out to be a bad idea, due to unspecified
>             callback invocation order

One thing I can imagine here is: simply push the dependency handling to
the client module. If the protocol is not found, just return
EFI_NOT_STARTED, and let the client module deal with that. The client
can set its own DEPEX, or register a protocol notify as necessary. So I
guess this could be solved after all, hopefully.

Laszlo

> - DXE only instance (exists for arm/aarch64 only): very different from
>   the x86 impl already (uses chunked MMIO at the bottom, not REP IO),
>   so unification with the above is hard
> 
> While writing this patch, I was racking my brain for hours about this. I
> think I must have restarted this patch three or four times just because
> of that.
> 
> Ultimately, it's the second driver where we use a meaningful boot script
> fragment, and usually the third occurrence of a pattern is where the
> generics start to emerge, and (hopefully) become a candidate for
> factoring out. I dislike the repetitive and hard-to-read nature of the
> opcode additions (hence the profuse comments above), but from the two
> instances we can look at (SmmControl2Dxe and this patch), I couldn't
> really abstract away a natural interface.
> 
> I think it is a relatively big task anyway, deserving its own BZ (not to
> be confused with the also separate BZ that I proposed for residual /
> cleanup patches on top of this series, should you recommend such.) If
> you felt inclined to research / prototype that, that would be huge...
> 
> Thanks!
> Laszlo
> 
>>
>> -Jordan

Re: [PATCH 5/5] OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at S3

[Jordan Justen]

On 2017-02-17 14:48:52, Laszlo Ersek wrote:
> On 02/17/17 23:41, Laszlo Ersek wrote:
> 
> [snip]
> 
> > I briefly considered a QemuFwCfgLib API that lets the caller inquire
> > about DMA enablement, but it just doesn't feel right, and (considering
> > the above) it's not even useful. Later I would like to abstract away
> > this S3 boot script munging, from both this driver and SmmControl2Dxe,
> > but the interface is a hard nut:
> > 
> > - SEC lib instance (exists only for x86): must return EFI_UNSUPPORTED
> > - PEI/DXE instance (exists only for x86): must be split into
> >   - PEI only: returns EFI_UNSUPPORTED
> >   - DXE only:
> >     - returns EFI_UNSUPPORTED if S3 is disabled
> >     - works if the protocol is available
> >     - what does it do if the protocol is not available *yet*?
> >       - can't use a DEPEX (whole client module won't load with S3
> >         disabled!)
> >       - register protocol notify in constructor? then what?
> >         - collect operations and write them to boot script at EndOfDxe?
> >           - that always turns out to be a bad idea, due to unspecified
> >             callback invocation order
> 
> One thing I can imagine here is: simply push the dependency handling to
> the client module. If the protocol is not found, just return
> EFI_NOT_STARTED, and let the client module deal with that. The client
> can set its own DEPEX, or register a protocol notify as necessary. So I
> guess this could be solved after all, hopefully.

Yeah, I figured the S3 depex is the tricky part. Also, adding an S3
specific interface doesn't fit well in QemuFwCfgLib, which I think
should focus on just the raw fw-cfg interface. Based on that, I'm not
particularly enthused about QemuFwCfgS3Enabled.

Maybe we could add a new S3+fw-cfg library interface? Maybe
QemuS3FwCfgLib? I'm not sure it makes sense, or solves all the
requirements anyhow...

...so, let's figure that out later. :)

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

> > - DXE only instance (exists for arm/aarch64 only): very different from
> >   the x86 impl already (uses chunked MMIO at the bottom, not REP IO),
> >   so unification with the above is hard
> > 
> > While writing this patch, I was racking my brain for hours about this. I
> > think I must have restarted this patch three or four times just because
> > of that.
> > 
> > Ultimately, it's the second driver where we use a meaningful boot script
> > fragment, and usually the third occurrence of a pattern is where the
> > generics start to emerge, and (hopefully) become a candidate for
> > factoring out. I dislike the repetitive and hard-to-read nature of the
> > opcode additions (hence the profuse comments above), but from the two
> > instances we can look at (SmmControl2Dxe and this patch), I couldn't
> > really abstract away a natural interface.
> > 
> > I think it is a relatively big task anyway, deserving its own BZ (not to
> > be confused with the also separate BZ that I proposed for residual /
> > cleanup patches on top of this series, should you recommend such.) If
> > you felt inclined to research / prototype that, that would be huge...
> > 
> > Thanks!
> > Laszlo
> > 
> >>
> >> -Jordan
> 
> 

Re: [PATCH 5/5] OvmfPkg/AcpiPlatformDxe: replay QEMU_LOADER_WRITE_POINTER commands at S3

[Laszlo Ersek]

On 02/21/17 01:17, Jordan Justen wrote:
> On 2017-02-17 14:48:52, Laszlo Ersek wrote:
>> On 02/17/17 23:41, Laszlo Ersek wrote:
>>
>> [snip]
>>
>>> I briefly considered a QemuFwCfgLib API that lets the caller inquire
>>> about DMA enablement, but it just doesn't feel right, and (considering
>>> the above) it's not even useful. Later I would like to abstract away
>>> this S3 boot script munging, from both this driver and SmmControl2Dxe,
>>> but the interface is a hard nut:
>>>
>>> - SEC lib instance (exists only for x86): must return EFI_UNSUPPORTED
>>> - PEI/DXE instance (exists only for x86): must be split into
>>>   - PEI only: returns EFI_UNSUPPORTED
>>>   - DXE only:
>>>     - returns EFI_UNSUPPORTED if S3 is disabled
>>>     - works if the protocol is available
>>>     - what does it do if the protocol is not available *yet*?
>>>       - can't use a DEPEX (whole client module won't load with S3
>>>         disabled!)
>>>       - register protocol notify in constructor? then what?
>>>         - collect operations and write them to boot script at EndOfDxe?
>>>           - that always turns out to be a bad idea, due to unspecified
>>>             callback invocation order
>>
>> One thing I can imagine here is: simply push the dependency handling to
>> the client module. If the protocol is not found, just return
>> EFI_NOT_STARTED, and let the client module deal with that. The client
>> can set its own DEPEX, or register a protocol notify as necessary. So I
>> guess this could be solved after all, hopefully.
> 
> Yeah, I figured the S3 depex is the tricky part. Also, adding an S3
> specific interface doesn't fit well in QemuFwCfgLib, which I think
> should focus on just the raw fw-cfg interface. Based on that, I'm not
> particularly enthused about QemuFwCfgS3Enabled.
> 
> Maybe we could add a new S3+fw-cfg library interface? Maybe
> QemuS3FwCfgLib? I'm not sure it makes sense, or solves all the
> requirements anyhow...

I think it would make sense, yes; I actually thought of that too, but
didn't dare suggest it, remembering how opposed you had been to a
separate IoFifoLib recently :)

> 
> ...so, let's figure that out later. :)

I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=394>  for this.

> 
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Series committed in range 85520606ad45..df73df138d9d.

I plan to post an "easy" followup series, but that won't change
functionality, and there's no similar time pressure for it.

Thank you very much for the prompt review!
Laszlo

> 
>>> - DXE only instance (exists for arm/aarch64 only): very different from
>>>   the x86 impl already (uses chunked MMIO at the bottom, not REP IO),
>>>   so unification with the above is hard
>>>
>>> While writing this patch, I was racking my brain for hours about this. I
>>> think I must have restarted this patch three or four times just because
>>> of that.
>>>
>>> Ultimately, it's the second driver where we use a meaningful boot script
>>> fragment, and usually the third occurrence of a pattern is where the
>>> generics start to emerge, and (hopefully) become a candidate for
>>> factoring out. I dislike the repetitive and hard-to-read nature of the
>>> opcode additions (hence the profuse comments above), but from the two
>>> instances we can look at (SmmControl2Dxe and this patch), I couldn't
>>> really abstract away a natural interface.
>>>
>>> I think it is a relatively big task anyway, deserving its own BZ (not to
>>> be confused with the also separate BZ that I proposed for residual /
>>> cleanup patches on top of this series, should you recommend such.) If
>>> you felt inclined to research / prototype that, that would be huge...
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>>
>>>> -Jordan
>>
>>