[PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add back log and fix code cohesion

[PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add back log and fix code cohesion

[Min Xu]

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

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.

There are below changes in this patch:
1) Add back debug log
2) Add error checking and handling if InstallProtocolInterface failed.
3) Delete the QemuAcpiTableNotify.h because it is superfluous.
4) Delete the global variable "mQemuAcpiHandle" instead of a local
   variable.
5) Install the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL with a NULL associated
   interface.

Above changes 2/4/5 are applied in both CloudHvAcpi.c and QemuFwCfgAcpi.c.

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         | 25 +++++------
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c       | 42 +++++++++++++------
 .../Include/Protocol/QemuAcpiTableNotify.h    | 27 ------------
 3 files changed, 42 insertions(+), 52 deletions(-)
 delete mode 100644 OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h

diff --git a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
index cbe8bb9b0c75..81f387438a64 100644
--- a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
@@ -18,13 +18,9 @@
 
 #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_STATUS
 EFIAPI
 InstallCloudHvTablesTdx (
@@ -33,13 +29,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);
 
@@ -92,12 +90,15 @@ InstallCloudHvTablesTdx (
   // Install a protocol to notify that the ACPI table provided by CH is
   // ready.
   //
-  gBS->InstallProtocolInterface (
-         &mChAcpiHandle,
-         &gQemuAcpiTableNotifyProtocolGuid,
-         EFI_NATIVE_INTERFACE,
-         &mChAcpiNotifyProtocol
-         );
+  Status = gBS->InstallProtocolInterface (
+                  &ChAcpiHandle,
+                  &gQemuAcpiTableNotifyProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  NULL
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
 
   return EFI_SUCCESS;
 }
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index c8dee17c13e6..22340b13e3ee 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -19,10 +19,7 @@
 #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;
 
 //
 // The user structure for the ordered collection that will track the fw_cfg
@@ -1103,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)) {
@@ -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
@@ -1275,17 +1289,19 @@ UninstallAcpiTables:
       --Installed;
       AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
     }
+
+    //
+    // Uninstall the notification if it has been installed.
+    //
+    if (QemuAcpiHandle != NULL) {
+      gBS->UninstallProtocolInterface (
+             QemuAcpiHandle,
+             &gQemuAcpiTableNotifyProtocolGuid,
+             NULL
+             );
+    }
   } else {
-    //
-    // Install a protocol to notify that the ACPI table provided by Qemu is
-    // ready.
-    //
-    gBS->InstallProtocolInterface (
-           &mQemuAcpiHandle,
-           &gQemuAcpiTableNotifyProtocolGuid,
-           EFI_NATIVE_INTERFACE,
-           &mAcpiNotifyProtocol
-           );
+    DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
   }
 
   for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);
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

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add back log and fix code cohesion

[Laszlo Ersek]

On 1/4/23 15:13, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
>
> 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.
>
> There are below changes in this patch:
> 1) Add back debug log
> 2) Add error checking and handling if InstallProtocolInterface failed.
> 3) Delete the QemuAcpiTableNotify.h because it is superfluous.
> 4) Delete the global variable "mQemuAcpiHandle" instead of a local
>    variable.
> 5) Install the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL with a NULL associated
>    interface.
>
> Above changes 2/4/5 are applied in both CloudHvAcpi.c and QemuFwCfgAcpi.c.
>
> 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         | 25 +++++------
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c       | 42 +++++++++++++------
>  .../Include/Protocol/QemuAcpiTableNotify.h    | 27 ------------
>  3 files changed, 42 insertions(+), 52 deletions(-)
>  delete mode 100644 OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h

First of all, I wouldn't like to make any comments on the
"CloudHvAcpi.c" source file. I wouldn't like my Reviewed-by to cover any
non-trivial change made to "CloudHvAcpi.c".

Therefore, I suggest that this patch be split up to a series of small
patches.

The first patch should, in my opinion, just remove the
QEMU_ACPI_TABLE_NOTIFY_PROTOCOL structure -- that is, "step 5". This
should be a trivial change to both QemuFwCfgAcpi.c and CloudHvAcpi.c,
and I'm comfortable reviewing that for both of these source files. This
patch can also cover "step 3", the removal of "QemuAcpiTableNotify.h".

Patches #2 and #3 should cover "step 4", *split* to separate patches
between CloudHvAcpi.c and QemuFwCfgAcpi.c. I would only like to review
the QemuFwCfgAcpi.c change, from these.

So then we are left with steps 1 and 2.

Patch #4 should just cover step 1, add back the debug log. This only
affects QemuFwCfgAcpi.c, which I'd like to review.

For step 2, patches #5 and #6 should add the missing error handling, and
move the protocol installation to the right spot, and extend the error
path accordingly. This must *definitely* be seprate for QemuFwCfgAcpi.c
and CloudHvAcpi.c, hence my request for splitting step 2 to two patches
(#5, #6). I won't look at the CloudHvAcpi.c, but will scrutinize the
heck out of QemuFwCfgAcpi.c.

--*--

I can already say that your current change for step 2 in
"QemuFwCfgAcpi.c" is wrong, or at least incomplete.

Please consider the following pattern:

It is frequently the case that (a) a function needs to create a set of
*permanent* resources (objects that are effectively the function's
"output"), which are supposed to be published if the functon entirely
succeeds, while at the same time (b) the function needs some *temporary*
resources for the creation of the permanent resource set. The
"permanent" resources must be preserved (= output to the caller, or
"committed" to the system as a side effect) if and only if all steps
succeed, whereas the "temporary" resources must be torn down (or rolled
back) *unconditionally*.

In InstallQemuFwCfgTables(), we have the following *temporary*
resources:

(1.1) LoaderStart -- the QEMU linker/loader script downloaded from
      fw_cfg

(1.2) AllocationsRestrictedTo32Bit -- a dictionary of fw_cfg blobs that
      must not be allocated from 64-bit address space

(1.3) Tracker -- a dictionary collecting the fw_cfg blobs that we have
      thus far downloaded from QEMU, as instructed by the linker/loader
      script

(1.4) InstalledKey -- an array collecting the keys of the ACPI tables
      we've thus far installed with the ACPI Table Protocol.

(1.5) SeenPointers -- a dictionary for one of the command types in the
      linker/loader script, ensuring that we don't attempt to install
      referenced tables more than once

And we have the following *permanent* resources (committed to the system
as a side-effect)

(2.1) S3Context -- effectively an S3 Boot Script representation. Some
      operations in the ACPI linker/loader script of QEMU -- namely, the
      "write pointer" commands -- convey information from the guest
      *back* to QEMU, over fw_cfg, and these must be replayed during S3
      resume. The system reset that occurs between suspend and resume
      clears out a bunch of information from QEMU, and these operations
      re-teach a set of information to QEMU.

(2.2) the *current runtime effect* of the same "write pointer" commands
      as discussed above. S3Context is for S3 resume, but the same
      commands must take primarily *right now*.

(2.3) the set of ACPI tables we've installed

Now, if you look at InstallQemuFwCfgTables() *before* your original
change (9fdc70af6ba8), you will see that:

(a) the publication of S3Context (permanent resource 2.1) as an S3 Boot
    Script is the *final* step in the function. Due to various reasons,
    rolling back this step is not possible. Therefore no action that
    could fail must ever be performed in the function after this action.

(b) The error path is constructed with *multiple labels* in such a way
    that the order of rollback operations is the *reverse order* of
    constructions, intermixing both temporary and permanent resources.
    When a constructive operation fails, a jump is taken such that the
    rollback steps for all constructive operations *not yet reached* are
    skipped, and the rollbacks for the constructions that *have*
    succeeded are all performed. We can call this a "cascade" of error
    handling labels.

(c) Note that in some (infrequent!) cases, *multiple* resources can be
    released under the same error handling label, but that is generally
    the exception. It is only possible if the nature of some
    construction operations is such that they cannot fail separately
    (but still need tear-down), or some of the resources are data sets
    which can be genuinely empty, and whose tear-down needs no
    separation between empty vs. non-empty.

    You can see this with the uninstallation of the ACPI tables we've
    installed (permanent resource 2.3), which is grouped together under
    the same "UninstallAcpiTables" label with the releasing of the
    "SeenPointers" dictionary (temporary resource 1.5).

    There is no grand theory behind this; it simply mirrors the
    construction path:

>   SeenPointers = OrderedCollectionInit (PointerCompare, PointerCompare);
>   if (SeenPointers == NULL) {
>     Status = EFI_OUT_OF_RESOURCES;
>     goto FreeKeys;
>   }
>
>   //
>   // second pass: identify and install ACPI tables
>   //
>   Installed = 0;
>   for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>     if (LoaderEntry->Type == QemuLoaderCmdAddPointer) {
>       Status = Process2ndPassCmdAddPointer (
>                  &LoaderEntry->Command.AddPointer,
>                  Tracker,
>                  AcpiProtocol,
>                  InstalledKey,
>                  &Installed,
>                  SeenPointers
>                  );
>       if (EFI_ERROR (Status)) {
>         goto UninstallAcpiTables;
>       }
>     }
>   }

    If "SeenPointers" (a dictionary) is initialized successfully, but
    *any* ACPI table installation fails in the loop later, then we must
    tear down "SeenPointers" (regardless of how many entries it
    contains) *and* uninstall all previously installed ACPI tables as
    well (using the keys we've put into InstalledKey -- their number is
    given by Installed).

(d) Very importantly, because we have both permanent and temporary
    resources, we must fall through the error path at the end of the
    function even if the function as a whole succeeds. Therefore the
    tear-down steps on the error path for the *permanent* resources are
    all *restricted* to "Status" differing from EFI_SUCCESS. In other
    words, we go through the entire cascade in this ("grand success")
    case, but omit those rollback steps that would affect our permanent
    resources, which are now all done. Only the temporaries are torn
    down.

    At the very end, we have "return Status", which is consistent with
    the following two requirements (which the function does satisfy):

    - any time we perform a "goto" to an error handling label, Status
      must be set to an error value, either manually, or by the
      operation that just failed (and because of which we're taking the
      goto)

    - by the time we reach the first error handling label via *normal
      progression* (no goto), "Status" must be set to EFI_SUCCESS. This
      is achievable in two ways: one, set Status to EFI_SUCCESS
      explicitly, right there; or two: prove that, by that point, Status
      will have been set to EFI_SUCCESS. In this function, the latter is
      the case.

--*--

Okay. After this brief introduction, consider what you're doing with the
protocol installation:

- the protocol in the protocol database is a new permanent resource (a
  system-wide side-effect)

- installing the protocol is an operation that can fail

- protocol installation *can* be rolled back (I discussed the TPL
  concerns around this in the Bugzilla, but I'm not going to repeat them
  here). This fact -- i.e. that the protocol installation *can* be
  rolled back -- is really nice BTW, because we already have one
  operation in the function that *cannot*. We can't have *two* final
  steps in the function, so it's just as well that the protocol
  installation *need not* be the last one.

So what you need to do in patch#5 or patch#6 is this (diff shown with 6
lines of context):

> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 38584fd608df..0c2b2215f316 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -1246,50 +1246,63 @@ InstallQemuFwCfgTables (
>        if (EFI_ERROR (Status)) {
>          goto UninstallAcpiTables;
>        }
>      }
>    }
>
> +  //
> +  // 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
>    // if it succeeds, it cannot be undone.
>    //
>    if (S3Context != NULL) {
>      Status = TransferS3ContextToBootScript (S3Context);
>      if (EFI_ERROR (Status)) {
> -      goto UninstallAcpiTables;
> +      goto UninstallQemuAcpiTableNotifyProtocol;
>      }
>
>      //
>      // Ownership of S3Context has been transferred.
>      //
>      S3Context = NULL;
>    }
>
> +UninstallQemuAcpiTableNotifyProtocol:
> +  if (EFI_ERROR (Status)) {
> +    gBS->UninstallProtocolInterface (
> +           QemuAcpiHandle,
> +           &gQemuAcpiTableNotifyProtocolGuid,
> +           NULL
> +           );
> +  }
> +
>  UninstallAcpiTables:
>    if (EFI_ERROR (Status)) {
>      //
>      // roll back partial installation
>      //
>      while (Installed > 0) {
>        --Installed;
>        AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
>      }
>    } else {
>      DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
> -    //
> -    // Install a protocol to notify that the ACPI table provided by Qemu is
> -    // ready.
> -    //
> -    gBS->InstallProtocolInterface (
> -           &mQemuAcpiHandle,
> -           &gQemuAcpiTableNotifyProtocolGuid,
> -           EFI_NATIVE_INTERFACE,
> -           &mAcpiNotifyProtocol
> -           );
>    }
>
>    for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);
>         SeenPointerEntry != NULL;
>         SeenPointerEntry = SeenPointerEntry2)
>    {

Your proposed solution on the error path -- placing the uninstallation
under the existent "UninstallAcpiTables" label, and gating it with
(QemuAcpiHandle != NULL) -- may not be wrong, functionally speaking, but
it still breaks the cohesion of the function.

Specifically, the cohesion-break is that your proposal creates a path
through the code where the (QemuAcpiHandle != NULL) condition is
evaluated, on the error path, *without control ever having reached*
InstallProtocolInterface() previously. Namely, if one of the
Process2ndPassCmdAddPointer() calls fails, in the loop above
InstallProtocolInterface(). Saving that code path is why you need to set
"QemuAcpiHandle" to NULL at the top of the function.

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.

--*--

Okay, yet another topic. Before you ask, let me comment on this (from
preexistent code):

> FreeS3Context:
>   if (S3Context != NULL) {
>     ReleaseS3Context (S3Context);
>   }

This is being gated so because S3Context undergoes an ownership transfer
-- in that last, impossible-to-rollback step on the construction path
--, and because S3Context is *optional* as well. So at this point
S3Context is non-NULL precisely if we've had a failure *AND* S3Context
was *needed* in the first place. Just checking EFI_ERROR (Status) is
insufficient, because even on error, S3Context could still be NULL (it's
optional), and once we check for nullity, checking Status becomes
superfluous. (S3Context!=NULL at this point implies EFI_ERROR(Status),
per above.)

--*--

Finally, I *will* admit that the original placement of the DEBUG message
is not ideal. It should not be on an "else" branch under the
"UninstallAcpiTables" label. Instead, it should be, and *stay*, just
above the very first error handling label, where we effectively declare
the function "completed". So the original incorrect placement of the
DEBUG is my mistake, from earlier -- I'm sorry.

Thus, in patch#4, please do reinstate the DEBUG just above the
"UninstallAcpiTables" label. And in patch #5 or #6, keep it above the
new label "UninstallQemuAcpiTableNotifyProtocol".

Thanks
Laszlo

>
> diff --git a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> index cbe8bb9b0c75..81f387438a64 100644
> --- a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> @@ -18,13 +18,9 @@
>
>  #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_STATUS
>  EFIAPI
>  InstallCloudHvTablesTdx (
> @@ -33,13 +29,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);
>
> @@ -92,12 +90,15 @@ InstallCloudHvTablesTdx (
>    // Install a protocol to notify that the ACPI table provided by CH is
>    // ready.
>    //
> -  gBS->InstallProtocolInterface (
> -         &mChAcpiHandle,
> -         &gQemuAcpiTableNotifyProtocolGuid,
> -         EFI_NATIVE_INTERFACE,
> -         &mChAcpiNotifyProtocol
> -         );
> +  Status = gBS->InstallProtocolInterface (
> +                  &ChAcpiHandle,
> +                  &gQemuAcpiTableNotifyProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  NULL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
>
>    return EFI_SUCCESS;
>  }
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index c8dee17c13e6..22340b13e3ee 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -19,10 +19,7 @@
>  #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;
>
>  //
>  // The user structure for the ordered collection that will track the fw_cfg
> @@ -1103,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)) {
> @@ -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
> @@ -1275,17 +1289,19 @@ UninstallAcpiTables:
>        --Installed;
>        AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
>      }
> +
> +    //
> +    // Uninstall the notification if it has been installed.
> +    //
> +    if (QemuAcpiHandle != NULL) {
> +      gBS->UninstallProtocolInterface (
> +             QemuAcpiHandle,
> +             &gQemuAcpiTableNotifyProtocolGuid,
> +             NULL
> +             );
> +    }
>    } else {
> -    //
> -    // Install a protocol to notify that the ACPI table provided by Qemu is
> -    // ready.
> -    //
> -    gBS->InstallProtocolInterface (
> -           &mQemuAcpiHandle,
> -           &gQemuAcpiTableNotifyProtocolGuid,
> -           EFI_NATIVE_INTERFACE,
> -           &mAcpiNotifyProtocol
> -           );
> +    DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>    }
>
>    for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);
> 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

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add back log and fix code cohesion

[Gerd Hoffmann]

On Wed, Jan 04, 2023 at 10:13:09PM +0800, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> 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.
> 
> There are below changes in this patch:
> 1) Add back debug log
> 2) Add error checking and handling if InstallProtocolInterface failed.
> 3) Delete the QemuAcpiTableNotify.h because it is superfluous.
> 4) Delete the global variable "mQemuAcpiHandle" instead of a local
>    variable.
> 5) Install the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL with a NULL associated
>    interface.

Five changes in a single patch.  Can this be splitted up please?

thanks,
  Gerd

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add back log and fix code cohesion

[Laszlo Ersek]

On 1/5/23 14:15, Gerd Hoffmann wrote:
> On Wed, Jan 04, 2023 at 10:13:09PM +0800, Min Xu wrote:
>> From: Min M Xu <min.m.xu@intel.com>
>>
>> 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.
>>
>> There are below changes in this patch:
>> 1) Add back debug log
>> 2) Add error checking and handling if InstallProtocolInterface failed.
>> 3) Delete the QemuAcpiTableNotify.h because it is superfluous.
>> 4) Delete the global variable "mQemuAcpiHandle" instead of a local
>>    variable.
>> 5) Install the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL with a NULL associated
>>    interface.
> 
> Five changes in a single patch.  Can this be splitted up please?

Right, I've requested 6 patches :)
Laszlo

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add back log and fix code cohesion

[Min Xu]

Laszlo & Gerd
Yes, I will provide a new patch-set with 6 patches. Thanks for your comments.

Min

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 5, 2023 11:11 PM
> To: Gerd Hoffmann <kraxel@redhat.com>; devel@edk2.groups.io; Xu, Min
> M <min.m.xu@intel.com>
> Cc: Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Boeuf, Sebastien
> <sebastien.boeuf@intel.com>
> Subject: Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add
> back log and fix code cohesion
> 
> On 1/5/23 14:15, Gerd Hoffmann wrote:
> > On Wed, Jan 04, 2023 at 10:13:09PM +0800, Min Xu wrote:
> >> From: Min M Xu <min.m.xu@intel.com>
> >>
> >> 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.
> >>
> >> There are below changes in this patch:
> >> 1) Add back debug log
> >> 2) Add error checking and handling if InstallProtocolInterface failed.
> >> 3) Delete the QemuAcpiTableNotify.h because it is superfluous.
> >> 4) Delete the global variable "mQemuAcpiHandle" instead of a local
> >>    variable.
> >> 5) Install the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL with a NULL
> associated
> >>    interface.
> >
> > Five changes in a single patch.  Can this be splitted up please?
> 
> Right, I've requested 6 patches :)
> Laszlo

Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add back log and fix code cohesion

[Min Xu]

Hi, Laszlo
I do appreciate the detailed and accurate explanation of how AcpiPlatformDxe downloads ACPI tables from QEMU and then install them. I will submit a new patch-set soon.

Thanks
Min

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 5, 2023 5:09 PM
> To: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>
> Cc: Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Boeuf, Sebastien
> <sebastien.boeuf@intel.com>; Michael Brown <mcb30@ipxe.org>; Marvin
> Häuser <mhaeuser@posteo.de>
> Subject: Re: [edk2-devel] [PATCH V1 1/1] OvmfPkg/AcpiPlatformDxe: Add
> back log and fix code cohesion
> 
> On 1/4/23 15:13, Min Xu wrote:
> > From: Min M Xu <min.m.xu@intel.com>
> >
> > 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.
> >
> > There are below changes in this patch:
> > 1) Add back debug log
> > 2) Add error checking and handling if InstallProtocolInterface failed.
> > 3) Delete the QemuAcpiTableNotify.h because it is superfluous.
> > 4) Delete the global variable "mQemuAcpiHandle" instead of a local
> >    variable.
> > 5) Install the QEMU_ACPI_TABLE_NOTIFY_PROTOCOL with a NULL
> associated
> >    interface.
> >
> > Above changes 2/4/5 are applied in both CloudHvAcpi.c and
> QemuFwCfgAcpi.c.
> >
> > 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         | 25 +++++------
> >  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c       | 42 +++++++++++++-----
> -
> >  .../Include/Protocol/QemuAcpiTableNotify.h    | 27 ------------
> >  3 files changed, 42 insertions(+), 52 deletions(-)  delete mode
> > 100644 OvmfPkg/Include/Protocol/QemuAcpiTableNotify.h
> 
> First of all, I wouldn't like to make any comments on the "CloudHvAcpi.c"
> source file. I wouldn't like my Reviewed-by to cover any non-trivial change
> made to "CloudHvAcpi.c".
> 
> Therefore, I suggest that this patch be split up to a series of small patches.
> 
> The first patch should, in my opinion, just remove the
> QEMU_ACPI_TABLE_NOTIFY_PROTOCOL structure -- that is, "step 5". This
> should be a trivial change to both QemuFwCfgAcpi.c and CloudHvAcpi.c, and
> I'm comfortable reviewing that for both of these source files. This patch can
> also cover "step 3", the removal of "QemuAcpiTableNotify.h".
> 
> Patches #2 and #3 should cover "step 4", *split* to separate patches between
> CloudHvAcpi.c and QemuFwCfgAcpi.c. I would only like to review the
> QemuFwCfgAcpi.c change, from these.
> 
> So then we are left with steps 1 and 2.
> 
> Patch #4 should just cover step 1, add back the debug log. This only affects
> QemuFwCfgAcpi.c, which I'd like to review.
> 
> For step 2, patches #5 and #6 should add the missing error handling, and
> move the protocol installation to the right spot, and extend the error path
> accordingly. This must *definitely* be seprate for QemuFwCfgAcpi.c and
> CloudHvAcpi.c, hence my request for splitting step 2 to two patches (#5, #6). I
> won't look at the CloudHvAcpi.c, but will scrutinize the heck out of
> QemuFwCfgAcpi.c.
> 
> --*--
> 
> I can already say that your current change for step 2 in "QemuFwCfgAcpi.c" is
> wrong, or at least incomplete.
> 
> Please consider the following pattern:
> 
> It is frequently the case that (a) a function needs to create a set of
> *permanent* resources (objects that are effectively the function's "output"),
> which are supposed to be published if the functon entirely succeeds, while at
> the same time (b) the function needs some *temporary* resources for the
> creation of the permanent resource set. The "permanent" resources must be
> preserved (= output to the caller, or "committed" to the system as a side
> effect) if and only if all steps succeed, whereas the "temporary" resources
> must be torn down (or rolled
> back) *unconditionally*.
> 
> In InstallQemuFwCfgTables(), we have the following *temporary*
> resources:
> 
> (1.1) LoaderStart -- the QEMU linker/loader script downloaded from
>       fw_cfg
> 
> (1.2) AllocationsRestrictedTo32Bit -- a dictionary of fw_cfg blobs that
>       must not be allocated from 64-bit address space
> 
> (1.3) Tracker -- a dictionary collecting the fw_cfg blobs that we have
>       thus far downloaded from QEMU, as instructed by the linker/loader
>       script
> 
> (1.4) InstalledKey -- an array collecting the keys of the ACPI tables
>       we've thus far installed with the ACPI Table Protocol.
> 
> (1.5) SeenPointers -- a dictionary for one of the command types in the
>       linker/loader script, ensuring that we don't attempt to install
>       referenced tables more than once
> 
> And we have the following *permanent* resources (committed to the system
> as a side-effect)
> 
> (2.1) S3Context -- effectively an S3 Boot Script representation. Some
>       operations in the ACPI linker/loader script of QEMU -- namely, the
>       "write pointer" commands -- convey information from the guest
>       *back* to QEMU, over fw_cfg, and these must be replayed during S3
>       resume. The system reset that occurs between suspend and resume
>       clears out a bunch of information from QEMU, and these operations
>       re-teach a set of information to QEMU.
> 
> (2.2) the *current runtime effect* of the same "write pointer" commands
>       as discussed above. S3Context is for S3 resume, but the same
>       commands must take primarily *right now*.
> 
> (2.3) the set of ACPI tables we've installed
> 
> Now, if you look at InstallQemuFwCfgTables() *before* your original change
> (9fdc70af6ba8), you will see that:
> 
> (a) the publication of S3Context (permanent resource 2.1) as an S3 Boot
>     Script is the *final* step in the function. Due to various reasons,
>     rolling back this step is not possible. Therefore no action that
>     could fail must ever be performed in the function after this action.
> 
> (b) The error path is constructed with *multiple labels* in such a way
>     that the order of rollback operations is the *reverse order* of
>     constructions, intermixing both temporary and permanent resources.
>     When a constructive operation fails, a jump is taken such that the
>     rollback steps for all constructive operations *not yet reached* are
>     skipped, and the rollbacks for the constructions that *have*
>     succeeded are all performed. We can call this a "cascade" of error
>     handling labels.
> 
> (c) Note that in some (infrequent!) cases, *multiple* resources can be
>     released under the same error handling label, but that is generally
>     the exception. It is only possible if the nature of some
>     construction operations is such that they cannot fail separately
>     (but still need tear-down), or some of the resources are data sets
>     which can be genuinely empty, and whose tear-down needs no
>     separation between empty vs. non-empty.
> 
>     You can see this with the uninstallation of the ACPI tables we've
>     installed (permanent resource 2.3), which is grouped together under
>     the same "UninstallAcpiTables" label with the releasing of the
>     "SeenPointers" dictionary (temporary resource 1.5).
> 
>     There is no grand theory behind this; it simply mirrors the
>     construction path:
> 
> >   SeenPointers = OrderedCollectionInit (PointerCompare, PointerCompare);
> >   if (SeenPointers == NULL) {
> >     Status = EFI_OUT_OF_RESOURCES;
> >     goto FreeKeys;
> >   }
> >
> >   //
> >   // second pass: identify and install ACPI tables
> >   //
> >   Installed = 0;
> >   for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry)
> {
> >     if (LoaderEntry->Type == QemuLoaderCmdAddPointer) {
> >       Status = Process2ndPassCmdAddPointer (
> >                  &LoaderEntry->Command.AddPointer,
> >                  Tracker,
> >                  AcpiProtocol,
> >                  InstalledKey,
> >                  &Installed,
> >                  SeenPointers
> >                  );
> >       if (EFI_ERROR (Status)) {
> >         goto UninstallAcpiTables;
> >       }
> >     }
> >   }
> 
>     If "SeenPointers" (a dictionary) is initialized successfully, but
>     *any* ACPI table installation fails in the loop later, then we must
>     tear down "SeenPointers" (regardless of how many entries it
>     contains) *and* uninstall all previously installed ACPI tables as
>     well (using the keys we've put into InstalledKey -- their number is
>     given by Installed).
> 
> (d) Very importantly, because we have both permanent and temporary
>     resources, we must fall through the error path at the end of the
>     function even if the function as a whole succeeds. Therefore the
>     tear-down steps on the error path for the *permanent* resources are
>     all *restricted* to "Status" differing from EFI_SUCCESS. In other
>     words, we go through the entire cascade in this ("grand success")
>     case, but omit those rollback steps that would affect our permanent
>     resources, which are now all done. Only the temporaries are torn
>     down.
> 
>     At the very end, we have "return Status", which is consistent with
>     the following two requirements (which the function does satisfy):
> 
>     - any time we perform a "goto" to an error handling label, Status
>       must be set to an error value, either manually, or by the
>       operation that just failed (and because of which we're taking the
>       goto)
> 
>     - by the time we reach the first error handling label via *normal
>       progression* (no goto), "Status" must be set to EFI_SUCCESS. This
>       is achievable in two ways: one, set Status to EFI_SUCCESS
>       explicitly, right there; or two: prove that, by that point, Status
>       will have been set to EFI_SUCCESS. In this function, the latter is
>       the case.
> 
> --*--
> 
> Okay. After this brief introduction, consider what you're doing with the
> protocol installation:
> 
> - the protocol in the protocol database is a new permanent resource (a
>   system-wide side-effect)
> 
> - installing the protocol is an operation that can fail
> 
> - protocol installation *can* be rolled back (I discussed the TPL
>   concerns around this in the Bugzilla, but I'm not going to repeat them
>   here). This fact -- i.e. that the protocol installation *can* be
>   rolled back -- is really nice BTW, because we already have one
>   operation in the function that *cannot*. We can't have *two* final
>   steps in the function, so it's just as well that the protocol
>   installation *need not* be the last one.
> 
> So what you need to do in patch#5 or patch#6 is this (diff shown with 6 lines
> of context):
> 
> > diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > index 38584fd608df..0c2b2215f316 100644
> > --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > @@ -1246,50 +1246,63 @@ InstallQemuFwCfgTables (
> >        if (EFI_ERROR (Status)) {
> >          goto UninstallAcpiTables;
> >        }
> >      }
> >    }
> >
> > +  //
> > +  // 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
> >    // if it succeeds, it cannot be undone.
> >    //
> >    if (S3Context != NULL) {
> >      Status = TransferS3ContextToBootScript (S3Context);
> >      if (EFI_ERROR (Status)) {
> > -      goto UninstallAcpiTables;
> > +      goto UninstallQemuAcpiTableNotifyProtocol;
> >      }
> >
> >      //
> >      // Ownership of S3Context has been transferred.
> >      //
> >      S3Context = NULL;
> >    }
> >
> > +UninstallQemuAcpiTableNotifyProtocol:
> > +  if (EFI_ERROR (Status)) {
> > +    gBS->UninstallProtocolInterface (
> > +           QemuAcpiHandle,
> > +           &gQemuAcpiTableNotifyProtocolGuid,
> > +           NULL
> > +           );
> > +  }
> > +
> >  UninstallAcpiTables:
> >    if (EFI_ERROR (Status)) {
> >      //
> >      // roll back partial installation
> >      //
> >      while (Installed > 0) {
> >        --Installed;
> >        AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
> >      }
> >    } else {
> >      DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__,
> Installed));
> > -    //
> > -    // Install a protocol to notify that the ACPI table provided by Qemu is
> > -    // ready.
> > -    //
> > -    gBS->InstallProtocolInterface (
> > -           &mQemuAcpiHandle,
> > -           &gQemuAcpiTableNotifyProtocolGuid,
> > -           EFI_NATIVE_INTERFACE,
> > -           &mAcpiNotifyProtocol
> > -           );
> >    }
> >
> >    for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);
> >         SeenPointerEntry != NULL;
> >         SeenPointerEntry = SeenPointerEntry2)
> >    {
> 
> Your proposed solution on the error path -- placing the uninstallation under
> the existent "UninstallAcpiTables" label, and gating it with
> (QemuAcpiHandle != NULL) -- may not be wrong, functionally speaking, but it
> still breaks the cohesion of the function.
> 
> Specifically, the cohesion-break is that your proposal creates a path through
> the code where the (QemuAcpiHandle != NULL) condition is evaluated, on
> the error path, *without control ever having reached*
> InstallProtocolInterface() previously. Namely, if one of the
> Process2ndPassCmdAddPointer() calls fails, in the loop above
> InstallProtocolInterface(). Saving that code path is why you need to set
> "QemuAcpiHandle" to NULL at the top of the function.
> 
> 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.
> 
> --*--
> 
> Okay, yet another topic. Before you ask, let me comment on this (from
> preexistent code):
> 
> > FreeS3Context:
> >   if (S3Context != NULL) {
> >     ReleaseS3Context (S3Context);
> >   }
> 
> This is being gated so because S3Context undergoes an ownership transfer
> -- in that last, impossible-to-rollback step on the construction path --, and
> because S3Context is *optional* as well. So at this point S3Context is non-
> NULL precisely if we've had a failure *AND* S3Context was *needed* in the
> first place. Just checking EFI_ERROR (Status) is insufficient, because even on
> error, S3Context could still be NULL (it's optional), and once we check for
> nullity, checking Status becomes superfluous. (S3Context!=NULL at this point
> implies EFI_ERROR(Status), per above.)
> 
> --*--
> 
> Finally, I *will* admit that the original placement of the DEBUG message is not
> ideal. It should not be on an "else" branch under the "UninstallAcpiTables"
> label. Instead, it should be, and *stay*, just above the very first error
> handling label, where we effectively declare the function "completed". So the
> original incorrect placement of the DEBUG is my mistake, from earlier -- I'm
> sorry.
> 
> Thus, in patch#4, please do reinstate the DEBUG just above the
> "UninstallAcpiTables" label. And in patch #5 or #6, keep it above the new
> label "UninstallQemuAcpiTableNotifyProtocol".
> 
> Thanks
> Laszlo
> 
> >
> > diff --git a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> > b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> > index cbe8bb9b0c75..81f387438a64 100644
> > --- a/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> > +++ b/OvmfPkg/AcpiPlatformDxe/CloudHvAcpi.c
> > @@ -18,13 +18,9 @@
> >
> >  #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_STATUS
> >  EFIAPI
> >  InstallCloudHvTablesTdx (
> > @@ -33,13 +29,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);
> >
> > @@ -92,12 +90,15 @@ InstallCloudHvTablesTdx (
> >    // Install a protocol to notify that the ACPI table provided by CH is
> >    // ready.
> >    //
> > -  gBS->InstallProtocolInterface (
> > -         &mChAcpiHandle,
> > -         &gQemuAcpiTableNotifyProtocolGuid,
> > -         EFI_NATIVE_INTERFACE,
> > -         &mChAcpiNotifyProtocol
> > -         );
> > +  Status = gBS->InstallProtocolInterface (
> > +                  &ChAcpiHandle,
> > +                  &gQemuAcpiTableNotifyProtocolGuid,
> > +                  EFI_NATIVE_INTERFACE,
> > +                  NULL
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> >
> >    return EFI_SUCCESS;
> >  }
> > diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > index c8dee17c13e6..22340b13e3ee 100644
> > --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > @@ -19,10 +19,7 @@
> >  #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;
> >
> >  //
> >  // The user structure for the ordered collection that will track the fw_cfg
> > @@ -1103,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)) {
> > @@ -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
> > @@ -1275,17 +1289,19 @@ UninstallAcpiTables:
> >        --Installed;
> >        AcpiProtocol->UninstallAcpiTable (AcpiProtocol, InstalledKey[Installed]);
> >      }
> > +
> > +    //
> > +    // Uninstall the notification if it has been installed.
> > +    //
> > +    if (QemuAcpiHandle != NULL) {
> > +      gBS->UninstallProtocolInterface (
> > +             QemuAcpiHandle,
> > +             &gQemuAcpiTableNotifyProtocolGuid,
> > +             NULL
> > +             );
> > +    }
> >    } else {
> > -    //
> > -    // Install a protocol to notify that the ACPI table provided by Qemu is
> > -    // ready.
> > -    //
> > -    gBS->InstallProtocolInterface (
> > -           &mQemuAcpiHandle,
> > -           &gQemuAcpiTableNotifyProtocolGuid,
> > -           EFI_NATIVE_INTERFACE,
> > -           &mAcpiNotifyProtocol
> > -           );
> > +    DEBUG ((DEBUG_INFO, "%a: installed %d tables\n", __FUNCTION__,
> Installed));
> >    }
> >
> >    for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);
> > 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