Re: [edk2-devel] [PATCH v2 1/5] OvmfPkg/OvmfXen: set PcdAcpiS3Enable at initialization

["Lin, Gary (HPS OE-Linux)" <gary.lin@hpe.com>]

On Mon, Aug 16, 2021 at 09:07:01AM +0200, Ard Biesheuvel wrote:
> Does this series have a cover letter?
> 
Sorry, I forgot to add Cc tags in the cover letter.
https://edk2.groups.io/g/devel/topic/84857762#79245

Gary Lin

> On Fri, 13 Aug 2021 at 08:13, Lin, Gary (HPS OE-Linux) <gary.lin@hpe.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.
> >
> > This issue mainly affects "HVM Direct Kernel Boot". If "acpi_s3" is
> > set as "True" in xl.cfg, then the S3 Support bit is set and passed
> > with fw_cfg.
> >
> > v2:
> >   - Amend the description and address "HVM Direct Kernel Boot"
> >   - Add the comment for the conditional test of QemuFwCfgS3Enabled()
> >   - Remove unused QemuFwCfgLib
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > 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>
> > Cc: Joey Li <jlee@suse.com>
> > Signed-off-by: Gary Lin <gary.lin@hpe.com>
> > ---
> >  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  2 ++
> >  OvmfPkg/XenPlatformPei/Platform.c         | 13 +++++++++++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> > index 597cb6fcd7ff..20c27ff34b6c 100644
> > --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> > +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> > @@ -57,6 +57,7 @@ [LibraryClasses]
> >    ResourcePublicationLib
> >
> >    PeiServicesLib
> >
> >    PeimEntryPoint
> >
> > +  QemuFwCfgS3Lib
> >
> >    MtrrLib
> >
> >    MemEncryptSevLib
> >
> >    PcdLib
> >
> > @@ -79,6 +80,7 @@ [Pcd]
> >    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
> >
> >    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
> >
> >    gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
> >
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
> >
> > diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
> > index a811e72ee301..e60478fdb493 100644
> > --- a/OvmfPkg/XenPlatformPei/Platform.c
> > +++ b/OvmfPkg/XenPlatformPei/Platform.c
> > @@ -26,6 +26,7 @@
> >  #include <Library/PciLib.h>
> >
> >  #include <Library/PeimEntryPoint.h>
> >
> >  #include <Library/PeiServicesLib.h>
> >
> > +#include <Library/QemuFwCfgS3Lib.h>
> >
> >  #include <Library/ResourcePublicationLib.h>
> >
> >  #include <Guid/MemoryTypeInformation.h>
> >
> >  #include <Ppi/MasterBootMode.h>
> >
> > @@ -423,6 +424,8 @@ InitializeXenPlatform (
> >    IN CONST EFI_PEI_SERVICES     **PeiServices
> >
> >    )
> >
> >  {
> >
> > +  EFI_STATUS    Status;
> >
> > +
> >
> >    DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));
> >
> >
> >
> >    DebugDumpCmos ();
> >
> > @@ -433,6 +436,16 @@ InitializeXenPlatform (
> >      CpuDeadLoop ();
> >
> >    }
> >
> >
> >
> > +  //
> >
> > +  // This S3 conditional test is mainly for HVM Direct Kernel Boot since
> >
> > +  // QEMU fwcfg isn't really supported other than that.
> >
> > +  //
> >
> > +  if (QemuFwCfgS3Enabled ()) {
> >
> > +    DEBUG ((DEBUG_INFO, "S3 support was detected on QEMU\n"));
> >
> > +    Status = PcdSetBoolS (PcdAcpiS3Enable, TRUE);
> >
> > +    ASSERT_EFI_ERROR (Status);
> >
> > +  }
> >
> > +
> >
> >    XenConnect ();
> >
> >
> >
> >    BootModeInitialization ();
> >
> > --
> > 2.31.1
> >
> >
> >
> > 
> >
> >