HI Laszlo I like the diagram in GMANE. Good work! Will you consider the option to merge CpuS3DataDxe into CpuMpDxe? Then we can reduce the driver number. Or can we put CpuS3DataDxe to UefiCpuPkg?
Same question for PiSmmCpuDxeSmm. Can we put it to UefiCpuPkg, if it is generic enough? I also found you say: that "ClearSmi() is a no-op on Q35." I am curious on why. Do you mean Q35 does not require EOS bit set??? Or my misunderstanding? Thank you Yao Jiewen -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo Ersek Sent: Wednesday, July 29, 2015 3:31 AM To: Paolo Bonzini; Fan, Jeff; [email protected] Cc: Chen Fan; Justen, Jordan L Subject: Re: [edk2] [PATCH 38/58] UefiCpuPkg: CpuDxe: optionally save MTRR settings to AcpiNVS memory block On 07/28/15 08:51, Paolo Bonzini wrote: > > > On 28/07/2015 08:05, Fan, Jeff wrote: >> Ersek, >> >> I have one comment for PCD PcdCpuSyncMtrrToAcpiNvs. >> >> I knew OvmfPkg implemented LockBox based on ACPI NVS. Saving MTRR setting in >> AcpiNVS is OK for OvmfPkg. > > If I understand correctly what you are saying, the AcpiNVS block is > only used for communication from CpuDxe to CpuS3DataDxe in patch 42. > CpuS3DataDxe saves the MTRR in SMRAM during > SmmReadyToLockEventNotify() and PiSmmCpuDxeSmm restores them during S3 > resume. So Laszlo's patches are doing exactly the "safe" thing, even though > they are not using LockBox. Yes, with minimal tweaks, this is correct. The minimal tweak is the following: while CpuS3DataDxe collects most *other* data for PiSmmCpuDxeSmm indeed as a "deep copy", right when it is notified about the installation of EFI_SMM_CONFIGURATION_PROTOCOL, the saving of MTRR configuration is delayed, and for that CpuS3DataDxe only saves the address of the AcpiNVS block -- that CpuDxe maintains -- for PiSmmCpuDxeSmm. This way other firmware code can continue massaging the MTRR settings. Normally this happens via EFI_DXE_SERVICES.SetMemorySpaceAttributes EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() call chains, ie. platform-independent firmware code decides "this memory range should be uncached", calls gDS->SetMemorySpaceAttributes(), which is ultimately implemented by the CpuDxe driver, in the form of massaging MTRR. Now, whenever this happens, the MTRR settings block in AcpiNVS will be updated by CpuDxe. When SMM-ready-to-lock is finally installed, then PiSmmCpuDxeSmm will copy the then-fresh MTRR settings to SMRAM. This is the reason for the extra indirection here -- in one sentence, most of the data can be saved by CpuS3DataDxe immediately when EFI_SMM_CONFIGURATION_PROTOCOL is installed, but the MTRR settings can validly change after that, so their saving is delayed until SMM-ready-to-lock. > >> But other platform may want to use more safe solution to save MTRR based on >> in SMM. >> >> I think that, for long term, saving MTRR setting by LockBox instead >> of using ACPI NVS memory directly. Then, different platforms could >> provide the different LockBox solutions. For short term, still saving >> MTRR setting in ACPI NVS in CpuDxe, and removing this PCD. That means >> we could CpuDxe implementation to use the long term solution in the >> future and needn't to remove one un-used PCD more. > > The PCD is consumed in CPUS3DataDxe. Correct. The dynamic PCD carries the address of the AcpiNVS block (containing the ever-fresh MTRR settings) from CpuDxe to CpuS3DataDxe, which CpuS3DataDxe then just passes on to PiSmmCpuDxeSmm, in the ACPI_CPU_DATA.MtrrTable field. Thanks! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
