Laszlo, Thanks for the feedback and the testing!
I have made the updates you suggested in this series except for removing the lines that initialize the RegisterTable fields. I left those there to make it easier for CPU/Platform package that needs to use this module as a starting point for a custom version. This series has been committed, so it is now available for use with your OVMF SMM v5 series. Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Wednesday, November 25, 2015 3:04 AM > To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-de...@ml01.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Fan, Jeff <jeff....@intel.com> > Subject: Re: [edk2] [PATCH v2 2/2] UefiCpuPkg/CpuS3DataDxe: Add module to > initialize ACPI_CPU_DATA for S3 > > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
