[PATCH V2 0/6] Refactor installation of gQemuAcpiTableNotifyProtocol

[PATCH V2 0/6] Refactor installation of gQemuAcpiTableNotifyProtocol

[Min Xu]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237

Commit 9fdc70af6ba8 breaks log analysis and code cohesion in 
AcpiPlatformDxe. See the detailed information in BZ#4237.

This patch-set contains below patches to fix above issues.
#1: Remove QEMU_ACPI_TABLE_NOTIFY_PROTOCOL and delete
    QemuAcpiTableNotify.h.
#2: Use local variable of ChAcpiHandle to store the handle.
#3: Use local variable of QemuAcpiHandle to store the handle.
#4: Add log to show how many ACPI tables installed.
#5: Refactor the installation of QemuAcpiTableNotifyProtocol and
    its error handling.
#6: Return error code if QemuAcpiTableNotifyProtocol is failed to be
    installed so that the caller can handle the error.

Code: https://github.com/mxu9/edk2/tree/AcpiPlatformDxe.v2

v2 changes:
 - Split v1 patch into 6 separate patches.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

Min M Xu (6):
  OvmfPkg/AcpiPlatformDxe: Remove QEMU_ACPI_TABLE_NOTIFY_PROTOCOL
  OvmfPkg/AcpiPlatformDxe: Use local variable in CloudHvAcpi.c
  OvmfPkg/AcpiPlatformDxe: Use local variable in QemuFwCfgAcpi.c
  OvmfPkg/AcpiPlatformDxe: Add log to show the installed tables
  OvmfPkg/AcpiPlatformDxe: Refactor QemuAcpiTableNotifyProtocol
  OvmfPkg/AcpiPlatformDxe: Return error if installing NotifyProtocol
    failed

 OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c         | 27 ++++++------
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c       | 44 ++++++++++++-------
 .../Include/Protocol/QemuAcpiTableNotify.h    | 27 ------------
 3 files changed, 43 insertions(+), 55 deletions(-)
 delete mode 100644 OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h

-- 
2.29.2.windows.2

[PATCH V2 1/6] OvmfPkg/AcpiPlatformDxe: Remove QEMU_ACPI_TABLE_NOTIFY_PROTOCOL

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237

The QEMU_ACPI_TABLE_NOTIFY_PROTOCOL structure is superfluous because NULL
protocol interfaces have been used in edk2 repeatedly. A protocol instance
can exist in the protocol database with a NULL associated interface.
Therefore the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL type, the
"QemuAcpiTableNotify.h" header, and the "mAcpiNotifyProtocol" global
variable can be removed.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c         |  7 ++---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c       |  6 ++---
 .../Include/Protocol/QemuAcpiTableNotify.h    | 27 -------------------
 3 files changed, 4 insertions(+), 36 deletions(-)
 delete mode 100644 OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h

diff --git a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
index cbe8bb9b0c75..ad39e4253478 100644
--- a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
@@ -15,15 +15,12 @@
 #include <Library/PcdLib.h>                               // PcdGet32()
 #include <Library/HobLib.h>                               // GetFirstGuidHob(), GetNextGuidHob()
 #include <Library/UefiBootServicesTableLib.h>             // gBS
-
 #include <Protocol/AcpiSystemDescriptionTable.h>
 #include <Protocol/AcpiTable.h>
-#include <Protocol/QemuAcpiTableNotify.h>                 // QEMU_ACPI_TABLE_NOTIFY_PROTOCOL
 
 #include "AcpiPlatform.h"
 
-EFI_HANDLE                       mChAcpiHandle = NULL;
-QEMU_ACPI_TABLE_NOTIFY_PROTOCOL  mChAcpiNotifyProtocol;
+EFI_HANDLE  mChAcpiHandle = NULL;
 
 EFI_STATUS
 EFIAPI
@@ -96,7 +93,7 @@ InstallCloudHvTablesTdx (
          &mChAcpiHandle,
          &gQemuAcpiTableNotifyProtocolGuid,
          EFI_NATIVE_INTERFACE,
-         &mChAcpiNotifyProtocol
+         NULL
          );
 
   return EFI_SUCCESS;
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index c8dee17c13e6..1a3852904df9 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -19,10 +19,8 @@
 #include <Library/QemuFwCfgS3Lib.h>           // QemuFwCfgS3Enabled()
 #include <Library/UefiBootServicesTableLib.h> // gBS
 
-#include <Protocol/QemuAcpiTableNotify.h>
 #include "AcpiPlatform.h"
-EFI_HANDLE                       mQemuAcpiHandle = NULL;
-QEMU_ACPI_TABLE_NOTIFY_PROTOCOL  mAcpiNotifyProtocol;
+EFI_HANDLE  mQemuAcpiHandle = NULL;
 
 //
 // The user structure for the ordered collection that will track the fw_cfg
@@ -1284,7 +1282,7 @@ UninstallAcpiTables:
            &mQemuAcpiHandle,
            &gQemuAcpiTableNotifyProtocolGuid,
            EFI_NATIVE_INTERFACE,
-           &mAcpiNotifyProtocol
+           NULL
            );
   }
 
diff --git a/OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h b/OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h
deleted file mode 100644
index a3dd2fc1dc91..000000000000
--- a/OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h
+++ /dev/null
@@ -1,27 +0,0 @@
-/** @file
-
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#ifndef QEMU_ACPI_TABLE_NOTIFY_H_
-#define QEMU_ACPI_TABLE_NOTIFY_H_
-
-#define QEMU_ACPI_TABLE_NOTIFY_GUID \
-  { 0x928939b2, 0x4235, 0x462f, { 0x95, 0x80, 0xf6, 0xa2, 0xb2, 0xc2, 0x1a, 0x4f } };
-
-///
-/// Forward declaration
-///
-typedef struct _QEMU_ACPI_TABLE_NOTIFY_PROTOCOL QEMU_ACPI_TABLE_NOTIFY_PROTOCOL;
-
-///
-/// Protocol structure
-///
-struct _QEMU_ACPI_TABLE_NOTIFY_PROTOCOL {
-  UINT8    Notify;
-};
-
-extern EFI_GUID  gQemuAcpiTableNotifyProtocolGuid;
-
-#endif
-- 
2.29.2.windows.2

[PATCH V2 2/6] OvmfPkg/AcpiPlatformDxe: Use local variable in CloudHvAcpi.c

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237

The handle of mChAcpiHandle is not needed for anything, beyond the
scope of the InstallCloudHvTablesTdx (). A local variable (ChAcpiHandle)
suffices for storing the handle.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
index ad39e4253478..d849a59526b5 100644
--- a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
@@ -20,8 +20,6 @@
 
 #include "AcpiPlatform.h"
 
-EFI_HANDLE  mChAcpiHandle = NULL;
-
 EFI_STATUS
 EFIAPI
 InstallCloudHvTablesTdx (
@@ -30,13 +28,15 @@ InstallCloudHvTablesTdx (
 {
   EFI_STATUS  Status;
   UINTN       TableHandle;
+  EFI_HANDLE  ChAcpiHandle;
 
   EFI_PEI_HOB_POINTERS         Hob;
   EFI_ACPI_DESCRIPTION_HEADER  *CurrentTable;
   EFI_ACPI_DESCRIPTION_HEADER  *DsdtTable;
 
-  DsdtTable   = NULL;
-  TableHandle = 0;
+  DsdtTable    = NULL;
+  TableHandle  = 0;
+  ChAcpiHandle = NULL;
 
   Hob.Guid = (EFI_HOB_GUID_TYPE *)GetFirstGuidHob (&gUefiOvmfPkgTdxAcpiHobGuid);
 
@@ -90,7 +90,7 @@ InstallCloudHvTablesTdx (
   // ready.
   //
   gBS->InstallProtocolInterface (
-         &mChAcpiHandle,
+         &ChAcpiHandle,
          &gQemuAcpiTableNotifyProtocolGuid,
          EFI_NATIVE_INTERFACE,
          NULL
-- 
2.29.2.windows.2

[PATCH V2 3/6] OvmfPkg/AcpiPlatformDxe: Use local variable in QemuFwCfgAcpi.c

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237

The handle of mQemuAcpiHandle is not needed for anything, beyond the
scope of the InstallQemuFwCfgTables(). So a local variable will
suffice for storing the handle.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 1a3852904df9..693cb8c8a83e 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -20,7 +20,6 @@
 #include <Library/UefiBootServicesTableLib.h> // gBS
 
 #include "AcpiPlatform.h"
-EFI_HANDLE  mQemuAcpiHandle = NULL;
 
 //
 // The user structure for the ordered collection that will track the fw_cfg
@@ -1101,6 +1100,9 @@ InstallQemuFwCfgTables (
   ORDERED_COLLECTION_ENTRY  *TrackerEntry, *TrackerEntry2;
   ORDERED_COLLECTION        *SeenPointers;
   ORDERED_COLLECTION_ENTRY  *SeenPointerEntry, *SeenPointerEntry2;
+  EFI_HANDLE                QemuAcpiHandle;
+
+  QemuAcpiHandle = NULL;
 
   Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize);
   if (EFI_ERROR (Status)) {
@@ -1279,7 +1281,7 @@ UninstallAcpiTables:
     // ready.
     //
     gBS->InstallProtocolInterface (
-           &mQemuAcpiHandle,
+           &QemuAcpiHandle,
            &gQemuAcpiTableNotifyProtocolGuid,
            EFI_NATIVE_INTERFACE,
            NULL
-- 
2.29.2.windows.2

[PATCH V2 4/6] OvmfPkg/AcpiPlatformDxe: Add log to show the installed tables

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237

Commit 9fdc70af6ba8 wrongly removed the log from InstallQemuFwCfgTables
after ACPI tables are successfully installed. This patch add the log
back after all operations succeed.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 693cb8c8a83e..f27a95957f47 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -1266,6 +1266,8 @@ InstallQemuFwCfgTables (
     S3Context = NULL;
   }
 
+  DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
+
 UninstallAcpiTables:
   if (EFI_ERROR (Status)) {
     //
-- 
2.29.2.windows.2

[PATCH V2 5/6] OvmfPkg/AcpiPlatformDxe: Refactor QemuAcpiTableNotifyProtocol

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237

Commit 9fdc70af6ba8 install the QemuAcpiTableNotifyProtocol at a
wrong positioin. It should be called before TransferS3ContextToBootScript
because TransferS3ContextToBootScript is the last operation in
InstallQemuFwCfgTables(). Another error is that we should check the
returned value after installing the QemuAcpiTableNotifyProtocol.

This patch refactors the installation and error handling of
QemuAcpiTableNotifyProtocol in InstallQemuFwCfgTables ().

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 36 ++++++++++++++++---------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index f27a95957f47..14ae13055a30 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -1249,6 +1249,20 @@ InstallQemuFwCfgTables (
     }
   }
 
+  //
+  // Install a protocol to notify that the ACPI table provided by Qemu is
+  // ready.
+  //
+  Status = gBS->InstallProtocolInterface (
+                  &QemuAcpiHandle,
+                  &gQemuAcpiTableNotifyProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  NULL
+                  );
+  if (EFI_ERROR (Status)) {
+    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
@@ -1257,7 +1271,7 @@ InstallQemuFwCfgTables (
   if (S3Context != NULL) {
     Status = TransferS3ContextToBootScript (S3Context);
     if (EFI_ERROR (Status)) {
-      goto UninstallAcpiTables;
+      goto UninstallQemuAcpiTableNotifyProtocol;
     }
 
     //
@@ -1268,6 +1282,15 @@ InstallQemuFwCfgTables (
 
   DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
 
+UninstallQemuAcpiTableNotifyProtocol:
+  if (EFI_ERROR (Status)) {
+    gBS->UninstallProtocolInterface (
+           QemuAcpiHandle,
+           &gQemuAcpiTableNotifyProtocolGuid,
+           NULL
+           );
+  }
+
 UninstallAcpiTables:
   if (EFI_ERROR (Status)) {
     //
@@ -1277,17 +1300,6 @@ UninstallAcpiTables:
       --Installed;
       AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
     }
-  } else {
-    //
-    // Install a protocol to notify that the ACPI table provided by Qemu is
-    // ready.
-    //
-    gBS->InstallProtocolInterface (
-           &QemuAcpiHandle,
-           &gQemuAcpiTableNotifyProtocolGuid,
-           EFI_NATIVE_INTERFACE,
-           NULL
-           );
   }
 
   for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);
-- 
2.29.2.windows.2

[PATCH V2 6/6] OvmfPkg/AcpiPlatformDxe: Return error if installing NotifyProtocol failed

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237

Installation of gQemuAcpiTableNotifyProtocol may fail. The error code
should be returned so that the caller can handle it.

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
index d849a59526b5..98e4f02203d8 100644
--- a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
@@ -89,12 +89,16 @@ InstallCloudHvTablesTdx (
   // Install a protocol to notify that the ACPI table provided by CH is
   // ready.
   //
-  gBS->InstallProtocolInterface (
-         &ChAcpiHandle,
-         &gQemuAcpiTableNotifyProtocolGuid,
-         EFI_NATIVE_INTERFACE,
-         NULL
-         );
+  Status = gBS->InstallProtocolInterface (
+                  &ChAcpiHandle,
+                  &gQemuAcpiTableNotifyProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  NULL
+                  );
+  if (EFI_ERROR (Status)) {
+    ASSERT_EFI_ERROR (Status);
+    return Status;
+  }
 
   return EFI_SUCCESS;
 }
-- 
2.29.2.windows.2

Re: [edk2-devel] [PATCH V2 1/6] OvmfPkg/AcpiPlatformDxe: Remove QEMU_ACPI_TABLE_NOTIFY_PROTOCOL

[Laszlo Ersek]

On 1/9/23 14:59, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237
> 
> The QEMU_ACPI_TABLE_NOTIFY_PROTOCOL structure is superfluous because NULL
> protocol interfaces have been used in edk2 repeatedly. A protocol instance
> can exist in the protocol database with a NULL associated interface.
> Therefore the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL type, the
> "QemuAcpiTableNotify.h" header, and the "mAcpiNotifyProtocol" global
> variable can be removed.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c         |  7 ++---
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c       |  6 ++---
>  .../Include/Protocol/QemuAcpiTableNotify.h    | 27 -------------------
>  3 files changed, 4 insertions(+), 36 deletions(-)
>  delete mode 100644 OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> index cbe8bb9b0c75..ad39e4253478 100644
> --- a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> @@ -15,15 +15,12 @@
>  #include <Library/PcdLib.h>                               // PcdGet32()
>  #include <Library/HobLib.h>                               // GetFirstGuidHob(), GetNextGuidHob()
>  #include <Library/UefiBootServicesTableLib.h>             // gBS
> -
>  #include <Protocol/AcpiSystemDescriptionTable.h>
>  #include <Protocol/AcpiTable.h>
> -#include <Protocol/QemuAcpiTableNotify.h>                 // QEMU_ACPI_TABLE_NOTIFY_PROTOCOL
>  
>  #include "AcpiPlatform.h"
>  
> -EFI_HANDLE                       mChAcpiHandle = NULL;
> -QEMU_ACPI_TABLE_NOTIFY_PROTOCOL  mChAcpiNotifyProtocol;
> +EFI_HANDLE  mChAcpiHandle = NULL;
>  
>  EFI_STATUS
>  EFIAPI
> @@ -96,7 +93,7 @@ InstallCloudHvTablesTdx (
>           &mChAcpiHandle,
>           &gQemuAcpiTableNotifyProtocolGuid,
>           EFI_NATIVE_INTERFACE,
> -         &mChAcpiNotifyProtocol
> +         NULL
>           );
>  
>    return EFI_SUCCESS;
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index c8dee17c13e6..1a3852904df9 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -19,10 +19,8 @@
>  #include <Library/QemuFwCfgS3Lib.h>           // QemuFwCfgS3Enabled()
>  #include <Library/UefiBootServicesTableLib.h> // gBS
>  
> -#include <Protocol/QemuAcpiTableNotify.h>
>  #include "AcpiPlatform.h"
> -EFI_HANDLE                       mQemuAcpiHandle = NULL;
> -QEMU_ACPI_TABLE_NOTIFY_PROTOCOL  mAcpiNotifyProtocol;
> +EFI_HANDLE  mQemuAcpiHandle = NULL;
>  
>  //
>  // The user structure for the ordered collection that will track the fw_cfg
> @@ -1284,7 +1282,7 @@ UninstallAcpiTables:
>             &mQemuAcpiHandle,
>             &gQemuAcpiTableNotifyProtocolGuid,
>             EFI_NATIVE_INTERFACE,
> -           &mAcpiNotifyProtocol
> +           NULL
>             );
>    }
>  
> diff --git a/OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h b/OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h
> deleted file mode 100644
> index a3dd2fc1dc91..000000000000
> --- a/OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -/** @file
> -
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#ifndef QEMU_ACPI_TABLE_NOTIFY_H_
> -#define QEMU_ACPI_TABLE_NOTIFY_H_
> -
> -#define QEMU_ACPI_TABLE_NOTIFY_GUID \
> -  { 0x928939b2, 0x4235, 0x462f, { 0x95, 0x80, 0xf6, 0xa2, 0xb2, 0xc2, 0x1a, 0x4f } };
> -
> -///
> -/// Forward declaration
> -///
> -typedef struct _QEMU_ACPI_TABLE_NOTIFY_PROTOCOL QEMU_ACPI_TABLE_NOTIFY_PROTOCOL;
> -
> -///
> -/// Protocol structure
> -///
> -struct _QEMU_ACPI_TABLE_NOTIFY_PROTOCOL {
> -  UINT8    Notify;
> -};
> -
> -extern EFI_GUID  gQemuAcpiTableNotifyProtocolGuid;
> -
> -#endif

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH V2 3/6] OvmfPkg/AcpiPlatformDxe: Use local variable in QemuFwCfgAcpi.c

[Laszlo Ersek]

On 1/9/23 14:59, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237
> 
> The handle of mQemuAcpiHandle is not needed for anything, beyond the
> scope of the InstallQemuFwCfgTables(). So a local variable will
> suffice for storing the handle.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 1a3852904df9..693cb8c8a83e 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -20,7 +20,6 @@
>  #include <Library/UefiBootServicesTableLib.h> // gBS
>  
>  #include "AcpiPlatform.h"
> -EFI_HANDLE  mQemuAcpiHandle = NULL;
>  
>  //
>  // The user structure for the ordered collection that will track the fw_cfg
> @@ -1101,6 +1100,9 @@ InstallQemuFwCfgTables (
>    ORDERED_COLLECTION_ENTRY  *TrackerEntry, *TrackerEntry2;
>    ORDERED_COLLECTION        *SeenPointers;
>    ORDERED_COLLECTION_ENTRY  *SeenPointerEntry, *SeenPointerEntry2;
> +  EFI_HANDLE                QemuAcpiHandle;
> +
> +  QemuAcpiHandle = NULL;
>  
>    Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize);
>    if (EFI_ERROR (Status)) {
> @@ -1279,7 +1281,7 @@ UninstallAcpiTables:
>      // ready.
>      //
>      gBS->InstallProtocolInterface (
> -           &mQemuAcpiHandle,
> +           &QemuAcpiHandle,
>             &gQemuAcpiTableNotifyProtocolGuid,
>             EFI_NATIVE_INTERFACE,
>             NULL

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH V2 4/6] OvmfPkg/AcpiPlatformDxe: Add log to show the installed tables

[Laszlo Ersek]

On 1/9/23 14:59, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237
> 
> Commit 9fdc70af6ba8 wrongly removed the log from InstallQemuFwCfgTables
> after ACPI tables are successfully installed. This patch add the log
> back after all operations succeed.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 693cb8c8a83e..f27a95957f47 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -1266,6 +1266,8 @@ InstallQemuFwCfgTables (
>      S3Context = NULL;
>    }
>  
> +  DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
> +
>  UninstallAcpiTables:
>    if (EFI_ERROR (Status)) {
>      //

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH V2 5/6] OvmfPkg/AcpiPlatformDxe: Refactor QemuAcpiTableNotifyProtocol

[Laszlo Ersek]

Hi Min,

On 1/9/23 14:59, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4237
> 
> Commit 9fdc70af6ba8 install the QemuAcpiTableNotifyProtocol at a
> wrong positioin. It should be called before TransferS3ContextToBootScript
> because TransferS3ContextToBootScript is the last operation in
> InstallQemuFwCfgTables(). Another error is that we should check the
> returned value after installing the QemuAcpiTableNotifyProtocol.
> 
> This patch refactors the installation and error handling of
> QemuAcpiTableNotifyProtocol in InstallQemuFwCfgTables ().
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 36 ++++++++++++++++---------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index f27a95957f47..14ae13055a30 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -1249,6 +1249,20 @@ InstallQemuFwCfgTables (
>      }
>    }
>  
> +  //
> +  // Install a protocol to notify that the ACPI table provided by Qemu is
> +  // ready.
> +  //
> +  Status = gBS->InstallProtocolInterface (
> +                  &QemuAcpiHandle,
> +                  &gQemuAcpiTableNotifyProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  NULL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    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
> @@ -1257,7 +1271,7 @@ InstallQemuFwCfgTables (
>    if (S3Context != NULL) {
>      Status = TransferS3ContextToBootScript (S3Context);
>      if (EFI_ERROR (Status)) {
> -      goto UninstallAcpiTables;
> +      goto UninstallQemuAcpiTableNotifyProtocol;
>      }
>  
>      //
> @@ -1268,6 +1282,15 @@ InstallQemuFwCfgTables (
>  
>    DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>  
> +UninstallQemuAcpiTableNotifyProtocol:
> +  if (EFI_ERROR (Status)) {
> +    gBS->UninstallProtocolInterface (
> +           QemuAcpiHandle,
> +           &gQemuAcpiTableNotifyProtocolGuid,
> +           NULL
> +           );
> +  }
> +
>  UninstallAcpiTables:
>    if (EFI_ERROR (Status)) {
>      //
> @@ -1277,17 +1300,6 @@ UninstallAcpiTables:
>        --Installed;
>        AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
>      }
> -  } else {
> -    //
> -    // Install a protocol to notify that the ACPI table provided by Qemu is
> -    // ready.
> -    //
> -    gBS->InstallProtocolInterface (
> -           &QemuAcpiHandle,
> -           &gQemuAcpiTableNotifyProtocolGuid,
> -           EFI_NATIVE_INTERFACE,
> -           NULL
> -           );
>    }
>  
>    for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);

Previously, I wrote:

> In fact, I request that you *not* set QemuAcpiHandle to NULL at the
> top of the function. Letting *only* InstallProtocolInterface() set it,
> upon success, is safe, as long as you use the pattern shown above.

That was wrong. When I wrote that, I forgot that, for
EFI_BOOT_SERVICES.InstallProtocolInterface(), the "Handle" parameter is
"IN OUT", not just OUT. In other words, we state that we want a new
handle by setting the Handle to NULL on input. My comment above missed
that, I'm sorry.

So, I think this patch is good. I do have small request though. Please
bear with me:

- in patch #3, can you move the "QemuAcpiHandle = NULL" assignment right
above the gBS->InstallProtocolInterface() call?

- and in patch #5 (i.e., here), can you please *keep* the
"QemuAcpiHandle = NULL" assignment together with the
gBS->InstallProtocolInterface() call that is being moved?

Because, you are right that we need to set QemuAcpiHandle to NULL before
we call InstallProtocolInterface(); however, I'd like to prevent the
function from making the impression that we depend on "initializing"
QemuAcpiHandle like that.

Sorry about the churn!

Thanks,
Laszlo

Re: [edk2-devel] [PATCH V2 5/6] OvmfPkg/AcpiPlatformDxe: Refactor QemuAcpiTableNotifyProtocol

[Min Xu]

On January 10, 2023 1:51 PM, Laszlo Ersek wrote:
> 
> So, I think this patch is good. I do have small request though. Please bear with
> me:
> 
> - in patch #3, can you move the "QemuAcpiHandle = NULL" assignment right
> above the gBS->InstallProtocolInterface() call?
> 
> - and in patch #5 (i.e., here), can you please *keep* the "QemuAcpiHandle =
> NULL" assignment together with the
> gBS->InstallProtocolInterface() call that is being moved?
> 
> Because, you are right that we need to set QemuAcpiHandle to NULL before
> we call InstallProtocolInterface(); however, I'd like to prevent the function
> from making the impression that we depend on "initializing"
> QemuAcpiHandle like that.
Sure. It will be updated in the next version.

Thanks
Min