MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library

[edk2] [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library

Posted by Laszlo Ersek 5 years, 11 months ago

Repo:   https://github.com/lersek/edk2.git
Branch: pci_cap

In message [1], Jordan suggested a general "capabilities helper lib that
could iterate & read the PCI capability structs". Patch #1 introduces
that library (class and central BASE instance) to MdePkg; the library
class is called PciCapLib.

[1] http://mid.mail-archive.com/150696619010.2454.9266470245604846575@jljusten-skl

PciCapLib offers APIs to parse capabilities lists, and to locate,
describe, read and write capabilities.

The library class targets a wide range of client module types; for
example, platform PEIMs, platform DXE drivers, and UEFI drivers that
follow the UEFI Driver Model. For this reason, the library class is
designed with PCI config access abstracted away, even beyond the PPI /
Protocol level, since those cannot cross the PEI / DXE boundary.

This (minimal) config space access method abstraction is called
PCI_CAP_DEV. It is *conceptually* a protocol. Client modules plug
PCI_CAP_DEV instances into PciCapLib for config space access, similarly
to how DiskIo and BlockIo instances are plugged into filesystem drivers.
Similarly to the filesystem driver example, PciCapLib is an example for
the "strategy pattern".

Patches #2 and #3 introduce small "factory" libraries that help client
modules produce PCI_CAP_DEV instances:

- PciCapPciSegmentLib -- incl. class and central BASE instance -- from
  patch #2 produces PCI_CAP_DEV instances on top of PciSegmentLib, using
  the "Segment:Bus:Device.Function" notation. This is meant for platform
  modules (PEIMs and DXE drivers), which generally have a working
  PciSegmentLib instance available.

- PciCapPciIoLib -- incl. class and central UEFI instance -- from patch
  #3 lets client modules produce PCI_CAP_DEV instances on top of
  EFI_PCI_IO_PROTOCOL instances. This is meant for UEFI drivers.

Patches #4 and #5 resolve the three new lib classes to their respective
central instances in the OvmfPkg and ArmVirtPkg DSC files.

Patch #6 converts OvmfPkg/PciHotPlugInitDxe to the new APIs, from manual
capability list parsing. Because PciHotPlugInitDxe is a platform DXE
driver, this conversion uses PciCapPciSegmentLib and PciCapLib.

Patch #7 converts OvmfPkg/Virtio10Dxe to the new APIs, from manual
capability list parsing. Because Virtio10Dxe is a UEFI driver, this
conversion uses PciCapPciIoLib and PciCapLib.

----[ jump to the patches now, and return if you have questions: I
      have answers, but you might find them too verbose in advance ]----

Q1: If PCI config access needs to be abstracted from PciCapLib, then why
    don't we provide multiple instances of PciCapLib? In other words,
    why the PCI_CAP_DEV "protocol" rather than multiple instances of
    PciCapLib?

A1: The point of lib classes and instances is that the class provides a
    common interface, and the instances provide different
    implementations. We have the opposite use case here (hence the
    "strategy pattern"): the implementation of the higher level
    algorithm is common, and the objects that plug into it differ at the
    *interface* level ("Segment:Bus:Device.Function" notation versus
    PciIo protocol).

    None of those notations are suitable for inclusion in the PciCapLib
    *class*; a more abstract interface is needed for identifying the PCI
    device to the library, for parsing of capabilities lists.

Q2: OK, let's say we can't (and shouldn't) include both notations in the
    PciCapLib class; can we settle on one of them, rather than
    PCI_CAP_DEV?

A2: No, we can't. PciIo is obviously unusable in PEIMs (and in quite a
    few platform DXE drivers too).

    It takes a bit longer to explain why the other choice, i.e. adopting
    the "Segment:Bus:Device.Function" notation for the PciCapLib
    interface, is not good either. It has to do with the portability of
    UEFI drivers that bind PciIo protocol instances:

    It is indeed easy enough to call PciIo->GetLocation() and plug the
    output values into a theoretical "Segment:Bus:Device.Function"
    interface. However, about the only two things that could consume
    such location identifiers and actually *implement* config space
    access would be:

    (a) the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read/Write member
        functions, where the member functions themselves take the
        "Bus:Device.Function" part, and the "Segment" part must be used
        for selecting the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instance
        itself, through the SegmentNumber member;

    (b) the PciSegmentLib interfaces.

    Building a PciCapLib instance that uses (a) into a UEFI driver is a
    bad idea, because there are platforms that provide PciIo instances
    -- hence the driver can generally run on them -- but don't provide
    PciRootBridgeIo instances.

    Building a PciCapLib instance that uses (b) into a UEFI driver is
    worse. The PciSegmentLib interfaces are not capable of returning
    errors. For example, if a device appears as PCI Express (implying
    its extended config space exists, potentially containing extended
    capabilities), but the underlying platform does not support extended
    config space access (for example because it only supports
    0xCF8/0xCFC, and not ECAM), then peeking at the extended config
    space of the device may invoke undefined behavior within
    PciSegmentLib, and the PciCapLib instance will be none the wiser.

    (In fact, for the same reason, even PciCapPciSegmentLib has to
    extend the "Segment:Bus:Device.Function" notation slightly; so that
    such issues are caught *before* the call is made to PciSegmentLib.)

Q3: Wait, I thought it wasn't possible for a PCI Express device to
    appear on a platform that lacked ECAM.

A3: Is that a question? :) Yes, it is plenty possible [2]. Same way as
    garbage / looped config spaces are [3].

[2] https://github.com/tianocore/edk2/commit/014b472053ae3
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=02d578e5edd9

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Thanks,
Laszlo

Laszlo Ersek (7):
  MdePkg: introduce PciCapLib
  MdePkg: introduce PciCapPciSegmentLib
  MdePkg: introduce PciCapPciIoLib
  OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
  ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
  OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib
  OvmfPkg/Virtio10Dxe: convert to PciCapLib

 ArmVirtPkg/ArmVirt.dsc.inc                                         |    3 +
 MdePkg/Include/Library/PciCapLib.h                                 |  429 +++++++++
 MdePkg/Include/Library/PciCapPciIoLib.h                            |   58 ++
 MdePkg/Include/Library/PciCapPciSegmentLib.h                       |   82 ++
 MdePkg/Library/BasePciCapLib/BasePciCapLib.c                       | 1007 ++++++++++++++++++++
 MdePkg/Library/BasePciCapLib/BasePciCapLib.h                       |   60 ++
 MdePkg/Library/BasePciCapLib/BasePciCapLib.inf                     |   37 +
 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c   |  226 +++++
 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h   |   47 +
 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf |   34 +
 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c             |  243 +++++
 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h             |   44 +
 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf           |   35 +
 MdePkg/MdePkg.dec                                                  |   14 +
 MdePkg/MdePkg.dsc                                                  |    3 +
 OvmfPkg/Include/IndustryStandard/Virtio10.h                        |    7 +-
 OvmfPkg/OvmfPkgIa32.dsc                                            |    3 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                         |    3 +
 OvmfPkg/OvmfPkgX64.dsc                                             |    3 +
 OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c                         |  267 ++----
 OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf                       |    5 +
 OvmfPkg/Virtio10Dxe/Virtio10.c                                     |  135 +--
 OvmfPkg/Virtio10Dxe/Virtio10.inf                                   |    2 +
 23 files changed, 2485 insertions(+), 262 deletions(-)
 create mode 100644 MdePkg/Include/Library/PciCapLib.h
 create mode 100644 MdePkg/Include/Library/PciCapPciIoLib.h
 create mode 100644 MdePkg/Include/Library/PciCapPciSegmentLib.h
 create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.c
 create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.h
 create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.inf
 create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
 create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
 create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
 create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
 create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
 create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf

-- 
2.14.1.3.gb7cf6e02401b

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

Re: [edk2] [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library

Posted by Laszlo Ersek 5 years, 11 months ago

ping

On 05/04/18 23:36, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: pci_cap
> 
> In message [1], Jordan suggested a general "capabilities helper lib that
> could iterate & read the PCI capability structs". Patch #1 introduces
> that library (class and central BASE instance) to MdePkg; the library
> class is called PciCapLib.
> 
> [1] http://mid.mail-archive.com/150696619010.2454.9266470245604846575@jljusten-skl
> 
> PciCapLib offers APIs to parse capabilities lists, and to locate,
> describe, read and write capabilities.
> 
> The library class targets a wide range of client module types; for
> example, platform PEIMs, platform DXE drivers, and UEFI drivers that
> follow the UEFI Driver Model. For this reason, the library class is
> designed with PCI config access abstracted away, even beyond the PPI /
> Protocol level, since those cannot cross the PEI / DXE boundary.
> 
> This (minimal) config space access method abstraction is called
> PCI_CAP_DEV. It is *conceptually* a protocol. Client modules plug
> PCI_CAP_DEV instances into PciCapLib for config space access, similarly
> to how DiskIo and BlockIo instances are plugged into filesystem drivers.
> Similarly to the filesystem driver example, PciCapLib is an example for
> the "strategy pattern".
> 
> Patches #2 and #3 introduce small "factory" libraries that help client
> modules produce PCI_CAP_DEV instances:
> 
> - PciCapPciSegmentLib -- incl. class and central BASE instance -- from
>   patch #2 produces PCI_CAP_DEV instances on top of PciSegmentLib, using
>   the "Segment:Bus:Device.Function" notation. This is meant for platform
>   modules (PEIMs and DXE drivers), which generally have a working
>   PciSegmentLib instance available.
> 
> - PciCapPciIoLib -- incl. class and central UEFI instance -- from patch
>   #3 lets client modules produce PCI_CAP_DEV instances on top of
>   EFI_PCI_IO_PROTOCOL instances. This is meant for UEFI drivers.
> 
> Patches #4 and #5 resolve the three new lib classes to their respective
> central instances in the OvmfPkg and ArmVirtPkg DSC files.
> 
> Patch #6 converts OvmfPkg/PciHotPlugInitDxe to the new APIs, from manual
> capability list parsing. Because PciHotPlugInitDxe is a platform DXE
> driver, this conversion uses PciCapPciSegmentLib and PciCapLib.
> 
> Patch #7 converts OvmfPkg/Virtio10Dxe to the new APIs, from manual
> capability list parsing. Because Virtio10Dxe is a UEFI driver, this
> conversion uses PciCapPciIoLib and PciCapLib.
> 
> ----[ jump to the patches now, and return if you have questions: I
>       have answers, but you might find them too verbose in advance ]----
> 
> Q1: If PCI config access needs to be abstracted from PciCapLib, then why
>     don't we provide multiple instances of PciCapLib? In other words,
>     why the PCI_CAP_DEV "protocol" rather than multiple instances of
>     PciCapLib?
> 
> A1: The point of lib classes and instances is that the class provides a
>     common interface, and the instances provide different
>     implementations. We have the opposite use case here (hence the
>     "strategy pattern"): the implementation of the higher level
>     algorithm is common, and the objects that plug into it differ at the
>     *interface* level ("Segment:Bus:Device.Function" notation versus
>     PciIo protocol).
> 
>     None of those notations are suitable for inclusion in the PciCapLib
>     *class*; a more abstract interface is needed for identifying the PCI
>     device to the library, for parsing of capabilities lists.
> 
> Q2: OK, let's say we can't (and shouldn't) include both notations in the
>     PciCapLib class; can we settle on one of them, rather than
>     PCI_CAP_DEV?
> 
> A2: No, we can't. PciIo is obviously unusable in PEIMs (and in quite a
>     few platform DXE drivers too).
> 
>     It takes a bit longer to explain why the other choice, i.e. adopting
>     the "Segment:Bus:Device.Function" notation for the PciCapLib
>     interface, is not good either. It has to do with the portability of
>     UEFI drivers that bind PciIo protocol instances:
> 
>     It is indeed easy enough to call PciIo->GetLocation() and plug the
>     output values into a theoretical "Segment:Bus:Device.Function"
>     interface. However, about the only two things that could consume
>     such location identifiers and actually *implement* config space
>     access would be:
> 
>     (a) the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read/Write member
>         functions, where the member functions themselves take the
>         "Bus:Device.Function" part, and the "Segment" part must be used
>         for selecting the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instance
>         itself, through the SegmentNumber member;
> 
>     (b) the PciSegmentLib interfaces.
> 
>     Building a PciCapLib instance that uses (a) into a UEFI driver is a
>     bad idea, because there are platforms that provide PciIo instances
>     -- hence the driver can generally run on them -- but don't provide
>     PciRootBridgeIo instances.
> 
>     Building a PciCapLib instance that uses (b) into a UEFI driver is
>     worse. The PciSegmentLib interfaces are not capable of returning
>     errors. For example, if a device appears as PCI Express (implying
>     its extended config space exists, potentially containing extended
>     capabilities), but the underlying platform does not support extended
>     config space access (for example because it only supports
>     0xCF8/0xCFC, and not ECAM), then peeking at the extended config
>     space of the device may invoke undefined behavior within
>     PciSegmentLib, and the PciCapLib instance will be none the wiser.
> 
>     (In fact, for the same reason, even PciCapPciSegmentLib has to
>     extend the "Segment:Bus:Device.Function" notation slightly; so that
>     such issues are caught *before* the call is made to PciSegmentLib.)
> 
> Q3: Wait, I thought it wasn't possible for a PCI Express device to
>     appear on a platform that lacked ECAM.
> 
> A3: Is that a question? :) Yes, it is plenty possible [2]. Same way as
>     garbage / looped config spaces are [3].
> 
> [2] https://github.com/tianocore/edk2/commit/014b472053ae3
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=02d578e5edd9
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (7):
>   MdePkg: introduce PciCapLib
>   MdePkg: introduce PciCapPciSegmentLib
>   MdePkg: introduce PciCapPciIoLib
>   OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
>   ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
>   OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib
>   OvmfPkg/Virtio10Dxe: convert to PciCapLib
> 
>  ArmVirtPkg/ArmVirt.dsc.inc                                         |    3 +
>  MdePkg/Include/Library/PciCapLib.h                                 |  429 +++++++++
>  MdePkg/Include/Library/PciCapPciIoLib.h                            |   58 ++
>  MdePkg/Include/Library/PciCapPciSegmentLib.h                       |   82 ++
>  MdePkg/Library/BasePciCapLib/BasePciCapLib.c                       | 1007 ++++++++++++++++++++
>  MdePkg/Library/BasePciCapLib/BasePciCapLib.h                       |   60 ++
>  MdePkg/Library/BasePciCapLib/BasePciCapLib.inf                     |   37 +
>  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c   |  226 +++++
>  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h   |   47 +
>  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf |   34 +
>  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c             |  243 +++++
>  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h             |   44 +
>  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf           |   35 +
>  MdePkg/MdePkg.dec                                                  |   14 +
>  MdePkg/MdePkg.dsc                                                  |    3 +
>  OvmfPkg/Include/IndustryStandard/Virtio10.h                        |    7 +-
>  OvmfPkg/OvmfPkgIa32.dsc                                            |    3 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                                         |    3 +
>  OvmfPkg/OvmfPkgX64.dsc                                             |    3 +
>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c                         |  267 ++----
>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf                       |    5 +
>  OvmfPkg/Virtio10Dxe/Virtio10.c                                     |  135 +--
>  OvmfPkg/Virtio10Dxe/Virtio10.inf                                   |    2 +
>  23 files changed, 2485 insertions(+), 262 deletions(-)
>  create mode 100644 MdePkg/Include/Library/PciCapLib.h
>  create mode 100644 MdePkg/Include/Library/PciCapPciIoLib.h
>  create mode 100644 MdePkg/Include/Library/PciCapPciSegmentLib.h
>  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.c
>  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.h
>  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.inf
>  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
>  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
>  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
>  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
>  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
>  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> 

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

Re: [edk2] [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library

Posted by Gao, Liming 5 years, 11 months ago

Laszlo:
  I have not fully understood its goal and design. Could you give me one more week to review it?

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, May 14, 2018 9:16 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library
> 
> ping
> 
> On 05/04/18 23:36, Laszlo Ersek wrote:
> > Repo:   https://github.com/lersek/edk2.git
> > Branch: pci_cap
> >
> > In message [1], Jordan suggested a general "capabilities helper lib that
> > could iterate & read the PCI capability structs". Patch #1 introduces
> > that library (class and central BASE instance) to MdePkg; the library
> > class is called PciCapLib.
> >
> > [1] http://mid.mail-archive.com/150696619010.2454.9266470245604846575@jljusten-skl
> >
> > PciCapLib offers APIs to parse capabilities lists, and to locate,
> > describe, read and write capabilities.
> >
> > The library class targets a wide range of client module types; for
> > example, platform PEIMs, platform DXE drivers, and UEFI drivers that
> > follow the UEFI Driver Model. For this reason, the library class is
> > designed with PCI config access abstracted away, even beyond the PPI /
> > Protocol level, since those cannot cross the PEI / DXE boundary.
> >
> > This (minimal) config space access method abstraction is called
> > PCI_CAP_DEV. It is *conceptually* a protocol. Client modules plug
> > PCI_CAP_DEV instances into PciCapLib for config space access, similarly
> > to how DiskIo and BlockIo instances are plugged into filesystem drivers.
> > Similarly to the filesystem driver example, PciCapLib is an example for
> > the "strategy pattern".
> >
> > Patches #2 and #3 introduce small "factory" libraries that help client
> > modules produce PCI_CAP_DEV instances:
> >
> > - PciCapPciSegmentLib -- incl. class and central BASE instance -- from
> >   patch #2 produces PCI_CAP_DEV instances on top of PciSegmentLib, using
> >   the "Segment:Bus:Device.Function" notation. This is meant for platform
> >   modules (PEIMs and DXE drivers), which generally have a working
> >   PciSegmentLib instance available.
> >
> > - PciCapPciIoLib -- incl. class and central UEFI instance -- from patch
> >   #3 lets client modules produce PCI_CAP_DEV instances on top of
> >   EFI_PCI_IO_PROTOCOL instances. This is meant for UEFI drivers.
> >
> > Patches #4 and #5 resolve the three new lib classes to their respective
> > central instances in the OvmfPkg and ArmVirtPkg DSC files.
> >
> > Patch #6 converts OvmfPkg/PciHotPlugInitDxe to the new APIs, from manual
> > capability list parsing. Because PciHotPlugInitDxe is a platform DXE
> > driver, this conversion uses PciCapPciSegmentLib and PciCapLib.
> >
> > Patch #7 converts OvmfPkg/Virtio10Dxe to the new APIs, from manual
> > capability list parsing. Because Virtio10Dxe is a UEFI driver, this
> > conversion uses PciCapPciIoLib and PciCapLib.
> >
> > ----[ jump to the patches now, and return if you have questions: I
> >       have answers, but you might find them too verbose in advance ]----
> >
> > Q1: If PCI config access needs to be abstracted from PciCapLib, then why
> >     don't we provide multiple instances of PciCapLib? In other words,
> >     why the PCI_CAP_DEV "protocol" rather than multiple instances of
> >     PciCapLib?
> >
> > A1: The point of lib classes and instances is that the class provides a
> >     common interface, and the instances provide different
> >     implementations. We have the opposite use case here (hence the
> >     "strategy pattern"): the implementation of the higher level
> >     algorithm is common, and the objects that plug into it differ at the
> >     *interface* level ("Segment:Bus:Device.Function" notation versus
> >     PciIo protocol).
> >
> >     None of those notations are suitable for inclusion in the PciCapLib
> >     *class*; a more abstract interface is needed for identifying the PCI
> >     device to the library, for parsing of capabilities lists.
> >
> > Q2: OK, let's say we can't (and shouldn't) include both notations in the
> >     PciCapLib class; can we settle on one of them, rather than
> >     PCI_CAP_DEV?
> >
> > A2: No, we can't. PciIo is obviously unusable in PEIMs (and in quite a
> >     few platform DXE drivers too).
> >
> >     It takes a bit longer to explain why the other choice, i.e. adopting
> >     the "Segment:Bus:Device.Function" notation for the PciCapLib
> >     interface, is not good either. It has to do with the portability of
> >     UEFI drivers that bind PciIo protocol instances:
> >
> >     It is indeed easy enough to call PciIo->GetLocation() and plug the
> >     output values into a theoretical "Segment:Bus:Device.Function"
> >     interface. However, about the only two things that could consume
> >     such location identifiers and actually *implement* config space
> >     access would be:
> >
> >     (a) the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read/Write member
> >         functions, where the member functions themselves take the
> >         "Bus:Device.Function" part, and the "Segment" part must be used
> >         for selecting the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instance
> >         itself, through the SegmentNumber member;
> >
> >     (b) the PciSegmentLib interfaces.
> >
> >     Building a PciCapLib instance that uses (a) into a UEFI driver is a
> >     bad idea, because there are platforms that provide PciIo instances
> >     -- hence the driver can generally run on them -- but don't provide
> >     PciRootBridgeIo instances.
> >
> >     Building a PciCapLib instance that uses (b) into a UEFI driver is
> >     worse. The PciSegmentLib interfaces are not capable of returning
> >     errors. For example, if a device appears as PCI Express (implying
> >     its extended config space exists, potentially containing extended
> >     capabilities), but the underlying platform does not support extended
> >     config space access (for example because it only supports
> >     0xCF8/0xCFC, and not ECAM), then peeking at the extended config
> >     space of the device may invoke undefined behavior within
> >     PciSegmentLib, and the PciCapLib instance will be none the wiser.
> >
> >     (In fact, for the same reason, even PciCapPciSegmentLib has to
> >     extend the "Segment:Bus:Device.Function" notation slightly; so that
> >     such issues are caught *before* the call is made to PciSegmentLib.)
> >
> > Q3: Wait, I thought it wasn't possible for a PCI Express device to
> >     appear on a platform that lacked ECAM.
> >
> > A3: Is that a question? :) Yes, it is plenty possible [2]. Same way as
> >     garbage / looped config spaces are [3].
> >
> > [2] https://github.com/tianocore/edk2/commit/014b472053ae3
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=02d578e5edd9
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > Thanks,
> > Laszlo
> >
> > Laszlo Ersek (7):
> >   MdePkg: introduce PciCapLib
> >   MdePkg: introduce PciCapPciSegmentLib
> >   MdePkg: introduce PciCapPciIoLib
> >   OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
> >   ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
> >   OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib
> >   OvmfPkg/Virtio10Dxe: convert to PciCapLib
> >
> >  ArmVirtPkg/ArmVirt.dsc.inc                                         |    3 +
> >  MdePkg/Include/Library/PciCapLib.h                                 |  429 +++++++++
> >  MdePkg/Include/Library/PciCapPciIoLib.h                            |   58 ++
> >  MdePkg/Include/Library/PciCapPciSegmentLib.h                       |   82 ++
> >  MdePkg/Library/BasePciCapLib/BasePciCapLib.c                       | 1007 ++++++++++++++++++++
> >  MdePkg/Library/BasePciCapLib/BasePciCapLib.h                       |   60 ++
> >  MdePkg/Library/BasePciCapLib/BasePciCapLib.inf                     |   37 +
> >  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c   |  226 +++++
> >  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h   |   47 +
> >  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf |   34 +
> >  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c             |  243 +++++
> >  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h             |   44 +
> >  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf           |   35 +
> >  MdePkg/MdePkg.dec                                                  |   14 +
> >  MdePkg/MdePkg.dsc                                                  |    3 +
> >  OvmfPkg/Include/IndustryStandard/Virtio10.h                        |    7 +-
> >  OvmfPkg/OvmfPkgIa32.dsc                                            |    3 +
> >  OvmfPkg/OvmfPkgIa32X64.dsc                                         |    3 +
> >  OvmfPkg/OvmfPkgX64.dsc                                             |    3 +
> >  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c                         |  267 ++----
> >  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf                       |    5 +
> >  OvmfPkg/Virtio10Dxe/Virtio10.c                                     |  135 +--
> >  OvmfPkg/Virtio10Dxe/Virtio10.inf                                   |    2 +
> >  23 files changed, 2485 insertions(+), 262 deletions(-)
> >  create mode 100644 MdePkg/Include/Library/PciCapLib.h
> >  create mode 100644 MdePkg/Include/Library/PciCapPciIoLib.h
> >  create mode 100644 MdePkg/Include/Library/PciCapPciSegmentLib.h
> >  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.c
> >  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.h
> >  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.inf
> >  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
> >  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
> >  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> >  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> >  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
> >  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> >

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

Re: [edk2] [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library

Posted by Laszlo Ersek 5 years, 11 months ago

On 05/14/18 17:06, Gao, Liming wrote:
> Laszlo:
>   I have not fully understood its goal and design. Could you give me one more week to review it?

Sure! Thank you.

Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, May 14, 2018 9:16 PM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library
>>
>> ping
>>
>> On 05/04/18 23:36, Laszlo Ersek wrote:
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: pci_cap
>>>
>>> In message [1], Jordan suggested a general "capabilities helper lib that
>>> could iterate & read the PCI capability structs". Patch #1 introduces
>>> that library (class and central BASE instance) to MdePkg; the library
>>> class is called PciCapLib.
>>>
>>> [1] http://mid.mail-archive.com/150696619010.2454.9266470245604846575@jljusten-skl
>>>
>>> PciCapLib offers APIs to parse capabilities lists, and to locate,
>>> describe, read and write capabilities.
>>>
>>> The library class targets a wide range of client module types; for
>>> example, platform PEIMs, platform DXE drivers, and UEFI drivers that
>>> follow the UEFI Driver Model. For this reason, the library class is
>>> designed with PCI config access abstracted away, even beyond the PPI /
>>> Protocol level, since those cannot cross the PEI / DXE boundary.
>>>
>>> This (minimal) config space access method abstraction is called
>>> PCI_CAP_DEV. It is *conceptually* a protocol. Client modules plug
>>> PCI_CAP_DEV instances into PciCapLib for config space access, similarly
>>> to how DiskIo and BlockIo instances are plugged into filesystem drivers.
>>> Similarly to the filesystem driver example, PciCapLib is an example for
>>> the "strategy pattern".
>>>
>>> Patches #2 and #3 introduce small "factory" libraries that help client
>>> modules produce PCI_CAP_DEV instances:
>>>
>>> - PciCapPciSegmentLib -- incl. class and central BASE instance -- from
>>>   patch #2 produces PCI_CAP_DEV instances on top of PciSegmentLib, using
>>>   the "Segment:Bus:Device.Function" notation. This is meant for platform
>>>   modules (PEIMs and DXE drivers), which generally have a working
>>>   PciSegmentLib instance available.
>>>
>>> - PciCapPciIoLib -- incl. class and central UEFI instance -- from patch
>>>   #3 lets client modules produce PCI_CAP_DEV instances on top of
>>>   EFI_PCI_IO_PROTOCOL instances. This is meant for UEFI drivers.
>>>
>>> Patches #4 and #5 resolve the three new lib classes to their respective
>>> central instances in the OvmfPkg and ArmVirtPkg DSC files.
>>>
>>> Patch #6 converts OvmfPkg/PciHotPlugInitDxe to the new APIs, from manual
>>> capability list parsing. Because PciHotPlugInitDxe is a platform DXE
>>> driver, this conversion uses PciCapPciSegmentLib and PciCapLib.
>>>
>>> Patch #7 converts OvmfPkg/Virtio10Dxe to the new APIs, from manual
>>> capability list parsing. Because Virtio10Dxe is a UEFI driver, this
>>> conversion uses PciCapPciIoLib and PciCapLib.
>>>
>>> ----[ jump to the patches now, and return if you have questions: I
>>>       have answers, but you might find them too verbose in advance ]----
>>>
>>> Q1: If PCI config access needs to be abstracted from PciCapLib, then why
>>>     don't we provide multiple instances of PciCapLib? In other words,
>>>     why the PCI_CAP_DEV "protocol" rather than multiple instances of
>>>     PciCapLib?
>>>
>>> A1: The point of lib classes and instances is that the class provides a
>>>     common interface, and the instances provide different
>>>     implementations. We have the opposite use case here (hence the
>>>     "strategy pattern"): the implementation of the higher level
>>>     algorithm is common, and the objects that plug into it differ at the
>>>     *interface* level ("Segment:Bus:Device.Function" notation versus
>>>     PciIo protocol).
>>>
>>>     None of those notations are suitable for inclusion in the PciCapLib
>>>     *class*; a more abstract interface is needed for identifying the PCI
>>>     device to the library, for parsing of capabilities lists.
>>>
>>> Q2: OK, let's say we can't (and shouldn't) include both notations in the
>>>     PciCapLib class; can we settle on one of them, rather than
>>>     PCI_CAP_DEV?
>>>
>>> A2: No, we can't. PciIo is obviously unusable in PEIMs (and in quite a
>>>     few platform DXE drivers too).
>>>
>>>     It takes a bit longer to explain why the other choice, i.e. adopting
>>>     the "Segment:Bus:Device.Function" notation for the PciCapLib
>>>     interface, is not good either. It has to do with the portability of
>>>     UEFI drivers that bind PciIo protocol instances:
>>>
>>>     It is indeed easy enough to call PciIo->GetLocation() and plug the
>>>     output values into a theoretical "Segment:Bus:Device.Function"
>>>     interface. However, about the only two things that could consume
>>>     such location identifiers and actually *implement* config space
>>>     access would be:
>>>
>>>     (a) the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read/Write member
>>>         functions, where the member functions themselves take the
>>>         "Bus:Device.Function" part, and the "Segment" part must be used
>>>         for selecting the EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instance
>>>         itself, through the SegmentNumber member;
>>>
>>>     (b) the PciSegmentLib interfaces.
>>>
>>>     Building a PciCapLib instance that uses (a) into a UEFI driver is a
>>>     bad idea, because there are platforms that provide PciIo instances
>>>     -- hence the driver can generally run on them -- but don't provide
>>>     PciRootBridgeIo instances.
>>>
>>>     Building a PciCapLib instance that uses (b) into a UEFI driver is
>>>     worse. The PciSegmentLib interfaces are not capable of returning
>>>     errors. For example, if a device appears as PCI Express (implying
>>>     its extended config space exists, potentially containing extended
>>>     capabilities), but the underlying platform does not support extended
>>>     config space access (for example because it only supports
>>>     0xCF8/0xCFC, and not ECAM), then peeking at the extended config
>>>     space of the device may invoke undefined behavior within
>>>     PciSegmentLib, and the PciCapLib instance will be none the wiser.
>>>
>>>     (In fact, for the same reason, even PciCapPciSegmentLib has to
>>>     extend the "Segment:Bus:Device.Function" notation slightly; so that
>>>     such issues are caught *before* the call is made to PciSegmentLib.)
>>>
>>> Q3: Wait, I thought it wasn't possible for a PCI Express device to
>>>     appear on a platform that lacked ECAM.
>>>
>>> A3: Is that a question? :) Yes, it is plenty possible [2]. Same way as
>>>     garbage / looped config spaces are [3].
>>>
>>> [2] https://github.com/tianocore/edk2/commit/014b472053ae3
>>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=02d578e5edd9
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>
>>> Thanks,
>>> Laszlo
>>>
>>> Laszlo Ersek (7):
>>>   MdePkg: introduce PciCapLib
>>>   MdePkg: introduce PciCapPciSegmentLib
>>>   MdePkg: introduce PciCapPciIoLib
>>>   OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
>>>   ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
>>>   OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib
>>>   OvmfPkg/Virtio10Dxe: convert to PciCapLib
>>>
>>>  ArmVirtPkg/ArmVirt.dsc.inc                                         |    3 +
>>>  MdePkg/Include/Library/PciCapLib.h                                 |  429 +++++++++
>>>  MdePkg/Include/Library/PciCapPciIoLib.h                            |   58 ++
>>>  MdePkg/Include/Library/PciCapPciSegmentLib.h                       |   82 ++
>>>  MdePkg/Library/BasePciCapLib/BasePciCapLib.c                       | 1007 ++++++++++++++++++++
>>>  MdePkg/Library/BasePciCapLib/BasePciCapLib.h                       |   60 ++
>>>  MdePkg/Library/BasePciCapLib/BasePciCapLib.inf                     |   37 +
>>>  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c   |  226 +++++
>>>  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h   |   47 +
>>>  MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf |   34 +
>>>  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c             |  243 +++++
>>>  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h             |   44 +
>>>  MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf           |   35 +
>>>  MdePkg/MdePkg.dec                                                  |   14 +
>>>  MdePkg/MdePkg.dsc                                                  |    3 +
>>>  OvmfPkg/Include/IndustryStandard/Virtio10.h                        |    7 +-
>>>  OvmfPkg/OvmfPkgIa32.dsc                                            |    3 +
>>>  OvmfPkg/OvmfPkgIa32X64.dsc                                         |    3 +
>>>  OvmfPkg/OvmfPkgX64.dsc                                             |    3 +
>>>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c                         |  267 ++----
>>>  OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf                       |    5 +
>>>  OvmfPkg/Virtio10Dxe/Virtio10.c                                     |  135 +--
>>>  OvmfPkg/Virtio10Dxe/Virtio10.inf                                   |    2 +
>>>  23 files changed, 2485 insertions(+), 262 deletions(-)
>>>  create mode 100644 MdePkg/Include/Library/PciCapLib.h
>>>  create mode 100644 MdePkg/Include/Library/PciCapPciIoLib.h
>>>  create mode 100644 MdePkg/Include/Library/PciCapPciSegmentLib.h
>>>  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.c
>>>  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.h
>>>  create mode 100644 MdePkg/Library/BasePciCapLib/BasePciCapLib.inf
>>>  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
>>>  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
>>>  create mode 100644 MdePkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
>>>  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
>>>  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
>>>  create mode 100644 MdePkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
>>>
> 

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