On 11/25/15 00:47, Michael Kinney wrote: > 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. > > This patch series is in response to the OvmfPkg patch series from > Laszlo Ersek that enables SMM on OVMF. The v4 version of the patch > series from Laszlo includes an OVMF specific CPU module to initialize > the ACPI_CPU_DATA structure. > > This proposed patch series replaces the patches listed below. > > [PATCH v4 27/41] OvmfPkg: > add skeleton QuarkPort/CpuS3DataDxe > > [PATCH v4 28/41] OvmfPkg: > QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.StartupVector > > [PATCH v4 29/41] OvmfPkg: > QuarkPort/CpuS3DataDxe: handle IDT, GDT and MCE in ACPI_CPU_DATA > > [PATCH v4 30/41] OvmfPkg: > QuarkPort/CpuS3DataDxe: handle StackAddress and StackSize > > [PATCH v4 31/41] OvmfPkg: > import CpuConfigLib header from Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg > > [PATCH v4 32/41] OvmfPkg: > QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.NumberOfCpus > > [PATCH v4 33/41] OvmfPkg: > QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.MtrrTable > > [PATCH v4 34/41] OvmfPkg: > QuarkPort/CpuS3DataDxe: handle register tables in ACPI_CPU_DATA > > [PATCH v4 35/41] OvmfPkg: > port CpuS3DataDxe to X64 > > [PATCH v4 36/41] OvmfPkg: > build QuarkPort/CpuS3DataDxe for -D SMM_REQUIRE > > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: "Yao, Jiewen" <jiewen....@intel.com> > Cc: "Fan, Jeff" <jeff....@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael Kinney <michael.d.kin...@intel.com> > --- > UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 271 > +++++++++++++++++++++++++++++++ > UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 64 ++++++++ > UefiCpuPkg/UefiCpuPkg.dsc | 1 + > 3 files changed, 336 insertions(+) > create mode 100644 UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > create mode 100644 UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
(1) The commit message looks great, thank you very much for the references. A small request: please mark, under "port CpuS3DataDxe to X64" (i.e., [PATCH v4 35/41]), that that patch was originally authored by Paolo Bonzini. There's no need to submit a v3 of this series just because of this; it can be fixed up before committing the patch. > > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > new file mode 100644 > index 0000000..7cf923d > --- /dev/null > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > @@ -0,0 +1,271 @@ > +/** @file > +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 - 2015, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2015, Red Hat, Inc. > + > +This program and the accompanying materials > +are licensed and made available under the terms and conditions of the BSD > License > +which accompanies this distribution. The full text of the license may be > found at > +http://opensource.org/licenses/bsd-license.php > + > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include <PiDxe.h> > + > +#include <AcpiCpuData.h> > + > +#include <Library/BaseLib.h> > +#include <Library/BaseMemoryLib.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/DebugLib.h> > +#include <Library/MtrrLib.h> > + > +#include <Protocol/MpService.h> > +#include <Guid/EventGroup.h> > + > +// > +// Data structure used to allocate ACPI_CPU_DATA and its supporting > structures > +// > +typedef struct { > + ACPI_CPU_DATA AcpuCpuData; (2) Please fix the typo in the name of this field: it should be Acp[i]CpuData. Apologies for not noticing it in v1. No need to submit a v3 just because of this; can be fixed up on commit. > + MTRR_SETTINGS MtrrTable; > + IA32_DESCRIPTOR GdtrProfile; > + IA32_DESCRIPTOR IdtrProfile; > +} ACPI_CPU_DATA_EX; > + > +/** > + Allocate EfiACPIMemoryNVS below 4G memory address. > + > + This function allocates EfiACPIMemoryNVS below 4G memory address. > + > + @param[in] Size Size of memory to allocate. > + > + @return Allocated address for output. > + > +**/ > +VOID * > +AllocateAcpiNvsMemoryBelow4G ( > + IN UINTN Size > + ) > +{ > + EFI_PHYSICAL_ADDRESS Address; > + EFI_STATUS Status; > + VOID *Buffer; > + > + Address = BASE_4GB - 1; > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + EFI_SIZE_TO_PAGES (Size), > + &Address > + ); > + if (EFI_ERROR (Status)) { > + return NULL; > + } > + > + Buffer = (VOID *)(UINTN)Address; > + ZeroMem (Buffer, Size); > + > + return Buffer; > +} > + > +/** > + Callback function executed when the EndOfDxe event group is signaled. > + > + We delay saving the MTRR settings until BDS signals EndOfDxe. > + > + @param[in] Event Event whose notification function is being invoked. > + @param[out] Context Pointer to the MTRR_SETTINGS buffer to fill in. > +**/ > +VOID > +EFIAPI > +SaveMtrrsOnEndOfDxe ( > + IN EFI_EVENT Event, > + OUT VOID *Context > + ) > +{ > + DEBUG ((EFI_D_VERBOSE, "%a\n", __FUNCTION__)); > + MtrrGetAllMtrrs (Context); > + > + // > + // Close event, so it will not be invoked again. > + // > + gBS->CloseEvent (Event); > +} > + > +/** > + The entry function of the CpuS3Data driver. > + > + Allocate and initialize all fields of the ACPI_CPU_DATA structure except > the > + MTRR settings. Register an event notification on > gEfiEndOfDxeEventGroupGuid > + to capture the ACPI_CPU_DATA MTRR settings. The PcdCpuS3DataAddress is > set > + to the address that ACPI_CPU_DATA is allocated. (3) I think the last sentence above should go like: [] PcdCpuS3DataAddress is set to the address that ACPI_CPU_DATA is allocated [at]. Can be fixed up at commit time. > + > + @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 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; > + > + // > + // Allocate ACPI NVS memory below 4G memory for use on ACPI S3 resume. > + // > + AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof (ACPI_CPU_DATA_EX)); > + ASSERT (AcpiCpuDataEx != NULL); > + AcpiCpuData = &AcpiCpuDataEx->AcpuCpuData; > + > + // > + // Get MP Services Protocol > + // > + Status = gBS->LocateProtocol ( > + &gEfiMpServiceProtocolGuid, > + NULL, > + (VOID **)&MpServices > + ); > + ASSERT_EFI_ERROR (Status); > + > + // > + // If there are 1 or more APs, then allocate a 4KB reserved page below 1MB > + // (4) I think this comment should be dropped now. The code is correct; the comment is stale. Can be fixed up when committing the patch. > + AcpiCpuData->StartupVector = BASE_1MB - 1; > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiReservedMemoryType, > + 1, > + &AcpiCpuData->StartupVector > + ); (5) The indentation of the function arguments and the closing paren doesn't seem right. Can be fixed up on commit. > + 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 > + // > + Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus * > AcpiCpuData->StackSize); > + ASSERT (Stack != NULL); > + AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack; > + > + // > + // Get the boot processor's GDT and IDT > + // > + AsmReadGdtr (&AcpiCpuDataEx->GdtrProfile); > + AsmReadIdtr (&AcpiCpuDataEx->IdtrProfile); Turns out you have removed the casts for v2 -- great, thanks! > + > + // > + // Allocate GDT and IDT in ACPI NVS and copy current GDT and IDT contents > + // > + GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1; > + IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1; > + Gdt = AllocateAcpiNvsMemoryBelow4G (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; > + > + // > + // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for > all CPUs > + // > + TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); > + RegisterTable = (CPU_REGISTER_TABLE *)AllocateAcpiNvsMemoryBelow4G > (TableSize); > + ASSERT (RegisterTable != NULL); > + for (Index = 0; Index < NumberOfCpus; Index++) { > + Status = MpServices->GetProcessorInfo ( > + MpServices, > + Index, > + &ProcessorInfoBuffer > + ); > + ASSERT_EFI_ERROR (Status); > + > + RegisterTable[Index].InitialApicId = > (UINT32)ProcessorInfoBuffer.ProcessorId; > + RegisterTable[Index].TableLength = 0; > + RegisterTable[Index].AllocatedSize = 0; > + RegisterTable[Index].RegisterTableEntry = NULL; > + > + RegisterTable[NumberOfCpus + Index].InitialApicId = > (UINT32)ProcessorInfoBuffer.ProcessorId; > + RegisterTable[NumberOfCpus + Index].TableLength = 0; > + RegisterTable[NumberOfCpus + Index].AllocatedSize = 0; > + RegisterTable[NumberOfCpus + Index].RegisterTableEntry = NULL; > + } (6) Because AllocateAcpiNvsMemoryBelow4G() zero-fills the allocated buffer, all field assignments except InitialApicId could be dropped, for further simplification. Not necessary to do, and if you opt to drop the zero / NULL assignments, that can be done on commit. ... On the other hand, if a different package needs to customize this module, they will probably appreciate the field assignments (the left hand sides anyway) already being spelled out -- so I guess this could actually prove helpful down the road. With the above minor tweaks (of which (6) is definitely optional): Reviewed-by: Laszlo Ersek <ler...@redhat.com> In addition, I rebased my series on top of this one. I tested S3 with 32-bit and 64-bit Fedora guests, and also with a 64-bit Windows 8.1 guest (all using 4 VCPUs and running on KVM). Everything worked fine. Series Tested-by: Laszlo Ersek <ler...@redhat.com> I'm ready to post my version 5 series soon after this one gets committed. Thank you! Laszlo > + 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 saves MTRRs for ACPI_CPU_DATA > + // > + Status = gBS->CreateEventEx ( > + EVT_NOTIFY_SIGNAL, > + TPL_CALLBACK, > + SaveMtrrsOnEndOfDxe, > + &AcpiCpuDataEx->MtrrTable, > + &gEfiEndOfDxeEventGroupGuid, > + &Event > + ); > + ASSERT_EFI_ERROR (Status); > + > + return EFI_SUCCESS; > +} > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > new file mode 100644 > index 0000000..2ef16a7 > --- /dev/null > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > @@ -0,0 +1,64 @@ > +## @file > +# 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-2015, Intel Corporation. All rights reserved.<BR> > +# Copyright (c) 2015, Red Hat, Inc. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD > License > +# which accompanies this distribution. The full text of the license may be > found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = CpuS3DataDxe > + FILE_GUID = 4D2E57EE-0E3F-44DD-93C4-D3B57E96945D > + 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] > + MdePkg/MdePkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > + > +[LibraryClasses] > + UefiDriverEntryPoint > + UefiBootServicesTableLib > + BaseMemoryLib > + DebugLib > + BaseLib > + MtrrLib > + > +[Guids] > + gEfiEndOfDxeEventGroupGuid ## CONSUMES > + > +[Protocols] > + gEfiMpServiceProtocolGuid ## CONSUMES > + > +[Pcd] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress ## PRODUCES > + > +[Depex] > + gEfiMpServiceProtocolGuid > diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc > index 756645f..4061050 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dsc > +++ b/UefiCpuPkg/UefiCpuPkg.dsc > @@ -101,6 +101,7 @@ > UefiCpuPkg/CpuDxe/CpuDxe.inf > UefiCpuPkg/CpuIo2Smm/CpuIo2Smm.inf > UefiCpuPkg/CpuMpPei/CpuMpPei.inf > + UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf > UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf > UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
