On 09/30/14 06:30, Gabriel L. Somlo wrote: > On Mon, Sep 29, 2014 at 03:17:23PM -0700, Jordan Justen wrote: >> My recommendations would be: >> >> OvmfPkg/Include/Library/PciHostBridge.h => >> OvmfPkg/Include/OvmfPlatforms.h >> >> Add DEVIDs to OvmfPkg/Include/OvmfPlatforms.h. Drop IS_Q35_HOSTBRIDGE, >> PCI_PM_REG. Maybe add Q35_PM_DEVICE=0x1f and PIIX4_PM_DEVICE=1. >> >> Add 2 Dynamic PCDs: >> * PcdPlatformHostBridgePciDevId >> * PcdPlatformPmDevice >> >> Set these PCDs in OvmfPkg/PlatformPei. >> >> OvmfPkg/Library/AcpiTimerLib/AcpiTimerLib.inf can be changed to read >> the PCD in a constructor and store the PM Device in a global variable. >> >> In OvmfPkg*.dsc: >> * Set LibraryClasses.common.PEIM to use >> MdePkg/Library/PeiPcdLib/PeiPcdLib.inf >> * Replace BasePcdLibNull.inf with DxePcdLib.inf as needed in DXE/UEFI >> LibraryClasses. >> >> (This paragraph is a bit tangential. Feel free to ignore it. :) I >> think this is not quite valid for pure UEFI drivers, since they should >> not rely on the PCD protocol. But, I think that OVMF as a platform can >> bend this rule. The problem is that OVMF's TimerLib would rely on the >> PCD protocol. We could make a separate UEFI version of the TimerLib >> that read the device ID and determine the PM device num rather than >> going through the PCD. >> >> Can you use PCI_DEVICE_ID_OFFSET rather than 2? >> >> It seems like breaking this into more than 1 patch would be >> appropriate. > > Thanks to everyone for the feedback and suggestions. I'll change the > location of the macros (and the way I'm naming them), and will bring > up how much of PciInitialization in BdsPlatform.c (beyond LNK routing) > should be done where in a separate email. > > Right now, I'd like to make sure I understand about PCDs :) > > I added DEBUG statements everywhere the IS_Q35_HOSTBRIDGE macro was > used: > > CsmSupportLib/LegacyInterrupt.c: GetLocation() > CsmSupportLib/LegacyInterrupt.c: GetAddress() > AcpiTimerLib.c: AcpiTimerLibConstructor() > AcpiTimerLib.c: InternalAcpiGetTimerTick() > BdsPlatform.c: PciInitialization() > BdsPlatform.c: AcpiInitialization() > PlatformPei/Platform.c: MiscInitialization() > > and got something approximating the following (on either piix or q35): > > PlatformPei/Platform.c: MiscInitialization() > AcpiTimerLib.c: AcpiTimerLibConstructor() x 15 > AcpiTimerLib.c: InternalAcpiGetTimerTick() x 11848 > BdsPlatform.c: PciInitialization() > BdsPlatform.c: AcpiInitialization() > AcpiTimerLib.c: InternalAcpiGetTimerTick() x 4666 > > CsmSupportLib/LegacyInterrupt.c never got called.
(You're not running a CSM enabled build. Not surprisingly. :)) > Other than that, > it's more or less the way Jordan said: > > 1. first, PlatformPei's MiscInitialization would set a PCD > 2. next, a bunch of instances of AcpiTimerLibConstructor() > would read the PCD and set a bunch of instances of global > variables (I assume this would be a "static foo" in AcpiTimerLib.c) > so that InternalAcpiGetTimerTick would not have to read the > host bridge DID (or the PCD) *each* *and* *every* *single* *time* :) > 3. BdsPlatform.c would read the PCD same as the > TimerLibConstructor, but since it's two functions running > once each, the savings would be minimal there. > > So, my n00b question is why bother with PCDs in the first place, given > that their use in AcpiTimerLib (where the bulk of the savings from > ditching the IS_Q35 macro is realized) may be frowned upon ? I agree that PCDs are not optimal here; see my other email. In some AcpiTimerLib clients they are downright unavailable. > Why set the global static variable based on retrieving a PCD and not > based on just reading the host bridge DID during the constructor ? You can't set globals in AcpiTimerLib, no matter where you'd populate them *from*. See my other email. > Either way, we save reading the bridge DID during each of the > bazillion calls to GetTimerTick, and what's reading the host bridge > DID a few tens of times (15xTimerLibConstructor plus three or four more) > if it gets us out of mixing PCDs with AcpiTimerLib ? The cost of the libraries I recommended in my other email is: - SEC and PEI_CORE: same as now - PEIM, DXE_CORE, DXE_RUNTIME_DRIVER, DXE_DRIVER: GetFirstGuidHob(). A linear search that doesn't trap to the hypervisor. I expect the HOB list to have a few tens of elements at most. Also, caching the HOB once found would turn this into O(1). - UEFI_DRIVER, UEFI_APPLICATION, alternative 1: library construction (ie. driver / app startup) is somewhat expensive, but access is O(1) afterwards (cached values). - UEFI_DRIVER, UEFI_APPLICATION, alternative 2: same HOB-based library instance as above. Thanks Laszlo ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
