On 09/30/14 23:42, Jordan Justen wrote:
> On 2014-09-30 02:36:59, Laszlo Ersek wrote:
>> On 09/30/14 00:17, Jordan Justen wrote:
>>> My recommendations would be:
>>>
>>> OvmfPkg/Include/Library/PciHostBridge.h =>
>>> OvmfPkg/Include/OvmfPlatforms.h
>>>
>>> Add DEVIDs to OvmfPkg/Include/OvmfPlatforms.h. Drop IS_Q35_HOSTBRIDGE,
>>> PCI_PM_REG. Maybe add Q35_PM_DEVICE=0x1f and PIIX4_PM_DEVICE=1.
>>>
>>> Add 2 Dynamic PCDs:
>>> * PcdPlatformHostBridgePciDevId
>>> * PcdPlatformPmDevice
>>>
>>> Set these PCDs in OvmfPkg/PlatformPei.
>>>
>>> OvmfPkg/Library/AcpiTimerLib/AcpiTimerLib.inf can be changed to read
>>> the PCD in a constructor and store the PM Device in a global variable.
>>
>> I don't think that will work under all circumstances; please see
>> <https://github.com/tianocore/edk2/commit/d3a24ff5>.
>
> UDK Debugger! *grumble, grumble*
>
>> The fact that AcpiTimerLib may be linked into SEC prevents it from using
>> both writeable global variables (because it runs from flash) *and* from
>> using dynamic PCDs (because the first phase that can use PCDs is PEI,
>> given that the underlying PPI is provided by a PEIM).
>>
>> In addition, setting the PCDs in PlatformPei could be too late for other
>> PEIMs that link against AcpiTimerLib.
>>
>>> In OvmfPkg*.dsc:
>>> * Set LibraryClasses.common.PEIM to use
>>> MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>> * Replace BasePcdLibNull.inf with DxePcdLib.inf as needed in DXE/UEFI
>>> LibraryClasses.
>>>
>>> (This paragraph is a bit tangential. Feel free to ignore it. :) I
>>> think this is not quite valid for pure UEFI drivers, since they should
>>> not rely on the PCD protocol. But, I think that OVMF as a platform can
>>> bend this rule. The problem is that OVMF's TimerLib would rely on the
>>> PCD protocol.
>>
>> Yes, that's the problem...
>>
>>> We could make a separate UEFI version of the TimerLib
>>> that read the device ID and determine the PM device num rather than
>>> going through the PCD.
>>
>> ... hence, using dynamic PCDs indiscriminately in our *one* AcpiTimerLib
>> instance would be problematic in at least four (4) module types:
>> - in SEC because it wouldn't work (under some build configs),
>> - in PEIM because some TimerLib-dependent PEIM might be dispatched
>> before PlatformPei sets the PCD,
>> - in DXE_CORE, because DXE_CORE has no access to PCDs at all -- the PPI
>> from PEI is gone, and the protocol from DXE is not yet available,
>> - and in UEFI_DRIVER because it wouldn't be nice ("portable").
>>
>> What about the following idea (if you're unsatisfied with the wholesale
>> "query the registers" approach):
>>
>> - Introduce a new library class OvmfPlatformsLib. It should export the
>> functions GetPlatformHostBridgePciDevId() and GetPlatformPmDevice().
>>
>> - Keep our one AcpiTimerLib instance, but make it dependent on the new
>> OvmfPlatformsLib class. No static data used in AcpiTimerLib; all
>> accesses call the new functions.
>>
>> - Introduce a new structure type that captures all the bits that differ
>> between Q35 and PIIX4. Minimally, this structure type would include
>> "PlatformHostBridgePciDevId" and "PlatformPmDevice".
>>
>> - Introduce a new GUID.
>>
>> - Modify PlatformPei to call BuildGuidHob(), with the new GUID, and the
>> populated structure as contents.
>>
>> - The OvmfPlatformsLib class would have the following *three* instances:
>>
>> (a) for SEC and PEI_CORE: interrogate the registers all the time.
>>
>> (b) for PEIM, DXE_CORE, DXE_RUNTIME_DRIVER, DXE_DRIVER: call
>> GetFirstGuidHob(). If it succeeds, return data from the structure
>> found, if it fails, interrogate the registers.
>>
>> This library will show the following behavior:
>>
>> - in PEIM code running before PlatformPei: query registers
>>
>> - in PEIM code running after PlatformPei: use HOB
>>
>> - in DXE_CORE: use HOB (yes, available)
>>
>> - in DXE_RUNTIME_DRIVER, DXE_DRIVER: use HOB
>>
>> (c1) for UEFI_DRIVER, UEFI_APPLICATION: we need to be "portable" here,
>> and HOB use is only guaranteed in the PI spec, not in the UEFI
>> spec.
>>
>> - This library instance should have a constructor that sets
>> "mPlatformHostBridgePciDevId" and "mPlatformPmDevice" (to serve
>> the functions from).
>>
>> - The UEFI way to initialize these variables is to find the host
>> bridge candidates, and verify their device IDs.
>>
>> - construct the expected device path for each candidate in turn
>>
>> - use gBS->LocateDevicePath() to find the PciIo interface on
>> the candidate
>>
>> - verify that we got a direct match
>>
>> - read a PCI_TYPE00 structure from the config space, and verify
>> the expected device ID for the candidate
>>
>> - if no candidate checks out, fail library construction
>>
>> (c2) A (much faster) alternative for UEFI_DRIVER, UEFI_APPLICATION:
>> the HOB-based library instance (b) would work just as well for
>> UEFI_DRIVER and UEFI_APPLICATION, at the price of some
>> theoretical portability. (Note that it's still better / less
>> intrusive than using PCDs.)
>>
>> - GetFirstGuidHob() has a valid implementation for these module
>> types (see "MdePkg/Library/DxeHobLib/DxeHobLib.inf").
>>
>> - That implementation is based on the UEFI System Config Table.
>>
>> - Although the GUID used in HobLibConstructor() to locate the
>> HOB list in the UEFI System Config Table -- ie.
>> gEfiHobListGuid -- is only part of the PI spec, and not the
>> UEFI spec, we already depend on it heavily, because:
>>
>> - HobLibConstructor() asserts that the HOB list be found,
>>
>> - and we *already* resolve the HobLib library class to
>> DxeHobLib for UEFI_DRIVER.
>>
>> - This dependency has a graceful, explicit failure mode, in
>> relation to portability: ASSERT() if HOB List is not found --
>> which will never happen on PI-compliant platforms -- and
>> query registers if HOB List is found but our specific HOB is
>> not found.
>
> I'm not sure about the OvmfPlatformsLib idea. It seems like something
> that ideally we could just read from a PCD.
>
> Of course, TimerLib gets pulled right into the center of everything,
> and can't use dynamic PCDs. (sigh)
>
> How about:
> PlatformPei:
> * Reads host bridge ID. Sets PCD.
> 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 think all of the other places Gabriel updated can read a dynamic PCD.
Believe it or not, this *did* cross my mind :)
I didn't recommend it because DXE_CORE cannot use PCDs (correctly shown
in your summary above), but DXE_CORE is there for the bulk of the boot,
unremovably.
I don't actually know if DxeCore uses TimerLib. Let's see in the build
report... Hm, yes, alas, DxeCore does uses TimerLib:
Module Summary
Module Name: DxeCore
Module INF Path: MdeModulePkg/Core/Dxe/DxeMain.inf
File GUID: D6A2CB7F-6A18-4e2f-B43B-9920A733700A
Size: 0x38A80 (226.62K)
Build Time Stamp: 1970-01-01 01:00:00
Driver Type: 0x5 (DXE_CORE)
[...]
Library
[...]
/.../OvmfPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
{TimerLib: C = AcpiTimerLibConstructor}
[...]
I guess Gabriel could count how many calls end up in the BaseTimerLib
instance. It's probably not significant. FWIW current master reads the
host bridge ID on every call, and it's not noticeable to me.
> I think this is also related to the various places we do 'if (Xen)
> elif (Qemu+FwCfg) else'. In those cases we do various other detection
> mechanisms, and I'm not real thrilled with how it turned out.
>
> What do you think? Does this seem like a reasonable plan for handling
> how OVMF supports the various platform types?
I don't know :)
For "end" packages other than OvmfPkg, different machines and different
"machine types" warrant different DSCs and different FDFs (or at least
some different define's), which then usually allow for static code
(FixedAtBuild and Feature PCDs).
We're making it a harder for ourselves trying to do everything
dynamically. (But it's probably easier for the end user I guess... Plus,
with different DSCs / FDFs we might have to build everything twice or
thrice. Ouch.)
Extracting the machine-type dependent bits into a separate library seems
reasonable, but I'm quite convinced it will be retro-fitted, and that it
will lack any kind of "design". We'll just collect and sequester the
ugly bits behind a header file and/or some PCDs. I don't mind of course :)
> I do think your OvmfPlatformsLib idea is preferable always reading the
> host bridge ID every time via a macro. I'm just not sure it makes
> sense to add 'yet another library' for this.
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.
But, again, it does sound good to me.
> Another idea would be to
> update PlatformHookLib to have a function for reading a platform
> ID. (That library is not well spec'd, so I guess we could argue that
> just about anything could be put in there. :)
"Ugh". :)
Let's go with BaseTimerLib / DxeTimerLib.
Thanks
Laszlo
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
