[edk2-devel] [RFC PATCH] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

Gary Lin via groups.io posted 1 patch 2 years, 8 months ago
Failed in applying to current master (apply log)
OvmfPkg/XenPlatformPei/Platform.c         | 10 ++++++++++
OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  3 +++
2 files changed, 13 insertions(+)
[edk2-devel] [RFC PATCH] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
Posted by Gary Lin via groups.io 2 years, 8 months ago
There are several functions in OvmfPkg/Library using
QemuFwCfgS3Enabled() to detect the S3 support status. However, in
MdeModulePkg, PcdAcpiS3Enable is used to check S3 support. Since
InitializeXenPlatform() didn't set PcdAcpiS3Enable as
InitializePlatform() did, this made the inconsistency between
drivers/functions.

For example, S3SaveStateDxe checked PcdAcpiS3Enable and skipped
S3BootScript because the default value is FALSE. On the other hand,
PlatformBootManagerBeforeConsole() from OvmfPkg/Library called
QemuFwCfgS3Enabled() and found it returned TRUE, so it invoked
SaveS3BootScript(). However, S3SaveStateDxe skipped S3BootScript, so
SaveS3BootScript() asserted due to EFI_NOT_FOUND.

Setting PcdAcpiS3Enable at InitializeXenPlatform() "fixes" the crash
reported by my colleague. The other possible direction is to replace
QemuFwCfgS3Enabled() with PcdAcpiS3Enable. I'm not sure which one is
the right fix.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Gary Lin <glin@suse.com>
---
 OvmfPkg/XenPlatformPei/Platform.c         | 10 ++++++++++
 OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
index a811e72ee301..f7edc979486e 100644
--- a/OvmfPkg/XenPlatformPei/Platform.c
+++ b/OvmfPkg/XenPlatformPei/Platform.c
@@ -26,6 +26,8 @@
 #include <Library/PciLib.h>

 #include <Library/PeimEntryPoint.h>

 #include <Library/PeiServicesLib.h>

+#include <Library/QemuFwCfgLib.h>

+#include <Library/QemuFwCfgS3Lib.h>

 #include <Library/ResourcePublicationLib.h>

 #include <Guid/MemoryTypeInformation.h>

 #include <Ppi/MasterBootMode.h>

@@ -423,6 +425,8 @@ InitializeXenPlatform (
   IN CONST EFI_PEI_SERVICES     **PeiServices

   )

 {

+  EFI_STATUS    Status;

+

   DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));

 

   DebugDumpCmos ();

@@ -433,6 +437,12 @@ InitializeXenPlatform (
     CpuDeadLoop ();

   }

 

+  if (QemuFwCfgS3Enabled ()) {

+    DEBUG ((DEBUG_INFO, "S3 support was detected on QEMU\n"));

+    Status = PcdSetBoolS (PcdAcpiS3Enable, TRUE);

+    ASSERT_EFI_ERROR (Status);

+  }

+

   XenConnect ();

 

   BootModeInitialization ();

diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
index 597cb6fcd7ff..1e22c0b2e2aa 100644
--- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
+++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
@@ -57,6 +57,8 @@ [LibraryClasses]
   ResourcePublicationLib

   PeiServicesLib

   PeimEntryPoint

+  QemuFwCfgLib

+  QemuFwCfgS3Lib

   MtrrLib

   MemEncryptSevLib

   PcdLib

@@ -79,6 +81,7 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base

   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size

   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes

+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable

   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize

   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved

   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode

-- 
2.31.1



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


Re: [edk2-devel] [RFC PATCH] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
Posted by Ard Biesheuvel 2 years, 8 months ago
On Thu, 8 Jul 2021 at 06:05, Gary Lin <glin@suse.com> wrote:
>
> There are several functions in OvmfPkg/Library using
> QemuFwCfgS3Enabled() to detect the S3 support status. However, in
> MdeModulePkg, PcdAcpiS3Enable is used to check S3 support. Since
> InitializeXenPlatform() didn't set PcdAcpiS3Enable as
> InitializePlatform() did, this made the inconsistency between
> drivers/functions.
>
> For example, S3SaveStateDxe checked PcdAcpiS3Enable and skipped
> S3BootScript because the default value is FALSE. On the other hand,
> PlatformBootManagerBeforeConsole() from OvmfPkg/Library called
> QemuFwCfgS3Enabled() and found it returned TRUE, so it invoked
> SaveS3BootScript(). However, S3SaveStateDxe skipped S3BootScript, so
> SaveS3BootScript() asserted due to EFI_NOT_FOUND.
>
> Setting PcdAcpiS3Enable at InitializeXenPlatform() "fixes" the crash
> reported by my colleague. The other possible direction is to replace
> QemuFwCfgS3Enabled() with PcdAcpiS3Enable. I'm not sure which one is
> the right fix.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Jim Fehlig <jfehlig@suse.com>
> Signed-off-by: Gary Lin <glin@suse.com>

This needs an ack from the Xen folks.

> ---
>  OvmfPkg/XenPlatformPei/Platform.c         | 10 ++++++++++
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  3 +++
>  2 files changed, 13 insertions(+)
>
> diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
> index a811e72ee301..f7edc979486e 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.c
> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> @@ -26,6 +26,8 @@
>  #include <Library/PciLib.h>
>  #include <Library/PeimEntryPoint.h>
>  #include <Library/PeiServicesLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +#include <Library/QemuFwCfgS3Lib.h>
>  #include <Library/ResourcePublicationLib.h>
>  #include <Guid/MemoryTypeInformation.h>
>  #include <Ppi/MasterBootMode.h>
> @@ -423,6 +425,8 @@ InitializeXenPlatform (
>    IN CONST EFI_PEI_SERVICES     **PeiServices
>    )
>  {
> +  EFI_STATUS    Status;
> +
>    DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));
>
>    DebugDumpCmos ();
> @@ -433,6 +437,12 @@ InitializeXenPlatform (
>      CpuDeadLoop ();
>    }
>
> +  if (QemuFwCfgS3Enabled ()) {
> +    DEBUG ((DEBUG_INFO, "S3 support was detected on QEMU\n"));
> +    Status = PcdSetBoolS (PcdAcpiS3Enable, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    XenConnect ();
>
>    BootModeInitialization ();
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 597cb6fcd7ff..1e22c0b2e2aa 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -57,6 +57,8 @@ [LibraryClasses]
>    ResourcePublicationLib
>    PeiServicesLib
>    PeimEntryPoint
> +  QemuFwCfgLib
> +  QemuFwCfgS3Lib
>    MtrrLib
>    MemEncryptSevLib
>    PcdLib
> @@ -79,6 +81,7 @@ [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
>    gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
> --
> 2.31.1
>


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