[PATCH v3 0/4] Fix OvmfXen boot failure due to s3 support state

[PATCH v3 0/4] Fix OvmfXen boot failure due to s3 support state

[Lin, Gary (HPS OE-Linux)]

When using HVM Direct kernel boot with OvmfXen, it could fail at the
S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
and PcdAcpiS3Enable.

This patch series initializes PcdAcpiS3Enable in 
. Besides, QemuFwCfgS3Enabled() is
replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
potential inconsistency.

Bugzilla links:
  https://bugzilla.tianocore.org/show_bug.cgi?id=3573

v3:
  - Update the description per Anthony's suggestion
  - Add the bugzilla links
  - Move the QemuKernelLoaderFsDxe patch out of this patch series
    and make it an independent patch
v2:
  - Amend the description and address "HVM Direct Kernel Boot"
  - Add the comment for the conditional test of QemuFwCfgS3Enabled()
  - Remove unused QemuFwCfgLib
  - Update my email address

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
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>
Cc: Joey Li <jlee@suse.com>

Gary Lin (4):
  OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
  OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support
  OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3
    support
  OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support

 OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf        |  3 +--
 .../PlatformBootManagerLib.inf                      |  1 +
 OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf           |  2 ++
 OvmfPkg/XenPlatformPei/XenPlatformPei.inf           |  2 ++
 OvmfPkg/Library/LockBoxLib/LockBoxDxe.c             |  4 +---
 .../Library/PlatformBootManagerLib/BdsPlatform.c    |  2 +-
 OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c             |  4 +---
 OvmfPkg/XenPlatformPei/Platform.c                   | 13 +++++++++++++
 8 files changed, 22 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH v3 1/4] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

[Lin, Gary (HPS OE-Linux)]

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

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.

This issue mainly affects "HVM Direct Kernel Boot". When used,
"fw_cfg" is enabled in QEMU and QemuFwCfgS3Enabled() returns true in
that case.

v3:
  - Update the description per Anthony's suggestion
  - Add the bugzilla link
v2:
  - Amend the description and address "HVM Direct Kernel Boot"
  - Add the comment for the conditional test of QemuFwCfgS3Enabled()
  - Remove unused QemuFwCfgLib

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
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>
Cc: Joey Li <jlee@suse.com>
Signed-off-by: Gary Lin <gary.lin@hpe.com>
---
 OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  2 ++
 OvmfPkg/XenPlatformPei/Platform.c         | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
index 597cb6fcd7ff..20c27ff34b6c 100644
--- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
+++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
@@ -57,6 +57,7 @@ [LibraryClasses]
   ResourcePublicationLib
   PeiServicesLib
   PeimEntryPoint
+  QemuFwCfgS3Lib
   MtrrLib
   MemEncryptSevLib
   PcdLib
@@ -79,6 +80,7 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
index a811e72ee301..e60478fdb493 100644
--- a/OvmfPkg/XenPlatformPei/Platform.c
+++ b/OvmfPkg/XenPlatformPei/Platform.c
@@ -26,6 +26,7 @@
 #include <Library/PciLib.h>
 #include <Library/PeimEntryPoint.h>
 #include <Library/PeiServicesLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
 #include <Library/ResourcePublicationLib.h>
 #include <Guid/MemoryTypeInformation.h>
 #include <Ppi/MasterBootMode.h>
@@ -423,6 +424,8 @@ InitializeXenPlatform (
   IN CONST EFI_PEI_SERVICES     **PeiServices
   )
 {
+  EFI_STATUS    Status;
+
   DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));
 
   DebugDumpCmos ();
@@ -433,6 +436,16 @@ InitializeXenPlatform (
     CpuDeadLoop ();
   }
 
+  //
+  // This S3 conditional test is mainly for HVM Direct Kernel Boot since
+  // QEMU fwcfg isn't really supported other than that.
+  //
+  if (QemuFwCfgS3Enabled ()) {
+    DEBUG ((DEBUG_INFO, "S3 support was detected on QEMU\n"));
+    Status = PcdSetBoolS (PcdAcpiS3Enable, TRUE);
+    ASSERT_EFI_ERROR (Status);
+  }
+
   XenConnect ();
 
   BootModeInitialization ();
-- 
2.31.1

Re: [PATCH v3 1/4] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

[Anthony PERARD]

On Mon, Aug 23, 2021 at 03:09:22PM +0800, Gary Lin wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> 
> 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.
> 
> This issue mainly affects "HVM Direct Kernel Boot". When used,
> "fw_cfg" is enabled in QEMU and QemuFwCfgS3Enabled() returns true in
> that case.
> 
> v3:
>   - Update the description per Anthony's suggestion
>   - Add the bugzilla link
> v2:
>   - Amend the description and address "HVM Direct Kernel Boot"
>   - Add the comment for the conditional test of QemuFwCfgS3Enabled()
>   - Remove unused QemuFwCfgLib
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 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>
> Cc: Joey Li <jlee@suse.com>
> Signed-off-by: Gary Lin <gary.lin@hpe.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [edk2-devel] [PATCH v3 1/4] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

[Gerd Hoffmann]

> +++ b/OvmfPkg/XenPlatformPei/Platform.c

> +  //
> +  // This S3 conditional test is mainly for HVM Direct Kernel Boot since
> +  // QEMU fwcfg isn't really supported other than that.
> +  //
> +  if (QemuFwCfgS3Enabled ()) {
> +    DEBUG ((DEBUG_INFO, "S3 support was detected on QEMU\n"));
> +    Status = PcdSetBoolS (PcdAcpiS3Enable, TRUE);
> +    ASSERT_EFI_ERROR (Status);
> +  }

OvmfPkg/PlatformPei/Platform.c already does that, so this makes kvm and
xen more consistent.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

[PATCH v3 2/4] OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support

[Lin, Gary (HPS OE-Linux)]

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

To avoid the potential inconsistency between PcdAcpiS3Enable and
QemuFwCfgS3Enabled(), this commit modifies LockBoxLib to detect
S3 support by PcdAcpiS3Enable as modules in MdeModulePkg do.

v3:
  - Add the bugzilla link

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Joey Li <jlee@suse.com>
Signed-off-by: Gary Lin <gary.lin@hpe.com>
---
 OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 3 +--
 OvmfPkg/Library/LockBoxLib/LockBoxDxe.c      | 4 +---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
index 38bcc577084a..9140b1ba9de9 100644
--- a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
+++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
@@ -33,8 +33,6 @@ [LibraryClasses]
   BaseMemoryLib
   DebugLib
   UefiBootServicesTableLib
-  QemuFwCfgLib
-  QemuFwCfgS3Lib
 
 [Protocols]
   gEfiLockBoxProtocolGuid    ## SOMETIMES_PRODUCES
@@ -42,6 +40,7 @@ [Protocols]
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
index b28ad4d2dba7..7dc2eea2395a 100644
--- a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
+++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
@@ -12,8 +12,6 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
-#include <Library/QemuFwCfgLib.h>
-#include <Library/QemuFwCfgS3Lib.h>
 #include <Protocol/LockBox.h>
 #include <LockBoxLib.h>
 
@@ -117,7 +115,7 @@ LockBoxDxeLibInitialize (
 
   Status = LockBoxLibInitialize ();
   if (!EFI_ERROR (Status)) {
-    if (QemuFwCfgS3Enabled ()) {
+    if (PcdGetBool (PcdAcpiS3Enable)) {
       //
       // When S3 enabled, the first driver run with this library linked will
       // have this library constructor to install LockBox protocol on the
-- 
2.31.1

Re: [edk2-devel] [PATCH v3 2/4] OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support

[Philippe Mathieu-Daudé]

On 8/23/21 9:09 AM, Lin, Gary (HPS OE-Linux) wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> 
> To avoid the potential inconsistency between PcdAcpiS3Enable and
> QemuFwCfgS3Enabled(), this commit modifies LockBoxLib to detect
> S3 support by PcdAcpiS3Enable as modules in MdeModulePkg do.
> 
> v3:
>   - Add the bugzilla link
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jim Fehlig <jfehlig@suse.com>
> Cc: Joey Li <jlee@suse.com>
> Signed-off-by: Gary Lin <gary.lin@hpe.com>
> ---
>  OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 3 +--
>  OvmfPkg/Library/LockBoxLib/LockBoxDxe.c      | 4 +---
>  2 files changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v3 2/4] OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support

[Gerd Hoffmann]

On Mon, Aug 23, 2021 at 03:09:23PM +0800, Lin, Gary (HPS OE-Linux) wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> 
> To avoid the potential inconsistency between PcdAcpiS3Enable and
> QemuFwCfgS3Enabled(), this commit modifies LockBoxLib to detect
> S3 support by PcdAcpiS3Enable as modules in MdeModulePkg do.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

[PATCH v3 3/4] OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3 support

[Lin, Gary (HPS OE-Linux)]

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

To avoid the potential inconsistency between PcdAcpiS3Enable and
QemuFwCfgS3Enabled(), this commit modifies PlatformBootManagerLib to
detect S3 support by PcdAcpiS3Enable as modules in MdeModulePkg do.

v3:
  - Add the bugzilla link

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Joey Li <jlee@suse.com>
Signed-off-by: Gary Lin <gary.lin@hpe.com>
---
 .../Library/PlatformBootManagerLib/PlatformBootManagerLib.inf   | 1 +
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c            | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index e470b9a6a3e5..c249a3cf1e35 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -61,6 +61,7 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate         ## CONSUMES
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits         ## CONSUMES
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index b0e97429372b..71f63b244828 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -379,7 +379,7 @@ PlatformBootManagerBeforeConsole (
   //
   EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid);
 
-  if (QemuFwCfgS3Enabled ()) {
+  if (PcdGetBool (PcdAcpiS3Enable)) {
     //
     // Save the boot script too. Note that this will require us to emit the
     // DxeSmmReadyToLock event just below, which in turn locks down SMM.
-- 
2.31.1

Re: [edk2-devel] [PATCH v3 3/4] OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3 support

[Gerd Hoffmann]

On Mon, Aug 23, 2021 at 03:09:24PM +0800, Lin, Gary (HPS OE-Linux) wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> 
> To avoid the potential inconsistency between PcdAcpiS3Enable and
> QemuFwCfgS3Enabled(), this commit modifies PlatformBootManagerLib to
> detect S3 support by PcdAcpiS3Enable as modules in MdeModulePkg do.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

[PATCH v3 4/4] OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support

[Lin, Gary (HPS OE-Linux)]

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

To avoid the potential inconsistency between PcdAcpiS3Enable and
QemuFwCfgS3Enabled(), this commit modifies SmmControl2Dxe to detect
S3 support by PcdAcpiS3Enable as modules in MdeModulePkg do.

v3:
  - Add the bugzilla link

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Joey Li <jlee@suse.com>
Signed-off-by: Gary Lin <gary.lin@hpe.com>
---
 OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 2 ++
 OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c   | 4 +---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
index b8fdea8deb84..4cad56516f49 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
@@ -39,6 +39,7 @@ [Sources]
 
 [Packages]
   MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
   OvmfPkg/OvmfPkg.dec
   UefiCpuPkg/UefiCpuPkg.dec
 
@@ -62,6 +63,7 @@ [Protocols]
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout ## SOMETIMES_PRODUCES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode      ## SOMETIMES_PRODUCES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
index 9547c202880f..be04baf7b288 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
@@ -25,8 +25,6 @@
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
 #include <Library/PciLib.h>
-#include <Library/QemuFwCfgLib.h>
-#include <Library/QemuFwCfgS3Lib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Protocol/S3SaveState.h>
 #include <Protocol/SmmControl2.h>
@@ -238,7 +236,7 @@ SmmControl2DxeEntryPoint (
   //
   mSmiFeatureNegotiation = NegotiateSmiFeatures ();
 
-  if (QemuFwCfgS3Enabled ()) {
+  if (PcdGetBool (PcdAcpiS3Enable)) {
     VOID *Registration;
 
     //
-- 
2.31.1

Re: [edk2-devel] [PATCH v3 4/4] OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support

[Gerd Hoffmann]

On Mon, Aug 23, 2021 at 03:09:25PM +0800, Lin, Gary (HPS OE-Linux) wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> 
> To avoid the potential inconsistency between PcdAcpiS3Enable and
> QemuFwCfgS3Enabled(), this commit modifies SmmControl2Dxe to detect
> S3 support by PcdAcpiS3Enable as modules in MdeModulePkg do.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [edk2-devel] [PATCH v3 0/4] Fix OvmfXen boot failure due to s3 support state

[Yao, Jiewen]

Series reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lin, Gary
> (HPS OE-Linux)
> Sent: Monday, August 23, 2021 3:09 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Anthony Perard <anthony.perard@citrix.com>; Julien Grall <julien@xen.org>;
> Jim Fehlig <jfehlig@suse.com>; Joey Li <jlee@suse.com>
> Subject: [edk2-devel] [PATCH v3 0/4] Fix OvmfXen boot failure due to s3 support
> state
> 
> When using HVM Direct kernel boot with OvmfXen, it could fail at the
> S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
> and PcdAcpiS3Enable.
> 
> This patch series initializes PcdAcpiS3Enable in
> . Besides, QemuFwCfgS3Enabled() is
> replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
> potential inconsistency.
> 
> Bugzilla links:
>   https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> 
> v3:
>   - Update the description per Anthony's suggestion
>   - Add the bugzilla links
>   - Move the QemuKernelLoaderFsDxe patch out of this patch series
>     and make it an independent patch
> v2:
>   - Amend the description and address "HVM Direct Kernel Boot"
>   - Add the comment for the conditional test of QemuFwCfgS3Enabled()
>   - Remove unused QemuFwCfgLib
>   - Update my email address
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 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>
> Cc: Joey Li <jlee@suse.com>
> 
> Gary Lin (4):
>   OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
>   OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support
>   OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3
>     support
>   OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support
> 
>  OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf        |  3 +--
>  .../PlatformBootManagerLib.inf                      |  1 +
>  OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf           |  2 ++
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf           |  2 ++
>  OvmfPkg/Library/LockBoxLib/LockBoxDxe.c             |  4 +---
>  .../Library/PlatformBootManagerLib/BdsPlatform.c    |  2 +-
>  OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c             |  4 +---
>  OvmfPkg/XenPlatformPei/Platform.c                   | 13 +++++++++++++
>  8 files changed, 22 insertions(+), 9 deletions(-)
> 
> --
> 2.31.1
> 
> 
> 
> 
> 

Re: [PATCH v3 0/4] Fix OvmfXen boot failure due to s3 support state

[Jim Fehlig]

On 8/23/21 1:09 AM, Gary Lin wrote:
> When using HVM Direct kernel boot with OvmfXen, it could fail at the
> S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
> and PcdAcpiS3Enable.
> 
> This patch series initializes PcdAcpiS3Enable in
> . Besides, QemuFwCfgS3Enabled() is
> replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
> potential inconsistency.
> 
> Bugzilla links:
>    https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> 
> v3:
>    - Update the description per Anthony's suggestion
>    - Add the bugzilla links
>    - Move the QemuKernelLoaderFsDxe patch out of this patch series
>      and make it an independent patch
> v2:
>    - Amend the description and address "HVM Direct Kernel Boot"
>    - Add the comment for the conditional test of QemuFwCfgS3Enabled()
>    - Remove unused QemuFwCfgLib
>    - Update my email address
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 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>
> Cc: Joey Li <jlee@suse.com>
> 
> Gary Lin (4):
>    OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
>    OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support
>    OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3
>      support
>    OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support
> 
>   OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf        |  3 +--
>   .../PlatformBootManagerLib.inf                      |  1 +
>   OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf           |  2 ++
>   OvmfPkg/XenPlatformPei/XenPlatformPei.inf           |  2 ++
>   OvmfPkg/Library/LockBoxLib/LockBoxDxe.c             |  4 +---
>   .../Library/PlatformBootManagerLib/BdsPlatform.c    |  2 +-
>   OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c             |  4 +---
>   OvmfPkg/XenPlatformPei/Platform.c                   | 13 +++++++++++++
>   8 files changed, 22 insertions(+), 9 deletions(-)

Tested-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

Re: [PATCH v3 0/4] Fix OvmfXen boot failure due to s3 support state

[Ard Biesheuvel]

On Tue, 24 Aug 2021 at 19:21, Jim Fehlig <jfehlig@suse.com> wrote:
>
> On 8/23/21 1:09 AM, Gary Lin wrote:
> > When using HVM Direct kernel boot with OvmfXen, it could fail at the
> > S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
> > and PcdAcpiS3Enable.
> >
> > This patch series initializes PcdAcpiS3Enable in
> > . Besides, QemuFwCfgS3Enabled() is
> > replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
> > potential inconsistency.
> >
> > Bugzilla links:
> >    https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> >
> > v3:
> >    - Update the description per Anthony's suggestion
> >    - Add the bugzilla links
> >    - Move the QemuKernelLoaderFsDxe patch out of this patch series
> >      and make it an independent patch
> > v2:
> >    - Amend the description and address "HVM Direct Kernel Boot"
> >    - Add the comment for the conditional test of QemuFwCfgS3Enabled()
> >    - Remove unused QemuFwCfgLib
> >    - Update my email address
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > 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>
> > Cc: Joey Li <jlee@suse.com>
> >
> > Gary Lin (4):
> >    OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
> >    OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support
> >    OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3
> >      support
> >    OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support
> >
> >   OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf        |  3 +--
> >   .../PlatformBootManagerLib.inf                      |  1 +
> >   OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf           |  2 ++
> >   OvmfPkg/XenPlatformPei/XenPlatformPei.inf           |  2 ++
> >   OvmfPkg/Library/LockBoxLib/LockBoxDxe.c             |  4 +---
> >   .../Library/PlatformBootManagerLib/BdsPlatform.c    |  2 +-
> >   OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c             |  4 +---
> >   OvmfPkg/XenPlatformPei/Platform.c                   | 13 +++++++++++++
> >   8 files changed, 22 insertions(+), 9 deletions(-)
>
> Tested-by: Jim Fehlig <jfehlig@suse.com>
>


Thanks for this series, and thanks all for the reviews.

Gary, could you please resend it with the following changes so it
applies cleanly:
- Move the bugzilla link into the signoff block, preceded by Ref:
- Remove v[23] etc version information from the patches, or move it
below the three dashes line so it does not end up in the commit log
- Incorporate all the review tags given in response to this v3
- Please find a way to send these patches that doesn't result in
mangled whitespace (hint: using Outlook/Exchange is probably not the
best choice)


Thanks,
Ard.

Re: [PATCH v3 0/4] Fix OvmfXen boot failure due to s3 support state

[Lin, Gary (HPS OE-Linux)]

On Fri, Aug 27, 2021 at 03:16:43PM +0200, Ard Biesheuvel wrote:
> On Tue, 24 Aug 2021 at 19:21, Jim Fehlig <jfehlig@suse.com> wrote:
> >
> > On 8/23/21 1:09 AM, Gary Lin wrote:
> > > When using HVM Direct kernel boot with OvmfXen, it could fail at the
> > > S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
> > > and PcdAcpiS3Enable.
> > >
> > > This patch series initializes PcdAcpiS3Enable in
> > > . Besides, QemuFwCfgS3Enabled() is
> > > replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
> > > potential inconsistency.
> > >
> > > Bugzilla links:
> > >    https://bugzilla.tianocore.org/show_bug.cgi?id=3573
> > >
> > > v3:
> > >    - Update the description per Anthony's suggestion
> > >    - Add the bugzilla links
> > >    - Move the QemuKernelLoaderFsDxe patch out of this patch series
> > >      and make it an independent patch
> > > v2:
> > >    - Amend the description and address "HVM Direct Kernel Boot"
> > >    - Add the comment for the conditional test of QemuFwCfgS3Enabled()
> > >    - Remove unused QemuFwCfgLib
> > >    - Update my email address
> > >
> > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > 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>
> > > Cc: Joey Li <jlee@suse.com>
> > >
> > > Gary Lin (4):
> > >    OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
> > >    OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support
> > >    OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3
> > >      support
> > >    OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support
> > >
> > >   OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf        |  3 +--
> > >   .../PlatformBootManagerLib.inf                      |  1 +
> > >   OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf           |  2 ++
> > >   OvmfPkg/XenPlatformPei/XenPlatformPei.inf           |  2 ++
> > >   OvmfPkg/Library/LockBoxLib/LockBoxDxe.c             |  4 +---
> > >   .../Library/PlatformBootManagerLib/BdsPlatform.c    |  2 +-
> > >   OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c             |  4 +---
> > >   OvmfPkg/XenPlatformPei/Platform.c                   | 13 +++++++++++++
> > >   8 files changed, 22 insertions(+), 9 deletions(-)
> >
> > Tested-by: Jim Fehlig <jfehlig@suse.com>
> >
> 
> 
> Thanks for this series, and thanks all for the reviews.
> 
> Gary, could you please resend it with the following changes so it
> applies cleanly:
> - Move the bugzilla link into the signoff block, preceded by Ref:
> - Remove v[23] etc version information from the patches, or move it
> below the three dashes line so it does not end up in the commit log
> - Incorporate all the review tags given in response to this v3
Will update the patches.

> - Please find a way to send these patches that doesn't result in
> mangled whitespace (hint: using Outlook/Exchange is probably not the
> best choice)
> 
Sorry for that. I forgot to set sendemail.transferEncoding to 8bit and
git chose quoted-printable automatically. I've updated my git config and
it should be fixed next time.

Thanks,

Gary Lin