From 1158fc8e2c7b30df8ab5f766ca64300b3b2de7e9 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 26 Feb 2020 23:11:56 +0100 Subject: OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During normal boot, CpuS3DataDxe allocates - an empty CPU_REGISTER_TABLE entry in the "ACPI_CPU_DATA.PreSmmInitRegisterTable" array, and - an empty CPU_REGISTER_TABLE entry in the "ACPI_CPU_DATA.RegisterTable" array, for every CPU whose APIC ID CpuS3DataDxe can learn. Currently EFI_MP_SERVICES_PROTOCOL is used for both determining the number of CPUs -- the protocol reports the present-at-boot CPU count --, and for retrieving the APIC IDs of those CPUs. Consequently, if a CPU is hot-plugged at OS runtime, then S3 resume breaks. That's because PiSmmCpuDxeSmm will not find the hot-added CPU's APIC ID associated with any CPU_REGISTER_TABLE object, in the SMRAM copies of either of the "RegisterTable" and "PreSmmInitRegisterTable" arrays. The failure to match the hot-added CPU's APIC ID trips the ASSERT() in SetRegister() [UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c]. If "PcdQ35SmramAtDefaultSmbase" is TRUE, then: - prepare CPU_REGISTER_TABLE objects for all possible CPUs, not just the present-at-boot CPUs (PlatformPei stored the possible CPU count to "PcdCpuMaxLogicalProcessorNumber"); - use QEMU_CPUHP_CMD_GET_ARCH_ID for filling in the "InitialApicId" fields of the CPU_REGISTER_TABLE objects. This provides full APIC ID coverage for PiSmmCpuDxeSmm during S3 resume, accommodating CPUs hot-added at OS runtime. This patch is best reviewed with $ git show -b Cc: Ard Biesheuvel Cc: Igor Mammedov Cc: Jiewen Yao Cc: Jordan Justen Cc: Michael Kinney Cc: Philippe Mathieu-Daudé Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512 Signed-off-by: Laszlo Ersek Message-Id: <20200226221156.29589-17-lersek@redhat.com> Reviewed-by: Ard Biesheuvel Tested-by: Boris Ostrovsky --- OvmfPkg/CpuS3DataDxe/CpuS3Data.c | 93 +++++++++++++++++++++++++---------- OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 4 ++ 2 files changed, 71 insertions(+), 26 deletions(-) diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c index 8bb9807cd5..bac7285aa2 100644 --- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c +++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include #include #include +#include #include #include #include @@ -30,6 +31,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include #include +#include +#include + // // Data structure used to allocate ACPI_CPU_DATA and its supporting structures // @@ -163,7 +167,6 @@ CpuS3DataInitialize ( ACPI_CPU_DATA *AcpiCpuData; EFI_MP_SERVICES_PROTOCOL *MpServices; UINTN NumberOfCpus; - UINTN NumberOfEnabledProcessors; VOID *Stack; UINTN TableSize; CPU_REGISTER_TABLE *RegisterTable; @@ -175,6 +178,7 @@ CpuS3DataInitialize ( VOID *Idt; EFI_EVENT Event; ACPI_CPU_DATA *OldAcpiCpuData; + BOOLEAN FetchPossibleApicIds; if (!PcdGetBool (PcdAcpiS3Enable)) { return EFI_UNSUPPORTED; @@ -190,24 +194,36 @@ CpuS3DataInitialize ( AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData; // - // Get MP Services Protocol + // The "SMRAM at default SMBASE" feature guarantees that + // QEMU_CPUHP_CMD_GET_ARCH_ID too is available. // - Status = gBS->LocateProtocol ( - &gEfiMpServiceProtocolGuid, - NULL, - (VOID **)&MpServices - ); - ASSERT_EFI_ERROR (Status); + FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase); - // - // Get the number of CPUs - // - Status = MpServices->GetNumberOfProcessors ( - MpServices, - &NumberOfCpus, - &NumberOfEnabledProcessors - ); - ASSERT_EFI_ERROR (Status); + if (FetchPossibleApicIds) { + NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber); + } else { + UINTN NumberOfEnabledProcessors; + + // + // Get MP Services Protocol + // + Status = gBS->LocateProtocol ( + &gEfiMpServiceProtocolGuid, + NULL, + (VOID **)&MpServices + ); + ASSERT_EFI_ERROR (Status); + + // + // Get the number of CPUs + // + Status = MpServices->GetNumberOfProcessors ( + MpServices, + &NumberOfCpus, + &NumberOfEnabledProcessors + ); + ASSERT_EFI_ERROR (Status); + } AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus; // @@ -263,20 +279,45 @@ CpuS3DataInitialize ( RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize); ASSERT (RegisterTable != NULL); + if (FetchPossibleApicIds) { + // + // Write a valid selector so that other hotplug registers can be + // accessed. + // + IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0); + // + // We'll be fetching the APIC IDs. + // + IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD, + QEMU_CPUHP_CMD_GET_ARCH_ID); + } for (Index = 0; Index < NumberOfCpus; Index++) { - Status = MpServices->GetProcessorInfo ( - MpServices, - Index, - &ProcessorInfoBuffer - ); - ASSERT_EFI_ERROR (Status); - - RegisterTable[Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId; + UINT32 InitialApicId; + + if (FetchPossibleApicIds) { + IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, + (UINT32)Index); + InitialApicId = IoRead32 ( + ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA); + } else { + Status = MpServices->GetProcessorInfo ( + MpServices, + Index, + &ProcessorInfoBuffer + ); + ASSERT_EFI_ERROR (Status); + InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId; + } + + DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__, + (UINT64)Index, InitialApicId)); + + RegisterTable[Index].InitialApicId = InitialApicId; RegisterTable[Index].TableLength = 0; RegisterTable[Index].AllocatedSize = 0; RegisterTable[Index].RegisterTableEntry = 0; - RegisterTable[NumberOfCpus + Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId; + RegisterTable[NumberOfCpus + Index].InitialApicId = InitialApicId; RegisterTable[NumberOfCpus + Index].TableLength = 0; RegisterTable[NumberOfCpus + Index].AllocatedSize = 0; RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0; diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf index f9679e0c33..ceae1d4078 100644 --- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf +++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf @@ -35,12 +35,14 @@ [Packages] MdeModulePkg/MdeModulePkg.dec MdePkg/MdePkg.dec + OvmfPkg/OvmfPkg.dec UefiCpuPkg/UefiCpuPkg.dec [LibraryClasses] BaseLib BaseMemoryLib DebugLib + IoLib MemoryAllocationLib MtrrLib UefiBootServicesTableLib @@ -55,7 +57,9 @@ [Pcd] gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## CONSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CONSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CONSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ## PRODUCES + gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## CONSUMES [Depex] gEfiMpServiceProtocolGuid -- cgit v1.2.3