| Message ID | 1457102794-25499-4-git-send-email-lersek@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On 03/04/16 21:38, Jordan Justen wrote: > On 2016-03-04 06:46:32, Laszlo Ersek wrote: >> This library is a trivial unification of the following two PciLib >> instances (and the result is easily diffable against each): >> - MdePkg/Library/BasePciLibCf8 >> - MdePkg/Library/BasePciLibPciExpress >> > > Mike, what would you think about a MdePkg/Library/BasePciLibPcieCf8 > lib instance that selected the access mode by a PCD? Is this too > platform specific for MdePkg? > > Laszlo, if Mike doesn't think this is reasonable for MdePkg, then how > about the lib name I suggested above instead? In other words make it a > PCIe vs. CF8 switch, and nothing to do with 440FX vs. Q35. Works for me, sure. However, the branching would still depend on the OVMF platform type. Is that okay? Thanks! Laszlo > -Jordan > >> The PCI config access method is determined in the constructor function, >> from the dynamic PCD "PcdOvmfHostBridgePciDevId" that is set by >> PlatformPei. >> >> The library instance is usable in DXE phase or later modules: the PciLib >> instances being unified have no firmware phase / client module type >> restrictions, and here the only PCD access is made in the constructor >> function. That is, even before a given client executable's entry point is >> invoked. >> >> The library instance depends on PlatformPei both for setting the PCD >> mentioned above, and also for enabling MMCONFIG on Q35. PEI and earlier >> phase modules are not expected to need extended config access even on Q35. >> >> Cc: Gabriel Somlo <somlo@cmu.edu> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Cc: Michał Zegan <webczat_200@poczta.onet.pl> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf | 47 ++++++ >> {MdePkg/Library/BasePciLibCf8 => OvmfPkg/Library/DxePciLibI440FxQ35}/PciLib.c | 161 +++++++++++++++----- >> 2 files changed, 173 insertions(+), 35 deletions(-) >> >> diff --git a/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf b/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >> new file mode 100644 >> index 000000000000..2bd10cc23282 >> --- /dev/null >> +++ b/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >> @@ -0,0 +1,47 @@ >> +## @file >> +# An instance of the PCI Library that is based on both the PCI CF8 Library and >> +# the PCI Express Library. >> +# >> +# This PciLib instance caches the OVMF platform type (I440FX vs. Q35) in >> +# its entry point function, then delegates function calls to one of the >> +# PciCf8Lib or PciExpressLib "backends" as appropriate. >> +# >> +# Copyright (C) 2016, Red Hat, Inc. >> +# >> +# Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.<BR> >> +# >> +# This program and the accompanying materials are licensed and made available >> +# under the terms and conditions of the BSD License which accompanies this >> +# distribution. The full text of the license may be found at >> +# http://opensource.org/licenses/bsd-license.php. >> +# >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >> +# IMPLIED. >> +## >> + >> +[Defines] >> + INF_VERSION = 0x00010005 >> + BASE_NAME = DxePciLibI440FxQ35 >> + FILE_GUID = 5360bff6-3911-4495-ae3c-b02ff004b585 >> + MODULE_TYPE = BASE >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = PciLib|DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION >> + CONSTRUCTOR = InitializeConfigAccessMethod >> + >> +# VALID_ARCHITECTURES = IA32 X64 >> + >> +[Sources] >> + PciLib.c >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + OvmfPkg/OvmfPkg.dec >> + >> +[LibraryClasses] >> + PcdLib >> + PciCf8Lib >> + PciExpressLib >> + >> +[Pcd] >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId >> diff --git a/MdePkg/Library/BasePciLibCf8/PciLib.c b/OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c >> similarity index 85% >> copy from MdePkg/Library/BasePciLibCf8/PciLib.c >> copy to OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c >> index f5f21475d8bd..6c1a272973af 100644 >> --- a/MdePkg/Library/BasePciLibCf8/PciLib.c >> +++ b/OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c >> @@ -1,6 +1,14 @@ >> /** @file >> - PCI Library functions that use I/O ports 0xCF8 and 0xCFC to perform >> - PCI Configuration cycles. Layers on top of one PCI CF8 Library instance. >> + PCI Library functions that use >> + (a) I/O ports 0xCF8 and 0xCFC to perform PCI Configuration cycles, layering >> + on top of one PCI CF8 Library instance; or >> + (b) PCI Library functions that use the 256 MB PCI Express MMIO window to >> + perform PCI Configuration cycles, layering on PCI Express Library. >> + >> + The decision is made in the entry point function, based on the OVMF platform >> + type, and then adhered to during the lifetime of the client module. >> + >> + Copyright (C) 2016, Red Hat, Inc. >> >> Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR> >> This program and the accompanying materials >> @@ -16,8 +24,25 @@ >> >> #include <Base.h> >> >> +#include <IndustryStandard/Q35MchIch9.h> >> + >> #include <Library/PciLib.h> >> #include <Library/PciCf8Lib.h> >> +#include <Library/PciExpressLib.h> >> +#include <Library/PcdLib.h> >> + >> +STATIC BOOLEAN mRunningOnQ35; >> + >> +RETURN_STATUS >> +EFIAPI >> +InitializeConfigAccessMethod ( >> + VOID >> + ) >> +{ >> + mRunningOnQ35 = (PcdGet16 (PcdOvmfHostBridgePciDevId) == >> + INTEL_Q35_MCH_DEVICE_ID); >> + return RETURN_SUCCESS; >> +} >> >> /** >> Registers a PCI device so PCI configuration registers may be accessed after >> @@ -46,7 +71,9 @@ PciRegisterForRuntimeAccess ( >> IN UINTN Address >> ) >> { >> - return PciCf8RegisterForRuntimeAccess (Address); >> + return mRunningOnQ35 ? >> + PciExpressRegisterForRuntimeAccess (Address) : >> + PciCf8RegisterForRuntimeAccess (Address); >> } >> >> /** >> @@ -70,7 +97,9 @@ PciRead8 ( >> IN UINTN Address >> ) >> { >> - return PciCf8Read8 (Address); >> + return mRunningOnQ35 ? >> + PciExpressRead8 (Address) : >> + PciCf8Read8 (Address); >> } >> >> /** >> @@ -96,7 +125,9 @@ PciWrite8 ( >> IN UINT8 Value >> ) >> { >> - return PciCf8Write8 (Address, Value); >> + return mRunningOnQ35 ? >> + PciExpressWrite8 (Address, Value) : >> + PciCf8Write8 (Address, Value); >> } >> >> /** >> @@ -126,7 +157,9 @@ PciOr8 ( >> IN UINT8 OrData >> ) >> { >> - return PciCf8Or8 (Address, OrData); >> + return mRunningOnQ35 ? >> + PciExpressOr8 (Address, OrData) : >> + PciCf8Or8 (Address, OrData); >> } >> >> /** >> @@ -156,7 +189,9 @@ PciAnd8 ( >> IN UINT8 AndData >> ) >> { >> - return PciCf8And8 (Address, AndData); >> + return mRunningOnQ35 ? >> + PciExpressAnd8 (Address, AndData) : >> + PciCf8And8 (Address, AndData); >> } >> >> /** >> @@ -189,7 +224,9 @@ PciAndThenOr8 ( >> IN UINT8 OrData >> ) >> { >> - return PciCf8AndThenOr8 (Address, AndData, OrData); >> + return mRunningOnQ35 ? >> + PciExpressAndThenOr8 (Address, AndData, OrData) : >> + PciCf8AndThenOr8 (Address, AndData, OrData); >> } >> >> /** >> @@ -221,7 +258,9 @@ PciBitFieldRead8 ( >> IN UINTN EndBit >> ) >> { >> - return PciCf8BitFieldRead8 (Address, StartBit, EndBit); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldRead8 (Address, StartBit, EndBit) : >> + PciCf8BitFieldRead8 (Address, StartBit, EndBit); >> } >> >> /** >> @@ -257,7 +296,9 @@ PciBitFieldWrite8 ( >> IN UINT8 Value >> ) >> { >> - return PciCf8BitFieldWrite8 (Address, StartBit, EndBit, Value); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldWrite8 (Address, StartBit, EndBit, Value) : >> + PciCf8BitFieldWrite8 (Address, StartBit, EndBit, Value); >> } >> >> /** >> @@ -296,7 +337,9 @@ PciBitFieldOr8 ( >> IN UINT8 OrData >> ) >> { >> - return PciCf8BitFieldOr8 (Address, StartBit, EndBit, OrData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldOr8 (Address, StartBit, EndBit, OrData) : >> + PciCf8BitFieldOr8 (Address, StartBit, EndBit, OrData); >> } >> >> /** >> @@ -335,7 +378,9 @@ PciBitFieldAnd8 ( >> IN UINT8 AndData >> ) >> { >> - return PciCf8BitFieldAnd8 (Address, StartBit, EndBit, AndData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldAnd8 (Address, StartBit, EndBit, AndData) : >> + PciCf8BitFieldAnd8 (Address, StartBit, EndBit, AndData); >> } >> >> /** >> @@ -379,7 +424,9 @@ PciBitFieldAndThenOr8 ( >> IN UINT8 OrData >> ) >> { >> - return PciCf8BitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData) : >> + PciCf8BitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData); >> } >> >> /** >> @@ -404,7 +451,9 @@ PciRead16 ( >> IN UINTN Address >> ) >> { >> - return PciCf8Read16 (Address); >> + return mRunningOnQ35 ? >> + PciExpressRead16 (Address) : >> + PciCf8Read16 (Address); >> } >> >> /** >> @@ -431,7 +480,9 @@ PciWrite16 ( >> IN UINT16 Value >> ) >> { >> - return PciCf8Write16 (Address, Value); >> + return mRunningOnQ35 ? >> + PciExpressWrite16 (Address, Value) : >> + PciCf8Write16 (Address, Value); >> } >> >> /** >> @@ -462,7 +513,9 @@ PciOr16 ( >> IN UINT16 OrData >> ) >> { >> - return PciCf8Or16 (Address, OrData); >> + return mRunningOnQ35 ? >> + PciExpressOr16 (Address, OrData) : >> + PciCf8Or16 (Address, OrData); >> } >> >> /** >> @@ -493,7 +546,9 @@ PciAnd16 ( >> IN UINT16 AndData >> ) >> { >> - return PciCf8And16 (Address, AndData); >> + return mRunningOnQ35 ? >> + PciExpressAnd16 (Address, AndData) : >> + PciCf8And16 (Address, AndData); >> } >> >> /** >> @@ -527,7 +582,9 @@ PciAndThenOr16 ( >> IN UINT16 OrData >> ) >> { >> - return PciCf8AndThenOr16 (Address, AndData, OrData); >> + return mRunningOnQ35 ? >> + PciExpressAndThenOr16 (Address, AndData, OrData) : >> + PciCf8AndThenOr16 (Address, AndData, OrData); >> } >> >> /** >> @@ -560,7 +617,9 @@ PciBitFieldRead16 ( >> IN UINTN EndBit >> ) >> { >> - return PciCf8BitFieldRead16 (Address, StartBit, EndBit); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldRead16 (Address, StartBit, EndBit) : >> + PciCf8BitFieldRead16 (Address, StartBit, EndBit); >> } >> >> /** >> @@ -597,7 +656,9 @@ PciBitFieldWrite16 ( >> IN UINT16 Value >> ) >> { >> - return PciCf8BitFieldWrite16 (Address, StartBit, EndBit, Value); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldWrite16 (Address, StartBit, EndBit, Value) : >> + PciCf8BitFieldWrite16 (Address, StartBit, EndBit, Value); >> } >> >> /** >> @@ -637,7 +698,9 @@ PciBitFieldOr16 ( >> IN UINT16 OrData >> ) >> { >> - return PciCf8BitFieldOr16 (Address, StartBit, EndBit, OrData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldOr16 (Address, StartBit, EndBit, OrData) : >> + PciCf8BitFieldOr16 (Address, StartBit, EndBit, OrData); >> } >> >> /** >> @@ -677,7 +740,9 @@ PciBitFieldAnd16 ( >> IN UINT16 AndData >> ) >> { >> - return PciCf8BitFieldAnd16 (Address, StartBit, EndBit, AndData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldAnd16 (Address, StartBit, EndBit, AndData) : >> + PciCf8BitFieldAnd16 (Address, StartBit, EndBit, AndData); >> } >> >> /** >> @@ -722,7 +787,9 @@ PciBitFieldAndThenOr16 ( >> IN UINT16 OrData >> ) >> { >> - return PciCf8BitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData) : >> + PciCf8BitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData); >> } >> >> /** >> @@ -747,7 +814,9 @@ PciRead32 ( >> IN UINTN Address >> ) >> { >> - return PciCf8Read32 (Address); >> + return mRunningOnQ35 ? >> + PciExpressRead32 (Address) : >> + PciCf8Read32 (Address); >> } >> >> /** >> @@ -774,7 +843,9 @@ PciWrite32 ( >> IN UINT32 Value >> ) >> { >> - return PciCf8Write32 (Address, Value); >> + return mRunningOnQ35 ? >> + PciExpressWrite32 (Address, Value) : >> + PciCf8Write32 (Address, Value); >> } >> >> /** >> @@ -805,7 +876,9 @@ PciOr32 ( >> IN UINT32 OrData >> ) >> { >> - return PciCf8Or32 (Address, OrData); >> + return mRunningOnQ35 ? >> + PciExpressOr32 (Address, OrData) : >> + PciCf8Or32 (Address, OrData); >> } >> >> /** >> @@ -836,7 +909,9 @@ PciAnd32 ( >> IN UINT32 AndData >> ) >> { >> - return PciCf8And32 (Address, AndData); >> + return mRunningOnQ35 ? >> + PciExpressAnd32 (Address, AndData) : >> + PciCf8And32 (Address, AndData); >> } >> >> /** >> @@ -870,7 +945,9 @@ PciAndThenOr32 ( >> IN UINT32 OrData >> ) >> { >> - return PciCf8AndThenOr32 (Address, AndData, OrData); >> + return mRunningOnQ35 ? >> + PciExpressAndThenOr32 (Address, AndData, OrData) : >> + PciCf8AndThenOr32 (Address, AndData, OrData); >> } >> >> /** >> @@ -903,7 +980,9 @@ PciBitFieldRead32 ( >> IN UINTN EndBit >> ) >> { >> - return PciCf8BitFieldRead32 (Address, StartBit, EndBit); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldRead32 (Address, StartBit, EndBit) : >> + PciCf8BitFieldRead32 (Address, StartBit, EndBit); >> } >> >> /** >> @@ -940,7 +1019,9 @@ PciBitFieldWrite32 ( >> IN UINT32 Value >> ) >> { >> - return PciCf8BitFieldWrite32 (Address, StartBit, EndBit, Value); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldWrite32 (Address, StartBit, EndBit, Value) : >> + PciCf8BitFieldWrite32 (Address, StartBit, EndBit, Value); >> } >> >> /** >> @@ -980,7 +1061,9 @@ PciBitFieldOr32 ( >> IN UINT32 OrData >> ) >> { >> - return PciCf8BitFieldOr32 (Address, StartBit, EndBit, OrData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldOr32 (Address, StartBit, EndBit, OrData) : >> + PciCf8BitFieldOr32 (Address, StartBit, EndBit, OrData); >> } >> >> /** >> @@ -1020,7 +1103,9 @@ PciBitFieldAnd32 ( >> IN UINT32 AndData >> ) >> { >> - return PciCf8BitFieldAnd32 (Address, StartBit, EndBit, AndData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldAnd32 (Address, StartBit, EndBit, AndData) : >> + PciCf8BitFieldAnd32 (Address, StartBit, EndBit, AndData); >> } >> >> /** >> @@ -1065,7 +1150,9 @@ PciBitFieldAndThenOr32 ( >> IN UINT32 OrData >> ) >> { >> - return PciCf8BitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData); >> + return mRunningOnQ35 ? >> + PciExpressBitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData) : >> + PciCf8BitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData); >> } >> >> /** >> @@ -1099,7 +1186,9 @@ PciReadBuffer ( >> OUT VOID *Buffer >> ) >> { >> - return PciCf8ReadBuffer (StartAddress, Size, Buffer); >> + return mRunningOnQ35 ? >> + PciExpressReadBuffer (StartAddress, Size, Buffer) : >> + PciCf8ReadBuffer (StartAddress, Size, Buffer); >> } >> >> /** >> @@ -1134,5 +1223,7 @@ PciWriteBuffer ( >> IN VOID *Buffer >> ) >> { >> - return PciCf8WriteBuffer (StartAddress, Size, Buffer); >> + return mRunningOnQ35 ? >> + PciExpressWriteBuffer (StartAddress, Size, Buffer) : >> + PciCf8WriteBuffer (StartAddress, Size, Buffer); >> } >> -- >> 1.8.3.1 >> >>
On 03/04/16 22:05, Laszlo Ersek wrote: > On 03/04/16 21:38, Jordan Justen wrote: >> On 2016-03-04 06:46:32, Laszlo Ersek wrote: >>> This library is a trivial unification of the following two PciLib >>> instances (and the result is easily diffable against each): >>> - MdePkg/Library/BasePciLibCf8 >>> - MdePkg/Library/BasePciLibPciExpress >>> >> >> Mike, what would you think about a MdePkg/Library/BasePciLibPcieCf8 >> lib instance that selected the access mode by a PCD? Is this too >> platform specific for MdePkg? >> >> Laszlo, if Mike doesn't think this is reasonable for MdePkg, then how >> about the lib name I suggested above instead? In other words make it a >> PCIe vs. CF8 switch, and nothing to do with 440FX vs. Q35. > > Works for me, sure. > > However, the branching would still depend on the OVMF platform type. Is > that okay? Also, since I intend to support only DXE & later modules with it, I thought you'd want a Dxe* prefix. Thanks Laszlo >>> The PCI config access method is determined in the constructor function, >>> from the dynamic PCD "PcdOvmfHostBridgePciDevId" that is set by >>> PlatformPei. >>> >>> The library instance is usable in DXE phase or later modules: the PciLib >>> instances being unified have no firmware phase / client module type >>> restrictions, and here the only PCD access is made in the constructor >>> function. That is, even before a given client executable's entry point is >>> invoked. >>> >>> The library instance depends on PlatformPei both for setting the PCD >>> mentioned above, and also for enabling MMCONFIG on Q35. PEI and earlier >>> phase modules are not expected to need extended config access even on Q35. >>> >>> Cc: Gabriel Somlo <somlo@cmu.edu> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Marcel Apfelbaum <marcel@redhat.com> >>> Cc: Michał Zegan <webczat_200@poczta.onet.pl> >>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>> --- >>> OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf | 47 ++++++ >>> {MdePkg/Library/BasePciLibCf8 => OvmfPkg/Library/DxePciLibI440FxQ35}/PciLib.c | 161 +++++++++++++++----- >>> 2 files changed, 173 insertions(+), 35 deletions(-)
On 03/05/16 00:47, Kinney, Michael D wrote: > Jordan, > > Given these libs are Base type, a Dynamic PCD could not be used. Only a fixed > or patchable PCD could be used, and if you go through path, you might as well > pick the right lib in the DSC at build time and get smaller size and better > performance. I'd like to discuss two things here. * First, I fully agree with you that determining the branch in this unified library instance *at build time* is functionally equivalent (but is size- and perf-wise inferior) to simply picking the right one of the currently available PciLib instances, at build time. I think that this library instance is special to virtualization. On the bare metal, you never face a situation where you don't know in advance whether your platform will be a PCIe system, or a PCI-only system. But, that's exactly what we have with QEMU (the i440fx and Q35 machine types), so this dynamism is needed. (In fact I find it very nice that the modularity of edk2 allowed me to do such a simple unification.) Another possibility for influencing the branch to take would be a GUID HOB. In this particular case however I only saw downsides in a HOB. On the source code side, it would take another GUID definition, another structure definition, possibly another header file. And on the functionality side, the only "extension" it would buy us would be the usability of the library instance in the DXE_CORE. (The DXE_CORE cannot use PCDs, but it can look at HOBs.) We couldn't use it in PEI anyway (without ugly depex trickery), because a PEIM using this library instance could be dispatched before the PEIM that produces the HOB. But, in OVMF, PEI is perfectly fine with just 0xCF8 access anyway. * Second, any given library instance has two kinds of "qualification" in its INF file, as far as client modules are concerned. One is MODULE_TYPE, and the other is the restriction list in LIBRARY_CLASS. These qualifications seem to overlap, in a (superficially) confusing manner: (a) the INF file spec says about MODULE_TYPE, For Library Modules, the MODULE_TYPE must specify the MODULE_TYPE of the module that will use the driver. Furthermore, in appendix G, MODULE_TYPE=BASE is explicitly allowed for libraries, and then BASE is documented as: Modules of this type can be ported to any execution environment. This module type is intended to be use by silicon module developers to produce source code that is not tied to any specific execution environment. (b) But the INF file also says Each LIBRARY_CLASS statement must provide the name of the library class supported, followed by the pipe "|" field separator and then a space " " delimited list of module types the library instances supports. Thus a conflict appears between MODULE_TYPE = BASE and LIBRARY_CLASS = WhateverLib|DXE_DRIVER because the former implies "any execution environment", whereas the second implies the DXE phase. However, this apparent conflict is resolved by the INF spec in the following: The MODULE_TYPE entry in the [Defines] section for a library only defines the module type that the build system must assume for building the library. It does not define the types of modules that a library may be linked with. Instead, the LIBRARY_CLASS entry in the [Defines] section optionally lists the supported module types that the library may be linked against. Therefore, the MODULE_TYPE for a library instance only determines prototype / calling convention compatibility. In my experience, this practically boils down to two things, and nothing more: - functions must return RETURN_STATUS vs. EFI_STATUS, - the constructor function of the library instance (if any) gets VOID if the MODULE_TYPE is BASE, vs. ImageHandle plus SystemTable if the MODULE_TYPE is DXE_DRIVER. That's it. What *really* matters is the restriction list in LIBRARY_CLASS; that's where the author of the library instance ensures that the instance doesn't get linked into a module (and firmware phase) that cannot provide the library instance with the necessary environment. Therefore, making a library instance MODULE_TYPE=BASE, and then restricting it to the DXE phase and later with LIBRARY_CLASS, is valid in my opinion. The library instance may not need ImageHandle and SystemTable for its own code at all, it is okay with returning RETURN_STATUS values, but it might need the environment to be DXE or later. The question then emerges, how we should *name* the library instance. In my experience, the name is supposed to reflect the actual client module / firmware phase restrictions. Which is why I named this instance DxeXXXXXX. In summary, if there is one bit of inconsistency in this patch, then that is the MODULE_TYPE setting. The name prefix (Dxe*) and the LIBRARY_CLASS restriction list are correct, they reflect my intent and reality. If you or Jordan wish that I flip MODULE_TYPE to DXE_DRIVER, I can certainly try that (all of the listed module types are able to provide ImageHandle and SystemTable to the constructor function). I think it's unnecessary (the code never directly uses ImageHandle or SystemTable), but if you think it would be more consistent, I can do that. So: - I can change MODULE_TYPE to DXE_DRIVER if you guys want that (although I think it's unnecessary), - I can also replace the "I440FxQ35" *suffix* with "PcieCf8" (although the actual functionality would still have to be based on the OVMF platform type, for which we already have a PCD in OVMF). > If we wanted to restrict the use of this new PciLib to PEIMs that run after the > PCD PEIM and DXE/SMM drivers that run after the PCD DXE driver, then a new > PciLib in MdePkg is possible. A PCD can be defined for the access method > CF8/MMCFG. An OVMF specific platform PEIM runs early (but after PCD PEIM) > and detects the if the currently running platform is 440FX or Q35. I see two problems with this: - Small problem: for OVMF specifically, we'd need to set yet another PCD for this purpose (near the spot where we set the platform type PCD already.) - Large problem: although such a centralized library instance could express the dependency on the PCD PPI / Protocol -- simply by depending on the phase-matching PcdLib instance --, it could not easily express the dependency on some code *actually setting* the PCD to the right value. For platforms that made the PCD fixed or patchable, this would be no problems, but for a platform where the PCD is dynamic, this could be misleading. For OVMF specifically, it would *not* be a problem. OVMF would never link the library into modules that run before the DXE phase, and the PCD would be set in PEI. However, if another platform would like to link the library into PEIMs as well, *and* keep the PCD dynamic, the build system itself would not catch the unspecified dispatch order. The platform would have to resort to BEFORE or AFTER directives, or order the relevant PEIMs with an APRIORI file. I find that *very* obscure for a core library instance that is supposed to be linkable into anything at all. In summary, this patch indeed makes the library instance (living under OvmfPkg) more restrictive than it could theoretically be, but it's just right for OVMF, *plus* any mis-use is immediately caught at build time. Whereas generalizing the library into a core module would: - not benefit actual physical platforms, - place obscure, explicit ordering requirements on platforms that would like to set the PCD dynamically in firmware phase P, then also consume the PCD -- through the library instance -- in the same phase P. (OVMF would *happen* to be immune to this problem, because the setting and the consuming are in separate phases.) > 2 options from this point: > > 1) Call LibPcdSetSku() in PcdLib to set the SKU, so Dynamic PCD values that are > specific to 440FX or Q35 are available. OVMF DSC file is updated for > multiple SKUs with the SKU specific settings including the new PCD for > the PCI Config access method. > > 2) Use PcdSetxx() to set the PCI Config access method PCD to the > appropriate value. > > I would recommend (1) if there is more than one PCD values that needs to > be different for 440FX and Q35. I've never heard of LibPcdSetSku(). :) In retrospect it's not surprising, there's not one call in the open source edk2 tree to LibPcdSetSku(). The function comments in "MdePkg/Include/Library/PcdLib.h" say: This function provides a means by which SKU support can be established in the PCD infrastructure. Sets the current SKU in the PCD database to the value specified by SkuId. SkuId is returned. If SkuId >= PCD_MAX_SKU_ID, then ASSERT(). @param SkuId The SKU value that will be used when the PCD service retrieves and sets values associated with a PCD token. Then, looking at the protocol member function that implements this, DxePcdSetSku() in "MdeModulePkg/Universal/PCD/Dxe/Pcd.c": Sets the SKU value for subsequent calls to set or get PCD token values. SetSku() sets the SKU Id to be used for subsequent calls to set or get PCD values. SetSku() is normally called only once by the system. For each item (token), the database can hold a single value that applies to all SKUs, or multiple values, where each value is associated with a specific SKU Id. Items with multiple, SKU-specific values are called SKU enabled. The SKU Id of zero is reserved as a default. The valid SkuId range is 1 to 255. For tokens that are not SKU enabled, the system ignores any set SKU Id and works with the single value for that token. For SKU-enabled tokens, the system will use the SKU Id set by the last call to SetSku(). If no SKU Id is set or the currently set SKU Id isn't valid for the specified token, the system uses the default SKU Id. If the system attempts to use the default SKU Id and no value has been set for that Id, the results are unpredictable. @param[in] SkuId The SKU value that will be used when the PCD service will retrieve and set values associated with a PCD token. This confirms my fears, actually. I think that multi-valued PCDs (dependent on SKU enablement or whatever else) are a terrible idea for OVMF. It enables the possibility that getting a dynamic PCD on the opposite sides of a SetSku() call will return different values, without ever touching the PCD itself. It can also cause practically-fixed PCDs (i.e. such that are not suitable for PcdSet calls()) to change value. I find this very hard to understand, debug, or *grep* for, and it would introduce yet more ordering requirements ("set the SKU as early as possible"). Hence my preference would definitely be (2), as far as I'm responsible for maintaining OvmfPkg. I didn't forget what I wrote just above (under "small problem") -- yes, using two PCDs would be ugly, but using SKU-enabled PCDs would be *much* worse. So here's what I would like, in decreasing order of preference: * Keep the patch, as is. (Most preferred.) * Keep the library instance under OvmfPkg, but set MODULE_TYPE to DXE_DRIVER, and/or replace the "I440FxQ35" suffix with "PcieCf8". * Turn the library instance into a core module. Don't restrict it to DXE and later. Consume a new core PCD, introducing obscure ordering requirements that are not enforced at build time. Set this new PCD explicitly in OVMF's PlatformPei, near the current PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId) call (where we capture the platform type). * Turn the library instance into a core module. Don't restrict it to DXE and later. Consume a new core PCD, introducing obscure ordering requirements that are not enforced at build time. Set SKU-dependent DynamicDefaults for this PCD (and maybe other OVMF PCDs as well) in OVMF's DSC files. Call LibPcdSetSku() in PlatformPei, instead of the current call to PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId). (Least preferred.) Thanks Laszlo > > Best regards, > > Mike > >> -----Original Message----- >> From: Justen, Jordan L >> Sent: Friday, March 4, 2016 12:39 PM >> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Kinney, Michael D >> <michael.d.kinney@intel.com> >> Cc: Gabriel Somlo <somlo@cmu.edu>; Marcel Apfelbaum <marcel@redhat.com>; Michał Zegan >> <webczat_200@poczta.onet.pl>; Ni, Ruiyu <ruiyu.ni@intel.com> >> Subject: Re: [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35 >> >> On 2016-03-04 06:46:32, Laszlo Ersek wrote: >>> This library is a trivial unification of the following two PciLib >>> instances (and the result is easily diffable against each): >>> - MdePkg/Library/BasePciLibCf8 >>> - MdePkg/Library/BasePciLibPciExpress >>> >> >> Mike, what would you think about a MdePkg/Library/BasePciLibPcieCf8 >> lib instance that selected the access mode by a PCD? Is this too >> platform specific for MdePkg? >> >> Laszlo, if Mike doesn't think this is reasonable for MdePkg, then how >> about the lib name I suggested above instead? In other words make it a >> PCIe vs. CF8 switch, and nothing to do with 440FX vs. Q35. >> >> -Jordan >> >>> The PCI config access method is determined in the constructor function, >>> from the dynamic PCD "PcdOvmfHostBridgePciDevId" that is set by >>> PlatformPei. >>> >>> The library instance is usable in DXE phase or later modules: the PciLib >>> instances being unified have no firmware phase / client module type >>> restrictions, and here the only PCD access is made in the constructor >>> function. That is, even before a given client executable's entry point is >>> invoked. >>> >>> The library instance depends on PlatformPei both for setting the PCD >>> mentioned above, and also for enabling MMCONFIG on Q35. PEI and earlier >>> phase modules are not expected to need extended config access even on Q35. >>> >>> Cc: Gabriel Somlo <somlo@cmu.edu> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Marcel Apfelbaum <marcel@redhat.com> >>> Cc: Michał Zegan <webczat_200@poczta.onet.pl> >>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>> --- >>> OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf | 47 >> ++++++ >>> {MdePkg/Library/BasePciLibCf8 => OvmfPkg/Library/DxePciLibI440FxQ35}/PciLib.c | 161 >> +++++++++++++++----- >>> 2 files changed, 173 insertions(+), 35 deletions(-) >>> >>> diff --git a/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >> b/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >>> new file mode 100644 >>> index 000000000000..2bd10cc23282 >>> --- /dev/null >>> +++ b/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >>> @@ -0,0 +1,47 @@ >>> +## @file >>> +# An instance of the PCI Library that is based on both the PCI CF8 Library and >>> +# the PCI Express Library. >>> +# >>> +# This PciLib instance caches the OVMF platform type (I440FX vs. Q35) in >>> +# its entry point function, then delegates function calls to one of the >>> +# PciCf8Lib or PciExpressLib "backends" as appropriate. >>> +# >>> +# Copyright (C) 2016, Red Hat, Inc. >>> +# >>> +# Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.<BR> >>> +# >>> +# This program and the accompanying materials are licensed and made available >>> +# under the terms and conditions of the BSD License which accompanies this >>> +# distribution. The full text of the license may be found at >>> +# http://opensource.org/licenses/bsd-license.php. >>> +# >>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >>> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >>> +# IMPLIED. >>> +## >>> + >>> +[Defines] >>> + INF_VERSION = 0x00010005 >>> + BASE_NAME = DxePciLibI440FxQ35 >>> + FILE_GUID = 5360bff6-3911-4495-ae3c-b02ff004b585 >>> + MODULE_TYPE = BASE >>> + VERSION_STRING = 1.0 >>> + LIBRARY_CLASS = PciLib|DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE >> DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION >>> + CONSTRUCTOR = InitializeConfigAccessMethod >>> + >>> +# VALID_ARCHITECTURES = IA32 X64 >>> + >>> +[Sources] >>> + PciLib.c >>> + >>> +[Packages] >>> + MdePkg/MdePkg.dec >>> + OvmfPkg/OvmfPkg.dec >>> + >>> +[LibraryClasses] >>> + PcdLib >>> + PciCf8Lib >>> + PciExpressLib >>> + >>> +[Pcd] >>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId >>> diff --git a/MdePkg/Library/BasePciLibCf8/PciLib.c >> b/OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c >>> similarity index 85% >>> copy from MdePkg/Library/BasePciLibCf8/PciLib.c >>> copy to OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c >>> index f5f21475d8bd..6c1a272973af 100644 >>> --- a/MdePkg/Library/BasePciLibCf8/PciLib.c >>> +++ b/OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c >>> @@ -1,6 +1,14 @@ >>> /** @file >>> - PCI Library functions that use I/O ports 0xCF8 and 0xCFC to perform >>> - PCI Configuration cycles. Layers on top of one PCI CF8 Library instance. >>> + PCI Library functions that use >>> + (a) I/O ports 0xCF8 and 0xCFC to perform PCI Configuration cycles, layering >>> + on top of one PCI CF8 Library instance; or >>> + (b) PCI Library functions that use the 256 MB PCI Express MMIO window to >>> + perform PCI Configuration cycles, layering on PCI Express Library. >>> + >>> + The decision is made in the entry point function, based on the OVMF platform >>> + type, and then adhered to during the lifetime of the client module. >>> + >>> + Copyright (C) 2016, Red Hat, Inc. >>> >>> Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR> >>> This program and the accompanying materials >>> @@ -16,8 +24,25 @@ >>> >>> #include <Base.h> >>> >>> +#include <IndustryStandard/Q35MchIch9.h> >>> + >>> #include <Library/PciLib.h> >>> #include <Library/PciCf8Lib.h> >>> +#include <Library/PciExpressLib.h> >>> +#include <Library/PcdLib.h> >>> + >>> +STATIC BOOLEAN mRunningOnQ35; >>> + >>> +RETURN_STATUS >>> +EFIAPI >>> +InitializeConfigAccessMethod ( >>> + VOID >>> + ) >>> +{ >>> + mRunningOnQ35 = (PcdGet16 (PcdOvmfHostBridgePciDevId) == >>> + INTEL_Q35_MCH_DEVICE_ID); >>> + return RETURN_SUCCESS; >>> +} >>> >>> /** >>> Registers a PCI device so PCI configuration registers may be accessed after >>> @@ -46,7 +71,9 @@ PciRegisterForRuntimeAccess ( >>> IN UINTN Address >>> ) >>> { >>> - return PciCf8RegisterForRuntimeAccess (Address); >>> + return mRunningOnQ35 ? >>> + PciExpressRegisterForRuntimeAccess (Address) : >>> + PciCf8RegisterForRuntimeAccess (Address); >>> } >>> >>> /** >>> @@ -70,7 +97,9 @@ PciRead8 ( >>> IN UINTN Address >>> ) >>> { >>> - return PciCf8Read8 (Address); >>> + return mRunningOnQ35 ? >>> + PciExpressRead8 (Address) : >>> + PciCf8Read8 (Address); >>> } >>> >>> /** >>> @@ -96,7 +125,9 @@ PciWrite8 ( >>> IN UINT8 Value >>> ) >>> { >>> - return PciCf8Write8 (Address, Value); >>> + return mRunningOnQ35 ? >>> + PciExpressWrite8 (Address, Value) : >>> + PciCf8Write8 (Address, Value); >>> } >>> >>> /** >>> @@ -126,7 +157,9 @@ PciOr8 ( >>> IN UINT8 OrData >>> ) >>> { >>> - return PciCf8Or8 (Address, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressOr8 (Address, OrData) : >>> + PciCf8Or8 (Address, OrData); >>> } >>> >>> /** >>> @@ -156,7 +189,9 @@ PciAnd8 ( >>> IN UINT8 AndData >>> ) >>> { >>> - return PciCf8And8 (Address, AndData); >>> + return mRunningOnQ35 ? >>> + PciExpressAnd8 (Address, AndData) : >>> + PciCf8And8 (Address, AndData); >>> } >>> >>> /** >>> @@ -189,7 +224,9 @@ PciAndThenOr8 ( >>> IN UINT8 OrData >>> ) >>> { >>> - return PciCf8AndThenOr8 (Address, AndData, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressAndThenOr8 (Address, AndData, OrData) : >>> + PciCf8AndThenOr8 (Address, AndData, OrData); >>> } >>> >>> /** >>> @@ -221,7 +258,9 @@ PciBitFieldRead8 ( >>> IN UINTN EndBit >>> ) >>> { >>> - return PciCf8BitFieldRead8 (Address, StartBit, EndBit); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldRead8 (Address, StartBit, EndBit) : >>> + PciCf8BitFieldRead8 (Address, StartBit, EndBit); >>> } >>> >>> /** >>> @@ -257,7 +296,9 @@ PciBitFieldWrite8 ( >>> IN UINT8 Value >>> ) >>> { >>> - return PciCf8BitFieldWrite8 (Address, StartBit, EndBit, Value); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldWrite8 (Address, StartBit, EndBit, Value) : >>> + PciCf8BitFieldWrite8 (Address, StartBit, EndBit, Value); >>> } >>> >>> /** >>> @@ -296,7 +337,9 @@ PciBitFieldOr8 ( >>> IN UINT8 OrData >>> ) >>> { >>> - return PciCf8BitFieldOr8 (Address, StartBit, EndBit, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldOr8 (Address, StartBit, EndBit, OrData) : >>> + PciCf8BitFieldOr8 (Address, StartBit, EndBit, OrData); >>> } >>> >>> /** >>> @@ -335,7 +378,9 @@ PciBitFieldAnd8 ( >>> IN UINT8 AndData >>> ) >>> { >>> - return PciCf8BitFieldAnd8 (Address, StartBit, EndBit, AndData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldAnd8 (Address, StartBit, EndBit, AndData) : >>> + PciCf8BitFieldAnd8 (Address, StartBit, EndBit, AndData); >>> } >>> >>> /** >>> @@ -379,7 +424,9 @@ PciBitFieldAndThenOr8 ( >>> IN UINT8 OrData >>> ) >>> { >>> - return PciCf8BitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData) : >>> + PciCf8BitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData); >>> } >>> >>> /** >>> @@ -404,7 +451,9 @@ PciRead16 ( >>> IN UINTN Address >>> ) >>> { >>> - return PciCf8Read16 (Address); >>> + return mRunningOnQ35 ? >>> + PciExpressRead16 (Address) : >>> + PciCf8Read16 (Address); >>> } >>> >>> /** >>> @@ -431,7 +480,9 @@ PciWrite16 ( >>> IN UINT16 Value >>> ) >>> { >>> - return PciCf8Write16 (Address, Value); >>> + return mRunningOnQ35 ? >>> + PciExpressWrite16 (Address, Value) : >>> + PciCf8Write16 (Address, Value); >>> } >>> >>> /** >>> @@ -462,7 +513,9 @@ PciOr16 ( >>> IN UINT16 OrData >>> ) >>> { >>> - return PciCf8Or16 (Address, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressOr16 (Address, OrData) : >>> + PciCf8Or16 (Address, OrData); >>> } >>> >>> /** >>> @@ -493,7 +546,9 @@ PciAnd16 ( >>> IN UINT16 AndData >>> ) >>> { >>> - return PciCf8And16 (Address, AndData); >>> + return mRunningOnQ35 ? >>> + PciExpressAnd16 (Address, AndData) : >>> + PciCf8And16 (Address, AndData); >>> } >>> >>> /** >>> @@ -527,7 +582,9 @@ PciAndThenOr16 ( >>> IN UINT16 OrData >>> ) >>> { >>> - return PciCf8AndThenOr16 (Address, AndData, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressAndThenOr16 (Address, AndData, OrData) : >>> + PciCf8AndThenOr16 (Address, AndData, OrData); >>> } >>> >>> /** >>> @@ -560,7 +617,9 @@ PciBitFieldRead16 ( >>> IN UINTN EndBit >>> ) >>> { >>> - return PciCf8BitFieldRead16 (Address, StartBit, EndBit); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldRead16 (Address, StartBit, EndBit) : >>> + PciCf8BitFieldRead16 (Address, StartBit, EndBit); >>> } >>> >>> /** >>> @@ -597,7 +656,9 @@ PciBitFieldWrite16 ( >>> IN UINT16 Value >>> ) >>> { >>> - return PciCf8BitFieldWrite16 (Address, StartBit, EndBit, Value); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldWrite16 (Address, StartBit, EndBit, Value) : >>> + PciCf8BitFieldWrite16 (Address, StartBit, EndBit, Value); >>> } >>> >>> /** >>> @@ -637,7 +698,9 @@ PciBitFieldOr16 ( >>> IN UINT16 OrData >>> ) >>> { >>> - return PciCf8BitFieldOr16 (Address, StartBit, EndBit, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldOr16 (Address, StartBit, EndBit, OrData) : >>> + PciCf8BitFieldOr16 (Address, StartBit, EndBit, OrData); >>> } >>> >>> /** >>> @@ -677,7 +740,9 @@ PciBitFieldAnd16 ( >>> IN UINT16 AndData >>> ) >>> { >>> - return PciCf8BitFieldAnd16 (Address, StartBit, EndBit, AndData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldAnd16 (Address, StartBit, EndBit, AndData) : >>> + PciCf8BitFieldAnd16 (Address, StartBit, EndBit, AndData); >>> } >>> >>> /** >>> @@ -722,7 +787,9 @@ PciBitFieldAndThenOr16 ( >>> IN UINT16 OrData >>> ) >>> { >>> - return PciCf8BitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData) >> : >>> + PciCf8BitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData); >>> } >>> >>> /** >>> @@ -747,7 +814,9 @@ PciRead32 ( >>> IN UINTN Address >>> ) >>> { >>> - return PciCf8Read32 (Address); >>> + return mRunningOnQ35 ? >>> + PciExpressRead32 (Address) : >>> + PciCf8Read32 (Address); >>> } >>> >>> /** >>> @@ -774,7 +843,9 @@ PciWrite32 ( >>> IN UINT32 Value >>> ) >>> { >>> - return PciCf8Write32 (Address, Value); >>> + return mRunningOnQ35 ? >>> + PciExpressWrite32 (Address, Value) : >>> + PciCf8Write32 (Address, Value); >>> } >>> >>> /** >>> @@ -805,7 +876,9 @@ PciOr32 ( >>> IN UINT32 OrData >>> ) >>> { >>> - return PciCf8Or32 (Address, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressOr32 (Address, OrData) : >>> + PciCf8Or32 (Address, OrData); >>> } >>> >>> /** >>> @@ -836,7 +909,9 @@ PciAnd32 ( >>> IN UINT32 AndData >>> ) >>> { >>> - return PciCf8And32 (Address, AndData); >>> + return mRunningOnQ35 ? >>> + PciExpressAnd32 (Address, AndData) : >>> + PciCf8And32 (Address, AndData); >>> } >>> >>> /** >>> @@ -870,7 +945,9 @@ PciAndThenOr32 ( >>> IN UINT32 OrData >>> ) >>> { >>> - return PciCf8AndThenOr32 (Address, AndData, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressAndThenOr32 (Address, AndData, OrData) : >>> + PciCf8AndThenOr32 (Address, AndData, OrData); >>> } >>> >>> /** >>> @@ -903,7 +980,9 @@ PciBitFieldRead32 ( >>> IN UINTN EndBit >>> ) >>> { >>> - return PciCf8BitFieldRead32 (Address, StartBit, EndBit); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldRead32 (Address, StartBit, EndBit) : >>> + PciCf8BitFieldRead32 (Address, StartBit, EndBit); >>> } >>> >>> /** >>> @@ -940,7 +1019,9 @@ PciBitFieldWrite32 ( >>> IN UINT32 Value >>> ) >>> { >>> - return PciCf8BitFieldWrite32 (Address, StartBit, EndBit, Value); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldWrite32 (Address, StartBit, EndBit, Value) : >>> + PciCf8BitFieldWrite32 (Address, StartBit, EndBit, Value); >>> } >>> >>> /** >>> @@ -980,7 +1061,9 @@ PciBitFieldOr32 ( >>> IN UINT32 OrData >>> ) >>> { >>> - return PciCf8BitFieldOr32 (Address, StartBit, EndBit, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldOr32 (Address, StartBit, EndBit, OrData) : >>> + PciCf8BitFieldOr32 (Address, StartBit, EndBit, OrData); >>> } >>> >>> /** >>> @@ -1020,7 +1103,9 @@ PciBitFieldAnd32 ( >>> IN UINT32 AndData >>> ) >>> { >>> - return PciCf8BitFieldAnd32 (Address, StartBit, EndBit, AndData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldAnd32 (Address, StartBit, EndBit, AndData) : >>> + PciCf8BitFieldAnd32 (Address, StartBit, EndBit, AndData); >>> } >>> >>> /** >>> @@ -1065,7 +1150,9 @@ PciBitFieldAndThenOr32 ( >>> IN UINT32 OrData >>> ) >>> { >>> - return PciCf8BitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData); >>> + return mRunningOnQ35 ? >>> + PciExpressBitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData) >> : >>> + PciCf8BitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData); >>> } >>> >>> /** >>> @@ -1099,7 +1186,9 @@ PciReadBuffer ( >>> OUT VOID *Buffer >>> ) >>> { >>> - return PciCf8ReadBuffer (StartAddress, Size, Buffer); >>> + return mRunningOnQ35 ? >>> + PciExpressReadBuffer (StartAddress, Size, Buffer) : >>> + PciCf8ReadBuffer (StartAddress, Size, Buffer); >>> } >>> >>> /** >>> @@ -1134,5 +1223,7 @@ PciWriteBuffer ( >>> IN VOID *Buffer >>> ) >>> { >>> - return PciCf8WriteBuffer (StartAddress, Size, Buffer); >>> + return mRunningOnQ35 ? >>> + PciExpressWriteBuffer (StartAddress, Size, Buffer) : >>> + PciCf8WriteBuffer (StartAddress, Size, Buffer); >>> } >>> -- >>> 1.8.3.1 >>> >>>
On 03/07/16 04:51, Jordan Justen wrote: > On 2016-03-06 06:28:26, Laszlo Ersek wrote: [snip] >> So here's what I would like, in decreasing order of preference: >> >> * Keep the patch, as is. (Most preferred.) >> >> * Keep the library instance under OvmfPkg, but set MODULE_TYPE to >> DXE_DRIVER, and/or replace the "I440FxQ35" suffix with "PcieCf8". >> >> * Turn the library instance into a core module. Don't restrict it to >> DXE and later. Consume a new core PCD, introducing obscure ordering >> requirements that are not enforced at build time. >> >> Set this new PCD explicitly in OVMF's PlatformPei, near the current >> >> PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId) >> >> call (where we capture the platform type). >> > > I don't see this as much different than depending on the > PcdOvmfHostBridgePciDevId. > > I prefer this, or the next option, but it is probably better to push > out the SKU thing until later. So, I guess I'd prefer this option for > now. If Mike doesn't think the library makes sense for MdePkg, then > I'd still like to define it generically enough that it *could* live in > MdePkg, even if it ends up in OVMF. (I seem to waste a lot of time on > such pointless things. For example, > OvmfPkg/Include/Library/PlatformFvbLib.h) > > I guess I'm fine with the library as currently defined. So, if you > would prefer that I make these changes, then I guess we can move > forward with what you currently have. I am not theoretically opposed to option #3 (I *am* opposed to option #4 below -- the SKU thing). So, if you were willing to accept this patch as-is (option #1), and were willing to refactor it for option #3 later, I would *greatly* appreciate it. (If the library already existed as described in option #3, I would simply use it without a second thought.) It's just my experience that I usually need two or three turns at this kind of refactoring until I can satisfy your taste with it. Functionally this library instance is trivial, so I'd prefer to move forward, and gladly leave the "upstreaming into core" to you, if that works for you. Thank you Laszlo > -Jordan > >> * Turn the library instance into a core module. Don't restrict it to >> DXE and later. Consume a new core PCD, introducing obscure ordering >> requirements that are not enforced at build time. >> >> Set SKU-dependent DynamicDefaults for this PCD (and maybe other OVMF >> PCDs as well) in OVMF's DSC files. Call LibPcdSetSku() in >> PlatformPei, instead of the current call to >> >> PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId). >> >> (Least preferred.) [snip] _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf b/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf new file mode 100644 index 000000000000..2bd10cc23282 --- /dev/null +++ b/OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf @@ -0,0 +1,47 @@ +## @file +# An instance of the PCI Library that is based on both the PCI CF8 Library and +# the PCI Express Library. +# +# This PciLib instance caches the OVMF platform type (I440FX vs. Q35) in +# its entry point function, then delegates function calls to one of the +# PciCf8Lib or PciExpressLib "backends" as appropriate. +# +# Copyright (C) 2016, Red Hat, Inc. +# +# Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.<BR> +# +# This program and the accompanying materials are licensed and made available +# under the terms and conditions of the BSD License which accompanies this +# distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php. +# +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR +# IMPLIED. +## + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = DxePciLibI440FxQ35 + FILE_GUID = 5360bff6-3911-4495-ae3c-b02ff004b585 + MODULE_TYPE = BASE + VERSION_STRING = 1.0 + LIBRARY_CLASS = PciLib|DXE_DRIVER DXE_RUNTIME_DRIVER SMM_CORE DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION + CONSTRUCTOR = InitializeConfigAccessMethod + +# VALID_ARCHITECTURES = IA32 X64 + +[Sources] + PciLib.c + +[Packages] + MdePkg/MdePkg.dec + OvmfPkg/OvmfPkg.dec + +[LibraryClasses] + PcdLib + PciCf8Lib + PciExpressLib + +[Pcd] + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId diff --git a/MdePkg/Library/BasePciLibCf8/PciLib.c b/OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c similarity index 85% copy from MdePkg/Library/BasePciLibCf8/PciLib.c copy to OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c index f5f21475d8bd..6c1a272973af 100644 --- a/MdePkg/Library/BasePciLibCf8/PciLib.c +++ b/OvmfPkg/Library/DxePciLibI440FxQ35/PciLib.c @@ -1,6 +1,14 @@ /** @file - PCI Library functions that use I/O ports 0xCF8 and 0xCFC to perform - PCI Configuration cycles. Layers on top of one PCI CF8 Library instance. + PCI Library functions that use + (a) I/O ports 0xCF8 and 0xCFC to perform PCI Configuration cycles, layering + on top of one PCI CF8 Library instance; or + (b) PCI Library functions that use the 256 MB PCI Express MMIO window to + perform PCI Configuration cycles, layering on PCI Express Library. + + The decision is made in the entry point function, based on the OVMF platform + type, and then adhered to during the lifetime of the client module. + + Copyright (C) 2016, Red Hat, Inc. Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR> This program and the accompanying materials @@ -16,8 +24,25 @@ #include <Base.h> +#include <IndustryStandard/Q35MchIch9.h> + #include <Library/PciLib.h> #include <Library/PciCf8Lib.h> +#include <Library/PciExpressLib.h> +#include <Library/PcdLib.h> + +STATIC BOOLEAN mRunningOnQ35; + +RETURN_STATUS +EFIAPI +InitializeConfigAccessMethod ( + VOID + ) +{ + mRunningOnQ35 = (PcdGet16 (PcdOvmfHostBridgePciDevId) == + INTEL_Q35_MCH_DEVICE_ID); + return RETURN_SUCCESS; +} /** Registers a PCI device so PCI configuration registers may be accessed after @@ -46,7 +71,9 @@ PciRegisterForRuntimeAccess ( IN UINTN Address ) { - return PciCf8RegisterForRuntimeAccess (Address); + return mRunningOnQ35 ? + PciExpressRegisterForRuntimeAccess (Address) : + PciCf8RegisterForRuntimeAccess (Address); } /** @@ -70,7 +97,9 @@ PciRead8 ( IN UINTN Address ) { - return PciCf8Read8 (Address); + return mRunningOnQ35 ? + PciExpressRead8 (Address) : + PciCf8Read8 (Address); } /** @@ -96,7 +125,9 @@ PciWrite8 ( IN UINT8 Value ) { - return PciCf8Write8 (Address, Value); + return mRunningOnQ35 ? + PciExpressWrite8 (Address, Value) : + PciCf8Write8 (Address, Value); } /** @@ -126,7 +157,9 @@ PciOr8 ( IN UINT8 OrData ) { - return PciCf8Or8 (Address, OrData); + return mRunningOnQ35 ? + PciExpressOr8 (Address, OrData) : + PciCf8Or8 (Address, OrData); } /** @@ -156,7 +189,9 @@ PciAnd8 ( IN UINT8 AndData ) { - return PciCf8And8 (Address, AndData); + return mRunningOnQ35 ? + PciExpressAnd8 (Address, AndData) : + PciCf8And8 (Address, AndData); } /** @@ -189,7 +224,9 @@ PciAndThenOr8 ( IN UINT8 OrData ) { - return PciCf8AndThenOr8 (Address, AndData, OrData); + return mRunningOnQ35 ? + PciExpressAndThenOr8 (Address, AndData, OrData) : + PciCf8AndThenOr8 (Address, AndData, OrData); } /** @@ -221,7 +258,9 @@ PciBitFieldRead8 ( IN UINTN EndBit ) { - return PciCf8BitFieldRead8 (Address, StartBit, EndBit); + return mRunningOnQ35 ? + PciExpressBitFieldRead8 (Address, StartBit, EndBit) : + PciCf8BitFieldRead8 (Address, StartBit, EndBit); } /** @@ -257,7 +296,9 @@ PciBitFieldWrite8 ( IN UINT8 Value ) { - return PciCf8BitFieldWrite8 (Address, StartBit, EndBit, Value); + return mRunningOnQ35 ? + PciExpressBitFieldWrite8 (Address, StartBit, EndBit, Value) : + PciCf8BitFieldWrite8 (Address, StartBit, EndBit, Value); } /** @@ -296,7 +337,9 @@ PciBitFieldOr8 ( IN UINT8 OrData ) { - return PciCf8BitFieldOr8 (Address, StartBit, EndBit, OrData); + return mRunningOnQ35 ? + PciExpressBitFieldOr8 (Address, StartBit, EndBit, OrData) : + PciCf8BitFieldOr8 (Address, StartBit, EndBit, OrData); } /** @@ -335,7 +378,9 @@ PciBitFieldAnd8 ( IN UINT8 AndData ) { - return PciCf8BitFieldAnd8 (Address, StartBit, EndBit, AndData); + return mRunningOnQ35 ? + PciExpressBitFieldAnd8 (Address, StartBit, EndBit, AndData) : + PciCf8BitFieldAnd8 (Address, StartBit, EndBit, AndData); } /** @@ -379,7 +424,9 @@ PciBitFieldAndThenOr8 ( IN UINT8 OrData ) { - return PciCf8BitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData); + return mRunningOnQ35 ? + PciExpressBitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData) : + PciCf8BitFieldAndThenOr8 (Address, StartBit, EndBit, AndData, OrData); } /** @@ -404,7 +451,9 @@ PciRead16 ( IN UINTN Address ) { - return PciCf8Read16 (Address); + return mRunningOnQ35 ? + PciExpressRead16 (Address) : + PciCf8Read16 (Address); } /** @@ -431,7 +480,9 @@ PciWrite16 ( IN UINT16 Value ) { - return PciCf8Write16 (Address, Value); + return mRunningOnQ35 ? + PciExpressWrite16 (Address, Value) : + PciCf8Write16 (Address, Value); } /** @@ -462,7 +513,9 @@ PciOr16 ( IN UINT16 OrData ) { - return PciCf8Or16 (Address, OrData); + return mRunningOnQ35 ? + PciExpressOr16 (Address, OrData) : + PciCf8Or16 (Address, OrData); } /** @@ -493,7 +546,9 @@ PciAnd16 ( IN UINT16 AndData ) { - return PciCf8And16 (Address, AndData); + return mRunningOnQ35 ? + PciExpressAnd16 (Address, AndData) : + PciCf8And16 (Address, AndData); } /** @@ -527,7 +582,9 @@ PciAndThenOr16 ( IN UINT16 OrData ) { - return PciCf8AndThenOr16 (Address, AndData, OrData); + return mRunningOnQ35 ? + PciExpressAndThenOr16 (Address, AndData, OrData) : + PciCf8AndThenOr16 (Address, AndData, OrData); } /** @@ -560,7 +617,9 @@ PciBitFieldRead16 ( IN UINTN EndBit ) { - return PciCf8BitFieldRead16 (Address, StartBit, EndBit); + return mRunningOnQ35 ? + PciExpressBitFieldRead16 (Address, StartBit, EndBit) : + PciCf8BitFieldRead16 (Address, StartBit, EndBit); } /** @@ -597,7 +656,9 @@ PciBitFieldWrite16 ( IN UINT16 Value ) { - return PciCf8BitFieldWrite16 (Address, StartBit, EndBit, Value); + return mRunningOnQ35 ? + PciExpressBitFieldWrite16 (Address, StartBit, EndBit, Value) : + PciCf8BitFieldWrite16 (Address, StartBit, EndBit, Value); } /** @@ -637,7 +698,9 @@ PciBitFieldOr16 ( IN UINT16 OrData ) { - return PciCf8BitFieldOr16 (Address, StartBit, EndBit, OrData); + return mRunningOnQ35 ? + PciExpressBitFieldOr16 (Address, StartBit, EndBit, OrData) : + PciCf8BitFieldOr16 (Address, StartBit, EndBit, OrData); } /** @@ -677,7 +740,9 @@ PciBitFieldAnd16 ( IN UINT16 AndData ) { - return PciCf8BitFieldAnd16 (Address, StartBit, EndBit, AndData); + return mRunningOnQ35 ? + PciExpressBitFieldAnd16 (Address, StartBit, EndBit, AndData) : + PciCf8BitFieldAnd16 (Address, StartBit, EndBit, AndData); } /** @@ -722,7 +787,9 @@ PciBitFieldAndThenOr16 ( IN UINT16 OrData ) { - return PciCf8BitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData); + return mRunningOnQ35 ? + PciExpressBitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData) : + PciCf8BitFieldAndThenOr16 (Address, StartBit, EndBit, AndData, OrData); } /** @@ -747,7 +814,9 @@ PciRead32 ( IN UINTN Address ) { - return PciCf8Read32 (Address); + return mRunningOnQ35 ? + PciExpressRead32 (Address) : + PciCf8Read32 (Address); } /** @@ -774,7 +843,9 @@ PciWrite32 ( IN UINT32 Value ) { - return PciCf8Write32 (Address, Value); + return mRunningOnQ35 ? + PciExpressWrite32 (Address, Value) : + PciCf8Write32 (Address, Value); } /** @@ -805,7 +876,9 @@ PciOr32 ( IN UINT32 OrData ) { - return PciCf8Or32 (Address, OrData); + return mRunningOnQ35 ? + PciExpressOr32 (Address, OrData) : + PciCf8Or32 (Address, OrData); } /** @@ -836,7 +909,9 @@ PciAnd32 ( IN UINT32 AndData ) { - return PciCf8And32 (Address, AndData); + return mRunningOnQ35 ? + PciExpressAnd32 (Address, AndData) : + PciCf8And32 (Address, AndData); } /** @@ -870,7 +945,9 @@ PciAndThenOr32 ( IN UINT32 OrData ) { - return PciCf8AndThenOr32 (Address, AndData, OrData); + return mRunningOnQ35 ? + PciExpressAndThenOr32 (Address, AndData, OrData) : + PciCf8AndThenOr32 (Address, AndData, OrData); } /** @@ -903,7 +980,9 @@ PciBitFieldRead32 ( IN UINTN EndBit ) { - return PciCf8BitFieldRead32 (Address, StartBit, EndBit); + return mRunningOnQ35 ? + PciExpressBitFieldRead32 (Address, StartBit, EndBit) : + PciCf8BitFieldRead32 (Address, StartBit, EndBit); } /** @@ -940,7 +1019,9 @@ PciBitFieldWrite32 ( IN UINT32 Value ) { - return PciCf8BitFieldWrite32 (Address, StartBit, EndBit, Value); + return mRunningOnQ35 ? + PciExpressBitFieldWrite32 (Address, StartBit, EndBit, Value) : + PciCf8BitFieldWrite32 (Address, StartBit, EndBit, Value); } /** @@ -980,7 +1061,9 @@ PciBitFieldOr32 ( IN UINT32 OrData ) { - return PciCf8BitFieldOr32 (Address, StartBit, EndBit, OrData); + return mRunningOnQ35 ? + PciExpressBitFieldOr32 (Address, StartBit, EndBit, OrData) : + PciCf8BitFieldOr32 (Address, StartBit, EndBit, OrData); } /** @@ -1020,7 +1103,9 @@ PciBitFieldAnd32 ( IN UINT32 AndData ) { - return PciCf8BitFieldAnd32 (Address, StartBit, EndBit, AndData); + return mRunningOnQ35 ? + PciExpressBitFieldAnd32 (Address, StartBit, EndBit, AndData) : + PciCf8BitFieldAnd32 (Address, StartBit, EndBit, AndData); } /** @@ -1065,7 +1150,9 @@ PciBitFieldAndThenOr32 ( IN UINT32 OrData ) { - return PciCf8BitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData); + return mRunningOnQ35 ? + PciExpressBitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData) : + PciCf8BitFieldAndThenOr32 (Address, StartBit, EndBit, AndData, OrData); } /** @@ -1099,7 +1186,9 @@ PciReadBuffer ( OUT VOID *Buffer ) { - return PciCf8ReadBuffer (StartAddress, Size, Buffer); + return mRunningOnQ35 ? + PciExpressReadBuffer (StartAddress, Size, Buffer) : + PciCf8ReadBuffer (StartAddress, Size, Buffer); } /** @@ -1134,5 +1223,7 @@ PciWriteBuffer ( IN VOID *Buffer ) { - return PciCf8WriteBuffer (StartAddress, Size, Buffer); + return mRunningOnQ35 ? + PciExpressWriteBuffer (StartAddress, Size, Buffer) : + PciCf8WriteBuffer (StartAddress, Size, Buffer); }
This library is a trivial unification of the following two PciLib instances (and the result is easily diffable against each): - MdePkg/Library/BasePciLibCf8 - MdePkg/Library/BasePciLibPciExpress The PCI config access method is determined in the constructor function, from the dynamic PCD "PcdOvmfHostBridgePciDevId" that is set by PlatformPei. The library instance is usable in DXE phase or later modules: the PciLib instances being unified have no firmware phase / client module type restrictions, and here the only PCD access is made in the constructor function. That is, even before a given client executable's entry point is invoked. The library instance depends on PlatformPei both for setting the PCD mentioned above, and also for enabling MMCONFIG on Q35. PEI and earlier phase modules are not expected to need extended config access even on Q35. Cc: Gabriel Somlo <somlo@cmu.edu> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Michał Zegan <webczat_200@poczta.onet.pl> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf | 47 ++++++ {MdePkg/Library/BasePciLibCf8 => OvmfPkg/Library/DxePciLibI440FxQ35}/PciLib.c | 161 +++++++++++++++----- 2 files changed, 173 insertions(+), 35 deletions(-)