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

["Anthony PERARD" <anthony.perard@citrix.com>]

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