Hi Jordan,
On Sat, Oct 04, 2014 at 07:35:25PM -0700, Jordan Justen wrote:
> > On Wed, Oct 01, 2014 at 12:31:58AM +0200, Laszlo Ersek wrote:
> >> On 09/30/14 23:42, Jordan Justen wrote:
> >> > 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'm fine with your proposal. The tradeoff is of course DxeCore, and that
> >> this way the common bits of AcpiTimerLib.c will have to be split out
> >> into a third .c file, potentially requiring some refactoring. Example:
> >> OvmfPkg/Library/QemuFwCfgLib.
> >>
> >> [...]
> >>
> >> Let's go with BaseTimerLib / DxeTimerLib.
>
> [...]
>
> Can you make three versions?
> OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>
> Set BaseAcpiTimerLib.inf as default under:
> [LibraryClasses]
>
> Use BaseRomAcpiTimerLib.inf for:
> [LibraryClasses.common.SEC]
>
> Use DxeAcpiTimerLib.inf version for these:
> [LibraryClasses.common.UEFI_DRIVER]
> [LibraryClasses.common.DXE_DRIVER]
> [LibraryClasses.common.UEFI_APPLICATION]
>
> BaseRomAcpiTimerLib.inf
> * Shares a common file with BaseAcpiTimerLib.inf for reading the host
> bus to determine the system type.
> * Reads host bus every time, and doesn't set any global variables
>
> BaseAcpiTimerLib.inf
> * Shares a common file with BaseAcpiTimerLib.inf for reading the host
> bus to determine the system type.
> * Sets a global module variable that allows the library to not have
> to read the host bus id every time.
>
> DxeAcpiTimerLib.inf
> * Read the PCD once and save it in a global module variable
>
> PlatformPei should also read the host bus id, and set a PCD to allow
> DxeAcpiTimerLib.inf to work.
My plan was to tackle splitting out AcpiTimerLib first, then learn
about and understand how PCDs work in practice :)
The first bit (how to create multiple instances of a library, and
connect instances to various stages like PEIM, DXE_CORE, etc) is done,
and I'm wondering whether the second bit (using PCDs) is actually worth
the complexity of adding a *third* version of AcpiTimerLib.
Right now, BaseAcpiTimerLib and DxeAcpiTimerLib share everything
except for the constructor and the GetTimerTick method.
The "Base" version queries the host bridge indiscriminately, both in
the constructor and in the GetTimerTick function. In my tests, the
GetTimerTick function never did get called (maybe because I'm not
using SEC and/or the debugger?)
The "Dxe" version queries the host bridge in the constructor, and
computes the IO address of the ACPI timer; In the GetTimerTick method,
we just do an IoRead32 on the cached ACPI timer IO address.
So, to decide how I feel about further splitting out DxeAcpiTimerLib
into a version which does use PCDs and another version which does not,
I added some DEBUG statements to track every time the host bridge
gets queried right now, and here's what I got:
PlatformPei:MiscInitialization()
Base:AcpiTimerLibConstructor() (PEIM)
Dxe:AcpiTimerLibConstructor() (DXE_CORE)
Dxe:AcpiTimerLibConstructor() (DXE_RUNTIME_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_RUNTIME_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_RUNTIME_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_RUNTIME_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_DRIVER)
Dxe:AcpiTimerLibConstructor() (UEFI_DRIVER)
Dxe:AcpiTimerLibConstructor() (UEFI_DRIVER)
Dxe:AcpiTimerLibConstructor() (UEFI_DRIVER)
Dxe:AcpiTimerLibConstructor() (UEFI_DRIVER)
Dxe:AcpiTimerLibConstructor() (DXE_DRIVER)
Dxe:AcpiTimerLibConstructor() (UEFI_DRIVER)
BdsPlatform:PciInitialization()
BdsPlatform:AcpiInitialization()
Dxe:AcpiTimerLibConstructor() (DXE_DRIVER)
I guess adding PCDs would save us from querying the host bridge about
half the time, at the expense of adding a third version of
AcpiTimerLib. I'm inclined to think the added complexity isn't worth
it, but then again I'm new at this and I may be missing some other
set of criteria which would make the whole thing worth it regardless.
I could also just post my current patch set so we can all be on the
same page when deciding how to proceed.
Let me know what you all think. Thanks,
--Gabriel
------------------------------------------------------------------------------
Slashdot TV. Videos for Nerds. Stuff that Matters.
http://pubads.g.doubleclick.net/gampad/clk?id=160591471&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
