Hi Eric, On 10/18/18 09:34, Eric Dong wrote: > AcpiCpuData add new fields, keep these fields if old data already existed. > > Cc: Ruiyu Ni <ruiyu...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <eric.d...@intel.com> > Reviewed-by: Ruiyu Ni <ruiyu...@intel.com> > --- > UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > index ef98239844..1b847e453a 100644 > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > @@ -259,6 +259,8 @@ CpuS3DataInitialize ( > 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 >
sorry my earlier point about CpuS3DataDxe may not have been clear. OVMF does not include any PEIM that consumes RegisterCpuFeaturesLib, so it does not set PcdCpuS3DataAddress, in the PEI phase (or in the DXE phase elsewhere, for that matter). That is, in this code, OVMF takes the other branch: "Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs". This series does not extend that branch, and as a result, the new fields are all zero-filled. (From the AllocateZeroPages() function call near the top of the CpuS3DataInitialize() function.) But, in patch #4, in PiSmmCpuDxeSmm, the GetAcpiCpuData() function calls AllocateCopyPool() -- copying data from normal RAM into SMRAM -- on the following pointers: - (UINT32 *)(UINTN)AcpiCpuData->CpuStatus.ValidCoreCountPerPackage - (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)AcpiCpuData->ApLocation These pointers will be NULL at that time. That's a problem in particular for "ApLocation", because the byte count of the copy is mAcpiCpuData.NumberOfCpus * sizeof (EFI_CPU_PHYSICAL_LOCATION) which is nonzero (even on OVMF). Now, what I'm suggesting is *not* that we extend this "other" branch in CpuS3DataInitialize(), so that we fill in the new fields sensibly. That can't be done in a platform-independent manner. For example, getting the CPU topology could be platform dependent. However, I am requesting that platforms be allowed to continue working with CpuS3DataDxe and PiSmmCpuDxeSmm even if they do not need the new feature (i.e. if they do not set any MSRs at S3 resume). That means that GetAcpiCpuData() in PiSmmCpuDxeSmm should work if the new fields are all zero. For explaining myself better, please consider a new Feature PCD that declares whether a platform sets MSRs at S3 resume. If it doesn't, then it doesn't need the new semaphore feature either. Then, if the FeaturePCD is FALSE, and the register stable *still* contains a semaphore / dependency operation, we can trigger an assert. Now, I know that new FeaturePCDs are not welcome. And, I don't really want to introduce one, I just used it as an example, for illustration. Because we can determine the same condition *dynamically* in PiSmmCpuDxeSmm. (And, perhaps we should document it in patch #1, "AcpiCpuData.h", as well.) For example: If the platform does not support MSR setting at S3 resume, and therefore it doesn't need the dependency semaphores, it should set both the CpuStatus.ValidCoreCountPerPackage field, and the ApLocation field, to 0. Then CpuS3DataDxe will need no further change beyond the present patch; however PiSmmCpuDxeSmm should check for those zero values, and disable the new feature dynamically. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel