(one comment in the middle) On 10/06/14 17:42, Gabriel L. Somlo wrote: > 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]
(I think DXE_RUNTIME_DRIVER might work too...) >> >> 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 (... because, although DXE_RUNTIME_DRIVERs survive into runtime, they are dispatched at boot time, hence their libraries' constructors still can access PCDs) >> >> 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, Adding the PCDs would remove the accesses from DXE_DRIVER and UEFI_DRIVER modules, and with the small parenthesized modification above, the PCDs would cover DXE_RUNTIME_DRIVER startups as well. But: > 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, ... this isn't all too different from what we're doing in QemuFwCfgLib (QemuFwCfgInitialize()). And *that* library constructor seems more heavy-weight (it reads and writes io-ports several times), and we don't mind doing that in each client module. (Of course I didn't check how the *number* of QemuFwCfgLib client modules relates to the number of TimerLib client modules.) I'd be fine with your suggestion, but I'll defer to Jordan. The separate library instance would be nice indeed, but I agree it might not be worth the hassle, especially given the constructor function in QemuFwCfgPeiDxe.c. Thanks Laszlo ------------------------------------------------------------------------------ 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
