On 2014-09-30 02:36:59, Laszlo Ersek wrote: > 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>.
UDK Debugger! *grumble, grumble*
> 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.
I'm not sure about the OvmfPlatformsLib idea. It seems like something
that ideally we could just read from a PCD.
Of course, TimerLib gets pulled right into the center of everything,
and can't use dynamic PCDs. (sigh)
How about:
PlatformPei:
* Reads host bridge ID. Sets PCD.
BaseTimerLib:
* Reads host bridge ID (duplicate code)
* Used for Sec, PeiCore, Pei drivers and DxeCore
DxeTimerLib:
* Reads dynamic PCD in constructor
* Used for everything else
I think all of the other places Gabriel updated can read a dynamic PCD.
I think this is also related to the various places we do 'if (Xen)
elif (Qemu+FwCfg) else'. In those cases we do various other detection
mechanisms, and I'm not real thrilled with how it turned out.
What do you think? Does this seem like a reasonable plan for handling
how OVMF supports the various platform types?
I do think your OvmfPlatformsLib idea is preferable always reading the
host bridge ID every time via a macro. I'm just not sure it makes
sense to add 'yet another library' for this. Another idea would be to
update PlatformHookLib to have a function for reading a platform
ID. (That library is not well spec'd, so I guess we could argue that
just about anything could be put in there. :)
-Jordan
signature.asc
Description: signature
------------------------------------------------------------------------------ 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
