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


Gary Lin
 

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


Ard Biesheuvel
 

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


Anthony PERARD
 

On Thu, Jul 08, 2021 at 12:05:49PM +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.

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.
QemuFwCfgS3Enabled() should always return false as we don't enable
QEMU's fwcfg interface in most case when running a Xen guest. So I don't
expect "PcdAcpiS3Enable" to ever been set via this patch.

Now, maybe we want to set PcdAcpiS3Enable unconditionally but I don't
know if S3 support is going to work as expected with OVMF under Xen, I
never look at that.

What kind of guest are you trying to fix? When does the crash happen?

Thanks,

--
Anthony PERARD


Anthony PERARD
 

It would have been nice to have this patch in a patch series with
"OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe", mostly to make it simpler
to understand the problem needed to be fixed.

On Thu, Jul 08, 2021 at 12:05:49PM +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 sounds like OvmfPkg would need to be fixed to use PcdAcpiS3Enable
instead of QemuFwCfgS3Enabled() in most placed and have a single place
where QemuFwCfgS3Enabled() is used to set PcdAcpiS3Enable. If you feel
like trying to fix that, that would be nice, and then we could probably
set PcdAcpiS3Enable unconditionally on OvmfXen (and maybe hope that S3
support actually works with Xen).

In the mean time, this patch is fine but wants better comments. First
two paragraphs are good, but the rest needs explanation on what we are
trying to fix/workaround, that is "Direct Kernel Boot" as it is called
in "man xl.cfg".

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.

Signed-off-by: Gary Lin <glin@suse.com>
---
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>
I don't think QemuFwCfgLib.h is needed, can you remove it?

+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/ResourcePublicationLib.h>
#include <Guid/MemoryTypeInformation.h>
#include <Ppi/MasterBootMode.h>
@@ -433,6 +437,12 @@ InitializeXenPlatform (
CpuDeadLoop ();
}

+ if (QemuFwCfgS3Enabled ()) {
This test needs a comment. QEMU's fwcfg isn't supposed to be available,
unless one try to use the Direct Kernel Boot functionality.

+ 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
Same here, QemuFwCfgLib doesn't seems to be needed or used.

Thanks,

--
Anthony PERARD


Ard Biesheuvel
 

On Mon, 19 Jul 2021 at 18:07, Anthony PERARD <anthony.perard@citrix.com> wrote:

It would have been nice to have this patch in a patch series with
"OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe", mostly to make it simpler
to understand the problem needed to be fixed.

On Thu, Jul 08, 2021 at 12:05:49PM +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 sounds like OvmfPkg would need to be fixed to use PcdAcpiS3Enable
instead of QemuFwCfgS3Enabled() in most placed and have a single place
where QemuFwCfgS3Enabled() is used to set PcdAcpiS3Enable. If you feel
like trying to fix that, that would be nice, and then we could probably
set PcdAcpiS3Enable unconditionally on OvmfXen (and maybe hope that S3
support actually works with Xen).
I don't think S3 suspend is the kind of thing that happens to work by
accident :-)


In the mean time, this patch is fine but wants better comments. First
two paragraphs are good, but the rest needs explanation on what we are
trying to fix/workaround, that is "Direct Kernel Boot" as it is called
in "man xl.cfg".
I don't have that man page. Could you elaborate?

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.

Signed-off-by: Gary Lin <glin@suse.com>
---
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>
I don't think QemuFwCfgLib.h is needed, can you remove it?

+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/ResourcePublicationLib.h>
#include <Guid/MemoryTypeInformation.h>
#include <Ppi/MasterBootMode.h>
@@ -433,6 +437,12 @@ InitializeXenPlatform (
CpuDeadLoop ();
}

+ if (QemuFwCfgS3Enabled ()) {
This test needs a comment. QEMU's fwcfg isn't supposed to be available,
unless one try to use the Direct Kernel Boot functionality.

+ 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
Same here, QemuFwCfgLib doesn't seems to be needed or used.

Thanks,

--
Anthony PERARD


Gary Lin
 

On Mon, Jul 19, 2021 at 05:07:21PM +0100, Anthony PERARD wrote:
It would have been nice to have this patch in a patch series with
"OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe", mostly to make it simpler
to understand the problem needed to be fixed.
To be honest, I don't have Xen environment and didn't realize that it's
about direct kernel boot until looking into another bug report. I just
compared InitializeXenPlatform() with InitializePlatform() and my
colleague told me OvmfXen works again after setting PcdAcpiS3Enable.

On Thu, Jul 08, 2021 at 12:05:49PM +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 sounds like OvmfPkg would need to be fixed to use PcdAcpiS3Enable
instead of QemuFwCfgS3Enabled() in most placed and have a single place
where QemuFwCfgS3Enabled() is used to set PcdAcpiS3Enable. If you feel
like trying to fix that, that would be nice, and then we could probably
set PcdAcpiS3Enable unconditionally on OvmfXen (and maybe hope that S3
support actually works with Xen).
That's why I marked this patch as RFC since the inconsistency could
exist in OVMF for KVM, not just Xen, so I would like to have feedbacks
from OvmfPkg maintainers. I'll amend the patch set to cover other
drivers/libraries in OvmfPkg.

In the mean time, this patch is fine but wants better comments. First
two paragraphs are good, but the rest needs explanation on what we are
trying to fix/workaround, that is "Direct Kernel Boot" as it is called
in "man xl.cfg".
Thanks for the suggestion. Will amend the comment in v2.

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.

Signed-off-by: Gary Lin <glin@suse.com>
---
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>
I don't think QemuFwCfgLib.h is needed, can you remove it?
Sure, will remove it from v2.

+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/ResourcePublicationLib.h>
#include <Guid/MemoryTypeInformation.h>
#include <Ppi/MasterBootMode.h>
@@ -433,6 +437,12 @@ InitializeXenPlatform (
CpuDeadLoop ();
}

+ if (QemuFwCfgS3Enabled ()) {
This test needs a comment. QEMU's fwcfg isn't supposed to be available,
unless one try to use the Direct Kernel Boot functionality.

+ 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
Same here, QemuFwCfgLib doesn't seems to be needed or used.
Will remove it from v2.

Thanks,

Gary Lin


Gary Lin
 

On Tue, Jul 20, 2021 at 02:52:12PM +0800, Gary Lin via groups.io wrote:
On Mon, Jul 19, 2021 at 05:07:21PM +0100, Anthony PERARD wrote:
It would have been nice to have this patch in a patch series with
"OvmfPkg/OvmfXen: add QemuKernelLoaderFsDxe", mostly to make it simpler
to understand the problem needed to be fixed.
To be honest, I don't have Xen environment and didn't realize that it's
about direct kernel boot until looking into another bug report. I just
compared InitializeXenPlatform() with InitializePlatform() and my
colleague told me OvmfXen works again after setting PcdAcpiS3Enable.

On Thu, Jul 08, 2021 at 12:05:49PM +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 sounds like OvmfPkg would need to be fixed to use PcdAcpiS3Enable
instead of QemuFwCfgS3Enabled() in most placed and have a single place
where QemuFwCfgS3Enabled() is used to set PcdAcpiS3Enable. If you feel
like trying to fix that, that would be nice, and then we could probably
set PcdAcpiS3Enable unconditionally on OvmfXen (and maybe hope that S3
support actually works with Xen).
That's why I marked this patch as RFC since the inconsistency could
exist in OVMF for KVM, not just Xen, so I would like to have feedbacks
from OvmfPkg maintainers. I'll amend the patch set to cover other
drivers/libraries in OvmfPkg.

In the mean time, this patch is fine but wants better comments. First
two paragraphs are good, but the rest needs explanation on what we are
trying to fix/workaround, that is "Direct Kernel Boot" as it is called
in "man xl.cfg".
Thanks for the suggestion. Will amend the comment in v2.
BTW, it seems to me that QEMU fwcfg is only used for Xen Direct Kernel
Boot. However, per xl.cfg manpage, it's possible to turn on or off S3
support by setting "acpi_s3" in xl.cfg, but PcdAcpiS3Enable wasn't set
in the current OvmfXen implementation. Just wonder how xl passes the S3
support bit to OvmfXen.

Thanks,

Gary Lin

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.

Signed-off-by: Gary Lin <glin@suse.com>
---
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>
I don't think QemuFwCfgLib.h is needed, can you remove it?
Sure, will remove it from v2.

+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/ResourcePublicationLib.h>
#include <Guid/MemoryTypeInformation.h>
#include <Ppi/MasterBootMode.h>
@@ -433,6 +437,12 @@ InitializeXenPlatform (
CpuDeadLoop ();
}

+ if (QemuFwCfgS3Enabled ()) {
This test needs a comment. QEMU's fwcfg isn't supposed to be available,
unless one try to use the Direct Kernel Boot functionality.

+ 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
Same here, QemuFwCfgLib doesn't seems to be needed or used.
Will remove it from v2.

Thanks,

Gary Lin






Anthony PERARD
 

On Wed, Jul 21, 2021 at 02:56:46PM +0800, Gary Lin wrote:
BTW, it seems to me that QEMU fwcfg is only used for Xen Direct Kernel
Boot. However, per xl.cfg manpage, it's possible to turn on or off S3
support by setting "acpi_s3" in xl.cfg, but PcdAcpiS3Enable wasn't set
in the current OvmfXen implementation. Just wonder how xl passes the S3
support bit to OvmfXen.
It doesn't. "acpi_s3" doesn't have any control over PcdAcpiS3Enable, and
doesn't have any control over QEMU, so QEMU would probably always say S3
is available.

What happen when "acpi_s3" is set or not is:
- the value is stored in xenstore
- hvmloader (which runs before OVMF) read the xenstore value and
adds the appropriate ACPI SSDT table to allow S3.

Cheers,

--
Anthony PERARD