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


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





Join devel@edk2.groups.io to automatically receive all group messages.