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
