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

["Gary Lin" <glin@suse.com>]

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