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 <g...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77949): https://edk2.groups.io/g/devel/message/77949
Mute This Topic: https://groups.io/mt/84061313/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
