On Wed, 26 Feb 2020 at 23:12, Laszlo Ersek <ler...@redhat.com> wrote:
>
> 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 <ard.biesheu...@linaro.org>
> Cc: Igor Mammedov <imamm...@redhat.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Michael Kinney <michael.d.kin...@intel.com>
> Cc: Philippe Mathieu-Daudé <phi...@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
>
> Notes:
> v2:
>
> - Pick up Ard's Acked-by, which is conditional on approval from Intel
> reviewers on Cc. (I'd like to save Ard the churn of re-acking
> unmodified patches.)
>
> OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 4 +
> OvmfPkg/CpuS3DataDxe/CpuS3Data.c | 91 ++++++++++++++------
> 2 files changed, 70 insertions(+), 25 deletions(-)
>
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index f9679e0c33b3..ceae1d4078c7 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -16,46 +16,50 @@
> #
> ##
>
> [Defines]
> INF_VERSION = 1.29
> BASE_NAME = CpuS3DataDxe
> FILE_GUID = 229B7EFD-DA02-46B9-93F4-E20C009F94E9
> MODULE_TYPE = DXE_DRIVER
> VERSION_STRING = 1.0
> ENTRY_POINT = CpuS3DataInitialize
>
> # The following information is for reference only and not required by the
> build
> # tools.
> #
> # VALID_ARCHITECTURES = IA32 X64
>
> [Sources]
> CpuS3Data.c
>
> [Packages]
> MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> UefiCpuPkg/UefiCpuPkg.dec
>
> [LibraryClasses]
> BaseLib
> BaseMemoryLib
> DebugLib
> + IoLib
> MemoryAllocationLib
> MtrrLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
>
> [Guids]
> gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event
>
> [Protocols]
> gEfiMpServiceProtocolGuid ## CONSUMES
>
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ##
> CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ##
> CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
> CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ##
> PRODUCES
> + gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ##
> CONSUMES
>
> [Depex]
> gEfiMpServiceProtocolGuid
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> index 8bb9807cd501..bac7285aa2f3 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -4,51 +4,55 @@ ACPI CPU Data initialization module
> This module initializes the ACPI_CPU_DATA structure and registers the address
> of this structure in the PcdCpuS3DataAddress PCD. This is a generic/simple
> version of this module. It does not provide a machine check handler or CPU
> register initialization tables for ACPI S3 resume. It also only supports the
> number of CPUs reported by the MP Services Protocol, so this module does not
> support hot plug CPUs. This module can be copied into a CPU specific package
> and customized if these additional features are required.
>
> Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.<BR>
> Copyright (c) 2015 - 2020, Red Hat, Inc.
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
>
> #include <PiDxe.h>
>
> #include <AcpiCpuData.h>
>
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/MtrrLib.h>
> #include <Library/UefiBootServicesTableLib.h>
>
> #include <Protocol/MpService.h>
> #include <Guid/EventGroup.h>
>
> +#include <IndustryStandard/Q35MchIch9.h>
> +#include <IndustryStandard/QemuCpuHotplug.h>
> +
> //
> // Data structure used to allocate ACPI_CPU_DATA and its supporting
> structures
> //
> typedef struct {
> ACPI_CPU_DATA AcpiCpuData;
> MTRR_SETTINGS MtrrTable;
> IA32_DESCRIPTOR GdtrProfile;
> IA32_DESCRIPTOR IdtrProfile;
> } ACPI_CPU_DATA_EX;
>
> /**
> Allocate EfiACPIMemoryNVS memory.
>
> @param[in] Size Size of memory to allocate.
>
> @return Allocated address for output.
>
> **/
> VOID *
> AllocateAcpiNvsMemory (
> IN UINTN Size
> )
> @@ -144,89 +148,101 @@ CpuS3DataOnEndOfDxe (
> to the address that ACPI_CPU_DATA is allocated at.
>
> @param[in] ImageHandle The firmware allocated handle for the EFI image.
> @param[in] SystemTable A pointer to the EFI System Table.
>
> @retval EFI_SUCCESS The entry point is executed successfully.
> @retval EFI_UNSUPPORTED Do not support ACPI S3.
> @retval other Some error occurs when executing this entry point.
>
> **/
> EFI_STATUS
> EFIAPI
> CpuS3DataInitialize (
> IN EFI_HANDLE ImageHandle,
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> EFI_STATUS Status;
> ACPI_CPU_DATA_EX *AcpiCpuDataEx;
> ACPI_CPU_DATA *AcpiCpuData;
> EFI_MP_SERVICES_PROTOCOL *MpServices;
> UINTN NumberOfCpus;
> - UINTN NumberOfEnabledProcessors;
> VOID *Stack;
> UINTN TableSize;
> CPU_REGISTER_TABLE *RegisterTable;
> UINTN Index;
> EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
> UINTN GdtSize;
> UINTN IdtSize;
> VOID *Gdt;
> VOID *Idt;
> EFI_EVENT Event;
> ACPI_CPU_DATA *OldAcpiCpuData;
> + BOOLEAN FetchPossibleApicIds;
>
> if (!PcdGetBool (PcdAcpiS3Enable)) {
> return EFI_UNSUPPORTED;
> }
>
> //
> // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> structure
> //
> OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
>
> AcpiCpuDataEx = AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX));
> ASSERT (AcpiCpuDataEx != NULL);
> 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;
>
> //
> // Initialize ACPI_CPU_DATA fields
> //
> AcpiCpuData->StackSize = PcdGet32 (PcdCpuApStackSize);
> AcpiCpuData->ApMachineCheckHandlerBase = 0;
> AcpiCpuData->ApMachineCheckHandlerSize = 0;
> AcpiCpuData->GdtrProfile =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->GdtrProfile;
> AcpiCpuData->IdtrProfile =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->IdtrProfile;
> AcpiCpuData->MtrrTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
>
> //
> // Allocate stack space for all CPUs.
> // Use ACPI NVS memory type because this data will be directly used by APs
> // in S3 resume phase in long mode. Also during S3 resume, the stack buffer
> // will only be used as scratch space. i.e. we won't read anything from it
> // before we write to it, in PiSmmCpuDxeSmm.
> //
> Stack = AllocateAcpiNvsMemory (NumberOfCpus * AcpiCpuData->StackSize);
> ASSERT (Stack != NULL);
> AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
> @@ -244,58 +260,83 @@ CpuS3DataInitialize (
> IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> Gdt = AllocateZeroPages (GdtSize + IdtSize);
> ASSERT (Gdt != NULL);
> Idt = (VOID *)((UINTN)Gdt + GdtSize);
> CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
> CopyMem (Idt, (VOID *)AcpiCpuDataEx->IdtrProfile.Base, IdtSize);
> AcpiCpuDataEx->GdtrProfile.Base = (UINTN)Gdt;
> AcpiCpuDataEx->IdtrProfile.Base = (UINTN)Idt;
>
> if (OldAcpiCpuData != NULL) {
> AcpiCpuData->RegisterTable = OldAcpiCpuData->RegisterTable;
> AcpiCpuData->PreSmmInitRegisterTable =
> OldAcpiCpuData->PreSmmInitRegisterTable;
> AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation;
> CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof
> (CPU_STATUS_INFORMATION));
> } else {
> //
> // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> //
> TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> 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);
> + UINT32 InitialApicId;
>
> - RegisterTable[Index].InitialApicId =
> (UINT32)ProcessorInfoBuffer.ProcessorId;
> + 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;
> }
> AcpiCpuData->RegisterTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> AcpiCpuData->PreSmmInitRegisterTable =
> (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> }
>
> //
> // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> structure
> //
> Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
> ASSERT_EFI_ERROR (Status);
>
> //
> // Register EFI_END_OF_DXE_EVENT_GROUP_GUID event.
> // The notification function allocates StartupVector and saves MTRRs for
> ACPI_CPU_DATA
> //
> Status = gBS->CreateEventEx (
> EVT_NOTIFY_SIGNAL,
> TPL_CALLBACK,
> CpuS3DataOnEndOfDxe,
> --
> 2.19.1.3.g30247aa5d201
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55210): https://edk2.groups.io/g/devel/message/55210
Mute This Topic: https://groups.io/mt/71575192/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
