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

["Gao, Liming" <liming.gao@intel.com>]

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
> >