On 11/04/14 01:22, Gabriel Somlo wrote:
> 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.
Thanks.
>> - 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_*.
I checked "MdePkg/Library/DxePcdLib/DxePcdLib.inf" just to be sure, and
yes, it does seem to allow DXE_SMM_DRIVER and SMM_CORE.
> 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() :)
I guess you could nonetheless factor out the common switch into a shared
utility function, and just distinguish PcdGet16() from PciRead16() with
a new ("polymorphic") function, like GetHostBridgeDevId().
Probably not worth the hassle though (new include file, new .c file to
be shared only between Base and Dxe, license blocks etc).
> 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! Let's see what Jordan thinks!
Cheers
Laszlo
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
