On 09/30/14 00:17, 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.
I don't think that will work under all circumstances; please see <https://github.com/tianocore/edk2/commit/d3a24ff5>. The fact that AcpiTimerLib may be linked into SEC prevents it from using both writeable global variables (because it runs from flash) *and* from using dynamic PCDs (because the first phase that can use PCDs is PEI, given that the underlying PPI is provided by a PEIM). In addition, setting the PCDs in PlatformPei could be too late for other PEIMs that link against AcpiTimerLib. > 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. Yes, that's the problem... > 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. ... hence, using dynamic PCDs indiscriminately in our *one* AcpiTimerLib instance would be problematic in at least four (4) module types: - in SEC because it wouldn't work (under some build configs), - in PEIM because some TimerLib-dependent PEIM might be dispatched before PlatformPei sets the PCD, - in DXE_CORE, because DXE_CORE has no access to PCDs at all -- the PPI from PEI is gone, and the protocol from DXE is not yet available, - and in UEFI_DRIVER because it wouldn't be nice ("portable"). What about the following idea (if you're unsatisfied with the wholesale "query the registers" approach): - Introduce a new library class OvmfPlatformsLib. It should export the functions GetPlatformHostBridgePciDevId() and GetPlatformPmDevice(). - Keep our one AcpiTimerLib instance, but make it dependent on the new OvmfPlatformsLib class. No static data used in AcpiTimerLib; all accesses call the new functions. - Introduce a new structure type that captures all the bits that differ between Q35 and PIIX4. Minimally, this structure type would include "PlatformHostBridgePciDevId" and "PlatformPmDevice". - Introduce a new GUID. - Modify PlatformPei to call BuildGuidHob(), with the new GUID, and the populated structure as contents. - The OvmfPlatformsLib class would have the following *three* instances: (a) for SEC and PEI_CORE: interrogate the registers all the time. (b) for PEIM, DXE_CORE, DXE_RUNTIME_DRIVER, DXE_DRIVER: call GetFirstGuidHob(). If it succeeds, return data from the structure found, if it fails, interrogate the registers. This library will show the following behavior: - in PEIM code running before PlatformPei: query registers - in PEIM code running after PlatformPei: use HOB - in DXE_CORE: use HOB (yes, available) - in DXE_RUNTIME_DRIVER, DXE_DRIVER: use HOB (c1) for UEFI_DRIVER, UEFI_APPLICATION: we need to be "portable" here, and HOB use is only guaranteed in the PI spec, not in the UEFI spec. - This library instance should have a constructor that sets "mPlatformHostBridgePciDevId" and "mPlatformPmDevice" (to serve the functions from). - The UEFI way to initialize these variables is to find the host bridge candidates, and verify their device IDs. - construct the expected device path for each candidate in turn - use gBS->LocateDevicePath() to find the PciIo interface on the candidate - verify that we got a direct match - read a PCI_TYPE00 structure from the config space, and verify the expected device ID for the candidate - if no candidate checks out, fail library construction (c2) A (much faster) alternative for UEFI_DRIVER, UEFI_APPLICATION: the HOB-based library instance (b) would work just as well for UEFI_DRIVER and UEFI_APPLICATION, at the price of some theoretical portability. (Note that it's still better / less intrusive than using PCDs.) - GetFirstGuidHob() has a valid implementation for these module types (see "MdePkg/Library/DxeHobLib/DxeHobLib.inf"). - That implementation is based on the UEFI System Config Table. - Although the GUID used in HobLibConstructor() to locate the HOB list in the UEFI System Config Table -- ie. gEfiHobListGuid -- is only part of the PI spec, and not the UEFI spec, we already depend on it heavily, because: - HobLibConstructor() asserts that the HOB list be found, - and we *already* resolve the HobLib library class to DxeHobLib for UEFI_DRIVER. - This dependency has a graceful, explicit failure mode, in relation to portability: ASSERT() if HOB List is not found -- which will never happen on PI-compliant platforms -- and query registers if HOB List is found but our specific HOB is not found. Thoughts? Thanks, Laszlo > Can you use PCI_DEVICE_ID_OFFSET rather than 2? > > It seems like breaking this into more than 1 patch would be > appropriate. > > Thanks! > > -Jordan ------------------------------------------------------------------------------ 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
