From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 55F072063D75A for ; Mon, 14 May 2018 10:19:29 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DACF840201A4; Mon, 14 May 2018 17:19:28 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-253.rdu2.redhat.com [10.10.120.253]) by smtp.corp.redhat.com (Postfix) with ESMTP id 952602166BAD; Mon, 14 May 2018 17:19:27 +0000 (UTC) To: "Gao, Liming" , edk2-devel-01 Cc: "Kinney, Michael D" , "Justen, Jordan L" , Ard Biesheuvel References: <20180504213637.11266-1-lersek@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E22903A@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <51afa285-c700-eb5b-c3d4-f8b4c235cba2@redhat.com> Date: Mon, 14 May 2018 19:19:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E22903A@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Mon, 14 May 2018 17:19:28 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Mon, 14 May 2018 17:19:28 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 0/7] MdePkg, OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 May 2018 17:19:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Kinney, Michael D ; Justen, Jordan L ; Gao, Liming >> ; Ard Biesheuvel >> 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 >>> Cc: Jordan Justen >>> Cc: Liming Gao >>> Cc: Michael D Kinney >>> >>> 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 >>> >