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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98187): https://edk2.groups.io/g/devel/message/98187
Mute This Topic: https://groups.io/mt/96152533/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98234): https://edk2.groups.io/g/devel/message/98234
Mute This Topic: https://groups.io/mt/96152533/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98235): https://edk2.groups.io/g/devel/message/98235
Mute This Topic: https://groups.io/mt/96152533/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-