From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.14792.1583158619339768305 for ; Mon, 02 Mar 2020 06:16:59 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=CrlcozNW; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id r7so12808448wro.2 for ; Mon, 02 Mar 2020 06:16:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=ldTJMD5nMVPRoHW5z07OYtnE0bez8TX0ZsrrEISC01g=; b=CrlcozNWr/koARl6YwwssHwjjhoiNmEesw+NO6zeNPg7W+ulk4siEF7OiITjZVgWqn LrwzWJ8k5+/NFfvAkK+03lpLB18z9eWwhyBc5GHd+kqHktopVNK8fTMhdvGzoITltkyb CgNxDl0nNoV8MveIonvML7xlWjiM/2YOhIO4AW46u3pM8SXPZogTMAqNLs42FNX5QwA0 mgInAWLWFDOo+DIrS1m1Q4auUjnervviuT+sqjW+wpwCwp8xOq8Zw1ugO8RLhgHstuyZ uhlCUusI6f4V6o4iwVx/7eXCTQICo7Bmpw3qWj+RUhcpTjpnmpm6X1aPV/K4ODKcm2IL MIUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=ldTJMD5nMVPRoHW5z07OYtnE0bez8TX0ZsrrEISC01g=; b=qunF6fYZiLkKIYLGbCPpOTC9cCDVrZss00amIpXo9pxeuo3PzUVSEnEzB2ybkLBsiH WmYSf8vGDTMS9o/+sHw6cjp+Av/jFHzyiUjwU5wROdDBCzYqPFZUNyfcAl+nU2R2V9GY +G1o1GaqVT7eGNGHmrSp8dRX0+ItvlMzBFv4IkUVKXKGW8iIQokOgqtNUTUb0mKGuSG9 afRIIOpkmurCQRajB8isC846LIxmPVJ1zfKPkh651XZkqMpVUPwBlOvSjmZR5wlwFbkn HWtRkAD69aDdTghgsdpWFG8cBaSTaeY3sw6JDn4XCsUTNBeORpxU5Gud8lWSZAPqcpU7 9UxA== X-Gm-Message-State: APjAAAXBQndGIplcZODaCOS77E0m3BO0Eu1li+EAxxoSxqOpaiq3kB+D 1cH24bYTpCACXTkCepVYmJm/4071x94WQ2xqy8CDiw== X-Google-Smtp-Source: APXvYqwJZVf6eHh6PpSkiO5Cyo6ufL0cy3jqLKXvYLvVD+hLeRcmSicNGeirRuBWZUxZuPpV2YqjsGSC4VECFHcyYEo= X-Received: by 2002:a05:6000:110b:: with SMTP id z11mr23140073wrw.252.1583158617823; Mon, 02 Mar 2020 06:16:57 -0800 (PST) MIME-Version: 1.0 References: <20200226221156.29589-1-lersek@redhat.com> <20200226221156.29589-17-lersek@redhat.com> In-Reply-To: <20200226221156.29589-17-lersek@redhat.com> From: "Ard Biesheuvel" Date: Mon, 2 Mar 2020 15:16:46 +0100 Message-ID: Subject: Re: [PATCH v2 16/16] OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug To: Laszlo Ersek Cc: edk2-devel-groups-io , Igor Mammedov , Jiewen Yao , Jordan Justen , Michael Kinney , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 26 Feb 2020 at 23:12, Laszlo Ersek 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 numbe= r > 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 copie= s > of either of the "RegisterTable" and "PreSmmInitRegisterTable" arrays. Th= e > 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" field= s > 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=C3=A9 > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1512 > Signed-off-by: Laszlo Ersek > Acked-by: Ard Biesheuvel Reviewed-by: Ard Biesheuvel > --- > > 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 =3D 1.29 > BASE_NAME =3D CpuS3DataDxe > FILE_GUID =3D 229B7EFD-DA02-46B9-93F4-E20C009F94E= 9 > MODULE_TYPE =3D DXE_DRIVER > VERSION_STRING =3D 1.0 > ENTRY_POINT =3D CpuS3DataInitialize > > # The following information is for reference only and not required by th= e build > # tools. > # > # VALID_ARCHITECTURES =3D 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 ## C= ONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## C= ONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## C= ONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ## P= RODUCES > + gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## C= ONSUMES > > [Depex] > gEfiMpServiceProtocolGuid > diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS= 3Data.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 ad= dress > of this structure in the PcdCpuS3DataAddress PCD. This is a generic/sim= ple > version of this module. It does not provide a machine check handler or = CPU > register initialization tables for ACPI S3 resume. It also only support= s 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 pa= ckage > and customized if these additional features are required. > > Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.
> Copyright (c) 2015 - 2020, Red Hat, Inc. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > #include > > #include > > #include > #include > #include > +#include > #include > #include > #include > > #include > #include > > +#include > +#include > + > // > // Data structure used to allocate ACPI_CPU_DATA and its supporting stru= ctures > // > 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 ima= ge. > @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 p= oint. > > **/ > 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 st= ructure > // > OldAcpiCpuData =3D (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAdd= ress); > > AcpiCpuDataEx =3D AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX)); > ASSERT (AcpiCpuDataEx !=3D NULL); > AcpiCpuData =3D &AcpiCpuDataEx->AcpiCpuData; > > // > - // Get MP Services Protocol > + // The "SMRAM at default SMBASE" feature guarantees that > + // QEMU_CPUHP_CMD_GET_ARCH_ID too is available. > // > - Status =3D gBS->LocateProtocol ( > - &gEfiMpServiceProtocolGuid, > - NULL, > - (VOID **)&MpServices > - ); > - ASSERT_EFI_ERROR (Status); > + FetchPossibleApicIds =3D PcdGetBool (PcdQ35SmramAtDefaultSmbase); > > - // > - // Get the number of CPUs > - // > - Status =3D MpServices->GetNumberOfProcessors ( > - MpServices, > - &NumberOfCpus, > - &NumberOfEnabledProcessors > - ); > - ASSERT_EFI_ERROR (Status); > + if (FetchPossibleApicIds) { > + NumberOfCpus =3D PcdGet32 (PcdCpuMaxLogicalProcessorNumber); > + } else { > + UINTN NumberOfEnabledProcessors; > + > + // > + // Get MP Services Protocol > + // > + Status =3D gBS->LocateProtocol ( > + &gEfiMpServiceProtocolGuid, > + NULL, > + (VOID **)&MpServices > + ); > + ASSERT_EFI_ERROR (Status); > + > + // > + // Get the number of CPUs > + // > + Status =3D MpServices->GetNumberOfProcessors ( > + MpServices, > + &NumberOfCpus, > + &NumberOfEnabledProcessors > + ); > + ASSERT_EFI_ERROR (Status); > + } > AcpiCpuData->NumberOfCpus =3D (UINT32)NumberOfCpus; > > // > // Initialize ACPI_CPU_DATA fields > // > AcpiCpuData->StackSize =3D PcdGet32 (PcdCpuApStackSize= ); > AcpiCpuData->ApMachineCheckHandlerBase =3D 0; > AcpiCpuData->ApMachineCheckHandlerSize =3D 0; > AcpiCpuData->GdtrProfile =3D (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDat= aEx->GdtrProfile; > AcpiCpuData->IdtrProfile =3D (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDat= aEx->IdtrProfile; > AcpiCpuData->MtrrTable =3D (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDat= aEx->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 b= uffer > // will only be used as scratch space. i.e. we won't read anything fro= m it > // before we write to it, in PiSmmCpuDxeSmm. > // > Stack =3D AllocateAcpiNvsMemory (NumberOfCpus * AcpiCpuData->StackSize= ); > ASSERT (Stack !=3D NULL); > AcpiCpuData->StackAddress =3D (EFI_PHYSICAL_ADDRESS)(UINTN)Stack; > @@ -244,58 +260,83 @@ CpuS3DataInitialize ( > IdtSize =3D AcpiCpuDataEx->IdtrProfile.Limit + 1; > Gdt =3D AllocateZeroPages (GdtSize + IdtSize); > ASSERT (Gdt !=3D NULL); > Idt =3D (VOID *)((UINTN)Gdt + GdtSize); > CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize); > CopyMem (Idt, (VOID *)AcpiCpuDataEx->IdtrProfile.Base, IdtSize); > AcpiCpuDataEx->GdtrProfile.Base =3D (UINTN)Gdt; > AcpiCpuDataEx->IdtrProfile.Base =3D (UINTN)Idt; > > if (OldAcpiCpuData !=3D NULL) { > AcpiCpuData->RegisterTable =3D OldAcpiCpuData->RegisterTab= le; > AcpiCpuData->PreSmmInitRegisterTable =3D OldAcpiCpuData->PreSmmInitR= egisterTable; > AcpiCpuData->ApLocation =3D OldAcpiCpuData->ApLocation; > CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof= (CPU_STATUS_INFORMATION)); > } else { > // > // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTab= le for all CPUs > // > TableSize =3D 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); > RegisterTable =3D (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize= ); > ASSERT (RegisterTable !=3D 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 =3D 0; Index < NumberOfCpus; Index++) { > - Status =3D MpServices->GetProcessorInfo ( > - MpServices, > - Index, > - &ProcessorInfoBuffer > - ); > - ASSERT_EFI_ERROR (Status); > + UINT32 InitialApicId; > > - RegisterTable[Index].InitialApicId =3D (UINT32)ProcessorInfoB= uffer.ProcessorId; > + if (FetchPossibleApicIds) { > + IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, > + (UINT32)Index); > + InitialApicId =3D IoRead32 ( > + ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA= ); > + } else { > + Status =3D MpServices->GetProcessorInfo ( > + MpServices, > + Index, > + &ProcessorInfoBuffer > + ); > + ASSERT_EFI_ERROR (Status); > + InitialApicId =3D (UINT32)ProcessorInfoBuffer.ProcessorId; > + } > + > + DEBUG ((DEBUG_VERBOSE, "%a: Index=3D%05Lu ApicId=3D0x%08x\n", __FU= NCTION__, > + (UINT64)Index, InitialApicId)); > + > + RegisterTable[Index].InitialApicId =3D InitialApicId; > RegisterTable[Index].TableLength =3D 0; > RegisterTable[Index].AllocatedSize =3D 0; > RegisterTable[Index].RegisterTableEntry =3D 0; > > - RegisterTable[NumberOfCpus + Index].InitialApicId =3D (UINT32= )ProcessorInfoBuffer.ProcessorId; > + RegisterTable[NumberOfCpus + Index].InitialApicId =3D Initial= ApicId; > RegisterTable[NumberOfCpus + Index].TableLength =3D 0; > RegisterTable[NumberOfCpus + Index].AllocatedSize =3D 0; > RegisterTable[NumberOfCpus + Index].RegisterTableEntry =3D 0; > } > AcpiCpuData->RegisterTable =3D (EFI_PHYSICAL_ADDRESS)(UINT= N)RegisterTable; > AcpiCpuData->PreSmmInitRegisterTable =3D (EFI_PHYSICAL_ADDRESS)(UINT= N)(RegisterTable + NumberOfCpus); > } > > // > // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA st= ructure > // > Status =3D 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 f= or ACPI_CPU_DATA > // > Status =3D gBS->CreateEventEx ( > EVT_NOTIFY_SIGNAL, > TPL_CALLBACK, > CpuS3DataOnEndOfDxe, > -- > 2.19.1.3.g30247aa5d201 >