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

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

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
> 
> 
> 
> 
> 
>