On Mon, Nov 03, 2014 at 08:09:28PM +0100, Laszlo Ersek wrote:
> comments below:
>
> On 11/01/14 23:20, Gabriel L. Somlo wrote:
> > Remove local power management register access macros in favor of
> > factored-out ones in OvmfPkg/Include/OvmfPlatforms.h
> >
> > Next, AcpiTimerLib is split out into three instances, for use during
> > various stages:
> >
> > - BaseRom: used during SEC, PEI_CORE, and PEIM;
> > - Dxe: used during DXE_DRIVER and DXE_RUNTIME_DRIVER;
> > - Base: used by default during all other stages.
> >
> > Most of the code remains in AcpiTimerLib.c, to be shared by all
> > instances. The two platform-dependent methods (constructor and
> > InternalAcpiGetTimerTick) are provided separately by source files
> > specific to each instance, namely [BaseRom|Base|Dxe]AcpiTimerLib.c.
> >
> > Since pre-DXE stages can't rely on storing data in global variables,
> > methods specific to the "BaseRom" instance will call platform
> > detection macros each time they're invoked.
>
> (1) This is mostly correct, but not entirely accurate. Refer back to
> Jordan's email
> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10156/focus=10240>.
>
> - PEI_CORE and PEIM run from RAM off the bat (which is a specialty of
> OVMF). They *can* set global variables. They should use "Base" (ie. the
> default).
OK, I can make that happen.
> - The Dxe instance resolution should be added for UEFI_DRIVER,
> UEFI_APPLICATION, SMM_CORE and DXE_SMM_DRIVER. (We agreed that we were
> willing to "bend the PCD rules" for OVMF; plus the SMM_CORE is loaded by
> the DXE_CORE if I recall correctly.)
>
> BaseRom: SEC
> Base (default): PEI_CORE, PEIM, DXE_CORE
> Dxe: DXE_DRIVER, DXE_RUNTIME_DRIVER, UEFI_DRIVER,
> UEFI_APPLICATION, SMM_CORE, DXE_SMM_DRIVER
OK: that would mean s/BasePcdLibNull.inf/DxePcdLib.inf/ for UEFI_* and
*_SMM_*.
On a side note, I really wish PcdLib would have a method like e.g.
"PcdSupported", hardcoded to return FALSE in BasePcdLibNull and true
in e.g. DxePcdLib. I could then merge the Base- and Dxe- varieties of
AcpiTimerLib back into one:
HostBridgeDevId = (PcdSupported ()) ?
PcdGet16 (PcdOvmfHostBridgePciDevId) :
PciRead16 (OVMF_HOSTBRIDGE_DID);
switch (HostBridgeDevId) {
case ....
}
... and not get killed by a BasePcdLibNull ASSERT(FALSE)
implementation of PcdGet16() :)
Then it would be down to simply which version of PcdLib we specify for
a given stage in the .dsc file.
That would be cool, from a "suggestion box" point of view...
> I think this patch is correct *if* we agree that:
> (a) PEIMs and PEI_CORE are allowed to use the "slowest" library instance
> instead of the mid-speed one,
> (b) and UEFI_DRIVER, UEFI_APPLICATION are allowed to use the mid-speed
> instance instead of the "fastest" one,
> (c) and SMM_CORE and DXE_SMM_DRIVER are allowed to use the mid-speed
> instance instead of the "fastest" one.
>
> I thought that we rejected (a) specifically, and rejected (b)
> specifically. We didn't discuss (c) in particular, but I think it's
> reasonable to use the "fastest" one in SMM stuff. (Which we currently
> don't have, yet.)
>
> If I'm wrong about the above points, then this patch is perfectly fine.
A lot of that conversation happened before I had enough context to
process everything that was said :)
So, if you and Jordan are both in agreement about this, I can redo
this patch and "promote" more stages towards "smarter" versions of
AcpiTimerLib... :)
Thanks,
--Gabriel
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
