[PATCH v2 0/5] Fix OvmfXen HVM Direct kernel boot failure

[PATCH v2 0/5] Fix OvmfXen HVM Direct kernel boot failure

[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. Besides, QemuKernelLoaderFsDxe wasn't included
in OvmfXen, so the firmware couldn't fetch kernel/initrd from fw_cfg.

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

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

Gary Lin (5):
  OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
  OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe
  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/OvmfXen.dsc                                 |  1 +
 OvmfPkg/OvmfXen.fdf                                 |  1 +
 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 +++++++++++++
 10 files changed, 24 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

[Lin, Gary (HPS OE-Linux)]

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". If "acpi_s3" is
set as "True" in xl.cfg, then the S3 Support bit is set and passed
with fw_cfg.

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

[PATCH v2 2/5] OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe

[Lin, Gary (HPS OE-Linux)]

Without QemuKernelLoaderFsDxe, QemuLoadKernelImage() couldn't download
the kernel, initrd, and kernel command line from QEMU's fw_cfg.

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/OvmfXen.dsc | 1 +
 OvmfPkg/OvmfXen.fdf | 1 +
 2 files changed, 2 insertions(+)

diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 3c1ca6bfd493..1a9c06c164a8 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -587,6 +587,7 @@ [Components]
       NULL|OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
 !endif
   }
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
   OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
   OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
   OvmfPkg/XenBusDxe/XenBusDxe.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index aeb9336fd5b7..8b5823555937 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -324,6 +324,7 @@ [FV.DXEFV]
 INF  MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
 INF  MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
 INF  MdeModulePkg/Application/UiApp/UiApp.inf
+INF  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
 INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
 INF  MdeModulePkg/Universal/PrintDxe/PrintDxe.inf
 INF  MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
-- 
2.31.1

[PATCH v2 3/5] OvmfPkg/LockBoxLib: use PcdAcpiS3Enable to detect S3 support

[Lin, Gary (HPS OE-Linux)]

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

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

[PATCH v2 4/5] OvmfPkg/PlatformBootManagerLib: use PcdAcpiS3Enable to detect S3 support

[Lin, Gary (HPS OE-Linux)]

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

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

[PATCH v2 5/5] OvmfPkg/SmmControl2Dxe: use PcdAcpiS3Enable to detect S3 support

[Lin, Gary (HPS OE-Linux)]

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

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 v2 0/5] Fix OvmfXen HVM Direct kernel boot failure

[Yao, Jiewen]

HI Gary
Several comment:

1) According to our process, we need a Bugzilla. Would you please file it?

2) I do not understand how HVM direct kernel boot is related to S3 enabling.
It seems 1/3/4/5 are for S3, while 2 is for missing driver.
Should we split them to 2 patch set?

3) Does the S3 issue only happen in direct kernel boot? Or is it a generic issue.

4) Have you validated non direct kernel boot to ensure it still works?


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lin, Gary
> (HPS OE-Linux)
> Sent: Friday, August 13, 2021 2:13 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH v2 0/5] Fix OvmfXen HVM Direct kernel boot
> failure
> 
> When using HVM Direct kernel boot with OvmfXen, it could fail at the
> S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
> and PcdAcpiS3Enable. Besides, QemuKernelLoaderFsDxe wasn't included
> in OvmfXen, so the firmware couldn't fetch kernel/initrd from fw_cfg.
> 
> This patch series initializes PcdAcpiS3Enable and adds
> QemuKernelLoaderFsDxe into OvmfXen. Besides, QemuFwCfgS3Enabled() is
> replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
> potential inconsistency.
> 
> 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
> 
> Gary Lin (5):
>   OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
>   OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe
>   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/OvmfXen.dsc                                 |  1 +
>  OvmfPkg/OvmfXen.fdf                                 |  1 +
>  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 +++++++++++++
>  10 files changed, 24 insertions(+), 9 deletions(-)
> 
> --
> 2.31.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 0/5] Fix OvmfXen HVM Direct kernel boot failure

[Lin, Gary (HPS OE-Linux)]

On Fri, Aug 13, 2021 at 09:55:48AM +0000, Yao, Jiewen wrote:
> HI Gary
Hi Jiewen,

> Several comment:
> 
> 1) According to our process, we need a Bugzilla. Would you please file it?
> 
Okay, will create a bugzilla entry to track the issue.

> 2) I do not understand how HVM direct kernel boot is related to S3 enabling.
> It seems 1/3/4/5 are for S3, while 2 is for missing driver.
> Should we split them to 2 patch set?
> 
Actually the inconsistency between QemuFwCfgS3Enabled() and
PcdAcpiS3Enable casued the boot failure of HVM direct kernel boot.
S3SaveStateDxe checked PcdAcpiS3Enable(=FALSE) and skipped S3BootScript.
On the other hand, PlatformBootManagerBeforeConsole() invoked
QemuFwCfgS3Enabled()(=TRUE) and tried to locate S3BootScript. Since
S3BootScript wasn't installed, it failed with EFI_NOT_FOUND and stopped
the booting. So Patch 1 and 2 are mainly for the booting issues in
OvmfXen. For Patch 3~5, I just try to eliminate the inconsistency to
avoid the potential problems.

I can split them into 2 or 3 patch sets if necessary.

> 3) Does the S3 issue only happen in direct kernel boot? Or is it a generic issue.
> 
> 4) Have you validated non direct kernel boot to ensure it still works?
> 
Jim,

Did you encounter any problem with non direct kernel boot VM?


Cheers,

Gary Lin

> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lin, Gary
> > (HPS OE-Linux)
> > Sent: Friday, August 13, 2021 2:13 PM
> > To: devel@edk2.groups.io
> > Subject: [edk2-devel] [PATCH v2 0/5] Fix OvmfXen HVM Direct kernel boot
> > failure
> > 
> > When using HVM Direct kernel boot with OvmfXen, it could fail at the
> > S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
> > and PcdAcpiS3Enable. Besides, QemuKernelLoaderFsDxe wasn't included
> > in OvmfXen, so the firmware couldn't fetch kernel/initrd from fw_cfg.
> > 
> > This patch series initializes PcdAcpiS3Enable and adds
> > QemuKernelLoaderFsDxe into OvmfXen. Besides, QemuFwCfgS3Enabled() is
> > replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
> > potential inconsistency.
> > 
> > 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
> > 
> > Gary Lin (5):
> >   OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
> >   OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe
> >   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/OvmfXen.dsc                                 |  1 +
> >  OvmfPkg/OvmfXen.fdf                                 |  1 +
> >  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 +++++++++++++
> >  10 files changed, 24 insertions(+), 9 deletions(-)
> > 
> > --
> > 2.31.1
> > 
> > 
> > 
> > 
> > 
> 

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

[Ard Biesheuvel]

Does this series have a cover letter?

On Fri, 13 Aug 2021 at 08:13, Lin, Gary (HPS OE-Linux) <gary.lin@hpe.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.
>
> This issue mainly affects "HVM Direct Kernel Boot". If "acpi_s3" is
> set as "True" in xl.cfg, then the S3 Support bit is set and passed
> with fw_cfg.
>
> 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: [edk2-devel] [PATCH v2 1/5] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

[Lin, Gary (HPS OE-Linux)]

On Mon, Aug 16, 2021 at 09:07:01AM +0200, Ard Biesheuvel wrote:
> Does this series have a cover letter?
> 
Sorry, I forgot to add Cc tags in the cover letter.
https://edk2.groups.io/g/devel/topic/84857762#79245

Gary Lin

> On Fri, 13 Aug 2021 at 08:13, Lin, Gary (HPS OE-Linux) <gary.lin@hpe.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.
> >
> > This issue mainly affects "HVM Direct Kernel Boot". If "acpi_s3" is
> > set as "True" in xl.cfg, then the S3 Support bit is set and passed
> > with fw_cfg.
> >
> > 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: [edk2-devel] [PATCH v2 0/5] Fix OvmfXen HVM Direct kernel boot failure

[Lin, Gary (HPS OE-Linux)]

On Fri, Aug 13, 2021 at 02:13:00PM +0800, Lin, Gary (HPS OE-Linux) wrote:
> When using HVM Direct kernel boot with OvmfXen, it could fail at the
> S3BootScript due to the inconsistency between QemuFwCfgS3Enabled()
> and PcdAcpiS3Enable. Besides, QemuKernelLoaderFsDxe wasn't included
> in OvmfXen, so the firmware couldn't fetch kernel/initrd from fw_cfg.
> 
> This patch series initializes PcdAcpiS3Enable and adds
> QemuKernelLoaderFsDxe into OvmfXen. Besides, QemuFwCfgS3Enabled() is
> replaced with PcdAcpiS3Enable in several OVMF libraries to avoid the
> potential inconsistency.
> 
> 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
> 
Add the missing Cc tags

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>

> Gary Lin (5):
>   OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization
>   OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe
>   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/OvmfXen.dsc                                 |  1 +
>  OvmfPkg/OvmfXen.fdf                                 |  1 +
>  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 +++++++++++++
>  10 files changed, 24 insertions(+), 9 deletions(-)
> 
> -- 
> 2.31.1
> 
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 0/5] Fix OvmfXen HVM Direct kernel boot failure

[Jim Fehlig]

On 8/15/21 7:34 PM, Gary Lin (HPS OE-Linux TDC) wrote:
> On Fri, Aug 13, 2021 at 09:55:48AM +0000, Yao, Jiewen wrote:
>> HI Gary
> Hi Jiewen,
> 
>> Several comment:
>>
>> 1) According to our process, we need a Bugzilla. Would you please file it?
>>
> Okay, will create a bugzilla entry to track the issue.
> 
>> 2) I do not understand how HVM direct kernel boot is related to S3 enabling.
>> It seems 1/3/4/5 are for S3, while 2 is for missing driver.
>> Should we split them to 2 patch set?
>>
> Actually the inconsistency between QemuFwCfgS3Enabled() and
> PcdAcpiS3Enable casued the boot failure of HVM direct kernel boot.
> S3SaveStateDxe checked PcdAcpiS3Enable(=FALSE) and skipped S3BootScript.
> On the other hand, PlatformBootManagerBeforeConsole() invoked
> QemuFwCfgS3Enabled()(=TRUE) and tried to locate S3BootScript. Since
> S3BootScript wasn't installed, it failed with EFI_NOT_FOUND and stopped
> the booting. So Patch 1 and 2 are mainly for the booting issues in
> OvmfXen. For Patch 3~5, I just try to eliminate the inconsistency to
> avoid the potential problems.
> 
> I can split them into 2 or 3 patch sets if necessary.
> 
>> 3) Does the S3 issue only happen in direct kernel boot? Or is it a generic issue.
>>
>> 4) Have you validated non direct kernel boot to ensure it still works?
>>
> Jim,
> 
> Did you encounter any problem with non direct kernel boot VM?

No problems encountered, works fine in my testing. Although I did not test these 
patches specifically, only the ones in the openSUSE Factory ovmf package.

Regards,
Jim

Re: [PATCH v2 1/5] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

[Anthony PERARD]

On Fri, Aug 13, 2021 at 02:13:01PM +0800, Gary Lin 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.
> 
> This issue mainly affects "HVM Direct Kernel Boot". If "acpi_s3" is
> set as "True" in xl.cfg, then the S3 Support bit is set and passed
> with fw_cfg.

The part about acpi_s3 isn't true, it doesn't affect fw_cfg.

Maybe this paragraph would be better:

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

The rest of the patch looks good, thanks.

-- 
Anthony PERARD

Re: [PATCH v2 2/5] OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe

[Anthony PERARD]

On Fri, Aug 13, 2021 at 02:13:02PM +0800, Gary Lin wrote:
> Without QemuKernelLoaderFsDxe, QemuLoadKernelImage() couldn't download
> the kernel, initrd, and kernel command line from QEMU's fw_cfg.

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

Thanks,

-- 
Anthony PERARD