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

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

[Gary Lin]

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.

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.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
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>
Signed-off-by: Gary Lin <glin@suse.com>
---
 OvmfPkg/XenPlatformPei/Platform.c         | 10 ++++++++++
 OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  3 +++
 2 files changed, 13 insertions(+)

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>
+#include <Library/QemuFwCfgS3Lib.h>
 #include <Library/ResourcePublicationLib.h>
 #include <Guid/MemoryTypeInformation.h>
 #include <Ppi/MasterBootMode.h>
@@ -423,6 +425,8 @@ InitializeXenPlatform (
   IN CONST EFI_PEI_SERVICES     **PeiServices
   )
 {
+  EFI_STATUS    Status;
+
   DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));
 
   DebugDumpCmos ();
@@ -433,6 +437,12 @@ InitializeXenPlatform (
     CpuDeadLoop ();
   }
 
+  if (QemuFwCfgS3Enabled ()) {
+    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
+  QemuFwCfgS3Lib
   MtrrLib
   MemEncryptSevLib
   PcdLib
@@ -79,6 +81,7 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
-- 
2.31.1

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

[Ard Biesheuvel]

On Thu, 8 Jul 2021 at 06:05, Gary Lin <glin@suse.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.
>
> 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.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> 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>
> Signed-off-by: Gary Lin <glin@suse.com>

This needs an ack from the Xen folks.

> ---
>  OvmfPkg/XenPlatformPei/Platform.c         | 10 ++++++++++
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  3 +++
>  2 files changed, 13 insertions(+)
>
> 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>
> +#include <Library/QemuFwCfgS3Lib.h>
>  #include <Library/ResourcePublicationLib.h>
>  #include <Guid/MemoryTypeInformation.h>
>  #include <Ppi/MasterBootMode.h>
> @@ -423,6 +425,8 @@ InitializeXenPlatform (
>    IN CONST EFI_PEI_SERVICES     **PeiServices
>    )
>  {
> +  EFI_STATUS    Status;
> +
>    DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));
>
>    DebugDumpCmos ();
> @@ -433,6 +437,12 @@ InitializeXenPlatform (
>      CpuDeadLoop ();
>    }
>
> +  if (QemuFwCfgS3Enabled ()) {
> +    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
> +  QemuFwCfgS3Lib
>    MtrrLib
>    MemEncryptSevLib
>    PcdLib
> @@ -79,6 +81,7 @@ [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
>    gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
> --
> 2.31.1
>

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

[Anthony PERARD]

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

QemuFwCfgS3Enabled() should always return false as we don't enable
QEMU's fwcfg interface in most case when running a Xen guest. So I don't
expect "PcdAcpiS3Enable" to ever been set via this patch.

Now, maybe we want to set PcdAcpiS3Enable unconditionally but I don't
know if S3 support is going to work as expected with OVMF under Xen, I
never look at that.

What kind of guest are you trying to fix? When does the crash happen?

Thanks,

-- 
Anthony PERARD

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

[Anthony PERARD]

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.

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

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

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

> +#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.

Thanks,

-- 
Anthony PERARD

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

[Ard Biesheuvel]

On Mon, 19 Jul 2021 at 18:07, Anthony PERARD <anthony.perard@citrix.com> 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.
>
> 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).
>

I don't think S3 suspend is the kind of thing that happens to work by
accident :-)


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

I don't have that man page. Could you elaborate?

> > 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?
>
> > +#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.
>
> Thanks,
>
> --
> Anthony PERARD

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

[Gary Lin]

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

Re: [edk2-devel] [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
> 
> 
> 
> 
> 
> 

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

[Anthony PERARD]

On Wed, Jul 21, 2021 at 02:56:46PM +0800, Gary Lin wrote:
> 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.

It doesn't. "acpi_s3" doesn't have any control over PcdAcpiS3Enable, and
doesn't have any control over QEMU, so QEMU would probably always say S3
is available.

What happen when "acpi_s3" is set or not is:
    - the value is stored in xenstore
    - hvmloader (which runs before OVMF) read the xenstore value and
      adds the appropriate ACPI SSDT table to allow S3.

Cheers,

-- 
Anthony PERARD