Thanks for the info. That makes sense. It is pity that there is no X64 version and SMP not work. We can make improvement step by step.
Thank you Yao Jiewen -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo Ersek Sent: Wednesday, July 29, 2015 9:25 PM To: Yao, Jiewen; 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 23:41, Yao, Jiewen wrote: > HI Laszlo > I like the diagram in GMANE. Good work! Thanks! > Will you consider the option to merge CpuS3DataDxe into CpuMpDxe? > Then we can reduce the driver number. Well, that's a hard question. The code that I'm gradually creating / porting under OvmfPkg/QuarkPort/CpuS3DataDxe/ *originates* from the Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuMpDxe driver in the Quark distribution. This is documented in the commit message of: [edk2] [PATCH 33/58] OvmfPkg: add skeleton QuarkPort/CpuS3DataDxe http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=357 It says, > The PiSmmCpuDxeSmm driver depends on an ACPI_CPU_DATA structure from > the Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg/CpuMpDxe/ driver. [...] > [...] Since CpuMpDxe is very complex and covers much more > functionality, we're going to gradually import only the above feature. > This patch pulls in the basic skeleton of the driver. I justified this (ie. the creation of a separate, slimmed-down driver) with the following facts: - edk2 does not have a separate CpuMpDxe driver at all, - between the functionalities of Quark's CpuMpDxe, and edk2's UefiCpuPkg/CpuDxe, there's a partial overlap. So importing CpuMpDxe whole-sale from Quark would have been overkill (it would have duplicated some functionality already present in UefiCpuPkg, and also included superfluous, complex stuff I didn't need at all). I thought that gathering / saving the ACPI_CPU_DATA structure was a task that was big enough and well defined enough to warrant a separate driver. Also, I wanted to minimize the impact on UefiCpuPkg. > Or can we put CpuS3DataDxe to UefiCpuPkg? Perhaps. In order to simplify things as much as possible, I analyzed the ACPI_CPU_DATA structure itself, and cut away some fields that might have made sense for a physical platform, but were not useful on QEMU. Please refer to the following patches: [edk2] [PATCH 32/58] OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.APState [edk2] [PATCH 43/58] OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.PreSmmInitRegisterTable [edk2] [PATCH 44/58] OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.RegisterTable Also, there were fields that I preserved, but filled in differently. Please see for example: [edk2] [PATCH 34/58] OvmfPkg: QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.StartupVector So, if what I have posted as OvmfPkg/QuarkPort/CpuS3DataDxe is acceptable for UefiCpuPkg/CpuS3DataDxe, according to reviewers, then I don't have anything against it. (The relevant patches can be easily identified: their subjects all start with "OvmfPkg: QuarkPort/CpuS3DataDxe".) > Same question for PiSmmCpuDxeSmm. Can we put it to UefiCpuPkg, if it is > generic enough? Same answer :) I think PiSmmCpuDxeSmm was already somewhat specific to Quark, to begin with, and then I modified it heavily. See the patches: - OvmfPkg: PiSmmCpuDxeSmm: eliminate SmmLib dependency - OvmfPkg: PiSmmCpuDxeSmm: eliminate CpuConfigLib linkage dependency - OvmfPkg: QuarkPort/CpuS3DataDxe: fill in ACPI_CPU_DATA.StartupVector (this one affects both PiSmmCpuDxeSmm and CpuS3DataDxe) - OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.PreSmmInitRegisterTable (ditto) - OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.RegisterTable (ditto) - OvmfPkg: QuarkPort/PiSmmCpuDxeSmm: hard-code CPU class identification (this one is definitely incorrect for physical hardware) I trimmed stuff as aggressively as possible for two reasons: - Analyzing and porting each single field in ACPI_CPU_DATA was a big effort. A single field (or a small group of fields belonging together) could easily take a day or more. - Testability. I can test on QEMU (TCG and KVM), but not on real hardware. So, *if* there had been a sufficiently generic driver in edk2 that had worked on physical platforms already, I might have included just that (and then some "extras" that are not important for, or not emulated by, a virtual machine, would have just decayed to no-ops at runtime.) In the other direction, it doesn't work; originally VM-targeted code cannot be expected to run on a physical platform. For that, the original driver in the Quark distribution should be generalized (to other processor types), and then imported to UefiCpuPkg. Other limitations of this PiSmmCpuDxeSmm port: - No support for an X64 SMM entry vector. (Intel could help a lot with this, by open sourcing a firmware package that has an X64 SMM entry vector.) In other words, only Ia32 is supported. Nonetheless, it's not *just* the SMM entry vector that's missing for X64. The 64-bit SMM save state area has two (incompatible) definitions, one from Intel and one from AMD. I found traces of the Intel definition in the code, but QEMU only supports the AMD definition. - SMP support (more than one VCPUs) does not work. Lifting this limitation will probably need work on both QEMU/KVM and the firmware code. The goal of this series is to provide a starting point for OVMF. We can then work forward and gradually implement / port X64 and SMP. Unfortunately, I cannot offer to port the drivers from the Quark distro to UefiCpuPkg. I'm not a firmware developer for physical platforms -- to say the least, I don't have the physical test lab for such work (let alone the expertise). > I also found you say: that "ClearSmi() is a no-op on Q35." ... Okay, after some searching, I think you are referring to: [edk2] [PATCH 18/58] OvmfPkg: PiSmmCpuDxeSmm: eliminate SmmLib dependency http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=337 > I am curious on why. > Do you mean Q35 does not require EOS bit set??? Or my misunderstanding? No, you understood correctly. I did not document the idea in the 18/58 patch because earlier in the series, I added a long *code* comment about the same. Please see the SmmControl2DxeClear() function in the patch [edk2] [PATCH 11/58] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=332 If you'd like, I can update the commit message of patch 18/58 "OvmfPkg: PiSmmCpuDxeSmm: eliminate SmmLib dependency" so that it references the earlier code comment with regard to ClearSmi(). Thanks! Laszlo > > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
