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. 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 ?
Why set the global static variable based on retrieving a PCD and not
based on just reading the host bridge DID during the constructor ?
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 ?
Or I may have misunderstood this in a big way, in which case my
apologies for being thick about it :)
Thanks much,
--Gabriel
> On 2014-09-27 15:06:45, Gabriel L. Somlo wrote:
> > Factor out logic to distinguish between Q35 and PIIX4 platforms
> > into a separate header (OvmfPkg/Include/Library/PciHostBridge.h),
> > then refer to these macros from all relevant source locations
> > throughouth OvmfPkg.
> >
> > This patch also adds a Q35-specific PIC IRQ routing initialization
> > function to OvmfPkg/Library/PlatformBdsLib/BdsPlatform.c
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Gabriel Somlo <[email protected]>
> > ---
> >
> > As discussed with Laszlo and Paolo in an earlier thread, this stuff should
> > hopefully be mostly uncontroversial and "the right thing to do" to support
> > Q35. Doesn't fix my weird OS X / Q35 / UHCI issues, but I guess that's a
> > whole different fight to be fought another day :)
> >
> > The one thing I'm a bit unsure about is PciInitializationQ35
> > (BdsPlatform.c);
> > In PciInitializationPIIX (which used to be plain PciInitialization before
> > this patch), there's a whole bunch of other devices (ide, video, network,
> > etc.) being initialized by writing to registers which should mostly be
> > read-only on Q35; Leaving that stuff out from PciInitializationQ35 doesn't
> > seem to have any effect, negative or otherwise, so I decided to stick to
> > LNK* routing initialization only.
> >
> > Any comments/suggestions much appreciated, as always.
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
