From: "Ard Biesheuvel" <ardb@kernel.org>
To: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
Cc: devel@edk2.groups.io, Ard Biesheuvel <ardb+tianocore@kernel.org>,
Jiewen Yao <jiewen.yao@intel.com>,
Jordan Justen <jordan.l.justen@intel.com>,
Gerd Hoffmann <kraxel@redhat.com>,
Anthony Perard <anthony.perard@citrix.com>,
Julien Grall <julien@xen.org>,
xen-devel@lists.xenproject.org
Subject: Re: [edk2-devel] [PATCH] OvmfPkg/OvmfXen: Fix S3
Date: Wed, 6 Sep 2023 11:24:48 +0200 [thread overview]
Message-ID: <CAMj1kXEcgbxMbh=B43KqKikVZMmz75f6Pd3rZB9fFF3eszghHA@mail.gmail.com> (raw)
In-Reply-To: <c79e568e0d1c85bef9a2efc4c854de8f80ea1487.1689244868.git.xenia.ragiadakou@amd.com>
On Thu, 13 Jul 2023 at 12:48, Xenia Ragiadakou <xenia.ragiadakou@amd.com> wrote:
>
> Currently, resuming an S3 suspended guest results in the following
> assertion failure:
> ASSERT MdePkg/Library/PeiResourcePublicationLib/PeiResourcePublicationLib.c(41): MemoryLength > 0
> This happens because some parts of the S3 suspend and resume paths
> are missing in order for S3 to work. For instance, the variables
> mS3AcpiReservedMemoryBase and mS3AcpiReservedMemoryBase are not
> initialized, regions that are used on S3 resume are either missing
> or not marked as ACPI NVS memory and can be corrupted by the OS.
> This patch adds the missing parts based heavily on the existing S3
> implementation of other virtual platforms.
>
> For S3 support, the provision of fw_cfg is required in order for
> suspend states to be retrieved.
>
> Another issue noticed is that when CalibrateLapicTimer() is called
> on S3 resume path, the shared info page is remapped to a different
> guest physical address. This remapping happens under guest's feet,
> so any subsequent attempt of the guest to access the shared info
> page results in nested page faults. This patch removes any local
> APIC timer initializion and calibration from S3 resume path.
>
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
Anyone care to review this?
> ---
> OvmfPkg/XenPlatformPei/Fv.c | 2 +-
> OvmfPkg/XenPlatformPei/MemDetect.c | 60 ++++++++++++++++++++++-
> OvmfPkg/XenPlatformPei/Platform.c | 11 ++++-
> OvmfPkg/XenPlatformPei/Platform.h | 2 +
> OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 7 +++
> 5 files changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/XenPlatformPei/Fv.c b/OvmfPkg/XenPlatformPei/Fv.c
> index 871a2c1c5b..37ecb3cfee 100644
> --- a/OvmfPkg/XenPlatformPei/Fv.c
> +++ b/OvmfPkg/XenPlatformPei/Fv.c
> @@ -37,7 +37,7 @@ PeiFvInitialization (
> BuildMemoryAllocationHob (
> PcdGet32 (PcdOvmfPeiMemFvBase),
> PcdGet32 (PcdOvmfPeiMemFvSize),
> - EfiBootServicesData
> + mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> );
>
> //
> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c
> index e552e7a55e..1724a4988f 100644
> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> @@ -283,6 +283,19 @@ PublishPeiMemory (
>
> LowerMemorySize = GetSystemMemorySizeBelow4gb ();
>
> + //
> + // If S3 is supported, then the S3 permanent PEI memory is placed next,
> + // downwards. Its size is primarily dictated by CpuMpPei. The formula below
> + // is an approximation.
> + //
> + if (mS3Supported) {
> + mS3AcpiReservedMemorySize = SIZE_512KB +
> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber) *
> + PcdGet32 (PcdCpuApStackSize);
> + mS3AcpiReservedMemoryBase = LowerMemorySize - mS3AcpiReservedMemorySize;
> + LowerMemorySize = mS3AcpiReservedMemoryBase;
> + }
> +
> if (mBootMode == BOOT_ON_S3_RESUME) {
> MemoryBase = mS3AcpiReservedMemoryBase;
> MemorySize = mS3AcpiReservedMemorySize;
> @@ -328,6 +341,51 @@ InitializeRamRegions (
> {
> XenPublishRamRegions ();
>
> + if (mS3Supported && (mBootMode != BOOT_ON_S3_RESUME)) {
> + //
> + // This is the memory range that will be used for PEI on S3 resume
> + //
> + BuildMemoryAllocationHob (
> + mS3AcpiReservedMemoryBase,
> + mS3AcpiReservedMemorySize,
> + EfiACPIMemoryNVS
> + );
> +
> + //
> + // Cover the initial RAM area used as stack and temporary PEI heap.
> + //
> + // This is reserved as ACPI NVS so it can be used on S3 resume.
> + //
> + BuildMemoryAllocationHob (
> + PcdGet32 (PcdOvmfSecPeiTempRamBase),
> + PcdGet32 (PcdOvmfSecPeiTempRamSize),
> + EfiACPIMemoryNVS
> + );
> +
> + //
> + // SEC stores its table of GUIDed section handlers here.
> + //
> + BuildMemoryAllocationHob (
> + PcdGet64 (PcdGuidedExtractHandlerTableAddress),
> + PcdGet32 (PcdGuidedExtractHandlerTableSize),
> + EfiACPIMemoryNVS
> + );
> +
> + #ifdef MDE_CPU_X64
> + //
> + // Reserve the initial page tables built by the reset vector code.
> + //
> + // Since this memory range will be used by the Reset Vector on S3
> + // resume, it must be reserved as ACPI NVS.
> + //
> + BuildMemoryAllocationHob (
> + (EFI_PHYSICAL_ADDRESS)(UINTN)PcdGet32 (PcdOvmfSecPageTablesBase),
> + (UINT64)(UINTN)PcdGet32 (PcdOvmfSecPageTablesSize),
> + EfiACPIMemoryNVS
> + );
> + #endif
> + }
> +
> if (mBootMode != BOOT_ON_S3_RESUME) {
> //
> // Reserve the lock box storage area
> @@ -346,7 +404,7 @@ InitializeRamRegions (
> BuildMemoryAllocationHob (
> (EFI_PHYSICAL_ADDRESS)(UINTN)PcdGet32 (PcdOvmfLockBoxStorageBase),
> (UINT64)(UINTN)PcdGet32 (PcdOvmfLockBoxStorageSize),
> - EfiBootServicesData
> + mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> );
> }
> }
> diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
> index c3fdf3d0b8..1b074cff33 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.c
> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> @@ -60,6 +60,8 @@ UINT16 mHostBridgeDevId;
>
> EFI_BOOT_MODE mBootMode = BOOT_WITH_FULL_CONFIGURATION;
>
> +BOOLEAN mS3Supported = FALSE;
> +
> VOID
> AddIoMemoryBaseSizeHob (
> EFI_PHYSICAL_ADDRESS MemoryBase,
> @@ -350,6 +352,11 @@ BootModeInitialization (
>
> if (CmosRead8 (0xF) == 0xFE) {
> mBootMode = BOOT_ON_S3_RESUME;
> + if (!mS3Supported) {
> + DEBUG ((DEBUG_ERROR, "ERROR: S3 not supported\n"));
> + ASSERT (FALSE);
> + CpuDeadLoop ();
> + }
> }
>
> CmosWrite8 (0xF, 0x00);
> @@ -463,6 +470,7 @@ InitializeXenPlatform (
> //
> if (QemuFwCfgS3Enabled ()) {
> DEBUG ((DEBUG_INFO, "S3 support was detected on QEMU\n"));
> + mS3Supported = TRUE;
> Status = PcdSetBoolS (PcdAcpiS3Enable, TRUE);
> ASSERT_EFI_ERROR (Status);
> }
> @@ -481,9 +489,8 @@ InitializeXenPlatform (
>
> InitializeRamRegions ();
>
> - CalibrateLapicTimer ();
> -
> if (mBootMode != BOOT_ON_S3_RESUME) {
> + CalibrateLapicTimer ();
> ReserveEmuVariableNvStore ();
> PeiFvInitialization ();
> MemMapInitialization ();
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index 7b4de128e7..fda66747cc 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -139,4 +139,6 @@ extern UINT8 mPhysMemAddressWidth;
>
> extern UINT16 mHostBridgeDevId;
>
> +extern BOOLEAN mS3Supported;
> +
> #endif // _PLATFORM_PEI_H_INCLUDED_
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 20c27ff34b..a359cf60ca 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -69,9 +69,13 @@
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
> gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize
> @@ -80,6 +84,7 @@
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
> gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
> + gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
> @@ -89,6 +94,8 @@
> gEfiMdePkgTokenSpaceGuid.PcdFSBClock
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
> gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
>
> gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtr
> gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtrSize
> --
> 2.34.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108319): https://edk2.groups.io/g/devel/message/108319
Mute This Topic: https://groups.io/mt/100121295/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-09-06 9:25 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-13 10:47 [PATCH] OvmfPkg/OvmfXen: Fix S3 Xenia Ragiadakou
2023-09-06 9:24 ` Ard Biesheuvel [this message]
2023-09-11 15:06 ` [edk2-devel] " Anthony PERARD via groups.io
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMj1kXEcgbxMbh=B43KqKikVZMmz75f6Pd3rZB9fFF3eszghHA@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox