On Mon, Oct 6, 2014 at 8:42 AM, Gabriel L. Somlo <gso...@gmail.com> 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] >> >> 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.
Regarding IS_Q35_HOSTBRIDGE (or, is it IS_PLATFORM_Q35 now?), I see these negatives: * Doesn't look like EDK II code * Reads PCI registers inside a macro * It seems very focused: "Is this Q35?" Laszlo had another suggestion, but it add a new library interface. So, it seems about as complex as my suggestion. This idea may add another library implementation, but I think it will also gain the feature of being able to read the 'platform type' via PCD. (For example, in the Platform BDS library, or the ACPI platform driver, etc...) -Jordan > 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 ------------------------------------------------------------------------------ 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 edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
