Re: [edk2] [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35

2016-03-07 Thread Laszlo Ersek 
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

Re: [edk2] [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35

2016-03-06 Thread Jordan Justen 
On 2016-03-06 06:28:26, Laszlo Ersek wrote:
> 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 DxeXX.
> 
> In summary, if there

Re: [edk2] [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35

2016-03-06 Thread Laszlo Ersek 
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 DxeXX.

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

Re: [edk2] [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35

2016-03-04 Thread Kinney, Michael D 
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.

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.  

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.  

Best regards,

Mike

> -Original Message-
> From: Justen, Jordan L
> Sent: Friday, March 4, 2016 12:39 PM
> To: Laszlo Ersek ; edk2-devel@lists.01.org; Kinney, 
> Michael D
> 
> Cc: Gabriel Somlo ; Marcel Apfelbaum ; 
> Michał Zegan
> ; Ni, Ruiyu 
> 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 
> > Cc: Jordan Justen 
> > Cc: Marcel Apfelbaum 
> > Cc: Michał Zegan 
> > Cc: Ruiyu Ni 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek 
> > ---
> >  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 ..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.
> > +#
> > +#  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

Re: [edk2] [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35

2016-03-04 Thread Laszlo Ersek 
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 
>>> Cc: Jordan Justen 
>>> Cc: Marcel Apfelbaum 
>>> Cc: Michał Zegan 
>>> Cc: Ruiyu Ni 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>  OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf  
>>>|  47 ++
>>>  {MdePkg/Library/BasePciLibCf8 => 
>>> OvmfPkg/Library/DxePciLibI440FxQ35}/PciLib.c | 161 +++-
>>>  2 files changed, 173 insertions(+), 35 deletions(-)

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35

2016-03-04 Thread Laszlo Ersek 
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 
>> Cc: Jordan Justen 
>> Cc: Marcel Apfelbaum 
>> Cc: Michał Zegan 
>> Cc: Ruiyu Ni 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  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 ..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.
>> +#
>> +#  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
>> +

Re: [edk2] [PATCH 3/5] OvmfPkg: add DxePciLibI440FxQ35

2016-03-04 Thread Jordan Justen 
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 
> Cc: Jordan Justen 
> Cc: Marcel Apfelbaum 
> Cc: Michał Zegan 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  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 ..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.
> +#
> +#  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.
>  
>