* [PATCH v2 1/7] OvmfPkg: introduce PciCapLib
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
@ 2018-05-23 20:21 ` Laszlo Ersek
2018-05-24 7:53 ` Ard Biesheuvel
2018-05-23 20:21 ` [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib Laszlo Ersek
` (6 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-23 20:21 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
Add a library class, and a BASE lib instance, to work more easily with PCI
capabilities in PCI config space. Functions are provided to parse
capabilities lists, and to locate, describe, read and write capabilities.
PCI config space access is abstracted away.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- move library from MdePkg to OvmfPkg, for initial introduction
OvmfPkg/OvmfPkg.dec | 4 +
OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf | 38 +
OvmfPkg/Include/Library/PciCapLib.h | 429 +++++++++
OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h | 60 ++
OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c | 1007 ++++++++++++++++++++
5 files changed, 1538 insertions(+)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index c01a2ca7219a..74818a2e2a19 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -31,6 +31,10 @@ [LibraryClasses]
#
NvVarsFileLib|Include/Library/NvVarsFileLib.h
+ ## @libraryclass Provides services to work with PCI capabilities in PCI
+ # config space.
+ PciCapLib|Include/Library/PciCapLib.h
+
## @libraryclass Access QEMU's firmware configuration interface
#
QemuFwCfgLib|Include/Library/QemuFwCfgLib.h
diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
new file mode 100644
index 000000000000..9a7428a589c2
--- /dev/null
+++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
@@ -0,0 +1,38 @@
+## @file
+# Work with PCI capabilities in PCI config space.
+#
+# Provides functions to parse capabilities lists, and to locate, describe, read
+# and write capabilities. PCI config space access is abstracted away.
+#
+# Copyright (C) 2018, Red Hat, Inc.
+#
+# 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 = 1.27
+ BASE_NAME = BasePciCapLib
+ FILE_GUID = 6957540D-F7B5-4D5B-BEE4-FC14114DCD3C
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = PciCapLib
+
+[Sources]
+ BasePciCapLib.h
+ BasePciCapLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseMemoryLib
+ DebugLib
+ MemoryAllocationLib
+ OrderedCollectionLib
diff --git a/OvmfPkg/Include/Library/PciCapLib.h b/OvmfPkg/Include/Library/PciCapLib.h
new file mode 100644
index 000000000000..22a1ad624bd3
--- /dev/null
+++ b/OvmfPkg/Include/Library/PciCapLib.h
@@ -0,0 +1,429 @@
+/** @file
+ Library class to work with PCI capabilities in PCI config space.
+
+ Provides functions to parse capabilities lists, and to locate, describe, read
+ and write capabilities. PCI config space access is abstracted away.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#ifndef __PCI_CAP_LIB_H__
+#define __PCI_CAP_LIB_H__
+
+#include <Uefi/UefiBaseType.h>
+
+//
+// Base structure for representing a PCI device -- down to the PCI function
+// level -- for the purposes of this library class. This is a forward
+// declaration that is completed below. Concrete implementations are supposed
+// to inherit and extend this type.
+//
+typedef struct PCI_CAP_DEV PCI_CAP_DEV;
+
+/**
+ Read the config space of a given PCI device (both normal and extended).
+
+ PCI_CAP_DEV_READ_CONFIG performs as few config space accesses as possible
+ (without attempting 64-bit wide accesses).
+
+ PCI_CAP_DEV_READ_CONFIG returns an unspecified error if accessing Size bytes
+ from SourceOffset exceeds the config space limit of the PCI device. Fewer
+ than Size bytes may have been read in this case.
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] SourceOffset Source offset in the config space of the PCI
+ device to start reading from.
+
+ @param[out] DestinationBuffer Buffer to store the read data to.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from config space to
+ DestinationBuffer.
+
+ @return Unspecified error codes. Fewer than Size bytes may
+ have been read.
+**/
+typedef
+RETURN_STATUS
+(EFIAPI *PCI_CAP_DEV_READ_CONFIG) (
+ IN PCI_CAP_DEV *PciDevice,
+ IN UINT16 SourceOffset,
+ OUT VOID *DestinationBuffer,
+ IN UINT16 Size
+ );
+
+/**
+ Write the config space of a given PCI device (both normal and extended).
+
+ PCI_CAP_DEV_WRITE_CONFIG performs as few config space accesses as possible
+ (without attempting 64-bit wide accesses).
+
+ PCI_CAP_DEV_WRITE_CONFIG returns an unspecified error if accessing Size bytes
+ at DestinationOffset exceeds the config space limit of the PCI device. Fewer
+ than Size bytes may have been written in this case.
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] DestinationOffset Destination offset in the config space of the
+ PCI device to start writing at.
+
+ @param[in] SourceBuffer Buffer to read the data to be stored from.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from SourceBuffer to
+ config space.
+
+ @return Unspecified error codes. Fewer than Size bytes may
+ have been written.
+**/
+typedef
+RETURN_STATUS
+(EFIAPI *PCI_CAP_DEV_WRITE_CONFIG) (
+ IN PCI_CAP_DEV *PciDevice,
+ IN UINT16 DestinationOffset,
+ IN VOID *SourceBuffer,
+ IN UINT16 Size
+ );
+
+//
+// Complete the PCI_CAP_DEV type here. The base abstraction only requires
+// config space accessors.
+//
+struct PCI_CAP_DEV {
+ PCI_CAP_DEV_READ_CONFIG ReadConfig;
+ PCI_CAP_DEV_WRITE_CONFIG WriteConfig;
+};
+
+//
+// Opaque data structure representing parsed PCI Capabilities Lists.
+//
+typedef struct PCI_CAP_LIST PCI_CAP_LIST;
+
+//
+// Opaque data structure representing a PCI Capability in a parsed Capability
+// List.
+//
+typedef struct PCI_CAP PCI_CAP;
+
+//
+// Distinguishes whether a Capability ID is 8-bit wide and interpreted in
+// normal config space, or 16-bit wide and interpreted in extended config
+// space. Capability ID definitions are relative to domain.
+//
+typedef enum {
+ PciCapNormal,
+ PciCapExtended
+} PCI_CAP_DOMAIN;
+
+//
+// Public data structure that PciCapGetInfo() fills in about a PCI_CAP object.
+//
+typedef struct {
+ PCI_CAP_DOMAIN Domain;
+ UINT16 CapId;
+ //
+ // The capability identified by Domain and CapId may have multiple instances
+ // in config space. NumInstances provides the total count of occurrences of
+ // the capability. It is always positive.
+ //
+ UINT16 NumInstances;
+ //
+ // Instance is the serial number, in capabilities list traversal order (not
+ // necessarily config space offset order), of the one capability instance
+ // that PciCapGetInfo() is reporting about. Instance is always smaller than
+ // NumInstances.
+ //
+ UINT16 Instance;
+ //
+ // The offset in config space at which the capability header of the
+ // capability instance starts.
+ //
+ UINT16 Offset;
+ //
+ // The deduced maximum size of the capability instance, including the
+ // capability header. This hint is an upper bound, calculated -- without
+ // regard to the internal structure of the capability -- from (a) the next
+ // lowest offset in configuration space that is known to be used by another
+ // capability, and (b) from the end of the config space identified by Domain,
+ // whichever is lower.
+ //
+ UINT16 MaxSizeHint;
+ //
+ // The version number of the capability instance. Always zero when Domain is
+ // PciCapNormal.
+ //
+ UINT8 Version;
+} PCI_CAP_INFO;
+
+
+/**
+ Parse the capabilities lists (both normal and extended, as applicable) of a
+ PCI device.
+
+ If the PCI device has no capabilities, that per se will not fail
+ PciCapListInit(); an empty capabilities list will be represented.
+
+ If the PCI device is found to be PCI Express, then an attempt will be made to
+ parse the extended capabilities list as well. If the first extended config
+ space access -- via PciDevice->ReadConfig() with SourceOffset=0x100 and
+ Size=4 -- fails, that per se will not fail PciCapListInit(); the device will
+ be assumed to have no extended capabilities.
+
+ @param[in] PciDevice Implementation-specific unique representation of the
+ PCI device in the PCI hierarchy.
+
+ @param[out] CapList Opaque data structure that holds an in-memory
+ representation of the parsed capabilities lists of
+ PciDevice.
+
+ @retval RETURN_SUCCESS The capabilities lists have been parsed from
+ config space.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval RETURN_DEVICE_ERROR A loop or some other kind of invalid pointer
+ was detected in the capabilities lists of
+ PciDevice.
+
+ @return Error codes propagated from
+ PciDevice->ReadConfig().
+**/
+RETURN_STATUS
+EFIAPI
+PciCapListInit (
+ IN PCI_CAP_DEV *PciDevice,
+ OUT PCI_CAP_LIST **CapList
+ );
+
+
+/**
+ Free the resources used by CapList.
+
+ @param[in] CapList The PCI_CAP_LIST object to free, originally produced by
+ PciCapListInit().
+**/
+VOID
+EFIAPI
+PciCapListUninit (
+ IN PCI_CAP_LIST *CapList
+ );
+
+
+/**
+ Locate a capability instance in the parsed capabilities lists.
+
+ @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
+
+ @param[in] Domain Distinguishes whether CapId is 8-bit wide and
+ interpreted in normal config space, or 16-bit wide and
+ interpreted in extended config space. Capability ID
+ definitions are relative to domain.
+
+ @param[in] CapId Capability identifier to look up.
+
+ @param[in] Instance Domain and CapId may identify a multi-instance
+ capability. When Instance is zero, the first instance of
+ the capability is located (in list traversal order --
+ which may not mean increasing config space offset
+ order). Higher Instance values locate subsequent
+ instances of the same capability (in list traversal
+ order).
+
+ @param[out] Cap The capability instance that matches the search
+ criteria. Cap is owned by CapList and becomes invalid
+ when CapList is freed with PciCapListUninit().
+ PciCapListFindCap() may be called with Cap set to NULL,
+ in order to test the existence of a specific capability
+ instance.
+
+ @retval RETURN_SUCCESS The capability instance identified by (Domain,
+ CapId, Instance) has been found.
+
+ @retval RETURN_NOT_FOUND The requested (Domain, CapId, Instance) capability
+ instance does not exist.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapListFindCap (
+ IN PCI_CAP_LIST *CapList,
+ IN PCI_CAP_DOMAIN Domain,
+ IN UINT16 CapId,
+ IN UINT16 Instance,
+ OUT PCI_CAP **Cap OPTIONAL
+ );
+
+
+/**
+ Locate the first instance of the capability given by (Domain, CapId) such
+ that the instance's Version is greater than or equal to MinVersion.
+
+ This is a convenience function that may save client code calls to
+ PciCapListFindCap() and PciCapGetInfo().
+
+ @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
+
+ @param[in] Domain Distinguishes whether CapId is 8-bit wide and
+ interpreted in normal config space, or 16-bit wide and
+ interpreted in extended config space. Capability ID
+ definitions are relative to domain.
+
+ @param[in] CapId Capability identifier to look up.
+
+ @param[in] MinVersion The minimum version that the capability instance is
+ required to have. Note that all capability instances
+ in Domain=PciCapNormal have Version=0.
+
+ @param[out] Cap The first capability instance that matches the search
+ criteria. Cap is owned by CapList and becomes invalid
+ when CapList is freed with PciCapListUninit().
+ PciCapListFindCapVersion() may be called with Cap set
+ to NULL, in order just to test whether the search
+ criteria are satisfiable.
+
+ @retval RETURN_SUCCESS The first capability instance matching (Domain,
+ CapId, MinVersion) has been located.
+
+ @retval RETURN_NOT_FOUND No capability instance matches (Domain, CapId,
+ MinVersion).
+**/
+RETURN_STATUS
+EFIAPI
+PciCapListFindCapVersion (
+ IN PCI_CAP_LIST *CapList,
+ IN PCI_CAP_DOMAIN Domain,
+ IN UINT16 CapId,
+ IN UINT8 MinVersion,
+ OUT PCI_CAP **Cap OPTIONAL
+ );
+
+
+/**
+ Get information about a PCI Capability instance.
+
+ @param[in] Cap The capability instance to get info about, located with
+ PciCapListFindCap*().
+
+ @param[out] Info A PCI_CAP_INFO structure that describes the properties of
+ Cap.
+
+ @retval RETURN_SUCCESS Fields of Info have been set.
+
+ @return Unspecified error codes, if filling in Info failed
+ for some reason.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapGetInfo (
+ IN PCI_CAP *Cap,
+ OUT PCI_CAP_INFO *Info
+ );
+
+
+/**
+ Read a slice of a capability instance.
+
+ The function performs as few config space accesses as possible (without
+ attempting 64-bit wide accesses). PciCapRead() performs bounds checking on
+ SourceOffsetInCap and Size, and only invokes PciDevice->ReadConfig() if the
+ requested transfer falls within Cap.
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] Cap The capability instance to read, located with
+ PciCapListFindCap*().
+
+ @param[in] SourceOffsetInCap Source offset relative to the capability
+ header to start reading from. A zero value
+ refers to the first byte of the capability
+ header.
+
+ @param[out] DestinationBuffer Buffer to store the read data to.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from Cap to
+ DestinationBuffer.
+
+ @retval RETURN_BAD_BUFFER_SIZE Reading Size bytes starting from
+ SourceOffsetInCap would not (entirely) be
+ contained within Cap, as suggested by
+ PCI_CAP_INFO.MaxSizeHint. No bytes have been
+ read.
+
+ @return Error codes propagated from
+ PciDevice->ReadConfig(). Fewer than Size
+ bytes may have been read.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapRead (
+ IN PCI_CAP_DEV *PciDevice,
+ IN PCI_CAP *Cap,
+ IN UINT16 SourceOffsetInCap,
+ OUT VOID *DestinationBuffer,
+ IN UINT16 Size
+ );
+
+
+/**
+ Write a slice of a capability instance.
+
+ The function performs as few config space accesses as possible (without
+ attempting 64-bit wide accesses). PciCapWrite() performs bounds checking on
+ DestinationOffsetInCap and Size, and only invokes PciDevice->WriteConfig() if
+ the requested transfer falls within Cap.
+
+ @param[in] PciDevice Implementation-specific unique
+ representation of the PCI device in the
+ PCI hierarchy.
+
+ @param[in] Cap The capability instance to write, located
+ with PciCapListFindCap*().
+
+ @param[in] DestinationOffsetInCap Destination offset relative to the
+ capability header to start writing at. A
+ zero value refers to the first byte of the
+ capability header.
+
+ @param[in] SourceBuffer Buffer to read the data to be stored from.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from
+ SourceBuffer to Cap.
+
+ @retval RETURN_BAD_BUFFER_SIZE Writing Size bytes starting at
+ DestinationOffsetInCap would not (entirely)
+ be contained within Cap, as suggested by
+ PCI_CAP_INFO.MaxSizeHint. No bytes have been
+ written.
+
+ @return Error codes propagated from
+ PciDevice->WriteConfig(). Fewer than Size
+ bytes may have been written.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapWrite (
+ IN PCI_CAP_DEV *PciDevice,
+ IN PCI_CAP *Cap,
+ IN UINT16 DestinationOffsetInCap,
+ IN VOID *SourceBuffer,
+ IN UINT16 Size
+ );
+
+#endif // __PCI_CAP_LIB_H__
diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h
new file mode 100644
index 000000000000..e631745834d9
--- /dev/null
+++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h
@@ -0,0 +1,60 @@
+/** @file
+ Work with PCI capabilities in PCI config space -- internal type definitions.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#ifndef __BASE_PCI_CAP_LIB_H__
+#define __BASE_PCI_CAP_LIB_H__
+
+#include <Library/OrderedCollectionLib.h>
+
+#include <Library/PciCapLib.h>
+
+//
+// Structure that uniquely identifies a capability instance and serves as key
+// for insertion and lookup.
+//
+typedef struct {
+ PCI_CAP_DOMAIN Domain;
+ UINT16 CapId;
+ UINT16 Instance;
+} PCI_CAP_KEY;
+
+//
+// In Instance==0 PCI_CAP objects, store NumInstances directly. In Instance>0
+// PCI_CAP objects, link Instance#0 of the same (Domain, CapId). This way
+// NumInstances needs maintenance in one object only, per (Domain, CapId) pair.
+//
+typedef union {
+ UINT16 NumInstances;
+ PCI_CAP *InstanceZero;
+} PCI_CAP_NUM_INSTANCES;
+
+//
+// Complete the incomplete PCI_CAP structure here.
+//
+struct PCI_CAP {
+ PCI_CAP_KEY Key;
+ PCI_CAP_NUM_INSTANCES NumInstancesUnion;
+ UINT16 Offset;
+ UINT16 MaxSizeHint;
+ UINT8 Version;
+};
+
+//
+// Complete the incomplete PCI_CAP_LIST structure here.
+//
+struct PCI_CAP_LIST {
+ ORDERED_COLLECTION *Capabilities;
+};
+
+#endif // __BASE_PCI_CAP_LIB_H__
diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
new file mode 100644
index 000000000000..6789359f0a54
--- /dev/null
+++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
@@ -0,0 +1,1007 @@
+/** @file
+ Work with PCI capabilities in PCI config space.
+
+ Provides functions to parse capabilities lists, and to locate, describe, read
+ and write capabilities. PCI config space access is abstracted away.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#include <IndustryStandard/PciExpress21.h>
+
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+
+#include "BasePciCapLib.h"
+
+
+/**
+ Compare a standalone PCI_CAP_KEY against a PCI_CAP containing an embedded
+ PCI_CAP_KEY.
+
+ @param[in] PciCapKey Pointer to the bare PCI_CAP_KEY.
+
+ @param[in] PciCap Pointer to the PCI_CAP with the embedded PCI_CAP_KEY.
+
+ @retval <0 If PciCapKey compares less than PciCap->Key.
+
+ @retval 0 If PciCapKey compares equal to PciCap->Key.
+
+ @retval >0 If PciCapKey compares greater than PciCap->Key.
+**/
+STATIC
+INTN
+EFIAPI
+ComparePciCapKey (
+ IN CONST VOID *PciCapKey,
+ IN CONST VOID *PciCap
+ )
+{
+ CONST PCI_CAP_KEY *Key1;
+ CONST PCI_CAP_KEY *Key2;
+
+ Key1 = PciCapKey;
+ Key2 = &((CONST PCI_CAP *)PciCap)->Key;
+
+ if (Key1->Domain < Key2->Domain) {
+ return -1;
+ }
+ if (Key1->Domain > Key2->Domain) {
+ return 1;
+ }
+ if (Key1->CapId < Key2->CapId) {
+ return -1;
+ }
+ if (Key1->CapId > Key2->CapId) {
+ return 1;
+ }
+ if (Key1->Instance < Key2->Instance) {
+ return -1;
+ }
+ if (Key1->Instance > Key2->Instance) {
+ return 1;
+ }
+ return 0;
+}
+
+
+/**
+ Compare two PCI_CAP objects based on PCI_CAP.Key.
+
+ @param[in] PciCap1 Pointer to the first PCI_CAP.
+
+ @param[in] PciCap2 Pointer to the second PCI_CAP.
+
+ @retval <0 If PciCap1 compares less than PciCap2.
+
+ @retval 0 If PciCap1 compares equal to PciCap2.
+
+ @retval >0 If PciCap1 compares greater than PciCap2.
+**/
+STATIC
+INTN
+EFIAPI
+ComparePciCap (
+ IN CONST VOID *PciCap1,
+ IN CONST VOID *PciCap2
+ )
+{
+ CONST PCI_CAP_KEY *PciCap1Key;
+
+ PciCap1Key = &((CONST PCI_CAP *)PciCap1)->Key;
+ return ComparePciCapKey (PciCap1Key, PciCap2);
+}
+
+
+/**
+ Compare the standalone UINT16 config space offset of a capability header
+ against a PCI_CAP containing an embedded Offset.
+
+ @param[in] CapHdrOffset Pointer to the bare UINT16 config space offset.
+
+ @param[in] PciCap Pointer to the PCI_CAP with the embedded Offset.
+
+ @retval <0 If CapHdrOffset compares less than PciCap->Offset.
+
+ @retval 0 If CapHdrOffset compares equal to PciCap->Offset.
+
+ @retval >0 If CapHdrOffset compares greater than PciCap->Offset.
+**/
+STATIC
+INTN
+EFIAPI
+ComparePciCapOffsetKey (
+ IN CONST VOID *CapHdrOffset,
+ IN CONST VOID *PciCap
+ )
+{
+ UINT16 Offset1;
+ UINT16 Offset2;
+
+ Offset1 = *(CONST UINT16 *)CapHdrOffset;
+ Offset2 = ((CONST PCI_CAP *)PciCap)->Offset;
+ //
+ // Note: both Offset1 and Offset2 are promoted to INT32 below, and the
+ // subtraction takes place between INT32 values.
+ //
+ return Offset1 - Offset2;
+}
+
+
+/**
+ Compare two PCI_CAP objects based on PCI_CAP.Offset.
+
+ @param[in] PciCap1 Pointer to the first PCI_CAP.
+
+ @param[in] PciCap2 Pointer to the second PCI_CAP.
+
+ @retval <0 If PciCap1 compares less than PciCap2.
+
+ @retval 0 If PciCap1 compares equal to PciCap2.
+
+ @retval >0 If PciCap1 compares greater than PciCap2.
+**/
+STATIC
+INTN
+EFIAPI
+ComparePciCapOffset (
+ IN CONST VOID *PciCap1,
+ IN CONST VOID *PciCap2
+ )
+{
+ UINT16 Offset1;
+ UINT16 Offset2;
+
+ Offset1 = ((CONST PCI_CAP *)PciCap1)->Offset;
+ Offset2 = ((CONST PCI_CAP *)PciCap2)->Offset;
+ //
+ // Note: both Offset1 and Offset2 are promoted to INT32 below, and the
+ // subtraction takes place between INT32 values.
+ //
+ return Offset1 - Offset2;
+}
+
+
+/**
+ Insert a new instance of the PCI capability given by (Domain, CapId) in
+ CapList.
+
+ @param[in,out] CapList The PCI_CAP_LIST into which the new PCI_CAP
+ should be inserted. CapList will own the new
+ PCI_CAP structure.
+
+ @param[in,out] CapHdrOffsets Link the new PCI_CAP structure into the
+ (non-owning) CapHdrOffsets collection as well.
+ CapHdrOffsets orders the PCI_CAP structures
+ based on the PCI_CAP.Offset member, and enables
+ the calculation of PCI_CAP.MaxSizeHint.
+
+ @param[in] Domain Whether the capability is normal or extended.
+
+ @param[in] CapId Capability ID (specific to Domain).
+
+ @param[in] Offset Config space offset at which the standard
+ header of the capability starts. The caller is
+ responsible for ensuring that Offset be DWORD
+ aligned. The caller is also responsible for
+ ensuring that Offset be within the config space
+ identified by Domain.
+
+ @param[in] Version The version number of the capability. The
+ caller is responsible for passing 0 as Version
+ if Domain is PciCapNormal.
+
+ @retval RETURN_SUCCESS Insertion successful.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval RETURN_DEVICE_ERROR A PCI_CAP with Offset is already linked by
+ CapHdrOffsets. This indicates a loop in the
+ capabilities list being parsed.
+**/
+STATIC
+RETURN_STATUS
+InsertPciCap (
+ IN OUT PCI_CAP_LIST *CapList,
+ IN OUT ORDERED_COLLECTION *CapHdrOffsets,
+ IN PCI_CAP_DOMAIN Domain,
+ IN UINT16 CapId,
+ IN UINT16 Offset,
+ IN UINT8 Version
+ )
+{
+ PCI_CAP *PciCap;
+ RETURN_STATUS Status;
+ ORDERED_COLLECTION_ENTRY *PciCapEntry;
+ PCI_CAP *InstanceZero;
+
+ ASSERT ((Offset & 0x3) == 0);
+ ASSERT (Offset < (Domain == PciCapNormal ?
+ PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET));
+ ASSERT (Domain == PciCapExtended || Version == 0);
+
+ //
+ // Set InstanceZero to suppress incorrect compiler/analyzer warnings.
+ //
+ InstanceZero = NULL;
+
+ //
+ // Allocate PciCap, and populate it assuming it is the first occurrence of
+ // (Domain, CapId). Note that PciCap->MaxSizeHint is not assigned the final
+ // value just yet.
+ //
+ PciCap = AllocatePool (sizeof *PciCap);
+ if (PciCap == NULL) {
+ return RETURN_OUT_OF_RESOURCES;
+ }
+ PciCap->Key.Domain = Domain;
+ PciCap->Key.CapId = CapId;
+ PciCap->Key.Instance = 0;
+ PciCap->NumInstancesUnion.NumInstances = 1;
+ PciCap->Offset = Offset;
+ PciCap->MaxSizeHint = 0;
+ PciCap->Version = Version;
+
+ //
+ // Add PciCap to CapList.
+ //
+ Status = OrderedCollectionInsert (CapList->Capabilities, &PciCapEntry,
+ PciCap);
+ if (RETURN_ERROR (Status)) {
+ if (Status == RETURN_OUT_OF_RESOURCES) {
+ goto FreePciCap;
+ }
+ ASSERT (Status == RETURN_ALREADY_STARTED);
+ //
+ // PciCap is not the first instance of (Domain, CapId). Add it as a new
+ // instance, taking the current instance count from Instance#0. Note that
+ // we don't bump the instance count maintained in Instance#0 just yet, to
+ // keep rollback on errors simple.
+ //
+ InstanceZero = OrderedCollectionUserStruct (PciCapEntry);
+ PciCap->Key.Instance = InstanceZero->NumInstancesUnion.NumInstances;
+ PciCap->NumInstancesUnion.InstanceZero = InstanceZero;
+
+ ASSERT (PciCap->Key.Instance > 0);
+ Status = OrderedCollectionInsert (CapList->Capabilities, &PciCapEntry,
+ PciCap);
+ if (Status == RETURN_OUT_OF_RESOURCES) {
+ goto FreePciCap;
+ }
+ }
+ //
+ // At this point, PciCap has been inserted in CapList->Capabilities, either
+ // with Instance==0 or with Instance>0. PciCapEntry is the iterator that
+ // links PciCap.
+ //
+ ASSERT_RETURN_ERROR (Status);
+
+ //
+ // Link PciCap into CapHdrOffsets too, to order it globally based on config
+ // space offset. Note that partial overlaps between capability headers is not
+ // possible: Offset is DWORD aligned, normal capability headers are 16-bit
+ // wide, and extended capability headers are 32-bit wide. Therefore any two
+ // capability headers either are distinct or start at the same offset
+ // (implying a loop in the respective capabilities list).
+ //
+ Status = OrderedCollectionInsert (CapHdrOffsets, NULL, PciCap);
+ if (RETURN_ERROR (Status)) {
+ if (Status == RETURN_ALREADY_STARTED) {
+ //
+ // Loop found; map return status accordingly.
+ //
+ Status = RETURN_DEVICE_ERROR;
+ }
+ goto DeletePciCapFromCapList;
+ }
+
+ //
+ // Now we can bump the instance count maintained in Instance#0, if PciCap is
+ // not the first instance of (Domain, CapId).
+ //
+ if (PciCap->Key.Instance > 0) {
+ InstanceZero->NumInstancesUnion.NumInstances++;
+ }
+ return RETURN_SUCCESS;
+
+DeletePciCapFromCapList:
+ OrderedCollectionDelete (CapList->Capabilities, PciCapEntry, NULL);
+
+FreePciCap:
+ FreePool (PciCap);
+
+ return Status;
+}
+
+
+/**
+ Calculate the MaxSizeHint member for a PCI_CAP object.
+
+ CalculatePciCapMaxSizeHint() may only be called once all capability instances
+ have been successfully processed by InsertPciCap().
+
+ @param[in,out] PciCap The PCI_CAP object for which to calculate the
+ MaxSizeHint member. The caller is responsible for
+ passing a PCI_CAP object that has been created by a
+ successful invocation of InsertPciCap().
+
+ @param[in] NextPciCap If NextPciCap is NULL, then the caller is responsible
+ for PciCap to represent the capability instance with
+ the highest header offset in all config space. If
+ NextPciCap is not NULL, then the caller is responsible
+ for (a) having created NextPciCap with a successful
+ invocation of InsertPciCap(), and (b) NextPciCap being
+ the direct successor of PciCap in config space offset
+ order, as ordered by ComparePciCapOffset().
+**/
+STATIC
+VOID
+CalculatePciCapMaxSizeHint (
+ IN OUT PCI_CAP *PciCap,
+ IN PCI_CAP *NextPciCap OPTIONAL
+ )
+{
+ UINT16 ConfigSpaceSize;
+
+ ConfigSpaceSize = (PciCap->Key.Domain == PciCapNormal ?
+ PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
+ //
+ // The following is guaranteed by the interface contract on
+ // CalculatePciCapMaxSizeHint().
+ //
+ ASSERT (NextPciCap == NULL || PciCap->Offset < NextPciCap->Offset);
+ //
+ // The following is guaranteed by the interface contract on InsertPciCap().
+ //
+ ASSERT (PciCap->Offset < ConfigSpaceSize);
+ //
+ // Thus we can safely subtract PciCap->Offset from either of
+ // - ConfigSpaceSize
+ // - and NextPciCap->Offset (if NextPciCap is not NULL).
+ //
+ // PciCap extends from PciCap->Offset to NextPciCap->Offset (if any), except
+ // it cannot cross config space boundary.
+ //
+ if (NextPciCap == NULL || NextPciCap->Offset >= ConfigSpaceSize) {
+ PciCap->MaxSizeHint = ConfigSpaceSize - PciCap->Offset;
+ return;
+ }
+ PciCap->MaxSizeHint = NextPciCap->Offset - PciCap->Offset;
+}
+
+
+/**
+ Debug dump a PCI_CAP_LIST object at the DEBUG_VERBOSE level.
+
+ @param[in] CapList The PCI_CAP_LIST object to dump.
+**/
+STATIC
+VOID
+EFIAPI
+DebugDumpPciCapList (
+ IN PCI_CAP_LIST *CapList
+ )
+{
+ DEBUG_CODE_BEGIN ();
+ ORDERED_COLLECTION_ENTRY *PciCapEntry;
+
+ for (PciCapEntry = OrderedCollectionMin (CapList->Capabilities);
+ PciCapEntry != NULL;
+ PciCapEntry = OrderedCollectionNext (PciCapEntry)) {
+ PCI_CAP *PciCap;
+ RETURN_STATUS Status;
+ PCI_CAP_INFO Info;
+
+ PciCap = OrderedCollectionUserStruct (PciCapEntry);
+ Status = PciCapGetInfo (PciCap, &Info);
+ //
+ // PciCapGetInfo() cannot fail in this library instance.
+ //
+ ASSERT_RETURN_ERROR (Status);
+
+ DEBUG ((DEBUG_VERBOSE,
+ "%a:%a: %a 0x%04x %03u/%03u v0x%x @0x%03x+0x%03x\n", gEfiCallerBaseName,
+ __FUNCTION__, (Info.Domain == PciCapNormal ? "Norm" : "Extd"),
+ Info.CapId, Info.Instance, Info.NumInstances, Info.Version, Info.Offset,
+ Info.MaxSizeHint));
+ }
+ DEBUG_CODE_END ();
+}
+
+
+/**
+ Empty a collection of PCI_CAP structures, optionally releasing the referenced
+ PCI_CAP structures themselves. Release the collection at last.
+
+ @param[in,out] PciCapCollection The collection to empty and release.
+
+ @param[in] FreePciCap TRUE if the PCI_CAP structures linked by
+ PciCapCollection should be released. When
+ FALSE, the caller is responsible for
+ retaining at least one reference to each
+ PCI_CAP structure originally linked by
+ PciCapCollection.
+**/
+STATIC
+VOID
+EmptyAndUninitPciCapCollection (
+ IN OUT ORDERED_COLLECTION *PciCapCollection,
+ IN BOOLEAN FreePciCap
+ )
+{
+ ORDERED_COLLECTION_ENTRY *PciCapEntry;
+ ORDERED_COLLECTION_ENTRY *NextEntry;
+
+ for (PciCapEntry = OrderedCollectionMin (PciCapCollection);
+ PciCapEntry != NULL;
+ PciCapEntry = NextEntry) {
+ PCI_CAP *PciCap;
+
+ NextEntry = OrderedCollectionNext (PciCapEntry);
+ OrderedCollectionDelete (PciCapCollection, PciCapEntry, (VOID **)&PciCap);
+ if (FreePciCap) {
+ FreePool (PciCap);
+ }
+ }
+ OrderedCollectionUninit (PciCapCollection);
+}
+
+
+/**
+ Parse the capabilities lists (both normal and extended, as applicable) of a
+ PCI device.
+
+ If the PCI device has no capabilities, that per se will not fail
+ PciCapListInit(); an empty capabilities list will be represented.
+
+ If the PCI device is found to be PCI Express, then an attempt will be made to
+ parse the extended capabilities list as well. If the first extended config
+ space access -- via PciDevice->ReadConfig() with SourceOffset=0x100 and
+ Size=4 -- fails, that per se will not fail PciCapListInit(); the device will
+ be assumed to have no extended capabilities.
+
+ @param[in] PciDevice Implementation-specific unique representation of the
+ PCI device in the PCI hierarchy.
+
+ @param[out] CapList Opaque data structure that holds an in-memory
+ representation of the parsed capabilities lists of
+ PciDevice.
+
+ @retval RETURN_SUCCESS The capabilities lists have been parsed from
+ config space.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval RETURN_DEVICE_ERROR A loop or some other kind of invalid pointer
+ was detected in the capabilities lists of
+ PciDevice.
+
+ @return Error codes propagated from
+ PciDevice->ReadConfig().
+**/
+RETURN_STATUS
+EFIAPI
+PciCapListInit (
+ IN PCI_CAP_DEV *PciDevice,
+ OUT PCI_CAP_LIST **CapList
+ )
+{
+ PCI_CAP_LIST *OutCapList;
+ RETURN_STATUS Status;
+ ORDERED_COLLECTION *CapHdrOffsets;
+ UINT16 PciStatusReg;
+ BOOLEAN DeviceIsExpress;
+ ORDERED_COLLECTION_ENTRY *OffsetEntry;
+
+ //
+ // Allocate the output structure.
+ //
+ OutCapList = AllocatePool (sizeof *OutCapList);
+ if (OutCapList == NULL) {
+ return RETURN_OUT_OF_RESOURCES;
+ }
+ //
+ // The OutCapList->Capabilities collection owns the PCI_CAP structures and
+ // orders them based on PCI_CAP.Key.
+ //
+ OutCapList->Capabilities = OrderedCollectionInit (ComparePciCap,
+ ComparePciCapKey);
+ if (OutCapList->Capabilities == NULL) {
+ Status = RETURN_OUT_OF_RESOURCES;
+ goto FreeOutCapList;
+ }
+
+ //
+ // The (temporary) CapHdrOffsets collection only references PCI_CAP
+ // structures, and orders them based on PCI_CAP.Offset.
+ //
+ CapHdrOffsets = OrderedCollectionInit (ComparePciCapOffset,
+ ComparePciCapOffsetKey);
+ if (CapHdrOffsets == NULL) {
+ Status = RETURN_OUT_OF_RESOURCES;
+ goto FreeCapabilities;
+ }
+
+ //
+ // Whether the device is PCI Express depends on the normal capability with
+ // identifier EFI_PCI_CAPABILITY_ID_PCIEXP.
+ //
+ DeviceIsExpress = FALSE;
+
+ //
+ // Check whether a normal capabilities list is present. If there's none,
+ // that's not an error; we'll just return OutCapList->Capabilities empty.
+ //
+ Status = PciDevice->ReadConfig (PciDevice, PCI_PRIMARY_STATUS_OFFSET,
+ &PciStatusReg, sizeof PciStatusReg);
+ if (RETURN_ERROR (Status)) {
+ goto FreeCapHdrOffsets;
+ }
+ if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) {
+ UINT8 NormalCapHdrOffset;
+
+ //
+ // Fetch the start offset of the normal capabilities list.
+ //
+ Status = PciDevice->ReadConfig (PciDevice, PCI_CAPBILITY_POINTER_OFFSET,
+ &NormalCapHdrOffset, sizeof NormalCapHdrOffset);
+ if (RETURN_ERROR (Status)) {
+ goto FreeCapHdrOffsets;
+ }
+
+ //
+ // Traverse the normal capabilities list.
+ //
+ NormalCapHdrOffset &= 0xFC;
+ while (NormalCapHdrOffset > 0) {
+ EFI_PCI_CAPABILITY_HDR NormalCapHdr;
+
+ Status = PciDevice->ReadConfig (PciDevice, NormalCapHdrOffset,
+ &NormalCapHdr, sizeof NormalCapHdr);
+ if (RETURN_ERROR (Status)) {
+ goto FreeCapHdrOffsets;
+ }
+
+ Status = InsertPciCap (OutCapList, CapHdrOffsets, PciCapNormal,
+ NormalCapHdr.CapabilityID, NormalCapHdrOffset, 0);
+ if (RETURN_ERROR (Status)) {
+ goto FreeCapHdrOffsets;
+ }
+
+ if (NormalCapHdr.CapabilityID == EFI_PCI_CAPABILITY_ID_PCIEXP) {
+ DeviceIsExpress = TRUE;
+ }
+ NormalCapHdrOffset = NormalCapHdr.NextItemPtr & 0xFC;
+ }
+ }
+
+ //
+ // If the device has been found PCI Express, attempt to traverse the extended
+ // capabilities list. It starts right after the normal config space.
+ //
+ if (DeviceIsExpress) {
+ UINT16 ExtendedCapHdrOffset;
+
+ ExtendedCapHdrOffset = PCI_MAX_CONFIG_OFFSET;
+ while (ExtendedCapHdrOffset > 0) {
+ PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER ExtendedCapHdr;
+
+ Status = PciDevice->ReadConfig (PciDevice, ExtendedCapHdrOffset,
+ &ExtendedCapHdr, sizeof ExtendedCapHdr);
+ //
+ // If the first extended config space access fails, assume the device has
+ // no extended capabilities. If the first extended config space access
+ // succeeds but we read an "all bits zero" extended capability header,
+ // that means (by spec) the device has no extended capabilities.
+ //
+ if (ExtendedCapHdrOffset == PCI_MAX_CONFIG_OFFSET &&
+ (RETURN_ERROR (Status) ||
+ IsZeroBuffer (&ExtendedCapHdr, sizeof ExtendedCapHdr))) {
+ break;
+ }
+ if (RETURN_ERROR (Status)) {
+ goto FreeCapHdrOffsets;
+ }
+
+ Status = InsertPciCap (OutCapList, CapHdrOffsets, PciCapExtended,
+ ExtendedCapHdr.CapabilityId, ExtendedCapHdrOffset,
+ ExtendedCapHdr.CapabilityVersion);
+ if (RETURN_ERROR (Status)) {
+ goto FreeCapHdrOffsets;
+ }
+
+ ExtendedCapHdrOffset = ExtendedCapHdr.NextCapabilityOffset & 0xFFC;
+ if (ExtendedCapHdrOffset > 0 &&
+ ExtendedCapHdrOffset < PCI_MAX_CONFIG_OFFSET) {
+ //
+ // Invalid capability pointer.
+ //
+ Status = RETURN_DEVICE_ERROR;
+ goto FreeCapHdrOffsets;
+ }
+ }
+ }
+
+ //
+ // Both capabilities lists have been parsed; compute the PCI_CAP.MaxSizeHint
+ // members if at least one capability has been found. In parallel, evacuate
+ // the CapHdrOffsets collection.
+ //
+ // At first, set OffsetEntry to the iterator of the PCI_CAP object with the
+ // lowest Offset (if such exists).
+ //
+ OffsetEntry = OrderedCollectionMin (CapHdrOffsets);
+ if (OffsetEntry != NULL) {
+ ORDERED_COLLECTION_ENTRY *NextOffsetEntry;
+ PCI_CAP *PciCap;
+
+ //
+ // Initialize NextOffsetEntry to the iterator of the PCI_CAP object with
+ // the second lowest Offset (if such exists).
+ //
+ NextOffsetEntry = OrderedCollectionNext (OffsetEntry);
+ //
+ // Calculate MaxSizeHint for all PCI_CAP objects except the one with the
+ // highest Offset.
+ //
+ while (NextOffsetEntry != NULL) {
+ PCI_CAP *NextPciCap;
+
+ OrderedCollectionDelete (CapHdrOffsets, OffsetEntry, (VOID **)&PciCap);
+ NextPciCap = OrderedCollectionUserStruct (NextOffsetEntry);
+ CalculatePciCapMaxSizeHint (PciCap, NextPciCap);
+
+ OffsetEntry = NextOffsetEntry;
+ NextOffsetEntry = OrderedCollectionNext (OffsetEntry);
+ }
+ //
+ // Calculate MaxSizeHint for the PCI_CAP object with the highest Offset.
+ //
+ OrderedCollectionDelete (CapHdrOffsets, OffsetEntry, (VOID **)&PciCap);
+ CalculatePciCapMaxSizeHint (PciCap, NULL);
+ }
+ ASSERT (OrderedCollectionIsEmpty (CapHdrOffsets));
+ OrderedCollectionUninit (CapHdrOffsets);
+
+ DebugDumpPciCapList (OutCapList);
+ *CapList = OutCapList;
+ return RETURN_SUCCESS;
+
+FreeCapHdrOffsets:
+ EmptyAndUninitPciCapCollection (CapHdrOffsets, FALSE);
+
+FreeCapabilities:
+ EmptyAndUninitPciCapCollection (OutCapList->Capabilities, TRUE);
+
+FreeOutCapList:
+ FreePool (OutCapList);
+
+ ASSERT (RETURN_ERROR (Status));
+ DEBUG ((DEBUG_ERROR, "%a:%a: %r\n", gEfiCallerBaseName, __FUNCTION__,
+ Status));
+ return Status;
+}
+
+
+/**
+ Free the resources used by CapList.
+
+ @param[in] CapList The PCI_CAP_LIST object to free, originally produced by
+ PciCapListInit().
+**/
+VOID
+EFIAPI
+PciCapListUninit (
+ IN PCI_CAP_LIST *CapList
+ )
+{
+ EmptyAndUninitPciCapCollection (CapList->Capabilities, TRUE);
+ FreePool (CapList);
+}
+
+
+/**
+ Locate a capability instance in the parsed capabilities lists.
+
+ @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
+
+ @param[in] Domain Distinguishes whether CapId is 8-bit wide and
+ interpreted in normal config space, or 16-bit wide and
+ interpreted in extended config space. Capability ID
+ definitions are relative to domain.
+
+ @param[in] CapId Capability identifier to look up.
+
+ @param[in] Instance Domain and CapId may identify a multi-instance
+ capability. When Instance is zero, the first instance of
+ the capability is located (in list traversal order --
+ which may not mean increasing config space offset
+ order). Higher Instance values locate subsequent
+ instances of the same capability (in list traversal
+ order).
+
+ @param[out] Cap The capability instance that matches the search
+ criteria. Cap is owned by CapList and becomes invalid
+ when CapList is freed with PciCapListUninit().
+ PciCapListFindCap() may be called with Cap set to NULL,
+ in order to test the existence of a specific capability
+ instance.
+
+ @retval RETURN_SUCCESS The capability instance identified by (Domain,
+ CapId, Instance) has been found.
+
+ @retval RETURN_NOT_FOUND The requested (Domain, CapId, Instance) capability
+ instance does not exist.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapListFindCap (
+ IN PCI_CAP_LIST *CapList,
+ IN PCI_CAP_DOMAIN Domain,
+ IN UINT16 CapId,
+ IN UINT16 Instance,
+ OUT PCI_CAP **Cap OPTIONAL
+ )
+{
+ PCI_CAP_KEY Key;
+ ORDERED_COLLECTION_ENTRY *PciCapEntry;
+
+ Key.Domain = Domain;
+ Key.CapId = CapId;
+ Key.Instance = Instance;
+
+ PciCapEntry = OrderedCollectionFind (CapList->Capabilities, &Key);
+ if (PciCapEntry == NULL) {
+ return RETURN_NOT_FOUND;
+ }
+ if (Cap != NULL) {
+ *Cap = OrderedCollectionUserStruct (PciCapEntry);
+ }
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Locate the first instance of the capability given by (Domain, CapId) such
+ that the instance's Version is greater than or equal to MinVersion.
+
+ This is a convenience function that may save client code calls to
+ PciCapListFindCap() and PciCapGetInfo().
+
+ @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
+
+ @param[in] Domain Distinguishes whether CapId is 8-bit wide and
+ interpreted in normal config space, or 16-bit wide and
+ interpreted in extended config space. Capability ID
+ definitions are relative to domain.
+
+ @param[in] CapId Capability identifier to look up.
+
+ @param[in] MinVersion The minimum version that the capability instance is
+ required to have. Note that all capability instances
+ in Domain=PciCapNormal have Version=0.
+
+ @param[out] Cap The first capability instance that matches the search
+ criteria. Cap is owned by CapList and becomes invalid
+ when CapList is freed with PciCapListUninit().
+ PciCapListFindCapVersion() may be called with Cap set
+ to NULL, in order just to test whether the search
+ criteria are satisfiable.
+
+ @retval RETURN_SUCCESS The first capability instance matching (Domain,
+ CapId, MinVersion) has been located.
+
+ @retval RETURN_NOT_FOUND No capability instance matches (Domain, CapId,
+ MinVersion).
+**/
+RETURN_STATUS
+EFIAPI
+PciCapListFindCapVersion (
+ IN PCI_CAP_LIST *CapList,
+ IN PCI_CAP_DOMAIN Domain,
+ IN UINT16 CapId,
+ IN UINT8 MinVersion,
+ OUT PCI_CAP **Cap OPTIONAL
+ )
+{
+ PCI_CAP_KEY Key;
+ ORDERED_COLLECTION_ENTRY *PciCapEntry;
+
+ //
+ // Start the version checks at Instance#0 of (Domain, CapId).
+ //
+ Key.Domain = Domain;
+ Key.CapId = CapId;
+ Key.Instance = 0;
+
+ for (PciCapEntry = OrderedCollectionFind (CapList->Capabilities, &Key);
+ PciCapEntry != NULL;
+ PciCapEntry = OrderedCollectionNext (PciCapEntry)) {
+ PCI_CAP *PciCap;
+
+ PciCap = OrderedCollectionUserStruct (PciCapEntry);
+ //
+ // PCI_CAP.Key ordering keeps instances of the same (Domain, CapId)
+ // adjacent to each other, so stop searching if either Domain or CapId
+ // changes.
+ //
+ if (PciCap->Key.Domain != Domain || PciCap->Key.CapId != CapId) {
+ break;
+ }
+ if (PciCap->Version >= MinVersion) {
+ //
+ // Match found.
+ //
+ if (Cap != NULL) {
+ *Cap = PciCap;
+ }
+ return RETURN_SUCCESS;
+ }
+ }
+ return RETURN_NOT_FOUND;
+}
+
+
+/**
+ Get information about a PCI Capability instance.
+
+ @param[in] Cap The capability instance to get info about, located with
+ PciCapListFindCap*().
+
+ @param[out] Info A PCI_CAP_INFO structure that describes the properties of
+ Cap.
+
+ @retval RETURN_SUCCESS Fields of Info have been set.
+
+ @return Unspecified error codes, if filling in Info failed
+ for some reason.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapGetInfo (
+ IN PCI_CAP *Cap,
+ OUT PCI_CAP_INFO *Info
+ )
+{
+ PCI_CAP *InstanceZero;
+
+ InstanceZero = (Cap->Key.Instance == 0 ? Cap :
+ Cap->NumInstancesUnion.InstanceZero);
+
+ Info->Domain = Cap->Key.Domain;
+ Info->CapId = Cap->Key.CapId;
+ Info->NumInstances = InstanceZero->NumInstancesUnion.NumInstances;
+ Info->Instance = Cap->Key.Instance;
+ Info->Offset = Cap->Offset;
+ Info->MaxSizeHint = Cap->MaxSizeHint;
+ Info->Version = Cap->Version;
+
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Read a slice of a capability instance.
+
+ The function performs as few config space accesses as possible (without
+ attempting 64-bit wide accesses). PciCapRead() performs bounds checking on
+ SourceOffsetInCap and Size, and only invokes PciDevice->ReadConfig() if the
+ requested transfer falls within Cap.
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] Cap The capability instance to read, located with
+ PciCapListFindCap*().
+
+ @param[in] SourceOffsetInCap Source offset relative to the capability
+ header to start reading from. A zero value
+ refers to the first byte of the capability
+ header.
+
+ @param[out] DestinationBuffer Buffer to store the read data to.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from Cap to
+ DestinationBuffer.
+
+ @retval RETURN_BAD_BUFFER_SIZE Reading Size bytes starting from
+ SourceOffsetInCap would not (entirely) be
+ contained within Cap, as suggested by
+ PCI_CAP_INFO.MaxSizeHint. No bytes have been
+ read.
+
+ @return Error codes propagated from
+ PciDevice->ReadConfig(). Fewer than Size
+ bytes may have been read.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapRead (
+ IN PCI_CAP_DEV *PciDevice,
+ IN PCI_CAP *Cap,
+ IN UINT16 SourceOffsetInCap,
+ OUT VOID *DestinationBuffer,
+ IN UINT16 Size
+ )
+{
+ //
+ // Note: all UINT16 values are promoted to INT32 below, and addition and
+ // comparison take place between INT32 values.
+ //
+ if (SourceOffsetInCap + Size > Cap->MaxSizeHint) {
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+ return PciDevice->ReadConfig (PciDevice, Cap->Offset + SourceOffsetInCap,
+ DestinationBuffer, Size);
+}
+
+
+/**
+ Write a slice of a capability instance.
+
+ The function performs as few config space accesses as possible (without
+ attempting 64-bit wide accesses). PciCapWrite() performs bounds checking on
+ DestinationOffsetInCap and Size, and only invokes PciDevice->WriteConfig() if
+ the requested transfer falls within Cap.
+
+ @param[in] PciDevice Implementation-specific unique
+ representation of the PCI device in the
+ PCI hierarchy.
+
+ @param[in] Cap The capability instance to write, located
+ with PciCapListFindCap*().
+
+ @param[in] DestinationOffsetInCap Destination offset relative to the
+ capability header to start writing at. A
+ zero value refers to the first byte of the
+ capability header.
+
+ @param[in] SourceBuffer Buffer to read the data to be stored from.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from
+ SourceBuffer to Cap.
+
+ @retval RETURN_BAD_BUFFER_SIZE Writing Size bytes starting at
+ DestinationOffsetInCap would not (entirely)
+ be contained within Cap, as suggested by
+ PCI_CAP_INFO.MaxSizeHint. No bytes have been
+ written.
+
+ @return Error codes propagated from
+ PciDevice->WriteConfig(). Fewer than Size
+ bytes may have been written.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapWrite (
+ IN PCI_CAP_DEV *PciDevice,
+ IN PCI_CAP *Cap,
+ IN UINT16 DestinationOffsetInCap,
+ IN VOID *SourceBuffer,
+ IN UINT16 Size
+ )
+{
+ //
+ // Note: all UINT16 values are promoted to INT32 below, and addition and
+ // comparison take place between INT32 values.
+ //
+ if (DestinationOffsetInCap + Size > Cap->MaxSizeHint) {
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+ return PciDevice->WriteConfig (PciDevice,
+ Cap->Offset + DestinationOffsetInCap, SourceBuffer,
+ Size);
+}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 1/7] OvmfPkg: introduce PciCapLib
2018-05-23 20:21 ` [PATCH v2 1/7] OvmfPkg: introduce PciCapLib Laszlo Ersek
@ 2018-05-24 7:53 ` Ard Biesheuvel
2018-05-24 14:39 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 7:53 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Add a library class, and a BASE lib instance, to work more easily with PCI
> capabilities in PCI config space. Functions are provided to parse
> capabilities lists, and to locate, describe, read and write capabilities.
> PCI config space access is abstracted away.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - move library from MdePkg to OvmfPkg, for initial introduction
>
> OvmfPkg/OvmfPkg.dec | 4 +
> OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf | 38 +
> OvmfPkg/Include/Library/PciCapLib.h | 429 +++++++++
> OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h | 60 ++
> OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c | 1007 ++++++++++++++++++++
> 5 files changed, 1538 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index c01a2ca7219a..74818a2e2a19 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -31,6 +31,10 @@ [LibraryClasses]
> #
> NvVarsFileLib|Include/Library/NvVarsFileLib.h
>
> + ## @libraryclass Provides services to work with PCI capabilities in PCI
> + # config space.
> + PciCapLib|Include/Library/PciCapLib.h
> +
> ## @libraryclass Access QEMU's firmware configuration interface
> #
> QemuFwCfgLib|Include/Library/QemuFwCfgLib.h
> diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
> new file mode 100644
> index 000000000000..9a7428a589c2
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
> @@ -0,0 +1,38 @@
> +## @file
> +# Work with PCI capabilities in PCI config space.
> +#
> +# Provides functions to parse capabilities lists, and to locate, describe, read
> +# and write capabilities. PCI config space access is abstracted away.
> +#
> +# Copyright (C) 2018, Red Hat, Inc.
> +#
> +# 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 = 1.27
> + BASE_NAME = BasePciCapLib
> + FILE_GUID = 6957540D-F7B5-4D5B-BEE4-FC14114DCD3C
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = PciCapLib
> +
> +[Sources]
> + BasePciCapLib.h
> + BasePciCapLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + DebugLib
> + MemoryAllocationLib
> + OrderedCollectionLib
> diff --git a/OvmfPkg/Include/Library/PciCapLib.h b/OvmfPkg/Include/Library/PciCapLib.h
> new file mode 100644
> index 000000000000..22a1ad624bd3
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciCapLib.h
> @@ -0,0 +1,429 @@
> +/** @file
> + Library class to work with PCI capabilities in PCI config space.
> +
> + Provides functions to parse capabilities lists, and to locate, describe, read
> + and write capabilities. PCI config space access is abstracted away.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#ifndef __PCI_CAP_LIB_H__
> +#define __PCI_CAP_LIB_H__
> +
> +#include <Uefi/UefiBaseType.h>
> +
> +//
> +// Base structure for representing a PCI device -- down to the PCI function
> +// level -- for the purposes of this library class. This is a forward
> +// declaration that is completed below. Concrete implementations are supposed
> +// to inherit and extend this type.
> +//
> +typedef struct PCI_CAP_DEV PCI_CAP_DEV;
> +
> +/**
> + Read the config space of a given PCI device (both normal and extended).
> +
> + PCI_CAP_DEV_READ_CONFIG performs as few config space accesses as possible
> + (without attempting 64-bit wide accesses).
> +
> + PCI_CAP_DEV_READ_CONFIG returns an unspecified error if accessing Size bytes
> + from SourceOffset exceeds the config space limit of the PCI device. Fewer
> + than Size bytes may have been read in this case.
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] SourceOffset Source offset in the config space of the PCI
> + device to start reading from.
> +
> + @param[out] DestinationBuffer Buffer to store the read data to.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from config space to
> + DestinationBuffer.
> +
> + @return Unspecified error codes. Fewer than Size bytes may
> + have been read.
> +**/
> +typedef
> +RETURN_STATUS
> +(EFIAPI *PCI_CAP_DEV_READ_CONFIG) (
> + IN PCI_CAP_DEV *PciDevice,
> + IN UINT16 SourceOffset,
> + OUT VOID *DestinationBuffer,
> + IN UINT16 Size
> + );
> +
> +/**
> + Write the config space of a given PCI device (both normal and extended).
> +
> + PCI_CAP_DEV_WRITE_CONFIG performs as few config space accesses as possible
> + (without attempting 64-bit wide accesses).
> +
> + PCI_CAP_DEV_WRITE_CONFIG returns an unspecified error if accessing Size bytes
> + at DestinationOffset exceeds the config space limit of the PCI device. Fewer
> + than Size bytes may have been written in this case.
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] DestinationOffset Destination offset in the config space of the
> + PCI device to start writing at.
> +
> + @param[in] SourceBuffer Buffer to read the data to be stored from.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from SourceBuffer to
> + config space.
> +
> + @return Unspecified error codes. Fewer than Size bytes may
> + have been written.
> +**/
> +typedef
> +RETURN_STATUS
> +(EFIAPI *PCI_CAP_DEV_WRITE_CONFIG) (
> + IN PCI_CAP_DEV *PciDevice,
> + IN UINT16 DestinationOffset,
> + IN VOID *SourceBuffer,
> + IN UINT16 Size
> + );
> +
> +//
> +// Complete the PCI_CAP_DEV type here. The base abstraction only requires
> +// config space accessors.
> +//
> +struct PCI_CAP_DEV {
> + PCI_CAP_DEV_READ_CONFIG ReadConfig;
> + PCI_CAP_DEV_WRITE_CONFIG WriteConfig;
> +};
> +
> +//
> +// Opaque data structure representing parsed PCI Capabilities Lists.
> +//
> +typedef struct PCI_CAP_LIST PCI_CAP_LIST;
> +
> +//
> +// Opaque data structure representing a PCI Capability in a parsed Capability
> +// List.
> +//
> +typedef struct PCI_CAP PCI_CAP;
> +
> +//
> +// Distinguishes whether a Capability ID is 8-bit wide and interpreted in
> +// normal config space, or 16-bit wide and interpreted in extended config
> +// space. Capability ID definitions are relative to domain.
> +//
> +typedef enum {
> + PciCapNormal,
> + PciCapExtended
> +} PCI_CAP_DOMAIN;
> +
> +//
> +// Public data structure that PciCapGetInfo() fills in about a PCI_CAP object.
> +//
> +typedef struct {
> + PCI_CAP_DOMAIN Domain;
> + UINT16 CapId;
> + //
> + // The capability identified by Domain and CapId may have multiple instances
> + // in config space. NumInstances provides the total count of occurrences of
> + // the capability. It is always positive.
> + //
> + UINT16 NumInstances;
> + //
> + // Instance is the serial number, in capabilities list traversal order (not
> + // necessarily config space offset order), of the one capability instance
> + // that PciCapGetInfo() is reporting about. Instance is always smaller than
> + // NumInstances.
> + //
> + UINT16 Instance;
> + //
> + // The offset in config space at which the capability header of the
> + // capability instance starts.
> + //
> + UINT16 Offset;
> + //
> + // The deduced maximum size of the capability instance, including the
> + // capability header. This hint is an upper bound, calculated -- without
> + // regard to the internal structure of the capability -- from (a) the next
> + // lowest offset in configuration space that is known to be used by another
> + // capability, and (b) from the end of the config space identified by Domain,
> + // whichever is lower.
> + //
> + UINT16 MaxSizeHint;
> + //
> + // The version number of the capability instance. Always zero when Domain is
> + // PciCapNormal.
> + //
> + UINT8 Version;
> +} PCI_CAP_INFO;
> +
> +
> +/**
> + Parse the capabilities lists (both normal and extended, as applicable) of a
> + PCI device.
> +
> + If the PCI device has no capabilities, that per se will not fail
> + PciCapListInit(); an empty capabilities list will be represented.
> +
> + If the PCI device is found to be PCI Express, then an attempt will be made to
> + parse the extended capabilities list as well. If the first extended config
> + space access -- via PciDevice->ReadConfig() with SourceOffset=0x100 and
> + Size=4 -- fails, that per se will not fail PciCapListInit(); the device will
> + be assumed to have no extended capabilities.
> +
> + @param[in] PciDevice Implementation-specific unique representation of the
> + PCI device in the PCI hierarchy.
> +
> + @param[out] CapList Opaque data structure that holds an in-memory
> + representation of the parsed capabilities lists of
> + PciDevice.
> +
> + @retval RETURN_SUCCESS The capabilities lists have been parsed from
> + config space.
> +
> + @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @retval RETURN_DEVICE_ERROR A loop or some other kind of invalid pointer
> + was detected in the capabilities lists of
> + PciDevice.
> +
> + @return Error codes propagated from
> + PciDevice->ReadConfig().
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapListInit (
> + IN PCI_CAP_DEV *PciDevice,
> + OUT PCI_CAP_LIST **CapList
> + );
> +
> +
> +/**
> + Free the resources used by CapList.
> +
> + @param[in] CapList The PCI_CAP_LIST object to free, originally produced by
> + PciCapListInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapListUninit (
> + IN PCI_CAP_LIST *CapList
> + );
> +
> +
> +/**
> + Locate a capability instance in the parsed capabilities lists.
> +
> + @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
> +
> + @param[in] Domain Distinguishes whether CapId is 8-bit wide and
> + interpreted in normal config space, or 16-bit wide and
> + interpreted in extended config space. Capability ID
> + definitions are relative to domain.
> +
> + @param[in] CapId Capability identifier to look up.
> +
> + @param[in] Instance Domain and CapId may identify a multi-instance
> + capability. When Instance is zero, the first instance of
> + the capability is located (in list traversal order --
> + which may not mean increasing config space offset
> + order). Higher Instance values locate subsequent
> + instances of the same capability (in list traversal
> + order).
> +
> + @param[out] Cap The capability instance that matches the search
> + criteria. Cap is owned by CapList and becomes invalid
> + when CapList is freed with PciCapListUninit().
> + PciCapListFindCap() may be called with Cap set to NULL,
> + in order to test the existence of a specific capability
> + instance.
> +
> + @retval RETURN_SUCCESS The capability instance identified by (Domain,
> + CapId, Instance) has been found.
> +
> + @retval RETURN_NOT_FOUND The requested (Domain, CapId, Instance) capability
> + instance does not exist.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapListFindCap (
> + IN PCI_CAP_LIST *CapList,
> + IN PCI_CAP_DOMAIN Domain,
> + IN UINT16 CapId,
> + IN UINT16 Instance,
> + OUT PCI_CAP **Cap OPTIONAL
> + );
> +
> +
> +/**
> + Locate the first instance of the capability given by (Domain, CapId) such
> + that the instance's Version is greater than or equal to MinVersion.
> +
> + This is a convenience function that may save client code calls to
> + PciCapListFindCap() and PciCapGetInfo().
> +
> + @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
> +
> + @param[in] Domain Distinguishes whether CapId is 8-bit wide and
> + interpreted in normal config space, or 16-bit wide and
> + interpreted in extended config space. Capability ID
> + definitions are relative to domain.
> +
> + @param[in] CapId Capability identifier to look up.
> +
> + @param[in] MinVersion The minimum version that the capability instance is
> + required to have. Note that all capability instances
> + in Domain=PciCapNormal have Version=0.
> +
> + @param[out] Cap The first capability instance that matches the search
> + criteria. Cap is owned by CapList and becomes invalid
> + when CapList is freed with PciCapListUninit().
> + PciCapListFindCapVersion() may be called with Cap set
> + to NULL, in order just to test whether the search
> + criteria are satisfiable.
> +
> + @retval RETURN_SUCCESS The first capability instance matching (Domain,
> + CapId, MinVersion) has been located.
> +
> + @retval RETURN_NOT_FOUND No capability instance matches (Domain, CapId,
> + MinVersion).
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapListFindCapVersion (
> + IN PCI_CAP_LIST *CapList,
> + IN PCI_CAP_DOMAIN Domain,
> + IN UINT16 CapId,
> + IN UINT8 MinVersion,
> + OUT PCI_CAP **Cap OPTIONAL
> + );
> +
> +
> +/**
> + Get information about a PCI Capability instance.
> +
> + @param[in] Cap The capability instance to get info about, located with
> + PciCapListFindCap*().
> +
> + @param[out] Info A PCI_CAP_INFO structure that describes the properties of
> + Cap.
> +
> + @retval RETURN_SUCCESS Fields of Info have been set.
> +
> + @return Unspecified error codes, if filling in Info failed
> + for some reason.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapGetInfo (
> + IN PCI_CAP *Cap,
> + OUT PCI_CAP_INFO *Info
> + );
> +
> +
> +/**
> + Read a slice of a capability instance.
> +
> + The function performs as few config space accesses as possible (without
> + attempting 64-bit wide accesses). PciCapRead() performs bounds checking on
> + SourceOffsetInCap and Size, and only invokes PciDevice->ReadConfig() if the
> + requested transfer falls within Cap.
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] Cap The capability instance to read, located with
> + PciCapListFindCap*().
> +
> + @param[in] SourceOffsetInCap Source offset relative to the capability
> + header to start reading from. A zero value
> + refers to the first byte of the capability
> + header.
> +
> + @param[out] DestinationBuffer Buffer to store the read data to.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from Cap to
> + DestinationBuffer.
> +
> + @retval RETURN_BAD_BUFFER_SIZE Reading Size bytes starting from
> + SourceOffsetInCap would not (entirely) be
> + contained within Cap, as suggested by
> + PCI_CAP_INFO.MaxSizeHint. No bytes have been
> + read.
> +
> + @return Error codes propagated from
> + PciDevice->ReadConfig(). Fewer than Size
> + bytes may have been read.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapRead (
> + IN PCI_CAP_DEV *PciDevice,
> + IN PCI_CAP *Cap,
> + IN UINT16 SourceOffsetInCap,
> + OUT VOID *DestinationBuffer,
> + IN UINT16 Size
> + );
> +
> +
> +/**
> + Write a slice of a capability instance.
> +
> + The function performs as few config space accesses as possible (without
> + attempting 64-bit wide accesses). PciCapWrite() performs bounds checking on
> + DestinationOffsetInCap and Size, and only invokes PciDevice->WriteConfig() if
> + the requested transfer falls within Cap.
> +
> + @param[in] PciDevice Implementation-specific unique
> + representation of the PCI device in the
> + PCI hierarchy.
> +
> + @param[in] Cap The capability instance to write, located
> + with PciCapListFindCap*().
> +
> + @param[in] DestinationOffsetInCap Destination offset relative to the
> + capability header to start writing at. A
> + zero value refers to the first byte of the
> + capability header.
> +
> + @param[in] SourceBuffer Buffer to read the data to be stored from.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from
> + SourceBuffer to Cap.
> +
> + @retval RETURN_BAD_BUFFER_SIZE Writing Size bytes starting at
> + DestinationOffsetInCap would not (entirely)
> + be contained within Cap, as suggested by
> + PCI_CAP_INFO.MaxSizeHint. No bytes have been
> + written.
> +
> + @return Error codes propagated from
> + PciDevice->WriteConfig(). Fewer than Size
> + bytes may have been written.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapWrite (
> + IN PCI_CAP_DEV *PciDevice,
> + IN PCI_CAP *Cap,
> + IN UINT16 DestinationOffsetInCap,
> + IN VOID *SourceBuffer,
> + IN UINT16 Size
> + );
> +
> +#endif // __PCI_CAP_LIB_H__
> diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h
> new file mode 100644
> index 000000000000..e631745834d9
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.h
> @@ -0,0 +1,60 @@
> +/** @file
> + Work with PCI capabilities in PCI config space -- internal type definitions.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#ifndef __BASE_PCI_CAP_LIB_H__
> +#define __BASE_PCI_CAP_LIB_H__
> +
> +#include <Library/OrderedCollectionLib.h>
> +
> +#include <Library/PciCapLib.h>
> +
> +//
> +// Structure that uniquely identifies a capability instance and serves as key
> +// for insertion and lookup.
> +//
> +typedef struct {
> + PCI_CAP_DOMAIN Domain;
> + UINT16 CapId;
> + UINT16 Instance;
> +} PCI_CAP_KEY;
> +
> +//
> +// In Instance==0 PCI_CAP objects, store NumInstances directly. In Instance>0
> +// PCI_CAP objects, link Instance#0 of the same (Domain, CapId). This way
> +// NumInstances needs maintenance in one object only, per (Domain, CapId) pair.
> +//
> +typedef union {
> + UINT16 NumInstances;
> + PCI_CAP *InstanceZero;
> +} PCI_CAP_NUM_INSTANCES;
> +
> +//
> +// Complete the incomplete PCI_CAP structure here.
> +//
> +struct PCI_CAP {
> + PCI_CAP_KEY Key;
> + PCI_CAP_NUM_INSTANCES NumInstancesUnion;
> + UINT16 Offset;
> + UINT16 MaxSizeHint;
> + UINT8 Version;
> +};
> +
> +//
> +// Complete the incomplete PCI_CAP_LIST structure here.
> +//
> +struct PCI_CAP_LIST {
> + ORDERED_COLLECTION *Capabilities;
> +};
> +
> +#endif // __BASE_PCI_CAP_LIB_H__
> diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> new file mode 100644
> index 000000000000..6789359f0a54
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> @@ -0,0 +1,1007 @@
> +/** @file
> + Work with PCI capabilities in PCI config space.
> +
> + Provides functions to parse capabilities lists, and to locate, describe, read
> + and write capabilities. PCI config space access is abstracted away.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#include <IndustryStandard/PciExpress21.h>
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +#include "BasePciCapLib.h"
> +
> +
> +/**
> + Compare a standalone PCI_CAP_KEY against a PCI_CAP containing an embedded
> + PCI_CAP_KEY.
> +
> + @param[in] PciCapKey Pointer to the bare PCI_CAP_KEY.
> +
> + @param[in] PciCap Pointer to the PCI_CAP with the embedded PCI_CAP_KEY.
> +
> + @retval <0 If PciCapKey compares less than PciCap->Key.
> +
> + @retval 0 If PciCapKey compares equal to PciCap->Key.
> +
> + @retval >0 If PciCapKey compares greater than PciCap->Key.
> +**/
> +STATIC
> +INTN
> +EFIAPI
> +ComparePciCapKey (
> + IN CONST VOID *PciCapKey,
> + IN CONST VOID *PciCap
> + )
> +{
> + CONST PCI_CAP_KEY *Key1;
> + CONST PCI_CAP_KEY *Key2;
> +
> + Key1 = PciCapKey;
> + Key2 = &((CONST PCI_CAP *)PciCap)->Key;
> +
> + if (Key1->Domain < Key2->Domain) {
> + return -1;
> + }
> + if (Key1->Domain > Key2->Domain) {
> + return 1;
> + }
> + if (Key1->CapId < Key2->CapId) {
> + return -1;
> + }
> + if (Key1->CapId > Key2->CapId) {
> + return 1;
> + }
> + if (Key1->Instance < Key2->Instance) {
> + return -1;
> + }
> + if (Key1->Instance > Key2->Instance) {
> + return 1;
> + }
> + return 0;
> +}
> +
> +
> +/**
> + Compare two PCI_CAP objects based on PCI_CAP.Key.
> +
> + @param[in] PciCap1 Pointer to the first PCI_CAP.
> +
> + @param[in] PciCap2 Pointer to the second PCI_CAP.
> +
> + @retval <0 If PciCap1 compares less than PciCap2.
> +
> + @retval 0 If PciCap1 compares equal to PciCap2.
> +
> + @retval >0 If PciCap1 compares greater than PciCap2.
> +**/
> +STATIC
> +INTN
> +EFIAPI
> +ComparePciCap (
> + IN CONST VOID *PciCap1,
> + IN CONST VOID *PciCap2
> + )
> +{
> + CONST PCI_CAP_KEY *PciCap1Key;
> +
> + PciCap1Key = &((CONST PCI_CAP *)PciCap1)->Key;
> + return ComparePciCapKey (PciCap1Key, PciCap2);
> +}
> +
> +
> +/**
> + Compare the standalone UINT16 config space offset of a capability header
> + against a PCI_CAP containing an embedded Offset.
> +
> + @param[in] CapHdrOffset Pointer to the bare UINT16 config space offset.
> +
> + @param[in] PciCap Pointer to the PCI_CAP with the embedded Offset.
> +
> + @retval <0 If CapHdrOffset compares less than PciCap->Offset.
> +
> + @retval 0 If CapHdrOffset compares equal to PciCap->Offset.
> +
> + @retval >0 If CapHdrOffset compares greater than PciCap->Offset.
> +**/
> +STATIC
> +INTN
> +EFIAPI
> +ComparePciCapOffsetKey (
> + IN CONST VOID *CapHdrOffset,
> + IN CONST VOID *PciCap
> + )
> +{
> + UINT16 Offset1;
> + UINT16 Offset2;
> +
> + Offset1 = *(CONST UINT16 *)CapHdrOffset;
> + Offset2 = ((CONST PCI_CAP *)PciCap)->Offset;
> + //
> + // Note: both Offset1 and Offset2 are promoted to INT32 below, and the
> + // subtraction takes place between INT32 values.
> + //
> + return Offset1 - Offset2;
> +}
> +
> +
> +/**
> + Compare two PCI_CAP objects based on PCI_CAP.Offset.
> +
> + @param[in] PciCap1 Pointer to the first PCI_CAP.
> +
> + @param[in] PciCap2 Pointer to the second PCI_CAP.
> +
> + @retval <0 If PciCap1 compares less than PciCap2.
> +
> + @retval 0 If PciCap1 compares equal to PciCap2.
> +
> + @retval >0 If PciCap1 compares greater than PciCap2.
> +**/
> +STATIC
> +INTN
> +EFIAPI
> +ComparePciCapOffset (
> + IN CONST VOID *PciCap1,
> + IN CONST VOID *PciCap2
> + )
> +{
> + UINT16 Offset1;
> + UINT16 Offset2;
> +
> + Offset1 = ((CONST PCI_CAP *)PciCap1)->Offset;
> + Offset2 = ((CONST PCI_CAP *)PciCap2)->Offset;
> + //
> + // Note: both Offset1 and Offset2 are promoted to INT32 below, and the
> + // subtraction takes place between INT32 values.
> + //
> + return Offset1 - Offset2;
> +}
> +
> +
> +/**
> + Insert a new instance of the PCI capability given by (Domain, CapId) in
> + CapList.
> +
> + @param[in,out] CapList The PCI_CAP_LIST into which the new PCI_CAP
> + should be inserted. CapList will own the new
> + PCI_CAP structure.
> +
> + @param[in,out] CapHdrOffsets Link the new PCI_CAP structure into the
> + (non-owning) CapHdrOffsets collection as well.
> + CapHdrOffsets orders the PCI_CAP structures
> + based on the PCI_CAP.Offset member, and enables
> + the calculation of PCI_CAP.MaxSizeHint.
> +
> + @param[in] Domain Whether the capability is normal or extended.
> +
> + @param[in] CapId Capability ID (specific to Domain).
> +
> + @param[in] Offset Config space offset at which the standard
> + header of the capability starts. The caller is
> + responsible for ensuring that Offset be DWORD
> + aligned. The caller is also responsible for
> + ensuring that Offset be within the config space
> + identified by Domain.
> +
> + @param[in] Version The version number of the capability. The
> + caller is responsible for passing 0 as Version
> + if Domain is PciCapNormal.
> +
> + @retval RETURN_SUCCESS Insertion successful.
> +
> + @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @retval RETURN_DEVICE_ERROR A PCI_CAP with Offset is already linked by
> + CapHdrOffsets. This indicates a loop in the
> + capabilities list being parsed.
> +**/
> +STATIC
> +RETURN_STATUS
> +InsertPciCap (
> + IN OUT PCI_CAP_LIST *CapList,
> + IN OUT ORDERED_COLLECTION *CapHdrOffsets,
> + IN PCI_CAP_DOMAIN Domain,
> + IN UINT16 CapId,
> + IN UINT16 Offset,
> + IN UINT8 Version
> + )
> +{
> + PCI_CAP *PciCap;
> + RETURN_STATUS Status;
> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
> + PCI_CAP *InstanceZero;
> +
> + ASSERT ((Offset & 0x3) == 0);
> + ASSERT (Offset < (Domain == PciCapNormal ?
> + PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET));
> + ASSERT (Domain == PciCapExtended || Version == 0);
> +
> + //
> + // Set InstanceZero to suppress incorrect compiler/analyzer warnings.
> + //
> + InstanceZero = NULL;
> +
> + //
> + // Allocate PciCap, and populate it assuming it is the first occurrence of
> + // (Domain, CapId). Note that PciCap->MaxSizeHint is not assigned the final
> + // value just yet.
> + //
> + PciCap = AllocatePool (sizeof *PciCap);
> + if (PciCap == NULL) {
> + return RETURN_OUT_OF_RESOURCES;
> + }
> + PciCap->Key.Domain = Domain;
> + PciCap->Key.CapId = CapId;
> + PciCap->Key.Instance = 0;
> + PciCap->NumInstancesUnion.NumInstances = 1;
> + PciCap->Offset = Offset;
> + PciCap->MaxSizeHint = 0;
> + PciCap->Version = Version;
> +
> + //
> + // Add PciCap to CapList.
> + //
> + Status = OrderedCollectionInsert (CapList->Capabilities, &PciCapEntry,
> + PciCap);
> + if (RETURN_ERROR (Status)) {
> + if (Status == RETURN_OUT_OF_RESOURCES) {
> + goto FreePciCap;
> + }
> + ASSERT (Status == RETURN_ALREADY_STARTED);
> + //
> + // PciCap is not the first instance of (Domain, CapId). Add it as a new
> + // instance, taking the current instance count from Instance#0. Note that
> + // we don't bump the instance count maintained in Instance#0 just yet, to
> + // keep rollback on errors simple.
> + //
> + InstanceZero = OrderedCollectionUserStruct (PciCapEntry);
> + PciCap->Key.Instance = InstanceZero->NumInstancesUnion.NumInstances;
> + PciCap->NumInstancesUnion.InstanceZero = InstanceZero;
> +
> + ASSERT (PciCap->Key.Instance > 0);
> + Status = OrderedCollectionInsert (CapList->Capabilities, &PciCapEntry,
> + PciCap);
> + if (Status == RETURN_OUT_OF_RESOURCES) {
> + goto FreePciCap;
> + }
> + }
> + //
> + // At this point, PciCap has been inserted in CapList->Capabilities, either
> + // with Instance==0 or with Instance>0. PciCapEntry is the iterator that
> + // links PciCap.
> + //
> + ASSERT_RETURN_ERROR (Status);
> +
> + //
> + // Link PciCap into CapHdrOffsets too, to order it globally based on config
> + // space offset. Note that partial overlaps between capability headers is not
> + // possible: Offset is DWORD aligned, normal capability headers are 16-bit
> + // wide, and extended capability headers are 32-bit wide. Therefore any two
> + // capability headers either are distinct or start at the same offset
> + // (implying a loop in the respective capabilities list).
> + //
> + Status = OrderedCollectionInsert (CapHdrOffsets, NULL, PciCap);
> + if (RETURN_ERROR (Status)) {
> + if (Status == RETURN_ALREADY_STARTED) {
> + //
> + // Loop found; map return status accordingly.
> + //
> + Status = RETURN_DEVICE_ERROR;
> + }
> + goto DeletePciCapFromCapList;
> + }
> +
> + //
> + // Now we can bump the instance count maintained in Instance#0, if PciCap is
> + // not the first instance of (Domain, CapId).
> + //
> + if (PciCap->Key.Instance > 0) {
> + InstanceZero->NumInstancesUnion.NumInstances++;
> + }
> + return RETURN_SUCCESS;
> +
> +DeletePciCapFromCapList:
> + OrderedCollectionDelete (CapList->Capabilities, PciCapEntry, NULL);
> +
> +FreePciCap:
> + FreePool (PciCap);
> +
> + return Status;
> +}
> +
> +
> +/**
> + Calculate the MaxSizeHint member for a PCI_CAP object.
> +
> + CalculatePciCapMaxSizeHint() may only be called once all capability instances
> + have been successfully processed by InsertPciCap().
> +
> + @param[in,out] PciCap The PCI_CAP object for which to calculate the
> + MaxSizeHint member. The caller is responsible for
> + passing a PCI_CAP object that has been created by a
> + successful invocation of InsertPciCap().
> +
> + @param[in] NextPciCap If NextPciCap is NULL, then the caller is responsible
> + for PciCap to represent the capability instance with
> + the highest header offset in all config space. If
> + NextPciCap is not NULL, then the caller is responsible
> + for (a) having created NextPciCap with a successful
> + invocation of InsertPciCap(), and (b) NextPciCap being
> + the direct successor of PciCap in config space offset
> + order, as ordered by ComparePciCapOffset().
> +**/
> +STATIC
> +VOID
> +CalculatePciCapMaxSizeHint (
> + IN OUT PCI_CAP *PciCap,
> + IN PCI_CAP *NextPciCap OPTIONAL
> + )
> +{
> + UINT16 ConfigSpaceSize;
> +
> + ConfigSpaceSize = (PciCap->Key.Domain == PciCapNormal ?
> + PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
> + //
> + // The following is guaranteed by the interface contract on
> + // CalculatePciCapMaxSizeHint().
> + //
> + ASSERT (NextPciCap == NULL || PciCap->Offset < NextPciCap->Offset);
> + //
> + // The following is guaranteed by the interface contract on InsertPciCap().
> + //
> + ASSERT (PciCap->Offset < ConfigSpaceSize);
> + //
> + // Thus we can safely subtract PciCap->Offset from either of
> + // - ConfigSpaceSize
> + // - and NextPciCap->Offset (if NextPciCap is not NULL).
> + //
> + // PciCap extends from PciCap->Offset to NextPciCap->Offset (if any), except
> + // it cannot cross config space boundary.
> + //
> + if (NextPciCap == NULL || NextPciCap->Offset >= ConfigSpaceSize) {
> + PciCap->MaxSizeHint = ConfigSpaceSize - PciCap->Offset;
> + return;
> + }
> + PciCap->MaxSizeHint = NextPciCap->Offset - PciCap->Offset;
> +}
> +
> +
> +/**
> + Debug dump a PCI_CAP_LIST object at the DEBUG_VERBOSE level.
> +
> + @param[in] CapList The PCI_CAP_LIST object to dump.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +DebugDumpPciCapList (
> + IN PCI_CAP_LIST *CapList
> + )
> +{
> + DEBUG_CODE_BEGIN ();
> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
> +
> + for (PciCapEntry = OrderedCollectionMin (CapList->Capabilities);
> + PciCapEntry != NULL;
> + PciCapEntry = OrderedCollectionNext (PciCapEntry)) {
> + PCI_CAP *PciCap;
> + RETURN_STATUS Status;
> + PCI_CAP_INFO Info;
> +
Move these out of the loop?
> + PciCap = OrderedCollectionUserStruct (PciCapEntry);
> + Status = PciCapGetInfo (PciCap, &Info);
> + //
> + // PciCapGetInfo() cannot fail in this library instance.
> + //
> + ASSERT_RETURN_ERROR (Status);
> +
> + DEBUG ((DEBUG_VERBOSE,
> + "%a:%a: %a 0x%04x %03u/%03u v0x%x @0x%03x+0x%03x\n", gEfiCallerBaseName,
> + __FUNCTION__, (Info.Domain == PciCapNormal ? "Norm" : "Extd"),
> + Info.CapId, Info.Instance, Info.NumInstances, Info.Version, Info.Offset,
> + Info.MaxSizeHint));
> + }
> + DEBUG_CODE_END ();
> +}
> +
> +
> +/**
> + Empty a collection of PCI_CAP structures, optionally releasing the referenced
> + PCI_CAP structures themselves. Release the collection at last.
> +
> + @param[in,out] PciCapCollection The collection to empty and release.
> +
> + @param[in] FreePciCap TRUE if the PCI_CAP structures linked by
> + PciCapCollection should be released. When
> + FALSE, the caller is responsible for
> + retaining at least one reference to each
> + PCI_CAP structure originally linked by
> + PciCapCollection.
> +**/
> +STATIC
> +VOID
> +EmptyAndUninitPciCapCollection (
> + IN OUT ORDERED_COLLECTION *PciCapCollection,
> + IN BOOLEAN FreePciCap
> + )
> +{
> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
> + ORDERED_COLLECTION_ENTRY *NextEntry;
> +
> + for (PciCapEntry = OrderedCollectionMin (PciCapCollection);
> + PciCapEntry != NULL;
> + PciCapEntry = NextEntry) {
> + PCI_CAP *PciCap;
> +
and this one
> + NextEntry = OrderedCollectionNext (PciCapEntry);
> + OrderedCollectionDelete (PciCapCollection, PciCapEntry, (VOID **)&PciCap);
> + if (FreePciCap) {
> + FreePool (PciCap);
> + }
> + }
> + OrderedCollectionUninit (PciCapCollection);
> +}
> +
> +
> +/**
> + Parse the capabilities lists (both normal and extended, as applicable) of a
> + PCI device.
> +
> + If the PCI device has no capabilities, that per se will not fail
> + PciCapListInit(); an empty capabilities list will be represented.
> +
> + If the PCI device is found to be PCI Express, then an attempt will be made to
> + parse the extended capabilities list as well. If the first extended config
> + space access -- via PciDevice->ReadConfig() with SourceOffset=0x100 and
> + Size=4 -- fails, that per se will not fail PciCapListInit(); the device will
> + be assumed to have no extended capabilities.
> +
> + @param[in] PciDevice Implementation-specific unique representation of the
> + PCI device in the PCI hierarchy.
> +
> + @param[out] CapList Opaque data structure that holds an in-memory
> + representation of the parsed capabilities lists of
> + PciDevice.
> +
> + @retval RETURN_SUCCESS The capabilities lists have been parsed from
> + config space.
> +
> + @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @retval RETURN_DEVICE_ERROR A loop or some other kind of invalid pointer
> + was detected in the capabilities lists of
> + PciDevice.
> +
> + @return Error codes propagated from
> + PciDevice->ReadConfig().
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapListInit (
> + IN PCI_CAP_DEV *PciDevice,
> + OUT PCI_CAP_LIST **CapList
> + )
> +{
> + PCI_CAP_LIST *OutCapList;
> + RETURN_STATUS Status;
> + ORDERED_COLLECTION *CapHdrOffsets;
> + UINT16 PciStatusReg;
> + BOOLEAN DeviceIsExpress;
> + ORDERED_COLLECTION_ENTRY *OffsetEntry;
> +
> + //
> + // Allocate the output structure.
> + //
> + OutCapList = AllocatePool (sizeof *OutCapList);
> + if (OutCapList == NULL) {
> + return RETURN_OUT_OF_RESOURCES;
> + }
> + //
> + // The OutCapList->Capabilities collection owns the PCI_CAP structures and
> + // orders them based on PCI_CAP.Key.
> + //
> + OutCapList->Capabilities = OrderedCollectionInit (ComparePciCap,
> + ComparePciCapKey);
> + if (OutCapList->Capabilities == NULL) {
> + Status = RETURN_OUT_OF_RESOURCES;
> + goto FreeOutCapList;
> + }
> +
> + //
> + // The (temporary) CapHdrOffsets collection only references PCI_CAP
> + // structures, and orders them based on PCI_CAP.Offset.
> + //
> + CapHdrOffsets = OrderedCollectionInit (ComparePciCapOffset,
> + ComparePciCapOffsetKey);
> + if (CapHdrOffsets == NULL) {
> + Status = RETURN_OUT_OF_RESOURCES;
> + goto FreeCapabilities;
> + }
> +
> + //
> + // Whether the device is PCI Express depends on the normal capability with
> + // identifier EFI_PCI_CAPABILITY_ID_PCIEXP.
> + //
> + DeviceIsExpress = FALSE;
> +
> + //
> + // Check whether a normal capabilities list is present. If there's none,
> + // that's not an error; we'll just return OutCapList->Capabilities empty.
> + //
> + Status = PciDevice->ReadConfig (PciDevice, PCI_PRIMARY_STATUS_OFFSET,
> + &PciStatusReg, sizeof PciStatusReg);
> + if (RETURN_ERROR (Status)) {
> + goto FreeCapHdrOffsets;
> + }
> + if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) {
> + UINT8 NormalCapHdrOffset;
> +
and this one
> + //
> + // Fetch the start offset of the normal capabilities list.
> + //
> + Status = PciDevice->ReadConfig (PciDevice, PCI_CAPBILITY_POINTER_OFFSET,
> + &NormalCapHdrOffset, sizeof NormalCapHdrOffset);
> + if (RETURN_ERROR (Status)) {
> + goto FreeCapHdrOffsets;
> + }
> +
> + //
> + // Traverse the normal capabilities list.
> + //
> + NormalCapHdrOffset &= 0xFC;
> + while (NormalCapHdrOffset > 0) {
> + EFI_PCI_CAPABILITY_HDR NormalCapHdr;
> +
and this one.
Perhaps I am missing something? After four instances, it seems
deliberate rather than accidental :-)
> + Status = PciDevice->ReadConfig (PciDevice, NormalCapHdrOffset,
> + &NormalCapHdr, sizeof NormalCapHdr);
> + if (RETURN_ERROR (Status)) {
> + goto FreeCapHdrOffsets;
> + }
> +
> + Status = InsertPciCap (OutCapList, CapHdrOffsets, PciCapNormal,
> + NormalCapHdr.CapabilityID, NormalCapHdrOffset, 0);
> + if (RETURN_ERROR (Status)) {
> + goto FreeCapHdrOffsets;
> + }
> +
> + if (NormalCapHdr.CapabilityID == EFI_PCI_CAPABILITY_ID_PCIEXP) {
> + DeviceIsExpress = TRUE;
> + }
> + NormalCapHdrOffset = NormalCapHdr.NextItemPtr & 0xFC;
> + }
> + }
> +
> + //
> + // If the device has been found PCI Express, attempt to traverse the extended
> + // capabilities list. It starts right after the normal config space.
> + //
> + if (DeviceIsExpress) {
> + UINT16 ExtendedCapHdrOffset;
> +
> + ExtendedCapHdrOffset = PCI_MAX_CONFIG_OFFSET;
> + while (ExtendedCapHdrOffset > 0) {
> + PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER ExtendedCapHdr;
> +
> + Status = PciDevice->ReadConfig (PciDevice, ExtendedCapHdrOffset,
> + &ExtendedCapHdr, sizeof ExtendedCapHdr);
> + //
> + // If the first extended config space access fails, assume the device has
> + // no extended capabilities. If the first extended config space access
> + // succeeds but we read an "all bits zero" extended capability header,
> + // that means (by spec) the device has no extended capabilities.
> + //
> + if (ExtendedCapHdrOffset == PCI_MAX_CONFIG_OFFSET &&
> + (RETURN_ERROR (Status) ||
> + IsZeroBuffer (&ExtendedCapHdr, sizeof ExtendedCapHdr))) {
> + break;
> + }
> + if (RETURN_ERROR (Status)) {
> + goto FreeCapHdrOffsets;
> + }
> +
> + Status = InsertPciCap (OutCapList, CapHdrOffsets, PciCapExtended,
> + ExtendedCapHdr.CapabilityId, ExtendedCapHdrOffset,
> + ExtendedCapHdr.CapabilityVersion);
> + if (RETURN_ERROR (Status)) {
> + goto FreeCapHdrOffsets;
> + }
> +
> + ExtendedCapHdrOffset = ExtendedCapHdr.NextCapabilityOffset & 0xFFC;
> + if (ExtendedCapHdrOffset > 0 &&
> + ExtendedCapHdrOffset < PCI_MAX_CONFIG_OFFSET) {
> + //
> + // Invalid capability pointer.
> + //
> + Status = RETURN_DEVICE_ERROR;
> + goto FreeCapHdrOffsets;
> + }
> + }
> + }
> +
> + //
> + // Both capabilities lists have been parsed; compute the PCI_CAP.MaxSizeHint
> + // members if at least one capability has been found. In parallel, evacuate
> + // the CapHdrOffsets collection.
> + //
> + // At first, set OffsetEntry to the iterator of the PCI_CAP object with the
> + // lowest Offset (if such exists).
> + //
> + OffsetEntry = OrderedCollectionMin (CapHdrOffsets);
> + if (OffsetEntry != NULL) {
> + ORDERED_COLLECTION_ENTRY *NextOffsetEntry;
> + PCI_CAP *PciCap;
> +
> + //
> + // Initialize NextOffsetEntry to the iterator of the PCI_CAP object with
> + // the second lowest Offset (if such exists).
> + //
> + NextOffsetEntry = OrderedCollectionNext (OffsetEntry);
> + //
> + // Calculate MaxSizeHint for all PCI_CAP objects except the one with the
> + // highest Offset.
> + //
> + while (NextOffsetEntry != NULL) {
> + PCI_CAP *NextPciCap;
> +
> + OrderedCollectionDelete (CapHdrOffsets, OffsetEntry, (VOID **)&PciCap);
> + NextPciCap = OrderedCollectionUserStruct (NextOffsetEntry);
> + CalculatePciCapMaxSizeHint (PciCap, NextPciCap);
> +
> + OffsetEntry = NextOffsetEntry;
> + NextOffsetEntry = OrderedCollectionNext (OffsetEntry);
> + }
> + //
> + // Calculate MaxSizeHint for the PCI_CAP object with the highest Offset.
> + //
> + OrderedCollectionDelete (CapHdrOffsets, OffsetEntry, (VOID **)&PciCap);
> + CalculatePciCapMaxSizeHint (PciCap, NULL);
> + }
> + ASSERT (OrderedCollectionIsEmpty (CapHdrOffsets));
> + OrderedCollectionUninit (CapHdrOffsets);
> +
> + DebugDumpPciCapList (OutCapList);
> + *CapList = OutCapList;
> + return RETURN_SUCCESS;
> +
> +FreeCapHdrOffsets:
> + EmptyAndUninitPciCapCollection (CapHdrOffsets, FALSE);
> +
> +FreeCapabilities:
> + EmptyAndUninitPciCapCollection (OutCapList->Capabilities, TRUE);
> +
> +FreeOutCapList:
> + FreePool (OutCapList);
> +
> + ASSERT (RETURN_ERROR (Status));
> + DEBUG ((DEBUG_ERROR, "%a:%a: %r\n", gEfiCallerBaseName, __FUNCTION__,
> + Status));
> + return Status;
> +}
> +
> +
> +/**
> + Free the resources used by CapList.
> +
> + @param[in] CapList The PCI_CAP_LIST object to free, originally produced by
> + PciCapListInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapListUninit (
> + IN PCI_CAP_LIST *CapList
> + )
> +{
> + EmptyAndUninitPciCapCollection (CapList->Capabilities, TRUE);
> + FreePool (CapList);
> +}
> +
> +
> +/**
> + Locate a capability instance in the parsed capabilities lists.
> +
> + @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
> +
> + @param[in] Domain Distinguishes whether CapId is 8-bit wide and
> + interpreted in normal config space, or 16-bit wide and
> + interpreted in extended config space. Capability ID
> + definitions are relative to domain.
> +
> + @param[in] CapId Capability identifier to look up.
> +
> + @param[in] Instance Domain and CapId may identify a multi-instance
> + capability. When Instance is zero, the first instance of
> + the capability is located (in list traversal order --
> + which may not mean increasing config space offset
> + order). Higher Instance values locate subsequent
> + instances of the same capability (in list traversal
> + order).
> +
> + @param[out] Cap The capability instance that matches the search
> + criteria. Cap is owned by CapList and becomes invalid
> + when CapList is freed with PciCapListUninit().
> + PciCapListFindCap() may be called with Cap set to NULL,
> + in order to test the existence of a specific capability
> + instance.
> +
> + @retval RETURN_SUCCESS The capability instance identified by (Domain,
> + CapId, Instance) has been found.
> +
> + @retval RETURN_NOT_FOUND The requested (Domain, CapId, Instance) capability
> + instance does not exist.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapListFindCap (
> + IN PCI_CAP_LIST *CapList,
> + IN PCI_CAP_DOMAIN Domain,
> + IN UINT16 CapId,
> + IN UINT16 Instance,
> + OUT PCI_CAP **Cap OPTIONAL
> + )
> +{
> + PCI_CAP_KEY Key;
> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
> +
> + Key.Domain = Domain;
> + Key.CapId = CapId;
> + Key.Instance = Instance;
> +
> + PciCapEntry = OrderedCollectionFind (CapList->Capabilities, &Key);
> + if (PciCapEntry == NULL) {
> + return RETURN_NOT_FOUND;
> + }
> + if (Cap != NULL) {
> + *Cap = OrderedCollectionUserStruct (PciCapEntry);
> + }
> + return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> + Locate the first instance of the capability given by (Domain, CapId) such
> + that the instance's Version is greater than or equal to MinVersion.
> +
> + This is a convenience function that may save client code calls to
> + PciCapListFindCap() and PciCapGetInfo().
> +
> + @param[in] CapList The PCI_CAP_LIST object produced by PciCapListInit().
> +
> + @param[in] Domain Distinguishes whether CapId is 8-bit wide and
> + interpreted in normal config space, or 16-bit wide and
> + interpreted in extended config space. Capability ID
> + definitions are relative to domain.
> +
> + @param[in] CapId Capability identifier to look up.
> +
> + @param[in] MinVersion The minimum version that the capability instance is
> + required to have. Note that all capability instances
> + in Domain=PciCapNormal have Version=0.
> +
> + @param[out] Cap The first capability instance that matches the search
> + criteria. Cap is owned by CapList and becomes invalid
> + when CapList is freed with PciCapListUninit().
> + PciCapListFindCapVersion() may be called with Cap set
> + to NULL, in order just to test whether the search
> + criteria are satisfiable.
> +
> + @retval RETURN_SUCCESS The first capability instance matching (Domain,
> + CapId, MinVersion) has been located.
> +
> + @retval RETURN_NOT_FOUND No capability instance matches (Domain, CapId,
> + MinVersion).
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapListFindCapVersion (
> + IN PCI_CAP_LIST *CapList,
> + IN PCI_CAP_DOMAIN Domain,
> + IN UINT16 CapId,
> + IN UINT8 MinVersion,
> + OUT PCI_CAP **Cap OPTIONAL
> + )
> +{
> + PCI_CAP_KEY Key;
> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
> +
> + //
> + // Start the version checks at Instance#0 of (Domain, CapId).
> + //
> + Key.Domain = Domain;
> + Key.CapId = CapId;
> + Key.Instance = 0;
> +
> + for (PciCapEntry = OrderedCollectionFind (CapList->Capabilities, &Key);
> + PciCapEntry != NULL;
> + PciCapEntry = OrderedCollectionNext (PciCapEntry)) {
> + PCI_CAP *PciCap;
> +
> + PciCap = OrderedCollectionUserStruct (PciCapEntry);
> + //
> + // PCI_CAP.Key ordering keeps instances of the same (Domain, CapId)
> + // adjacent to each other, so stop searching if either Domain or CapId
> + // changes.
> + //
> + if (PciCap->Key.Domain != Domain || PciCap->Key.CapId != CapId) {
> + break;
> + }
> + if (PciCap->Version >= MinVersion) {
> + //
> + // Match found.
> + //
> + if (Cap != NULL) {
> + *Cap = PciCap;
> + }
> + return RETURN_SUCCESS;
> + }
> + }
> + return RETURN_NOT_FOUND;
> +}
> +
> +
> +/**
> + Get information about a PCI Capability instance.
> +
> + @param[in] Cap The capability instance to get info about, located with
> + PciCapListFindCap*().
> +
> + @param[out] Info A PCI_CAP_INFO structure that describes the properties of
> + Cap.
> +
> + @retval RETURN_SUCCESS Fields of Info have been set.
> +
> + @return Unspecified error codes, if filling in Info failed
> + for some reason.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapGetInfo (
> + IN PCI_CAP *Cap,
> + OUT PCI_CAP_INFO *Info
> + )
> +{
> + PCI_CAP *InstanceZero;
> +
Nit: add
ASSERT (Info != NULL);
here?
I know it seems rather arbitrary to add it here and not anywhere else,
but PciCapGetInfo() is part of the API, and dereferencing Info [which
may be the result of e.g., a pool allocation] for writing is
particularly bad.
> + InstanceZero = (Cap->Key.Instance == 0 ? Cap :
> + Cap->NumInstancesUnion.InstanceZero);
> +
> + Info->Domain = Cap->Key.Domain;
> + Info->CapId = Cap->Key.CapId;
> + Info->NumInstances = InstanceZero->NumInstancesUnion.NumInstances;
> + Info->Instance = Cap->Key.Instance;
> + Info->Offset = Cap->Offset;
> + Info->MaxSizeHint = Cap->MaxSizeHint;
> + Info->Version = Cap->Version;
> +
> + return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> + Read a slice of a capability instance.
> +
> + The function performs as few config space accesses as possible (without
> + attempting 64-bit wide accesses). PciCapRead() performs bounds checking on
> + SourceOffsetInCap and Size, and only invokes PciDevice->ReadConfig() if the
> + requested transfer falls within Cap.
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] Cap The capability instance to read, located with
> + PciCapListFindCap*().
> +
> + @param[in] SourceOffsetInCap Source offset relative to the capability
> + header to start reading from. A zero value
> + refers to the first byte of the capability
> + header.
> +
> + @param[out] DestinationBuffer Buffer to store the read data to.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from Cap to
> + DestinationBuffer.
> +
> + @retval RETURN_BAD_BUFFER_SIZE Reading Size bytes starting from
> + SourceOffsetInCap would not (entirely) be
> + contained within Cap, as suggested by
> + PCI_CAP_INFO.MaxSizeHint. No bytes have been
> + read.
> +
> + @return Error codes propagated from
> + PciDevice->ReadConfig(). Fewer than Size
> + bytes may have been read.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapRead (
> + IN PCI_CAP_DEV *PciDevice,
> + IN PCI_CAP *Cap,
> + IN UINT16 SourceOffsetInCap,
> + OUT VOID *DestinationBuffer,
> + IN UINT16 Size
> + )
> +{
> + //
> + // Note: all UINT16 values are promoted to INT32 below, and addition and
> + // comparison take place between INT32 values.
> + //
> + if (SourceOffsetInCap + Size > Cap->MaxSizeHint) {
> + return RETURN_BAD_BUFFER_SIZE;
> + }
> + return PciDevice->ReadConfig (PciDevice, Cap->Offset + SourceOffsetInCap,
> + DestinationBuffer, Size);
> +}
> +
> +
> +/**
> + Write a slice of a capability instance.
> +
> + The function performs as few config space accesses as possible (without
> + attempting 64-bit wide accesses). PciCapWrite() performs bounds checking on
> + DestinationOffsetInCap and Size, and only invokes PciDevice->WriteConfig() if
> + the requested transfer falls within Cap.
> +
> + @param[in] PciDevice Implementation-specific unique
> + representation of the PCI device in the
> + PCI hierarchy.
> +
> + @param[in] Cap The capability instance to write, located
> + with PciCapListFindCap*().
> +
> + @param[in] DestinationOffsetInCap Destination offset relative to the
> + capability header to start writing at. A
> + zero value refers to the first byte of the
> + capability header.
> +
> + @param[in] SourceBuffer Buffer to read the data to be stored from.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from
> + SourceBuffer to Cap.
> +
> + @retval RETURN_BAD_BUFFER_SIZE Writing Size bytes starting at
> + DestinationOffsetInCap would not (entirely)
> + be contained within Cap, as suggested by
> + PCI_CAP_INFO.MaxSizeHint. No bytes have been
> + written.
> +
> + @return Error codes propagated from
> + PciDevice->WriteConfig(). Fewer than Size
> + bytes may have been written.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapWrite (
> + IN PCI_CAP_DEV *PciDevice,
> + IN PCI_CAP *Cap,
> + IN UINT16 DestinationOffsetInCap,
> + IN VOID *SourceBuffer,
> + IN UINT16 Size
> + )
> +{
> + //
> + // Note: all UINT16 values are promoted to INT32 below, and addition and
> + // comparison take place between INT32 values.
> + //
> + if (DestinationOffsetInCap + Size > Cap->MaxSizeHint) {
> + return RETURN_BAD_BUFFER_SIZE;
> + }
> + return PciDevice->WriteConfig (PciDevice,
> + Cap->Offset + DestinationOffsetInCap, SourceBuffer,
> + Size);
> +}
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 1/7] OvmfPkg: introduce PciCapLib
2018-05-24 7:53 ` Ard Biesheuvel
@ 2018-05-24 14:39 ` Laszlo Ersek
2018-05-24 14:41 ` Ard Biesheuvel
0 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-24 14:39 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen
On 05/24/18 09:53, Ard Biesheuvel wrote:
>> +STATIC
>> +VOID
>> +EFIAPI
>> +DebugDumpPciCapList (
>> + IN PCI_CAP_LIST *CapList
>> + )
>> +{
>> + DEBUG_CODE_BEGIN ();
>> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
>> +
>> + for (PciCapEntry = OrderedCollectionMin (CapList->Capabilities);
>> + PciCapEntry != NULL;
>> + PciCapEntry = OrderedCollectionNext (PciCapEntry)) {
>> + PCI_CAP *PciCap;
>> + RETURN_STATUS Status;
>> + PCI_CAP_INFO Info;
>> +
>
> Move these out of the loop?
>> +STATIC
>> +VOID
>> +EmptyAndUninitPciCapCollection (
>> + IN OUT ORDERED_COLLECTION *PciCapCollection,
>> + IN BOOLEAN FreePciCap
>> + )
>> +{
>> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
>> + ORDERED_COLLECTION_ENTRY *NextEntry;
>> +
>> + for (PciCapEntry = OrderedCollectionMin (PciCapCollection);
>> + PciCapEntry != NULL;
>> + PciCapEntry = NextEntry) {
>> + PCI_CAP *PciCap;
>> +
>
> and this one
>> +RETURN_STATUS
>> +EFIAPI
>> +PciCapListInit (
>> + IN PCI_CAP_DEV *PciDevice,
>> + OUT PCI_CAP_LIST **CapList
>> + )
>> +{
>> + PCI_CAP_LIST *OutCapList;
>> + RETURN_STATUS Status;
>> + ORDERED_COLLECTION *CapHdrOffsets;
>> + UINT16 PciStatusReg;
>> + BOOLEAN DeviceIsExpress;
>> + ORDERED_COLLECTION_ENTRY *OffsetEntry;
>> +
>> + //
>> + // Allocate the output structure.
>> + //
>> + OutCapList = AllocatePool (sizeof *OutCapList);
>> + if (OutCapList == NULL) {
>> + return RETURN_OUT_OF_RESOURCES;
>> + }
>> + //
>> + // The OutCapList->Capabilities collection owns the PCI_CAP structures and
>> + // orders them based on PCI_CAP.Key.
>> + //
>> + OutCapList->Capabilities = OrderedCollectionInit (ComparePciCap,
>> + ComparePciCapKey);
>> + if (OutCapList->Capabilities == NULL) {
>> + Status = RETURN_OUT_OF_RESOURCES;
>> + goto FreeOutCapList;
>> + }
>> +
>> + //
>> + // The (temporary) CapHdrOffsets collection only references PCI_CAP
>> + // structures, and orders them based on PCI_CAP.Offset.
>> + //
>> + CapHdrOffsets = OrderedCollectionInit (ComparePciCapOffset,
>> + ComparePciCapOffsetKey);
>> + if (CapHdrOffsets == NULL) {
>> + Status = RETURN_OUT_OF_RESOURCES;
>> + goto FreeCapabilities;
>> + }
>> +
>> + //
>> + // Whether the device is PCI Express depends on the normal capability with
>> + // identifier EFI_PCI_CAPABILITY_ID_PCIEXP.
>> + //
>> + DeviceIsExpress = FALSE;
>> +
>> + //
>> + // Check whether a normal capabilities list is present. If there's none,
>> + // that's not an error; we'll just return OutCapList->Capabilities empty.
>> + //
>> + Status = PciDevice->ReadConfig (PciDevice, PCI_PRIMARY_STATUS_OFFSET,
>> + &PciStatusReg, sizeof PciStatusReg);
>> + if (RETURN_ERROR (Status)) {
>> + goto FreeCapHdrOffsets;
>> + }
>> + if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) {
>> + UINT8 NormalCapHdrOffset;
>> +
>
> and this one
>
>> + //
>> + // Fetch the start offset of the normal capabilities list.
>> + //
>> + Status = PciDevice->ReadConfig (PciDevice, PCI_CAPBILITY_POINTER_OFFSET,
>> + &NormalCapHdrOffset, sizeof NormalCapHdrOffset);
>> + if (RETURN_ERROR (Status)) {
>> + goto FreeCapHdrOffsets;
>> + }
>> +
>> + //
>> + // Traverse the normal capabilities list.
>> + //
>> + NormalCapHdrOffset &= 0xFC;
>> + while (NormalCapHdrOffset > 0) {
>> + EFI_PCI_CAPABILITY_HDR NormalCapHdr;
>> +
>
> and this one.
>
> Perhaps I am missing something? After four instances, it seems
> deliberate rather than accidental :-)
It's totally deliberate. C89 supports scoping auto variables more
tightly than at the function scope, and I liberally use that feature --
scoping local variables as tightly as possible helps humans reason about
data flow. This is characteristic of all C code I write.
The coding style writes,
https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/54_code_file_structure.html#54-code-file-structure
"Data declarations may follow the opening brace of a compound statement,
regardless of nesting depth, and before any code generating statements
have been entered. Other than at the outermost block of a function body,
this type of declaration is strongly discouraged."
It's discouraged, but not outright forbidden, and personally I find that
lumping all local variables together at the top decreases readability
and harms my ability to reason about data flow.
If you'd really like me to move these variable definitions to the tops
of their respective functions, knowing my motive, I can do it, of
course. Do you want me to? :)
>> +RETURN_STATUS
>> +EFIAPI
>> +PciCapGetInfo (
>> + IN PCI_CAP *Cap,
>> + OUT PCI_CAP_INFO *Info
>> + )
>> +{
>> + PCI_CAP *InstanceZero;
>> +
>
> Nit: add
>
> ASSERT (Info != NULL);
>
> here?
>
> I know it seems rather arbitrary to add it here and not anywhere else,
> but PciCapGetInfo() is part of the API, and dereferencing Info [which
> may be the result of e.g., a pool allocation] for writing is
> particularly bad.
I will add the ASSERT().
(I hope I didn't miss any of your comments!)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 1/7] OvmfPkg: introduce PciCapLib
2018-05-24 14:39 ` Laszlo Ersek
@ 2018-05-24 14:41 ` Ard Biesheuvel
2018-05-24 17:25 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 14:41 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 24 May 2018 at 16:39, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/24/18 09:53, Ard Biesheuvel wrote:
>
>>> +STATIC
>>> +VOID
>>> +EFIAPI
>>> +DebugDumpPciCapList (
>>> + IN PCI_CAP_LIST *CapList
>>> + )
>>> +{
>>> + DEBUG_CODE_BEGIN ();
>>> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
>>> +
>>> + for (PciCapEntry = OrderedCollectionMin (CapList->Capabilities);
>>> + PciCapEntry != NULL;
>>> + PciCapEntry = OrderedCollectionNext (PciCapEntry)) {
>>> + PCI_CAP *PciCap;
>>> + RETURN_STATUS Status;
>>> + PCI_CAP_INFO Info;
>>> +
>>
>> Move these out of the loop?
>
>>> +STATIC
>>> +VOID
>>> +EmptyAndUninitPciCapCollection (
>>> + IN OUT ORDERED_COLLECTION *PciCapCollection,
>>> + IN BOOLEAN FreePciCap
>>> + )
>>> +{
>>> + ORDERED_COLLECTION_ENTRY *PciCapEntry;
>>> + ORDERED_COLLECTION_ENTRY *NextEntry;
>>> +
>>> + for (PciCapEntry = OrderedCollectionMin (PciCapCollection);
>>> + PciCapEntry != NULL;
>>> + PciCapEntry = NextEntry) {
>>> + PCI_CAP *PciCap;
>>> +
>>
>> and this one
>
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +PciCapListInit (
>>> + IN PCI_CAP_DEV *PciDevice,
>>> + OUT PCI_CAP_LIST **CapList
>>> + )
>>> +{
>>> + PCI_CAP_LIST *OutCapList;
>>> + RETURN_STATUS Status;
>>> + ORDERED_COLLECTION *CapHdrOffsets;
>>> + UINT16 PciStatusReg;
>>> + BOOLEAN DeviceIsExpress;
>>> + ORDERED_COLLECTION_ENTRY *OffsetEntry;
>>> +
>>> + //
>>> + // Allocate the output structure.
>>> + //
>>> + OutCapList = AllocatePool (sizeof *OutCapList);
>>> + if (OutCapList == NULL) {
>>> + return RETURN_OUT_OF_RESOURCES;
>>> + }
>>> + //
>>> + // The OutCapList->Capabilities collection owns the PCI_CAP structures and
>>> + // orders them based on PCI_CAP.Key.
>>> + //
>>> + OutCapList->Capabilities = OrderedCollectionInit (ComparePciCap,
>>> + ComparePciCapKey);
>>> + if (OutCapList->Capabilities == NULL) {
>>> + Status = RETURN_OUT_OF_RESOURCES;
>>> + goto FreeOutCapList;
>>> + }
>>> +
>>> + //
>>> + // The (temporary) CapHdrOffsets collection only references PCI_CAP
>>> + // structures, and orders them based on PCI_CAP.Offset.
>>> + //
>>> + CapHdrOffsets = OrderedCollectionInit (ComparePciCapOffset,
>>> + ComparePciCapOffsetKey);
>>> + if (CapHdrOffsets == NULL) {
>>> + Status = RETURN_OUT_OF_RESOURCES;
>>> + goto FreeCapabilities;
>>> + }
>>> +
>>> + //
>>> + // Whether the device is PCI Express depends on the normal capability with
>>> + // identifier EFI_PCI_CAPABILITY_ID_PCIEXP.
>>> + //
>>> + DeviceIsExpress = FALSE;
>>> +
>>> + //
>>> + // Check whether a normal capabilities list is present. If there's none,
>>> + // that's not an error; we'll just return OutCapList->Capabilities empty.
>>> + //
>>> + Status = PciDevice->ReadConfig (PciDevice, PCI_PRIMARY_STATUS_OFFSET,
>>> + &PciStatusReg, sizeof PciStatusReg);
>>> + if (RETURN_ERROR (Status)) {
>>> + goto FreeCapHdrOffsets;
>>> + }
>>> + if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) {
>>> + UINT8 NormalCapHdrOffset;
>>> +
>>
>> and this one
>>
>>> + //
>>> + // Fetch the start offset of the normal capabilities list.
>>> + //
>>> + Status = PciDevice->ReadConfig (PciDevice, PCI_CAPBILITY_POINTER_OFFSET,
>>> + &NormalCapHdrOffset, sizeof NormalCapHdrOffset);
>>> + if (RETURN_ERROR (Status)) {
>>> + goto FreeCapHdrOffsets;
>>> + }
>>> +
>>> + //
>>> + // Traverse the normal capabilities list.
>>> + //
>>> + NormalCapHdrOffset &= 0xFC;
>>> + while (NormalCapHdrOffset > 0) {
>>> + EFI_PCI_CAPABILITY_HDR NormalCapHdr;
>>> +
>>
>> and this one.
>>
>> Perhaps I am missing something? After four instances, it seems
>> deliberate rather than accidental :-)
>
> It's totally deliberate. C89 supports scoping auto variables more
> tightly than at the function scope, and I liberally use that feature --
> scoping local variables as tightly as possible helps humans reason about
> data flow. This is characteristic of all C code I write.
>
> The coding style writes,
>
> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/54_code_file_structure.html#54-code-file-structure
>
> "Data declarations may follow the opening brace of a compound statement,
> regardless of nesting depth, and before any code generating statements
> have been entered. Other than at the outermost block of a function body,
> this type of declaration is strongly discouraged."
>
> It's discouraged, but not outright forbidden, and personally I find that
> lumping all local variables together at the top decreases readability
> and harms my ability to reason about data flow.
>
> If you'd really like me to move these variable definitions to the tops
> of their respective functions, knowing my motive, I can do it, of
> course. Do you want me to? :)
>
Please don't. And thanks for educating me. I fully agree that
declaring all variables at function scope is silly, and I always
assumed it was mandated by the coding style.
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +PciCapGetInfo (
>>> + IN PCI_CAP *Cap,
>>> + OUT PCI_CAP_INFO *Info
>>> + )
>>> +{
>>> + PCI_CAP *InstanceZero;
>>> +
>>
>> Nit: add
>>
>> ASSERT (Info != NULL);
>>
>> here?
>>
>> I know it seems rather arbitrary to add it here and not anywhere else,
>> but PciCapGetInfo() is part of the API, and dereferencing Info [which
>> may be the result of e.g., a pool allocation] for writing is
>> particularly bad.
>
>
> I will add the ASSERT().
>
> (I hope I didn't miss any of your comments!)
>
It's just a nit, feel free to ignore.
In any case,
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 1/7] OvmfPkg: introduce PciCapLib
2018-05-24 14:41 ` Ard Biesheuvel
@ 2018-05-24 17:25 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-24 17:25 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen
On 05/24/18 16:41, Ard Biesheuvel wrote:
> On 24 May 2018 at 16:39, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 05/24/18 09:53, Ard Biesheuvel wrote:
>>>> +RETURN_STATUS
>>>> +EFIAPI
>>>> +PciCapGetInfo (
>>>> + IN PCI_CAP *Cap,
>>>> + OUT PCI_CAP_INFO *Info
>>>> + )
>>>> +{
>>>> + PCI_CAP *InstanceZero;
>>>> +
>>>
>>> Nit: add
>>>
>>> ASSERT (Info != NULL);
>>>
>>> here?
>>>
>>> I know it seems rather arbitrary to add it here and not anywhere else,
>>> but PciCapGetInfo() is part of the API, and dereferencing Info [which
>>> may be the result of e.g., a pool allocation] for writing is
>>> particularly bad.
>>
>>
>> I will add the ASSERT().
>>
>> (I hope I didn't miss any of your comments!)
>>
>
> It's just a nit, feel free to ignore.
>
> In any case,
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
Thanks! I'll add the ASSERT.
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
2018-05-23 20:21 ` [PATCH v2 1/7] OvmfPkg: introduce PciCapLib Laszlo Ersek
@ 2018-05-23 20:21 ` Laszlo Ersek
2018-05-24 8:08 ` Ard Biesheuvel
2018-05-23 20:21 ` [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib Laszlo Ersek
` (5 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-23 20:21 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
Add a library class, and a BASE lib instance, that are layered on top of
PciCapLib, and allow clients to plug a PciSegmentLib backend into
PciCapLib, for config space access.
(Side note:
The "MaxDomain" parameter is provided because, in practice, platforms
exist where a PCI Express device may show up on a root bridge such that
the root bridge doesn't support access to extended config space. Earlier
the same issue was handled for MdeModulePkg/PciHostBridgeDxe in commit
014b472053ae3. However, that solution does not apply to the PciSegmentLib
class, because:
(1) The config space accessor functions of the PciSegmentLib class, such
as PciSegmentReadBuffer(), have no way of informing the caller whether
access to extended config space actually succeeds.
(For example, in the UefiPciSegmentLibPciRootBridgeIo instace, which
could in theory benefit from commit 014b472053ae3, the
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() status code is explicitly
ignored, because there's no way for the lib instance to propagate it
to the PciSegmentLib caller. If the
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() call fails, then
DxePciSegmentLibPciRootBridgeIoReadWorker() returns Data with
indeterminate value.)
(2) There is no *general* way for any firmware platform to provide, or
use, a PciSegmentLib instance in which access to extended config space
always succeeds.
In brief, on a platform where config space may be limited to 256 bytes,
access to extended config space through PciSegmentLib may invoke undefined
behavior; therefore PciCapPciSegmentLib must give platforms a way to
prevent such access.)
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- move library from MdePkg to OvmfPkg, for initial introduction
OvmfPkg/OvmfPkg.dec | 5 +
OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf | 35 +++
OvmfPkg/Include/Library/PciCapPciSegmentLib.h | 82 +++++++
OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h | 47 ++++
OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c | 226 ++++++++++++++++++++
5 files changed, 395 insertions(+)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 74818a2e2a19..fbec1cfe4a8e 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -35,6 +35,11 @@ [LibraryClasses]
# config space.
PciCapLib|Include/Library/PciCapLib.h
+ ## @libraryclass Layered on top of PciCapLib, allows clients to plug a
+ # PciSegmentLib backend into PciCapLib, for config space
+ # access.
+ PciCapPciSegmentLib|Include/Library/PciCapPciSegmentLib.h
+
## @libraryclass Access QEMU's firmware configuration interface
#
QemuFwCfgLib|Include/Library/QemuFwCfgLib.h
diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
new file mode 100644
index 000000000000..e3cf5de49b15
--- /dev/null
+++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
@@ -0,0 +1,35 @@
+## @file
+# Plug a PciSegmentLib backend into PciCapLib, for config space access.
+#
+# Copyright (C) 2018, Red Hat, Inc.
+#
+# 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 = 1.27
+ BASE_NAME = BasePciCapPciSegmentLib
+ FILE_GUID = ED011855-AA31-43B9-ACC0-BF45A05C5985
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = PciCapPciSegmentLib
+
+[Sources]
+ BasePciCapPciSegmentLib.h
+ BasePciCapPciSegmentLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ MemoryAllocationLib
+ PciSegmentLib
diff --git a/OvmfPkg/Include/Library/PciCapPciSegmentLib.h b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h
new file mode 100644
index 000000000000..6b6930288d16
--- /dev/null
+++ b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h
@@ -0,0 +1,82 @@
+/** @file
+ Library class layered on top of PciCapLib that allows clients to plug a
+ PciSegmentLib backend into PciCapLib, for config space access.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#ifndef __PCI_CAP_PCI_SEGMENT_LIB_H__
+#define __PCI_CAP_PCI_SEGMENT_LIB_H__
+
+#include <Library/PciCapLib.h>
+
+
+/**
+ Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function
+ quadruplet. The config space accessors are based upon PciSegmentLib.
+
+ @param[in] MaxDomain If MaxDomain is PciCapExtended, then
+ PciDevice->ReadConfig() and PciDevice->WriteConfig()
+ will delegate extended config space accesses too to
+ PciSegmentReadBuffer() and PciSegmentWriteBuffer(),
+ respectively. Otherwise, PciDevice->ReadConfig() and
+ PciDevice->WriteConfig() will reject accesses to
+ extended config space with RETURN_UNSUPPORTED, without
+ calling PciSegmentReadBuffer() or
+ PciSegmentWriteBuffer(). By setting MaxDomain to
+ PciCapNormal, the platform can prevent undefined
+ PciSegmentLib behavior when the PCI root bridge under
+ the PCI device at Segment:Bus:Device.Function doesn't
+ support extended config space.
+
+ @param[in] Segment 16-bit wide segment number.
+
+ @param[in] Bus 8-bit wide bus number.
+
+ @param[in] Device 5-bit wide device number.
+
+ @param[in] Function 3-bit wide function number.
+
+ @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
+ PciDevice can be passed to the PciCapLib APIs.
+
+ @retval RETURN_SUCCESS PciDevice has been constructed and output.
+
+ @retval RETURN_INVALID_PARAMETER Device or Function does not fit in the
+ permitted number of bits.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapPciSegmentDeviceInit (
+ IN PCI_CAP_DOMAIN MaxDomain,
+ IN UINT16 Segment,
+ IN UINT8 Bus,
+ IN UINT8 Device,
+ IN UINT8 Function,
+ OUT PCI_CAP_DEV **PciDevice
+ );
+
+
+/**
+ Free the resources used by PciDevice.
+
+ @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
+ PciCapPciSegmentDeviceInit().
+**/
+VOID
+EFIAPI
+PciCapPciSegmentDeviceUninit (
+ IN PCI_CAP_DEV *PciDevice
+ );
+
+#endif // __PCI_CAP_PCI_SEGMENT_LIB_H__
diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
new file mode 100644
index 000000000000..3ce15fe0fb57
--- /dev/null
+++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
@@ -0,0 +1,47 @@
+/** @file
+ Plug a PciSegmentLib backend into PciCapLib, for config space access --
+ internal macro and type definitions.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#ifndef __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
+#define __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
+
+#include <Library/DebugLib.h>
+
+#include <Library/PciCapPciSegmentLib.h>
+
+#define SEGMENT_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'S', 'G', 'M', 'N', 'T')
+
+typedef struct {
+ //
+ // Signature identifying the derived class.
+ //
+ UINT64 Signature;
+ //
+ // Members added by the derived class, specific to the use of PciSegmentLib.
+ //
+ PCI_CAP_DOMAIN MaxDomain;
+ UINT16 SegmentNr;
+ UINT8 BusNr;
+ UINT8 DeviceNr;
+ UINT8 FunctionNr;
+ //
+ // Base class.
+ //
+ PCI_CAP_DEV BaseDevice;
+} SEGMENT_DEV;
+
+#define SEGMENT_DEV_FROM_PCI_CAP_DEV(PciDevice) \
+ CR (PciDevice, SEGMENT_DEV, BaseDevice, SEGMENT_DEV_SIG)
+
+#endif // __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
new file mode 100644
index 000000000000..57eb6b625b56
--- /dev/null
+++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
@@ -0,0 +1,226 @@
+/** @file
+ Plug a PciSegmentLib backend into PciCapLib, for config space access.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#include <IndustryStandard/Pci23.h>
+
+#include <Library/BaseLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PciSegmentLib.h>
+
+#include "BasePciCapPciSegmentLib.h"
+
+
+/**
+ Read the config space of a given PCI device (both normal and extended).
+
+ SegmentDevReadConfig() performs as few config space accesses as possible
+ (without attempting 64-bit wide accesses).
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] SourceOffset Source offset in the config space of the PCI
+ device to start reading from.
+
+ @param[out] DestinationBuffer Buffer to store the read data to.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from config
+ space to DestinationBuffer.
+
+ @retval RETURN_UNSUPPORTED Accessing Size bytes from SourceOffset exceeds
+ the config space limit of the PCI device.
+ Although PCI_CAP_DEV_READ_CONFIG allows reading
+ fewer than Size bytes in this case,
+ SegmentDevReadConfig() will read none.
+**/
+STATIC
+RETURN_STATUS
+EFIAPI
+SegmentDevReadConfig (
+ IN PCI_CAP_DEV *PciDevice,
+ IN UINT16 SourceOffset,
+ OUT VOID *DestinationBuffer,
+ IN UINT16 Size
+ )
+{
+ SEGMENT_DEV *SegmentDev;
+ UINT16 ConfigSpaceSize;
+ UINT64 SourceAddress;
+
+ SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
+ ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ?
+ PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
+ //
+ // Note that all UINT16 variables below are promoted to INT32, and the
+ // addition and the comparison is carried out in INT32.
+ //
+ if (SourceOffset + Size > ConfigSpaceSize) {
+ return RETURN_UNSUPPORTED;
+ }
+ SourceAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr,
+ SegmentDev->BusNr, SegmentDev->DeviceNr,
+ SegmentDev->FunctionNr, SourceOffset);
+ PciSegmentReadBuffer (SourceAddress, Size, DestinationBuffer);
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Write the config space of a given PCI device (both normal and extended).
+
+ SegmentDevWriteConfig() performs as few config space accesses as possible
+ (without attempting 64-bit wide accesses).
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] DestinationOffset Destination offset in the config space of the
+ PCI device to start writing at.
+
+ @param[in] SourceBuffer Buffer to read the data to be stored from.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from
+ SourceBuffer to config space.
+
+ @retval RETURN_UNSUPPORTED Accessing Size bytes at DestinationOffset exceeds
+ the config space limit of the PCI device.
+ Although PCI_CAP_DEV_WRITE_CONFIG allows writing
+ fewer than Size bytes in this case,
+ SegmentDevWriteConfig() will write none.
+**/
+STATIC
+RETURN_STATUS
+EFIAPI
+SegmentDevWriteConfig (
+ IN PCI_CAP_DEV *PciDevice,
+ IN UINT16 DestinationOffset,
+ IN VOID *SourceBuffer,
+ IN UINT16 Size
+ )
+{
+ SEGMENT_DEV *SegmentDev;
+ UINT16 ConfigSpaceSize;
+ UINT64 DestinationAddress;
+
+ SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
+ ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ?
+ PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
+ //
+ // Note that all UINT16 variables below are promoted to INT32, and the
+ // addition and the comparison is carried out in INT32.
+ //
+ if (DestinationOffset + Size > ConfigSpaceSize) {
+ return RETURN_UNSUPPORTED;
+ }
+ DestinationAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr,
+ SegmentDev->BusNr, SegmentDev->DeviceNr,
+ SegmentDev->FunctionNr, DestinationOffset);
+ PciSegmentWriteBuffer (DestinationAddress, Size, SourceBuffer);
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function
+ quadruplet. The config space accessors are based upon PciSegmentLib.
+
+ @param[in] MaxDomain If MaxDomain is PciCapExtended, then
+ PciDevice->ReadConfig() and PciDevice->WriteConfig()
+ will delegate extended config space accesses too to
+ PciSegmentReadBuffer() and PciSegmentWriteBuffer(),
+ respectively. Otherwise, PciDevice->ReadConfig() and
+ PciDevice->WriteConfig() will reject accesses to
+ extended config space with RETURN_UNSUPPORTED, without
+ calling PciSegmentReadBuffer() or
+ PciSegmentWriteBuffer(). By setting MaxDomain to
+ PciCapNormal, the platform can prevent undefined
+ PciSegmentLib behavior when the PCI root bridge under
+ the PCI device at Segment:Bus:Device.Function doesn't
+ support extended config space.
+
+ @param[in] Segment 16-bit wide segment number.
+
+ @param[in] Bus 8-bit wide bus number.
+
+ @param[in] Device 5-bit wide device number.
+
+ @param[in] Function 3-bit wide function number.
+
+ @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
+ PciDevice can be passed to the PciCapLib APIs.
+
+ @retval RETURN_SUCCESS PciDevice has been constructed and output.
+
+ @retval RETURN_INVALID_PARAMETER Device or Function does not fit in the
+ permitted number of bits.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+**/
+RETURN_STATUS
+EFIAPI
+PciCapPciSegmentDeviceInit (
+ IN PCI_CAP_DOMAIN MaxDomain,
+ IN UINT16 Segment,
+ IN UINT8 Bus,
+ IN UINT8 Device,
+ IN UINT8 Function,
+ OUT PCI_CAP_DEV **PciDevice
+ )
+{
+ SEGMENT_DEV *SegmentDev;
+
+ if (Device > PCI_MAX_DEVICE || Function > PCI_MAX_FUNC) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ SegmentDev = AllocatePool (sizeof *SegmentDev);
+ if (SegmentDev == NULL) {
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ SegmentDev->Signature = SEGMENT_DEV_SIG;
+ SegmentDev->MaxDomain = MaxDomain;
+ SegmentDev->SegmentNr = Segment;
+ SegmentDev->BusNr = Bus;
+ SegmentDev->DeviceNr = Device;
+ SegmentDev->FunctionNr = Function;
+ SegmentDev->BaseDevice.ReadConfig = SegmentDevReadConfig;
+ SegmentDev->BaseDevice.WriteConfig = SegmentDevWriteConfig;
+
+ *PciDevice = &SegmentDev->BaseDevice;
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Free the resources used by PciDevice.
+
+ @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
+ PciCapPciSegmentDeviceInit().
+**/
+VOID
+EFIAPI
+PciCapPciSegmentDeviceUninit (
+ IN PCI_CAP_DEV *PciDevice
+ )
+{
+ SEGMENT_DEV *SegmentDev;
+
+ SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
+ FreePool (SegmentDev);
+}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib
2018-05-23 20:21 ` [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib Laszlo Ersek
@ 2018-05-24 8:08 ` Ard Biesheuvel
2018-05-24 14:43 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 8:08 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Add a library class, and a BASE lib instance, that are layered on top of
> PciCapLib, and allow clients to plug a PciSegmentLib backend into
> PciCapLib, for config space access.
>
> (Side note:
>
For the record: I am fine with keeping this side note in the commit log.
> The "MaxDomain" parameter is provided because, in practice, platforms
> exist where a PCI Express device may show up on a root bridge such that
> the root bridge doesn't support access to extended config space. Earlier
> the same issue was handled for MdeModulePkg/PciHostBridgeDxe in commit
> 014b472053ae3. However, that solution does not apply to the PciSegmentLib
> class, because:
>
> (1) The config space accessor functions of the PciSegmentLib class, such
> as PciSegmentReadBuffer(), have no way of informing the caller whether
> access to extended config space actually succeeds.
>
> (For example, in the UefiPciSegmentLibPciRootBridgeIo instace, which
> could in theory benefit from commit 014b472053ae3, the
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() status code is explicitly
> ignored, because there's no way for the lib instance to propagate it
> to the PciSegmentLib caller. If the
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() call fails, then
> DxePciSegmentLibPciRootBridgeIoReadWorker() returns Data with
> indeterminate value.)
>
> (2) There is no *general* way for any firmware platform to provide, or
> use, a PciSegmentLib instance in which access to extended config space
> always succeeds.
>
> In brief, on a platform where config space may be limited to 256 bytes,
> access to extended config space through PciSegmentLib may invoke undefined
> behavior; therefore PciCapPciSegmentLib must give platforms a way to
> prevent such access.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - move library from MdePkg to OvmfPkg, for initial introduction
>
> OvmfPkg/OvmfPkg.dec | 5 +
> OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf | 35 +++
> OvmfPkg/Include/Library/PciCapPciSegmentLib.h | 82 +++++++
> OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h | 47 ++++
> OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c | 226 ++++++++++++++++++++
> 5 files changed, 395 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 74818a2e2a19..fbec1cfe4a8e 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -35,6 +35,11 @@ [LibraryClasses]
> # config space.
> PciCapLib|Include/Library/PciCapLib.h
>
> + ## @libraryclass Layered on top of PciCapLib, allows clients to plug a
> + # PciSegmentLib backend into PciCapLib, for config space
> + # access.
> + PciCapPciSegmentLib|Include/Library/PciCapPciSegmentLib.h
> +
> ## @libraryclass Access QEMU's firmware configuration interface
> #
> QemuFwCfgLib|Include/Library/QemuFwCfgLib.h
> diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> new file mode 100644
> index 000000000000..e3cf5de49b15
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> @@ -0,0 +1,35 @@
> +## @file
> +# Plug a PciSegmentLib backend into PciCapLib, for config space access.
> +#
> +# Copyright (C) 2018, Red Hat, Inc.
> +#
> +# 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 = 1.27
> + BASE_NAME = BasePciCapPciSegmentLib
> + FILE_GUID = ED011855-AA31-43B9-ACC0-BF45A05C5985
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = PciCapPciSegmentLib
> +
> +[Sources]
> + BasePciCapPciSegmentLib.h
> + BasePciCapPciSegmentLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseLib
> + DebugLib
> + MemoryAllocationLib
> + PciSegmentLib
> diff --git a/OvmfPkg/Include/Library/PciCapPciSegmentLib.h b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h
> new file mode 100644
> index 000000000000..6b6930288d16
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h
> @@ -0,0 +1,82 @@
> +/** @file
> + Library class layered on top of PciCapLib that allows clients to plug a
> + PciSegmentLib backend into PciCapLib, for config space access.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#ifndef __PCI_CAP_PCI_SEGMENT_LIB_H__
> +#define __PCI_CAP_PCI_SEGMENT_LIB_H__
> +
> +#include <Library/PciCapLib.h>
> +
> +
> +/**
> + Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function
> + quadruplet. The config space accessors are based upon PciSegmentLib.
> +
> + @param[in] MaxDomain If MaxDomain is PciCapExtended, then
> + PciDevice->ReadConfig() and PciDevice->WriteConfig()
> + will delegate extended config space accesses too to
> + PciSegmentReadBuffer() and PciSegmentWriteBuffer(),
> + respectively. Otherwise, PciDevice->ReadConfig() and
> + PciDevice->WriteConfig() will reject accesses to
> + extended config space with RETURN_UNSUPPORTED, without
> + calling PciSegmentReadBuffer() or
> + PciSegmentWriteBuffer(). By setting MaxDomain to
> + PciCapNormal, the platform can prevent undefined
> + PciSegmentLib behavior when the PCI root bridge under
> + the PCI device at Segment:Bus:Device.Function doesn't
> + support extended config space.
> +
> + @param[in] Segment 16-bit wide segment number.
> +
> + @param[in] Bus 8-bit wide bus number.
> +
> + @param[in] Device 5-bit wide device number.
> +
> + @param[in] Function 3-bit wide function number.
> +
> + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
> + PciDevice can be passed to the PciCapLib APIs.
> +
> + @retval RETURN_SUCCESS PciDevice has been constructed and output.
> +
> + @retval RETURN_INVALID_PARAMETER Device or Function does not fit in the
> + permitted number of bits.
> +
> + @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapPciSegmentDeviceInit (
> + IN PCI_CAP_DOMAIN MaxDomain,
> + IN UINT16 Segment,
> + IN UINT8 Bus,
> + IN UINT8 Device,
> + IN UINT8 Function,
> + OUT PCI_CAP_DEV **PciDevice
> + );
> +
> +
> +/**
> + Free the resources used by PciDevice.
> +
> + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
> + PciCapPciSegmentDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciSegmentDeviceUninit (
> + IN PCI_CAP_DEV *PciDevice
> + );
> +
> +#endif // __PCI_CAP_PCI_SEGMENT_LIB_H__
> diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
> new file mode 100644
> index 000000000000..3ce15fe0fb57
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h
> @@ -0,0 +1,47 @@
> +/** @file
> + Plug a PciSegmentLib backend into PciCapLib, for config space access --
> + internal macro and type definitions.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#ifndef __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
> +#define __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
> +
> +#include <Library/DebugLib.h>
> +
> +#include <Library/PciCapPciSegmentLib.h>
> +
> +#define SEGMENT_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'S', 'G', 'M', 'N', 'T')
> +
> +typedef struct {
> + //
> + // Signature identifying the derived class.
> + //
> + UINT64 Signature;
> + //
> + // Members added by the derived class, specific to the use of PciSegmentLib.
> + //
> + PCI_CAP_DOMAIN MaxDomain;
> + UINT16 SegmentNr;
> + UINT8 BusNr;
> + UINT8 DeviceNr;
> + UINT8 FunctionNr;
> + //
> + // Base class.
> + //
> + PCI_CAP_DEV BaseDevice;
> +} SEGMENT_DEV;
> +
> +#define SEGMENT_DEV_FROM_PCI_CAP_DEV(PciDevice) \
> + CR (PciDevice, SEGMENT_DEV, BaseDevice, SEGMENT_DEV_SIG)
> +
> +#endif // __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__
> diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
> new file mode 100644
> index 000000000000..57eb6b625b56
> --- /dev/null
> +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c
> @@ -0,0 +1,226 @@
> +/** @file
> + Plug a PciSegmentLib backend into PciCapLib, for config space access.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#include <IndustryStandard/Pci23.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PciSegmentLib.h>
> +
> +#include "BasePciCapPciSegmentLib.h"
> +
> +
> +/**
> + Read the config space of a given PCI device (both normal and extended).
> +
> + SegmentDevReadConfig() performs as few config space accesses as possible
> + (without attempting 64-bit wide accesses).
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] SourceOffset Source offset in the config space of the PCI
> + device to start reading from.
> +
> + @param[out] DestinationBuffer Buffer to store the read data to.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from config
> + space to DestinationBuffer.
> +
> + @retval RETURN_UNSUPPORTED Accessing Size bytes from SourceOffset exceeds
> + the config space limit of the PCI device.
> + Although PCI_CAP_DEV_READ_CONFIG allows reading
> + fewer than Size bytes in this case,
> + SegmentDevReadConfig() will read none.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +SegmentDevReadConfig (
> + IN PCI_CAP_DEV *PciDevice,
> + IN UINT16 SourceOffset,
> + OUT VOID *DestinationBuffer,
> + IN UINT16 Size
> + )
> +{
> + SEGMENT_DEV *SegmentDev;
> + UINT16 ConfigSpaceSize;
> + UINT64 SourceAddress;
> +
> + SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
> + ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ?
> + PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
> + //
> + // Note that all UINT16 variables below are promoted to INT32, and the
> + // addition and the comparison is carried out in INT32.
> + //
> + if (SourceOffset + Size > ConfigSpaceSize) {
> + return RETURN_UNSUPPORTED;
> + }
> + SourceAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr,
> + SegmentDev->BusNr, SegmentDev->DeviceNr,
> + SegmentDev->FunctionNr, SourceOffset);
> + PciSegmentReadBuffer (SourceAddress, Size, DestinationBuffer);
> + return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> + Write the config space of a given PCI device (both normal and extended).
> +
> + SegmentDevWriteConfig() performs as few config space accesses as possible
> + (without attempting 64-bit wide accesses).
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] DestinationOffset Destination offset in the config space of the
> + PCI device to start writing at.
> +
> + @param[in] SourceBuffer Buffer to read the data to be stored from.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from
> + SourceBuffer to config space.
> +
> + @retval RETURN_UNSUPPORTED Accessing Size bytes at DestinationOffset exceeds
> + the config space limit of the PCI device.
> + Although PCI_CAP_DEV_WRITE_CONFIG allows writing
> + fewer than Size bytes in this case,
> + SegmentDevWriteConfig() will write none.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +SegmentDevWriteConfig (
> + IN PCI_CAP_DEV *PciDevice,
> + IN UINT16 DestinationOffset,
> + IN VOID *SourceBuffer,
> + IN UINT16 Size
> + )
> +{
> + SEGMENT_DEV *SegmentDev;
> + UINT16 ConfigSpaceSize;
> + UINT64 DestinationAddress;
> +
> + SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
> + ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ?
> + PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET);
> + //
> + // Note that all UINT16 variables below are promoted to INT32, and the
> + // addition and the comparison is carried out in INT32.
> + //
> + if (DestinationOffset + Size > ConfigSpaceSize) {
> + return RETURN_UNSUPPORTED;
> + }
> + DestinationAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr,
> + SegmentDev->BusNr, SegmentDev->DeviceNr,
> + SegmentDev->FunctionNr, DestinationOffset);
> + PciSegmentWriteBuffer (DestinationAddress, Size, SourceBuffer);
> + return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> + Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function
> + quadruplet. The config space accessors are based upon PciSegmentLib.
> +
> + @param[in] MaxDomain If MaxDomain is PciCapExtended, then
> + PciDevice->ReadConfig() and PciDevice->WriteConfig()
> + will delegate extended config space accesses too to
> + PciSegmentReadBuffer() and PciSegmentWriteBuffer(),
> + respectively. Otherwise, PciDevice->ReadConfig() and
> + PciDevice->WriteConfig() will reject accesses to
> + extended config space with RETURN_UNSUPPORTED, without
> + calling PciSegmentReadBuffer() or
> + PciSegmentWriteBuffer(). By setting MaxDomain to
> + PciCapNormal, the platform can prevent undefined
> + PciSegmentLib behavior when the PCI root bridge under
> + the PCI device at Segment:Bus:Device.Function doesn't
> + support extended config space.
> +
> + @param[in] Segment 16-bit wide segment number.
> +
> + @param[in] Bus 8-bit wide bus number.
> +
> + @param[in] Device 5-bit wide device number.
> +
> + @param[in] Function 3-bit wide function number.
> +
> + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
> + PciDevice can be passed to the PciCapLib APIs.
> +
> + @retval RETURN_SUCCESS PciDevice has been constructed and output.
> +
> + @retval RETURN_INVALID_PARAMETER Device or Function does not fit in the
> + permitted number of bits.
> +
> + @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +PciCapPciSegmentDeviceInit (
> + IN PCI_CAP_DOMAIN MaxDomain,
> + IN UINT16 Segment,
> + IN UINT8 Bus,
> + IN UINT8 Device,
> + IN UINT8 Function,
> + OUT PCI_CAP_DEV **PciDevice
> + )
> +{
> + SEGMENT_DEV *SegmentDev;
> +
> + if (Device > PCI_MAX_DEVICE || Function > PCI_MAX_FUNC) {
> + return RETURN_INVALID_PARAMETER;
> + }
> +
> + SegmentDev = AllocatePool (sizeof *SegmentDev);
> + if (SegmentDev == NULL) {
> + return RETURN_OUT_OF_RESOURCES;
> + }
> +
> + SegmentDev->Signature = SEGMENT_DEV_SIG;
> + SegmentDev->MaxDomain = MaxDomain;
> + SegmentDev->SegmentNr = Segment;
> + SegmentDev->BusNr = Bus;
> + SegmentDev->DeviceNr = Device;
> + SegmentDev->FunctionNr = Function;
> + SegmentDev->BaseDevice.ReadConfig = SegmentDevReadConfig;
> + SegmentDev->BaseDevice.WriteConfig = SegmentDevWriteConfig;
> +
> + *PciDevice = &SegmentDev->BaseDevice;
> + return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> + Free the resources used by PciDevice.
> +
> + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
> + PciCapPciSegmentDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciSegmentDeviceUninit (
> + IN PCI_CAP_DEV *PciDevice
> + )
> +{
> + SEGMENT_DEV *SegmentDev;
> +
> + SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice);
> + FreePool (SegmentDev);
> +}
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib
2018-05-24 8:08 ` Ard Biesheuvel
@ 2018-05-24 14:43 ` Laszlo Ersek
2018-05-24 14:55 ` Ard Biesheuvel
0 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-24 14:43 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen
On 05/24/18 10:08, Ard Biesheuvel wrote:
> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>> Add a library class, and a BASE lib instance, that are layered on top of
>> PciCapLib, and allow clients to plug a PciSegmentLib backend into
>> PciCapLib, for config space access.
>>
>> (Side note:
>>
>
> For the record: I am fine with keeping this side note in the commit log.
Appreciated! :)
(I've found no other comments from you on this patch, so I guess you'll
state something general towards the end.)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib
2018-05-24 14:43 ` Laszlo Ersek
@ 2018-05-24 14:55 ` Ard Biesheuvel
0 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 14:55 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 24 May 2018 at 16:43, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/24/18 10:08, Ard Biesheuvel wrote:
>> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Add a library class, and a BASE lib instance, that are layered on top of
>>> PciCapLib, and allow clients to plug a PciSegmentLib backend into
>>> PciCapLib, for config space access.
>>>
>>> (Side note:
>>>
>>
>> For the record: I am fine with keeping this side note in the commit log.
>
> Appreciated! :)
>
> (I've found no other comments from you on this patch, so I guess you'll
> state something general towards the end.)
>
Ah yes
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
2018-05-23 20:21 ` [PATCH v2 1/7] OvmfPkg: introduce PciCapLib Laszlo Ersek
2018-05-23 20:21 ` [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib Laszlo Ersek
@ 2018-05-23 20:21 ` Laszlo Ersek
2018-05-24 8:13 ` Ard Biesheuvel
2018-05-23 20:21 ` [PATCH v2 4/7] OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib Laszlo Ersek
` (4 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-23 20:21 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
Add a library class, and a UEFI_DRIVER lib instance, that are layered on
top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
into PciCapLib, for config space access.
(Side note:
Although the UEFI spec says that EFI_PCI_IO_PROTOCOL_CONFIG() returns
EFI_UNSUPPORTED if "[t]he address range specified by Offset, Width, and
Count is not valid for the PCI configuration header of the PCI
controller", this patch doesn't directly document the EFI_UNSUPPORTED
error code, for ProtoDevTransferConfig() and its callers
ProtoDevReadConfig() and ProtoDevWriteConfig(). Instead, the patch refers
to "unspecified error codes". The reason is that in edk2, the
PciIoConfigRead() and PciIoConfigWrite() functions [1] can also return
EFI_INVALID_PARAMETER for the above situation.
Namely, PciIoConfigRead() and PciIoConfigWrite() first call
PciIoVerifyConfigAccess(), which indeed produces the standard
EFI_UNSUPPORTED error code, if the device's config space is exceeded.
However, if PciIoVerifyConfigAccess() passes, and we reach
RootBridgeIoPciRead() and RootBridgeIoPciWrite() [2], then
RootBridgeIoCheckParameter() can still fail, e.g. if the root bridge
doesn't support extended config space (see commit 014b472053ae3).
For all kinds of Limit violations in IO, MMIO, and config space,
RootBridgeIoCheckParameter() returns EFI_INVALID_PARAMETER, not
EFI_UNSUPPORTED. That error code is then propagated up to, and out of,
PciIoConfigRead() and PciIoConfigWrite().
[1] MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
[2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
)
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- move library from MdePkg to OvmfPkg, for initial introduction
OvmfPkg/OvmfPkg.dec | 5 +
OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf | 36 +++
OvmfPkg/Include/Library/PciCapPciIoLib.h | 58 +++++
OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h | 44 ++++
OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c | 243 ++++++++++++++++++++
5 files changed, 386 insertions(+)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index fbec1cfe4a8e..dc5597db4136 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -35,6 +35,11 @@ [LibraryClasses]
# config space.
PciCapLib|Include/Library/PciCapLib.h
+ ## @libraryclass Layered on top of PciCapLib, allows clients to plug an
+ # EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config
+ # space access.
+ PciCapPciIoLib|Include/Library/PciCapPciIoLib.h
+
## @libraryclass Layered on top of PciCapLib, allows clients to plug a
# PciSegmentLib backend into PciCapLib, for config space
# access.
diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
new file mode 100644
index 000000000000..2e14acb0ab75
--- /dev/null
+++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
@@ -0,0 +1,36 @@
+## @file
+# Plug an EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access.
+#
+# Copyright (C) 2018, Red Hat, Inc.
+#
+# 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 = 1.27
+ BASE_NAME = UefiPciCapPciIoLib
+ FILE_GUID = 4102F4FE-DA10-4F0F-AC18-4982ED506154
+ MODULE_TYPE = UEFI_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = PciCapPciIoLib
+
+[Sources]
+ UefiPciCapPciIoLib.h
+ UefiPciCapPciIoLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ DebugLib
+ MemoryAllocationLib
+
+[Protocols]
+ gEfiPciIoProtocolGuid ## CONSUMES
diff --git a/OvmfPkg/Include/Library/PciCapPciIoLib.h b/OvmfPkg/Include/Library/PciCapPciIoLib.h
new file mode 100644
index 000000000000..553715fd5080
--- /dev/null
+++ b/OvmfPkg/Include/Library/PciCapPciIoLib.h
@@ -0,0 +1,58 @@
+/** @file
+ Library class layered on top of PciCapLib that allows clients to plug an
+ EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#ifndef __PCI_CAP_PCI_IO_LIB_H__
+#define __PCI_CAP_PCI_IO_LIB_H__
+
+#include <Protocol/PciIo.h>
+
+#include <Library/PciCapLib.h>
+
+
+/**
+ Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config
+ space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and
+ EFI_PCI_IO_PROTOCOL.Pci.Write().
+
+ @param[in] PciIo EFI_PCI_IO_PROTOCOL representation of the PCI device.
+
+ @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
+ PciDevice can be passed to the PciCapLib APIs.
+
+ @retval EFI_SUCCESS PciDevice has been constructed and output.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+**/
+EFI_STATUS
+EFIAPI
+PciCapPciIoDeviceInit (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ OUT PCI_CAP_DEV **PciDevice
+ );
+
+
+/**
+ Free the resources used by PciDevice.
+
+ @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
+ PciCapPciIoDeviceInit().
+**/
+VOID
+EFIAPI
+PciCapPciIoDeviceUninit (
+ IN PCI_CAP_DEV *PciDevice
+ );
+
+#endif // __PCI_CAP_PCI_IO_LIB_H__
diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
new file mode 100644
index 000000000000..a94f65e5a4e3
--- /dev/null
+++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
@@ -0,0 +1,44 @@
+/** @file
+ Plug an EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access
+ -- internal macro and type definitions.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#ifndef __UEFI_PCI_CAP_PCI_IO_LIB_H__
+#define __UEFI_PCI_CAP_PCI_IO_LIB_H__
+
+#include <Library/DebugLib.h>
+
+#include <Library/PciCapPciIoLib.h>
+
+#define PROTO_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'I', 'O', 'P', 'R', 'T')
+
+typedef struct {
+ //
+ // Signature identifying the derived class.
+ //
+ UINT64 Signature;
+ //
+ // Members added by the derived class, specific to the use of
+ // EFI_PCI_IO_PROTOCOL.
+ //
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ //
+ // Base class.
+ //
+ PCI_CAP_DEV BaseDevice;
+} PROTO_DEV;
+
+#define PROTO_DEV_FROM_PCI_CAP_DEV(PciDevice) \
+ CR (PciDevice, PROTO_DEV, BaseDevice, PROTO_DEV_SIG)
+
+#endif // __UEFI_PCI_CAP_PCI_IO_LIB_H__
diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
new file mode 100644
index 000000000000..84369e4dc3a8
--- /dev/null
+++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
@@ -0,0 +1,243 @@
+/** @file
+ Plug an EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access.
+
+ Copyright (C) 2018, Red Hat, Inc.
+
+ 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.
+**/
+
+#include <Library/MemoryAllocationLib.h>
+
+#include "UefiPciCapPciIoLib.h"
+
+
+/**
+ Transfer bytes between the config space of a given PCI device and a memory
+ buffer.
+
+ ProtoDevTransferConfig() performs as few config space accesses as possible
+ (without attempting 64-bit wide accesses).
+
+ @param[in] PciIo The EFI_PCI_IO_PROTOCOL representation of the
+ PCI device.
+
+ @param[in] TransferFunction The EFI_PCI_IO_PROTOCOL_CONFIG function that
+ implements the transfer. The direction of the
+ transfer is inherent to TransferFunction.
+ TransferFunction() is required to return an
+ unspecified error if any sub-transfer within
+ Size bytes from ConfigOffset exceeds the config
+ space limit of the PCI device.
+
+ @param[in] ConfigOffset The offset in the config space of the PCI device
+ at which the transfer should commence.
+
+ @param[in,out] Buffer The memory buffer where the transfer should
+ occur.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval EFI_SUCCESS Size bytes have been transferred between config space
+ and Buffer.
+
+ @return Error codes propagated from TransferFunction(). Fewer
+ than Size bytes may have been transferred.
+**/
+STATIC
+EFI_STATUS
+ProtoDevTransferConfig (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
+ IN UINT16 ConfigOffset,
+ IN OUT UINT8 *Buffer,
+ IN UINT16 Size
+ )
+{
+ while (Size > 0) {
+ EFI_PCI_IO_PROTOCOL_WIDTH Width;
+ UINT16 Count;
+ EFI_STATUS Status;
+ UINT16 Progress;
+
+ //
+ // Pick the largest access size that is allowed by the remaining transfer
+ // Size and by the alignment of ConfigOffset.
+ //
+ // When the largest access size is available, transfer as many bytes as
+ // possible in one iteration of the loop. Otherwise, transfer only one
+ // unit, to improve the alignment.
+ //
+ if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
+ Width = EfiPciIoWidthUint32;
+ Count = Size >> Width;
+ } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
+ Width = EfiPciIoWidthUint16;
+ Count = 1;
+ } else {
+ Width = EfiPciIoWidthUint8;
+ Count = 1;
+ }
+ Status = TransferFunction (PciIo, Width, ConfigOffset, Count, Buffer);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Progress = Count << Width;
+ ConfigOffset += Progress;
+ Buffer += Progress;
+ Size -= Progress;
+ }
+ return EFI_SUCCESS;
+}
+
+
+/**
+ Read the config space of a given PCI device (both normal and extended).
+
+ ProtoDevReadConfig() performs as few config space accesses as possible
+ (without attempting 64-bit wide accesses).
+
+ ProtoDevReadConfig() returns an unspecified error if accessing Size bytes
+ from SourceOffset exceeds the config space limit of the PCI device. Fewer
+ than Size bytes may have been read in this case.
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] SourceOffset Source offset in the config space of the PCI
+ device to start reading from.
+
+ @param[out] DestinationBuffer Buffer to store the read data to.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from config space to
+ DestinationBuffer.
+
+ @return Error codes propagated from
+ EFI_PCI_IO_PROTOCOL.Pci.Read(). Fewer than Size bytes
+ may have been read.
+**/
+STATIC
+RETURN_STATUS
+EFIAPI
+ProtoDevReadConfig (
+ IN PCI_CAP_DEV *PciDevice,
+ IN UINT16 SourceOffset,
+ OUT VOID *DestinationBuffer,
+ IN UINT16 Size
+ )
+{
+ PROTO_DEV *ProtoDev;
+
+ ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
+ return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Read,
+ SourceOffset, DestinationBuffer, Size);
+}
+
+
+/**
+ Write the config space of a given PCI device (both normal and extended).
+
+ ProtoDevWriteConfig() performs as few config space accesses as possible
+ (without attempting 64-bit wide accesses).
+
+ ProtoDevWriteConfig() returns an unspecified error if accessing Size bytes at
+ DestinationOffset exceeds the config space limit of the PCI device. Fewer
+ than Size bytes may have been written in this case.
+
+ @param[in] PciDevice Implementation-specific unique representation
+ of the PCI device in the PCI hierarchy.
+
+ @param[in] DestinationOffset Destination offset in the config space of the
+ PCI device to start writing at.
+
+ @param[in] SourceBuffer Buffer to read the data to be stored from.
+
+ @param[in] Size The number of bytes to transfer.
+
+ @retval RETURN_SUCCESS Size bytes have been transferred from SourceBuffer to
+ config space.
+
+ @return Error codes propagated from
+ EFI_PCI_IO_PROTOCOL.Pci.Write(). Fewer than Size
+ bytes may have been written.
+**/
+STATIC
+RETURN_STATUS
+EFIAPI
+ProtoDevWriteConfig (
+ IN PCI_CAP_DEV *PciDevice,
+ IN UINT16 DestinationOffset,
+ IN VOID *SourceBuffer,
+ IN UINT16 Size
+ )
+{
+ PROTO_DEV *ProtoDev;
+
+ ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
+ return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Write,
+ DestinationOffset, SourceBuffer, Size);
+}
+
+
+/**
+ Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config
+ space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and
+ EFI_PCI_IO_PROTOCOL.Pci.Write().
+
+ @param[in] PciIo EFI_PCI_IO_PROTOCOL representation of the PCI device.
+
+ @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
+ PciDevice can be passed to the PciCapLib APIs.
+
+ @retval EFI_SUCCESS PciDevice has been constructed and output.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+**/
+EFI_STATUS
+EFIAPI
+PciCapPciIoDeviceInit (
+ IN EFI_PCI_IO_PROTOCOL *PciIo,
+ OUT PCI_CAP_DEV **PciDevice
+ )
+{
+ PROTO_DEV *ProtoDev;
+
+ ProtoDev = AllocatePool (sizeof *ProtoDev);
+ if (ProtoDev == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ ProtoDev->Signature = PROTO_DEV_SIG;
+ ProtoDev->PciIo = PciIo;
+ ProtoDev->BaseDevice.ReadConfig = ProtoDevReadConfig;
+ ProtoDev->BaseDevice.WriteConfig = ProtoDevWriteConfig;
+
+ *PciDevice = &ProtoDev->BaseDevice;
+ return EFI_SUCCESS;
+}
+
+
+/**
+ Free the resources used by PciDevice.
+
+ @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
+ PciCapPciIoDeviceInit().
+**/
+VOID
+EFIAPI
+PciCapPciIoDeviceUninit (
+ IN PCI_CAP_DEV *PciDevice
+ )
+{
+ PROTO_DEV *ProtoDev;
+
+ ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
+ FreePool (ProtoDev);
+}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib
2018-05-23 20:21 ` [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib Laszlo Ersek
@ 2018-05-24 8:13 ` Ard Biesheuvel
2018-05-24 14:50 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 8:13 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Add a library class, and a UEFI_DRIVER lib instance, that are layered on
> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
> into PciCapLib, for config space access.
>
> (Side note:
>
Again, please retain the below.
> Although the UEFI spec says that EFI_PCI_IO_PROTOCOL_CONFIG() returns
> EFI_UNSUPPORTED if "[t]he address range specified by Offset, Width, and
> Count is not valid for the PCI configuration header of the PCI
> controller", this patch doesn't directly document the EFI_UNSUPPORTED
> error code, for ProtoDevTransferConfig() and its callers
> ProtoDevReadConfig() and ProtoDevWriteConfig(). Instead, the patch refers
> to "unspecified error codes". The reason is that in edk2, the
> PciIoConfigRead() and PciIoConfigWrite() functions [1] can also return
> EFI_INVALID_PARAMETER for the above situation.
>
> Namely, PciIoConfigRead() and PciIoConfigWrite() first call
> PciIoVerifyConfigAccess(), which indeed produces the standard
> EFI_UNSUPPORTED error code, if the device's config space is exceeded.
> However, if PciIoVerifyConfigAccess() passes, and we reach
> RootBridgeIoPciRead() and RootBridgeIoPciWrite() [2], then
> RootBridgeIoCheckParameter() can still fail, e.g. if the root bridge
> doesn't support extended config space (see commit 014b472053ae3).
>
> For all kinds of Limit violations in IO, MMIO, and config space,
> RootBridgeIoCheckParameter() returns EFI_INVALID_PARAMETER, not
> EFI_UNSUPPORTED. That error code is then propagated up to, and out of,
> PciIoConfigRead() and PciIoConfigWrite().
>
> [1] MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> [2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> )
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - move library from MdePkg to OvmfPkg, for initial introduction
>
> OvmfPkg/OvmfPkg.dec | 5 +
> OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf | 36 +++
> OvmfPkg/Include/Library/PciCapPciIoLib.h | 58 +++++
> OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h | 44 ++++
> OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c | 243 ++++++++++++++++++++
> 5 files changed, 386 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index fbec1cfe4a8e..dc5597db4136 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -35,6 +35,11 @@ [LibraryClasses]
> # config space.
> PciCapLib|Include/Library/PciCapLib.h
>
> + ## @libraryclass Layered on top of PciCapLib, allows clients to plug an
> + # EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config
> + # space access.
> + PciCapPciIoLib|Include/Library/PciCapPciIoLib.h
> +
> ## @libraryclass Layered on top of PciCapLib, allows clients to plug a
> # PciSegmentLib backend into PciCapLib, for config space
> # access.
> diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> new file mode 100644
> index 000000000000..2e14acb0ab75
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> @@ -0,0 +1,36 @@
> +## @file
> +# Plug an EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access.
> +#
> +# Copyright (C) 2018, Red Hat, Inc.
> +#
> +# 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 = 1.27
> + BASE_NAME = UefiPciCapPciIoLib
> + FILE_GUID = 4102F4FE-DA10-4F0F-AC18-4982ED506154
> + MODULE_TYPE = UEFI_DRIVER
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = PciCapPciIoLib
> +
> +[Sources]
> + UefiPciCapPciIoLib.h
> + UefiPciCapPciIoLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + DebugLib
> + MemoryAllocationLib
> +
> +[Protocols]
> + gEfiPciIoProtocolGuid ## CONSUMES
> diff --git a/OvmfPkg/Include/Library/PciCapPciIoLib.h b/OvmfPkg/Include/Library/PciCapPciIoLib.h
> new file mode 100644
> index 000000000000..553715fd5080
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/PciCapPciIoLib.h
> @@ -0,0 +1,58 @@
> +/** @file
> + Library class layered on top of PciCapLib that allows clients to plug an
> + EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#ifndef __PCI_CAP_PCI_IO_LIB_H__
> +#define __PCI_CAP_PCI_IO_LIB_H__
> +
> +#include <Protocol/PciIo.h>
> +
> +#include <Library/PciCapLib.h>
> +
> +
> +/**
> + Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config
> + space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and
> + EFI_PCI_IO_PROTOCOL.Pci.Write().
> +
> + @param[in] PciIo EFI_PCI_IO_PROTOCOL representation of the PCI device.
> +
> + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
> + PciDevice can be passed to the PciCapLib APIs.
> +
> + @retval EFI_SUCCESS PciDevice has been constructed and output.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +**/
> +EFI_STATUS
> +EFIAPI
> +PciCapPciIoDeviceInit (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + OUT PCI_CAP_DEV **PciDevice
> + );
> +
> +
> +/**
> + Free the resources used by PciDevice.
> +
> + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
> + PciCapPciIoDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciIoDeviceUninit (
> + IN PCI_CAP_DEV *PciDevice
> + );
> +
> +#endif // __PCI_CAP_PCI_IO_LIB_H__
> diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
> new file mode 100644
> index 000000000000..a94f65e5a4e3
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h
> @@ -0,0 +1,44 @@
> +/** @file
> + Plug an EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access
> + -- internal macro and type definitions.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#ifndef __UEFI_PCI_CAP_PCI_IO_LIB_H__
> +#define __UEFI_PCI_CAP_PCI_IO_LIB_H__
> +
> +#include <Library/DebugLib.h>
> +
> +#include <Library/PciCapPciIoLib.h>
> +
> +#define PROTO_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'I', 'O', 'P', 'R', 'T')
> +
> +typedef struct {
> + //
> + // Signature identifying the derived class.
> + //
> + UINT64 Signature;
> + //
> + // Members added by the derived class, specific to the use of
> + // EFI_PCI_IO_PROTOCOL.
> + //
> + EFI_PCI_IO_PROTOCOL *PciIo;
> + //
> + // Base class.
> + //
> + PCI_CAP_DEV BaseDevice;
> +} PROTO_DEV;
> +
> +#define PROTO_DEV_FROM_PCI_CAP_DEV(PciDevice) \
> + CR (PciDevice, PROTO_DEV, BaseDevice, PROTO_DEV_SIG)
> +
> +#endif // __UEFI_PCI_CAP_PCI_IO_LIB_H__
> diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> new file mode 100644
> index 000000000000..84369e4dc3a8
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> @@ -0,0 +1,243 @@
> +/** @file
> + Plug an EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config space access.
> +
> + Copyright (C) 2018, Red Hat, Inc.
> +
> + 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.
> +**/
> +
> +#include <Library/MemoryAllocationLib.h>
> +
> +#include "UefiPciCapPciIoLib.h"
> +
> +
> +/**
> + Transfer bytes between the config space of a given PCI device and a memory
> + buffer.
> +
> + ProtoDevTransferConfig() performs as few config space accesses as possible
> + (without attempting 64-bit wide accesses).
> +
> + @param[in] PciIo The EFI_PCI_IO_PROTOCOL representation of the
> + PCI device.
> +
> + @param[in] TransferFunction The EFI_PCI_IO_PROTOCOL_CONFIG function that
> + implements the transfer. The direction of the
> + transfer is inherent to TransferFunction.
> + TransferFunction() is required to return an
> + unspecified error if any sub-transfer within
> + Size bytes from ConfigOffset exceeds the config
> + space limit of the PCI device.
> +
> + @param[in] ConfigOffset The offset in the config space of the PCI device
> + at which the transfer should commence.
> +
> + @param[in,out] Buffer The memory buffer where the transfer should
> + occur.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval EFI_SUCCESS Size bytes have been transferred between config space
> + and Buffer.
> +
> + @return Error codes propagated from TransferFunction(). Fewer
> + than Size bytes may have been transferred.
> +**/
> +STATIC
> +EFI_STATUS
> +ProtoDevTransferConfig (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
> + IN UINT16 ConfigOffset,
> + IN OUT UINT8 *Buffer,
> + IN UINT16 Size
> + )
> +{
> + while (Size > 0) {
> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
> + UINT16 Count;
> + EFI_STATUS Status;
> + UINT16 Progress;
> +
> + //
> + // Pick the largest access size that is allowed by the remaining transfer
> + // Size and by the alignment of ConfigOffset.
> + //
> + // When the largest access size is available, transfer as many bytes as
> + // possible in one iteration of the loop. Otherwise, transfer only one
> + // unit, to improve the alignment.
> + //
> + if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
Ugh. Just use '4' or sizeof(UINT32).
> + Width = EfiPciIoWidthUint32;
> + Count = Size >> Width;
> + } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
> + Width = EfiPciIoWidthUint16;
> + Count = 1;
> + } else {
> + Width = EfiPciIoWidthUint8;
> + Count = 1;
> + }
> + Status = TransferFunction (PciIo, Width, ConfigOffset, Count, Buffer);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Progress = Count << Width;
> + ConfigOffset += Progress;
> + Buffer += Progress;
> + Size -= Progress;
> + }
> + return EFI_SUCCESS;
> +}
> +
> +
> +/**
> + Read the config space of a given PCI device (both normal and extended).
> +
> + ProtoDevReadConfig() performs as few config space accesses as possible
> + (without attempting 64-bit wide accesses).
> +
> + ProtoDevReadConfig() returns an unspecified error if accessing Size bytes
> + from SourceOffset exceeds the config space limit of the PCI device. Fewer
> + than Size bytes may have been read in this case.
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] SourceOffset Source offset in the config space of the PCI
> + device to start reading from.
> +
> + @param[out] DestinationBuffer Buffer to store the read data to.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from config space to
> + DestinationBuffer.
> +
> + @return Error codes propagated from
> + EFI_PCI_IO_PROTOCOL.Pci.Read(). Fewer than Size bytes
> + may have been read.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +ProtoDevReadConfig (
> + IN PCI_CAP_DEV *PciDevice,
> + IN UINT16 SourceOffset,
> + OUT VOID *DestinationBuffer,
> + IN UINT16 Size
> + )
> +{
> + PROTO_DEV *ProtoDev;
> +
> + ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
> + return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Read,
> + SourceOffset, DestinationBuffer, Size);
> +}
> +
> +
> +/**
> + Write the config space of a given PCI device (both normal and extended).
> +
> + ProtoDevWriteConfig() performs as few config space accesses as possible
> + (without attempting 64-bit wide accesses).
> +
> + ProtoDevWriteConfig() returns an unspecified error if accessing Size bytes at
> + DestinationOffset exceeds the config space limit of the PCI device. Fewer
> + than Size bytes may have been written in this case.
> +
> + @param[in] PciDevice Implementation-specific unique representation
> + of the PCI device in the PCI hierarchy.
> +
> + @param[in] DestinationOffset Destination offset in the config space of the
> + PCI device to start writing at.
> +
> + @param[in] SourceBuffer Buffer to read the data to be stored from.
> +
> + @param[in] Size The number of bytes to transfer.
> +
> + @retval RETURN_SUCCESS Size bytes have been transferred from SourceBuffer to
> + config space.
> +
> + @return Error codes propagated from
> + EFI_PCI_IO_PROTOCOL.Pci.Write(). Fewer than Size
> + bytes may have been written.
> +**/
> +STATIC
> +RETURN_STATUS
> +EFIAPI
> +ProtoDevWriteConfig (
> + IN PCI_CAP_DEV *PciDevice,
> + IN UINT16 DestinationOffset,
> + IN VOID *SourceBuffer,
> + IN UINT16 Size
> + )
> +{
> + PROTO_DEV *ProtoDev;
> +
> + ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
> + return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Write,
> + DestinationOffset, SourceBuffer, Size);
> +}
> +
> +
> +/**
> + Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config
> + space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and
> + EFI_PCI_IO_PROTOCOL.Pci.Write().
> +
> + @param[in] PciIo EFI_PCI_IO_PROTOCOL representation of the PCI device.
> +
> + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above.
> + PciDevice can be passed to the PciCapLib APIs.
> +
> + @retval EFI_SUCCESS PciDevice has been constructed and output.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +**/
> +EFI_STATUS
> +EFIAPI
> +PciCapPciIoDeviceInit (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + OUT PCI_CAP_DEV **PciDevice
> + )
> +{
> + PROTO_DEV *ProtoDev;
> +
> + ProtoDev = AllocatePool (sizeof *ProtoDev);
> + if (ProtoDev == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + ProtoDev->Signature = PROTO_DEV_SIG;
> + ProtoDev->PciIo = PciIo;
> + ProtoDev->BaseDevice.ReadConfig = ProtoDevReadConfig;
> + ProtoDev->BaseDevice.WriteConfig = ProtoDevWriteConfig;
> +
> + *PciDevice = &ProtoDev->BaseDevice;
> + return EFI_SUCCESS;
> +}
> +
> +
> +/**
> + Free the resources used by PciDevice.
> +
> + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by
> + PciCapPciIoDeviceInit().
> +**/
> +VOID
> +EFIAPI
> +PciCapPciIoDeviceUninit (
> + IN PCI_CAP_DEV *PciDevice
> + )
> +{
> + PROTO_DEV *ProtoDev;
> +
> + ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice);
> + FreePool (ProtoDev);
> +}
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib
2018-05-24 8:13 ` Ard Biesheuvel
@ 2018-05-24 14:50 ` Laszlo Ersek
2018-05-24 14:54 ` Ard Biesheuvel
0 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-24 14:50 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen
On 05/24/18 10:13, Ard Biesheuvel wrote:
> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>> Add a library class, and a UEFI_DRIVER lib instance, that are layered on
>> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
>> into PciCapLib, for config space access.
>>
>> (Side note:
>>
>
> Again, please retain the below.
Will do.
>> +STATIC
>> +EFI_STATUS
>> +ProtoDevTransferConfig (
>> + IN EFI_PCI_IO_PROTOCOL *PciIo,
>> + IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
>> + IN UINT16 ConfigOffset,
>> + IN OUT UINT8 *Buffer,
>> + IN UINT16 Size
>> + )
>> +{
>> + while (Size > 0) {
>> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
>> + UINT16 Count;
>> + EFI_STATUS Status;
>> + UINT16 Progress;
>> +
>> + //
>> + // Pick the largest access size that is allowed by the remaining transfer
>> + // Size and by the alignment of ConfigOffset.
>> + //
>> + // When the largest access size is available, transfer as many bytes as
>> + // possible in one iteration of the loop. Otherwise, transfer only one
>> + // unit, to improve the alignment.
>> + //
>> + if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
>
> Ugh. Just use '4' or sizeof(UINT32).
>
>> + Width = EfiPciIoWidthUint32;
>> + Count = Size >> Width;
>> + } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
>> + Width = EfiPciIoWidthUint16;
>> + Count = 1;
>> + } else {
>> + Width = EfiPciIoWidthUint8;
>> + Count = 1;
>> + }
I used "BITx" and "(BITx - 1)" for consistency, and because they seemed
to convey the idea well (namely, shifting down the alignment).
I'm fine replacing "BIT2" with "4", but then I believe I should also
replace "(BIT2 - 1)" with "3". Similarly, replace "BIT1" with "2", and
"(BIT1 -1)" with 1.
Do you prefer the current code or the decimal constants?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib
2018-05-24 14:50 ` Laszlo Ersek
@ 2018-05-24 14:54 ` Ard Biesheuvel
2018-05-24 14:54 ` Ard Biesheuvel
2018-05-24 17:22 ` Laszlo Ersek
0 siblings, 2 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 14:54 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 24 May 2018 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/24/18 10:13, Ard Biesheuvel wrote:
>> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Add a library class, and a UEFI_DRIVER lib instance, that are layered on
>>> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
>>> into PciCapLib, for config space access.
>>>
>>> (Side note:
>>>
>>
>> Again, please retain the below.
>
> Will do.
>
>>> +STATIC
>>> +EFI_STATUS
>>> +ProtoDevTransferConfig (
>>> + IN EFI_PCI_IO_PROTOCOL *PciIo,
>>> + IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
>>> + IN UINT16 ConfigOffset,
>>> + IN OUT UINT8 *Buffer,
>>> + IN UINT16 Size
>>> + )
>>> +{
>>> + while (Size > 0) {
>>> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
>>> + UINT16 Count;
>>> + EFI_STATUS Status;
>>> + UINT16 Progress;
>>> +
>>> + //
>>> + // Pick the largest access size that is allowed by the remaining transfer
>>> + // Size and by the alignment of ConfigOffset.
>>> + //
>>> + // When the largest access size is available, transfer as many bytes as
>>> + // possible in one iteration of the loop. Otherwise, transfer only one
>>> + // unit, to improve the alignment.
>>> + //
>>> + if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
>>
>> Ugh. Just use '4' or sizeof(UINT32).
>>
>>> + Width = EfiPciIoWidthUint32;
>>> + Count = Size >> Width;
>>> + } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
>>> + Width = EfiPciIoWidthUint16;
>>> + Count = 1;
>>> + } else {
>>> + Width = EfiPciIoWidthUint8;
>>> + Count = 1;
>>> + }
>
> I used "BITx" and "(BITx - 1)" for consistency, and because they seemed
> to convey the idea well (namely, shifting down the alignment).
>
> I'm fine replacing "BIT2" with "4", but then I believe I should also
> replace "(BIT2 - 1)" with "3". Similarly, replace "BIT1" with "2", and
> "(BIT1 -1)" with 1.
>
> Do you prefer the current code or the decimal constants?
>
IMHO, BITx is for bitmasks, not for numerical constants.
So yes, if you think (as do I) that sizeof(UINTnn) is too wordy, just
use the plain numbers please.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib
2018-05-24 14:54 ` Ard Biesheuvel
@ 2018-05-24 14:54 ` Ard Biesheuvel
2018-05-24 17:22 ` Laszlo Ersek
1 sibling, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 14:54 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 24 May 2018 at 16:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 24 May 2018 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 05/24/18 10:13, Ard Biesheuvel wrote:
>>> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> Add a library class, and a UEFI_DRIVER lib instance, that are layered on
>>>> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
>>>> into PciCapLib, for config space access.
>>>>
>>>> (Side note:
>>>>
>>>
>>> Again, please retain the below.
>>
>> Will do.
>>
>>>> +STATIC
>>>> +EFI_STATUS
>>>> +ProtoDevTransferConfig (
>>>> + IN EFI_PCI_IO_PROTOCOL *PciIo,
>>>> + IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
>>>> + IN UINT16 ConfigOffset,
>>>> + IN OUT UINT8 *Buffer,
>>>> + IN UINT16 Size
>>>> + )
>>>> +{
>>>> + while (Size > 0) {
>>>> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
>>>> + UINT16 Count;
>>>> + EFI_STATUS Status;
>>>> + UINT16 Progress;
>>>> +
>>>> + //
>>>> + // Pick the largest access size that is allowed by the remaining transfer
>>>> + // Size and by the alignment of ConfigOffset.
>>>> + //
>>>> + // When the largest access size is available, transfer as many bytes as
>>>> + // possible in one iteration of the loop. Otherwise, transfer only one
>>>> + // unit, to improve the alignment.
>>>> + //
>>>> + if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
>>>
>>> Ugh. Just use '4' or sizeof(UINT32).
>>>
>>>> + Width = EfiPciIoWidthUint32;
>>>> + Count = Size >> Width;
>>>> + } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
>>>> + Width = EfiPciIoWidthUint16;
>>>> + Count = 1;
>>>> + } else {
>>>> + Width = EfiPciIoWidthUint8;
>>>> + Count = 1;
>>>> + }
>>
>> I used "BITx" and "(BITx - 1)" for consistency, and because they seemed
>> to convey the idea well (namely, shifting down the alignment).
>>
>> I'm fine replacing "BIT2" with "4", but then I believe I should also
>> replace "(BIT2 - 1)" with "3". Similarly, replace "BIT1" with "2", and
>> "(BIT1 -1)" with 1.
>>
>> Do you prefer the current code or the decimal constants?
>>
>
> IMHO, BITx is for bitmasks, not for numerical constants.
>
> So yes, if you think (as do I) that sizeof(UINTnn) is too wordy, just
> use the plain numbers please.
With that,
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib
2018-05-24 14:54 ` Ard Biesheuvel
2018-05-24 14:54 ` Ard Biesheuvel
@ 2018-05-24 17:22 ` Laszlo Ersek
1 sibling, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-24 17:22 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen
On 05/24/18 16:54, Ard Biesheuvel wrote:
> On 24 May 2018 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 05/24/18 10:13, Ard Biesheuvel wrote:
>>> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> Add a library class, and a UEFI_DRIVER lib instance, that are layered on
>>>> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
>>>> into PciCapLib, for config space access.
>>>>
>>>> (Side note:
>>>>
>>>
>>> Again, please retain the below.
>>
>> Will do.
>>
>>>> +STATIC
>>>> +EFI_STATUS
>>>> +ProtoDevTransferConfig (
>>>> + IN EFI_PCI_IO_PROTOCOL *PciIo,
>>>> + IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
>>>> + IN UINT16 ConfigOffset,
>>>> + IN OUT UINT8 *Buffer,
>>>> + IN UINT16 Size
>>>> + )
>>>> +{
>>>> + while (Size > 0) {
>>>> + EFI_PCI_IO_PROTOCOL_WIDTH Width;
>>>> + UINT16 Count;
>>>> + EFI_STATUS Status;
>>>> + UINT16 Progress;
>>>> +
>>>> + //
>>>> + // Pick the largest access size that is allowed by the remaining transfer
>>>> + // Size and by the alignment of ConfigOffset.
>>>> + //
>>>> + // When the largest access size is available, transfer as many bytes as
>>>> + // possible in one iteration of the loop. Otherwise, transfer only one
>>>> + // unit, to improve the alignment.
>>>> + //
>>>> + if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
>>>
>>> Ugh. Just use '4' or sizeof(UINT32).
>>>
>>>> + Width = EfiPciIoWidthUint32;
>>>> + Count = Size >> Width;
>>>> + } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
>>>> + Width = EfiPciIoWidthUint16;
>>>> + Count = 1;
>>>> + } else {
>>>> + Width = EfiPciIoWidthUint8;
>>>> + Count = 1;
>>>> + }
>>
>> I used "BITx" and "(BITx - 1)" for consistency, and because they seemed
>> to convey the idea well (namely, shifting down the alignment).
>>
>> I'm fine replacing "BIT2" with "4", but then I believe I should also
>> replace "(BIT2 - 1)" with "3". Similarly, replace "BIT1" with "2", and
>> "(BIT1 -1)" with 1.
>>
>> Do you prefer the current code or the decimal constants?
>>
>
> IMHO, BITx is for bitmasks, not for numerical constants.
>
> So yes, if you think (as do I) that sizeof(UINTnn) is too wordy, just
> use the plain numbers please.
>
OK, I'll do that. Thanks!
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 4/7] OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
` (2 preceding siblings ...)
2018-05-23 20:21 ` [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib Laszlo Ersek
@ 2018-05-23 20:21 ` Laszlo Ersek
2018-05-24 8:14 ` Ard Biesheuvel
2018-05-23 20:21 ` [PATCH v2 5/7] ArmVirtPkg: " Laszlo Ersek
` (3 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-23 20:21 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
Resolve the PciCapLib, PciCapPciSegmentLib, and PciCapPciIoLib classes to
their single respective instances. Later patches will use these lib
classes in OvmfPkg drivers.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- update references from MdePkg to OvmfPkg
OvmfPkg/OvmfPkgIa32.dsc | 3 +++
OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++
OvmfPkg/OvmfPkgX64.dsc | 3 +++
3 files changed, 9 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 251434a9ff7c..a2c995b910cd 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -127,6 +127,9 @@ [LibraryClasses]
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
+ PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
+ PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
+ PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index ce247a59d61a..bc7db229d2d9 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -132,6 +132,9 @@ [LibraryClasses]
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
+ PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
+ PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
+ PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 67f7e155ee3e..0767b34d1877 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -132,6 +132,9 @@ [LibraryClasses]
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
+ PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
+ PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
+ PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 4/7] OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
2018-05-23 20:21 ` [PATCH v2 4/7] OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib Laszlo Ersek
@ 2018-05-24 8:14 ` Ard Biesheuvel
0 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 8:14 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Resolve the PciCapLib, PciCapPciSegmentLib, and PciCapPciIoLib classes to
> their single respective instances. Later patches will use these lib
> classes in OvmfPkg drivers.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Notes:
> v2:
> - update references from MdePkg to OvmfPkg
>
> OvmfPkg/OvmfPkgIa32.dsc | 3 +++
> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++
> OvmfPkg/OvmfPkgX64.dsc | 3 +++
> 3 files changed, 9 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 251434a9ff7c..a2c995b910cd 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -127,6 +127,9 @@ [LibraryClasses]
> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> + PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
> + PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> + PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
> SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index ce247a59d61a..bc7db229d2d9 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -132,6 +132,9 @@ [LibraryClasses]
> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> + PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
> + PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> + PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
> SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 67f7e155ee3e..0767b34d1877 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -132,6 +132,9 @@ [LibraryClasses]
> PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
> PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
> PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> + PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
> + PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> + PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
> IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
> OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
> SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 5/7] ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
` (3 preceding siblings ...)
2018-05-23 20:21 ` [PATCH v2 4/7] OvmfPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib Laszlo Ersek
@ 2018-05-23 20:21 ` Laszlo Ersek
2018-05-24 8:14 ` Ard Biesheuvel
2018-05-23 20:21 ` [PATCH v2 6/7] OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib Laszlo Ersek
` (2 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-23 20:21 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
Resolve the PciCapLib, PciCapPciSegmentLib, and PciCapPciIoLib classes to
their single respective instances under OvmfPkg. Later patches will use
those lib classes in OvmfPkg drivers, some of which are included in
ArmVirt platforms.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- update references from MdePkg to OvmfPkg
ArmVirtPkg/ArmVirt.dsc.inc | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 35bccb3dc1f4..766e4f598a07 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -131,6 +131,9 @@ [LibraryClasses.common]
# PCI Libraries
PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
PciExpressLib|ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
+ PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
+ PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
+ PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
# USB Libraries
UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 5/7] ArmVirtPkg: resolve PciCapLib, PciCapPciSegmentLib, PciCapPciIoLib
2018-05-23 20:21 ` [PATCH v2 5/7] ArmVirtPkg: " Laszlo Ersek
@ 2018-05-24 8:14 ` Ard Biesheuvel
0 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 8:14 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Resolve the PciCapLib, PciCapPciSegmentLib, and PciCapPciIoLib classes to
> their single respective instances under OvmfPkg. Later patches will use
> those lib classes in OvmfPkg drivers, some of which are included in
> ArmVirt platforms.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Notes:
> v2:
> - update references from MdePkg to OvmfPkg
>
> ArmVirtPkg/ArmVirt.dsc.inc | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 35bccb3dc1f4..766e4f598a07 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -131,6 +131,9 @@ [LibraryClasses.common]
> # PCI Libraries
> PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> PciExpressLib|ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
> + PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
> + PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
> + PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
>
> # USB Libraries
> UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 6/7] OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
` (4 preceding siblings ...)
2018-05-23 20:21 ` [PATCH v2 5/7] ArmVirtPkg: " Laszlo Ersek
@ 2018-05-23 20:21 ` Laszlo Ersek
2018-05-24 8:15 ` Ard Biesheuvel
2018-05-23 20:21 ` [PATCH v2 7/7] OvmfPkg/Virtio10Dxe: " Laszlo Ersek
2018-05-24 20:04 ` [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
7 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-23 20:21 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
Replace the manual capability list parsing in OvmfPkg/PciHotPlugInitDxe
with PciCapLib and PciCapPciSegmentLib API calls.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- no changes
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf | 5 +
OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c | 267 +++++++-------------
2 files changed, 102 insertions(+), 170 deletions(-)
diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
index 38043986eb67..cc2b60d44263 100644
--- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
+++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
@@ -35,6 +35,8 @@ [LibraryClasses]
DebugLib
DevicePathLib
MemoryAllocationLib
+ PciCapLib
+ PciCapPciSegmentLib
PciLib
UefiBootServicesTableLib
UefiDriverEntryPoint
@@ -42,5 +44,8 @@ [LibraryClasses]
[Protocols]
gEfiPciHotPlugInitProtocolGuid ## ALWAYS_PRODUCES
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId ## CONSUMES
+
[Depex]
TRUE
diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
index 177e1a62120d..3449796878ef 100644
--- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
+++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
@@ -14,6 +14,7 @@
**/
#include <IndustryStandard/Acpi10.h>
+#include <IndustryStandard/Q35MchIch9.h>
#include <IndustryStandard/QemuPciBridgeCapabilities.h>
#include <Library/BaseLib.h>
@@ -21,12 +22,20 @@
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/PciCapLib.h>
+#include <Library/PciCapPciSegmentLib.h>
#include <Library/PciLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Protocol/PciHotPlugInit.h>
#include <Protocol/PciRootBridgeIo.h>
+//
+// TRUE if the PCI platform supports extended config space, FALSE otherwise.
+//
+STATIC BOOLEAN mPciExtConfSpaceSupported;
+
+
//
// The protocol interface this driver produces.
//
@@ -248,91 +257,11 @@ HighBitSetRoundUp64 (
}
-/**
- Read a slice from conventional PCI config space at the given offset, then
- advance the offset.
-
- @param[in] PciAddress The address of the PCI Device -- Bus, Device, Function
- -- in UEFI (not PciLib) encoding.
-
- @param[in,out] Offset On input, the offset in conventional PCI config space
- to start reading from. On output, the offset of the
- first byte that was not read.
-
- @param[in] Size The number of bytes to read.
-
- @param[out] Buffer On output, the bytes read from PCI config space are
- stored in this object.
-**/
-STATIC
-VOID
-ReadConfigSpace (
- IN CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *PciAddress,
- IN OUT UINT8 *Offset,
- IN UINT8 Size,
- OUT VOID *Buffer
- )
-{
- PciReadBuffer (
- PCI_LIB_ADDRESS (
- PciAddress->Bus,
- PciAddress->Device,
- PciAddress->Function,
- *Offset
- ),
- Size,
- Buffer
- );
- *Offset += Size;
-}
-
-
-/**
- Convenience wrapper macro for ReadConfigSpace().
-
- Given the following conditions:
-
- - HeaderField is the first field in the structure pointed-to by Struct,
-
- - Struct->HeaderField has been populated from the conventional PCI config
- space of the PCI device identified by PciAddress,
-
- - *Offset points one past HeaderField in the conventional PCI config space of
- the PCI device identified by PciAddress,
-
- populate the rest of *Struct from conventional PCI config space, starting at
- *Offset. Finally, increment *Offset so that it point one past *Struct.
-
- @param[in] PciAddress The address of the PCI Device -- Bus, Device, Function
- -- in UEFI (not PciLib) encoding. Type: pointer to
- CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS.
-
- @param[in,out] Offset On input, the offset in conventional PCI config space
- to start reading from; one past Struct->HeaderField.
- On output, the offset of the first byte that was not
- read; one past *Struct. Type: pointer to UINT8.
-
- @param[out] Struct The structure to complete. Type: pointer to structure
- object.
-
- @param[in] HeaderField The name of the first field in *Struct, after which
- *Struct should be populated. Type: structure member
- identifier.
-**/
-#define COMPLETE_CONFIG_SPACE_STRUCT(PciAddress, Offset, Struct, HeaderField) \
- ReadConfigSpace ( \
- (PciAddress), \
- (Offset), \
- (UINT8)(sizeof *(Struct) - sizeof ((Struct)->HeaderField)), \
- &((Struct)->HeaderField) + 1 \
- )
-
-
/**
Look up the QEMU-specific Resource Reservation capability in the conventional
config space of a Hotplug Controller (that is, PCI Bridge).
- This function performs as few config space reads as possible.
+ On error, the contents of ReservationHint are indeterminate.
@param[in] HpcPciAddress The address of the PCI Bridge -- Bus, Device,
Function -- in UEFI (not PciLib) encoding.
@@ -343,8 +272,9 @@ ReadConfigSpace (
@retval EFI_SUCCESS The capability has been found, ReservationHint has
been populated.
- @retval EFI_NOT_FOUND The capability is missing. The contents of
- ReservationHint are now indeterminate.
+ @retval EFI_NOT_FOUND The capability is missing.
+
+ @return Error codes from PciCapPciSegmentLib and PciCapLib.
**/
STATIC
EFI_STATUS
@@ -353,10 +283,12 @@ QueryReservationHint (
OUT QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION *ReservationHint
)
{
- UINT16 PciVendorId;
- UINT16 PciStatus;
- UINT8 PciCapPtr;
- UINT8 Offset;
+ UINT16 PciVendorId;
+ EFI_STATUS Status;
+ PCI_CAP_DEV *PciDevice;
+ PCI_CAP_LIST *CapList;
+ UINT16 VendorInstance;
+ PCI_CAP *VendorCap;
//
// Check the vendor identifier.
@@ -374,108 +306,101 @@ QueryReservationHint (
}
//
- // Check the Capabilities List bit in the PCI Status Register.
+ // Parse the capabilities lists.
//
- PciStatus = PciRead16 (
- PCI_LIB_ADDRESS (
- HpcPciAddress->Bus,
- HpcPciAddress->Device,
- HpcPciAddress->Function,
- PCI_PRIMARY_STATUS_OFFSET
- )
- );
- if ((PciStatus & EFI_PCI_STATUS_CAPABILITY) == 0) {
- return EFI_NOT_FOUND;
+ Status = PciCapPciSegmentDeviceInit (
+ mPciExtConfSpaceSupported ? PciCapExtended : PciCapNormal,
+ 0, // Segment
+ HpcPciAddress->Bus,
+ HpcPciAddress->Device,
+ HpcPciAddress->Function,
+ &PciDevice
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = PciCapListInit (PciDevice, &CapList);
+ if (EFI_ERROR (Status)) {
+ goto UninitPciDevice;
}
//
- // Fetch the start of the Capabilities List.
- //
- PciCapPtr = PciRead8 (
- PCI_LIB_ADDRESS (
- HpcPciAddress->Bus,
- HpcPciAddress->Device,
- HpcPciAddress->Function,
- PCI_CAPBILITY_POINTER_OFFSET
- )
- );
-
- //
- // Scan the Capabilities List until we find the terminator element, or the
- // Resource Reservation capability.
+ // Scan the vendor capability instances for the Resource Reservation
+ // capability.
//
- for (Offset = PciCapPtr & 0xFC;
- Offset > 0;
- Offset = ReservationHint->BridgeHdr.VendorHdr.Hdr.NextItemPtr & 0xFC) {
- BOOLEAN EnoughRoom;
-
- //
- // Check if the Resource Reservation capability would fit into config space
- // at this offset.
- //
- EnoughRoom = (BOOLEAN)(
- Offset <= PCI_MAX_CONFIG_OFFSET - sizeof *ReservationHint
- );
+ VendorInstance = 0;
+ for (;;) {
+ UINT8 VendorLength;
+ UINT8 BridgeCapType;
- //
- // Read the standard capability header so we can check the capability ID
- // (if necessary) and advance to the next capability.
- //
- ReadConfigSpace (
- HpcPciAddress,
- &Offset,
- (UINT8)sizeof ReservationHint->BridgeHdr.VendorHdr.Hdr,
- &ReservationHint->BridgeHdr.VendorHdr.Hdr
- );
- if (!EnoughRoom ||
- (ReservationHint->BridgeHdr.VendorHdr.Hdr.CapabilityID !=
- EFI_PCI_CAPABILITY_ID_VENDOR)) {
- continue;
+ Status = PciCapListFindCap (
+ CapList,
+ PciCapNormal,
+ EFI_PCI_CAPABILITY_ID_VENDOR,
+ VendorInstance++,
+ &VendorCap
+ );
+ if (EFI_ERROR (Status)) {
+ goto UninitCapList;
}
//
- // Read the rest of the vendor capability header so we can check the
- // capability length.
+ // Check the vendor capability length.
//
- COMPLETE_CONFIG_SPACE_STRUCT (
- HpcPciAddress,
- &Offset,
- &ReservationHint->BridgeHdr.VendorHdr,
- Hdr
- );
- if (ReservationHint->BridgeHdr.VendorHdr.Length !=
- sizeof *ReservationHint) {
+ Status = PciCapRead (
+ PciDevice,
+ VendorCap,
+ OFFSET_OF (EFI_PCI_CAPABILITY_VENDOR_HDR, Length),
+ &VendorLength,
+ sizeof VendorLength
+ );
+ if (EFI_ERROR (Status)) {
+ goto UninitCapList;
+ }
+ if (VendorLength != sizeof *ReservationHint) {
continue;
}
//
- // Read the rest of the QEMU bridge capability header so we can check the
- // capability type.
+ // Check the vendor bridge capability type.
//
- COMPLETE_CONFIG_SPACE_STRUCT (
- HpcPciAddress,
- &Offset,
- &ReservationHint->BridgeHdr,
- VendorHdr
- );
- if (ReservationHint->BridgeHdr.Type !=
+ Status = PciCapRead (
+ PciDevice,
+ VendorCap,
+ OFFSET_OF (QEMU_PCI_BRIDGE_CAPABILITY_HDR, Type),
+ &BridgeCapType,
+ sizeof BridgeCapType
+ );
+ if (EFI_ERROR (Status)) {
+ goto UninitCapList;
+ }
+ if (BridgeCapType ==
QEMU_PCI_BRIDGE_CAPABILITY_TYPE_RESOURCE_RESERVATION) {
- continue;
+ //
+ // We have a match.
+ //
+ break;
}
-
- //
- // Read the body of the reservation hint.
- //
- COMPLETE_CONFIG_SPACE_STRUCT (
- HpcPciAddress,
- &Offset,
- ReservationHint,
- BridgeHdr
- );
- return EFI_SUCCESS;
}
- return EFI_NOT_FOUND;
+ //
+ // Populate ReservationHint.
+ //
+ Status = PciCapRead (
+ PciDevice,
+ VendorCap,
+ 0, // SourceOffsetInCap
+ ReservationHint,
+ sizeof *ReservationHint
+ );
+
+UninitCapList:
+ PciCapListUninit (CapList);
+
+UninitPciDevice:
+ PciCapPciSegmentDeviceUninit (PciDevice);
+
+ return Status;
}
@@ -870,6 +795,8 @@ DriverInitialize (
{
EFI_STATUS Status;
+ mPciExtConfSpaceSupported = (PcdGet16 (PcdOvmfHostBridgePciDevId) ==
+ INTEL_Q35_MCH_DEVICE_ID);
mPciHotPlugInit.GetRootHpcList = GetRootHpcList;
mPciHotPlugInit.InitializeRootHpc = InitializeRootHpc;
mPciHotPlugInit.GetResourcePadding = GetResourcePadding;
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 6/7] OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib
2018-05-23 20:21 ` [PATCH v2 6/7] OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib Laszlo Ersek
@ 2018-05-24 8:15 ` Ard Biesheuvel
0 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 8:15 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Replace the manual capability list parsing in OvmfPkg/PciHotPlugInitDxe
> with PciCapLib and PciCapPciSegmentLib API calls.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Notes:
> v2:
> - no changes
>
> OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf | 5 +
> OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c | 267 +++++++-------------
> 2 files changed, 102 insertions(+), 170 deletions(-)
>
> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> index 38043986eb67..cc2b60d44263 100644
> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
> @@ -35,6 +35,8 @@ [LibraryClasses]
> DebugLib
> DevicePathLib
> MemoryAllocationLib
> + PciCapLib
> + PciCapPciSegmentLib
> PciLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> @@ -42,5 +44,8 @@ [LibraryClasses]
> [Protocols]
> gEfiPciHotPlugInitProtocolGuid ## ALWAYS_PRODUCES
>
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId ## CONSUMES
> +
> [Depex]
> TRUE
> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
> index 177e1a62120d..3449796878ef 100644
> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c
> @@ -14,6 +14,7 @@
> **/
>
> #include <IndustryStandard/Acpi10.h>
> +#include <IndustryStandard/Q35MchIch9.h>
> #include <IndustryStandard/QemuPciBridgeCapabilities.h>
>
> #include <Library/BaseLib.h>
> @@ -21,12 +22,20 @@
> #include <Library/DebugLib.h>
> #include <Library/DevicePathLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/PciCapLib.h>
> +#include <Library/PciCapPciSegmentLib.h>
> #include <Library/PciLib.h>
> #include <Library/UefiBootServicesTableLib.h>
>
> #include <Protocol/PciHotPlugInit.h>
> #include <Protocol/PciRootBridgeIo.h>
>
> +//
> +// TRUE if the PCI platform supports extended config space, FALSE otherwise.
> +//
> +STATIC BOOLEAN mPciExtConfSpaceSupported;
> +
> +
> //
> // The protocol interface this driver produces.
> //
> @@ -248,91 +257,11 @@ HighBitSetRoundUp64 (
> }
>
>
> -/**
> - Read a slice from conventional PCI config space at the given offset, then
> - advance the offset.
> -
> - @param[in] PciAddress The address of the PCI Device -- Bus, Device, Function
> - -- in UEFI (not PciLib) encoding.
> -
> - @param[in,out] Offset On input, the offset in conventional PCI config space
> - to start reading from. On output, the offset of the
> - first byte that was not read.
> -
> - @param[in] Size The number of bytes to read.
> -
> - @param[out] Buffer On output, the bytes read from PCI config space are
> - stored in this object.
> -**/
> -STATIC
> -VOID
> -ReadConfigSpace (
> - IN CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *PciAddress,
> - IN OUT UINT8 *Offset,
> - IN UINT8 Size,
> - OUT VOID *Buffer
> - )
> -{
> - PciReadBuffer (
> - PCI_LIB_ADDRESS (
> - PciAddress->Bus,
> - PciAddress->Device,
> - PciAddress->Function,
> - *Offset
> - ),
> - Size,
> - Buffer
> - );
> - *Offset += Size;
> -}
> -
> -
> -/**
> - Convenience wrapper macro for ReadConfigSpace().
> -
> - Given the following conditions:
> -
> - - HeaderField is the first field in the structure pointed-to by Struct,
> -
> - - Struct->HeaderField has been populated from the conventional PCI config
> - space of the PCI device identified by PciAddress,
> -
> - - *Offset points one past HeaderField in the conventional PCI config space of
> - the PCI device identified by PciAddress,
> -
> - populate the rest of *Struct from conventional PCI config space, starting at
> - *Offset. Finally, increment *Offset so that it point one past *Struct.
> -
> - @param[in] PciAddress The address of the PCI Device -- Bus, Device, Function
> - -- in UEFI (not PciLib) encoding. Type: pointer to
> - CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS.
> -
> - @param[in,out] Offset On input, the offset in conventional PCI config space
> - to start reading from; one past Struct->HeaderField.
> - On output, the offset of the first byte that was not
> - read; one past *Struct. Type: pointer to UINT8.
> -
> - @param[out] Struct The structure to complete. Type: pointer to structure
> - object.
> -
> - @param[in] HeaderField The name of the first field in *Struct, after which
> - *Struct should be populated. Type: structure member
> - identifier.
> -**/
> -#define COMPLETE_CONFIG_SPACE_STRUCT(PciAddress, Offset, Struct, HeaderField) \
> - ReadConfigSpace ( \
> - (PciAddress), \
> - (Offset), \
> - (UINT8)(sizeof *(Struct) - sizeof ((Struct)->HeaderField)), \
> - &((Struct)->HeaderField) + 1 \
> - )
> -
> -
> /**
> Look up the QEMU-specific Resource Reservation capability in the conventional
> config space of a Hotplug Controller (that is, PCI Bridge).
>
> - This function performs as few config space reads as possible.
> + On error, the contents of ReservationHint are indeterminate.
>
> @param[in] HpcPciAddress The address of the PCI Bridge -- Bus, Device,
> Function -- in UEFI (not PciLib) encoding.
> @@ -343,8 +272,9 @@ ReadConfigSpace (
> @retval EFI_SUCCESS The capability has been found, ReservationHint has
> been populated.
>
> - @retval EFI_NOT_FOUND The capability is missing. The contents of
> - ReservationHint are now indeterminate.
> + @retval EFI_NOT_FOUND The capability is missing.
> +
> + @return Error codes from PciCapPciSegmentLib and PciCapLib.
> **/
> STATIC
> EFI_STATUS
> @@ -353,10 +283,12 @@ QueryReservationHint (
> OUT QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION *ReservationHint
> )
> {
> - UINT16 PciVendorId;
> - UINT16 PciStatus;
> - UINT8 PciCapPtr;
> - UINT8 Offset;
> + UINT16 PciVendorId;
> + EFI_STATUS Status;
> + PCI_CAP_DEV *PciDevice;
> + PCI_CAP_LIST *CapList;
> + UINT16 VendorInstance;
> + PCI_CAP *VendorCap;
>
> //
> // Check the vendor identifier.
> @@ -374,108 +306,101 @@ QueryReservationHint (
> }
>
> //
> - // Check the Capabilities List bit in the PCI Status Register.
> + // Parse the capabilities lists.
> //
> - PciStatus = PciRead16 (
> - PCI_LIB_ADDRESS (
> - HpcPciAddress->Bus,
> - HpcPciAddress->Device,
> - HpcPciAddress->Function,
> - PCI_PRIMARY_STATUS_OFFSET
> - )
> - );
> - if ((PciStatus & EFI_PCI_STATUS_CAPABILITY) == 0) {
> - return EFI_NOT_FOUND;
> + Status = PciCapPciSegmentDeviceInit (
> + mPciExtConfSpaceSupported ? PciCapExtended : PciCapNormal,
> + 0, // Segment
> + HpcPciAddress->Bus,
> + HpcPciAddress->Device,
> + HpcPciAddress->Function,
> + &PciDevice
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = PciCapListInit (PciDevice, &CapList);
> + if (EFI_ERROR (Status)) {
> + goto UninitPciDevice;
> }
>
> //
> - // Fetch the start of the Capabilities List.
> - //
> - PciCapPtr = PciRead8 (
> - PCI_LIB_ADDRESS (
> - HpcPciAddress->Bus,
> - HpcPciAddress->Device,
> - HpcPciAddress->Function,
> - PCI_CAPBILITY_POINTER_OFFSET
> - )
> - );
> -
> - //
> - // Scan the Capabilities List until we find the terminator element, or the
> - // Resource Reservation capability.
> + // Scan the vendor capability instances for the Resource Reservation
> + // capability.
> //
> - for (Offset = PciCapPtr & 0xFC;
> - Offset > 0;
> - Offset = ReservationHint->BridgeHdr.VendorHdr.Hdr.NextItemPtr & 0xFC) {
> - BOOLEAN EnoughRoom;
> -
> - //
> - // Check if the Resource Reservation capability would fit into config space
> - // at this offset.
> - //
> - EnoughRoom = (BOOLEAN)(
> - Offset <= PCI_MAX_CONFIG_OFFSET - sizeof *ReservationHint
> - );
> + VendorInstance = 0;
> + for (;;) {
> + UINT8 VendorLength;
> + UINT8 BridgeCapType;
>
> - //
> - // Read the standard capability header so we can check the capability ID
> - // (if necessary) and advance to the next capability.
> - //
> - ReadConfigSpace (
> - HpcPciAddress,
> - &Offset,
> - (UINT8)sizeof ReservationHint->BridgeHdr.VendorHdr.Hdr,
> - &ReservationHint->BridgeHdr.VendorHdr.Hdr
> - );
> - if (!EnoughRoom ||
> - (ReservationHint->BridgeHdr.VendorHdr.Hdr.CapabilityID !=
> - EFI_PCI_CAPABILITY_ID_VENDOR)) {
> - continue;
> + Status = PciCapListFindCap (
> + CapList,
> + PciCapNormal,
> + EFI_PCI_CAPABILITY_ID_VENDOR,
> + VendorInstance++,
> + &VendorCap
> + );
> + if (EFI_ERROR (Status)) {
> + goto UninitCapList;
> }
>
> //
> - // Read the rest of the vendor capability header so we can check the
> - // capability length.
> + // Check the vendor capability length.
> //
> - COMPLETE_CONFIG_SPACE_STRUCT (
> - HpcPciAddress,
> - &Offset,
> - &ReservationHint->BridgeHdr.VendorHdr,
> - Hdr
> - );
> - if (ReservationHint->BridgeHdr.VendorHdr.Length !=
> - sizeof *ReservationHint) {
> + Status = PciCapRead (
> + PciDevice,
> + VendorCap,
> + OFFSET_OF (EFI_PCI_CAPABILITY_VENDOR_HDR, Length),
> + &VendorLength,
> + sizeof VendorLength
> + );
> + if (EFI_ERROR (Status)) {
> + goto UninitCapList;
> + }
> + if (VendorLength != sizeof *ReservationHint) {
> continue;
> }
>
> //
> - // Read the rest of the QEMU bridge capability header so we can check the
> - // capability type.
> + // Check the vendor bridge capability type.
> //
> - COMPLETE_CONFIG_SPACE_STRUCT (
> - HpcPciAddress,
> - &Offset,
> - &ReservationHint->BridgeHdr,
> - VendorHdr
> - );
> - if (ReservationHint->BridgeHdr.Type !=
> + Status = PciCapRead (
> + PciDevice,
> + VendorCap,
> + OFFSET_OF (QEMU_PCI_BRIDGE_CAPABILITY_HDR, Type),
> + &BridgeCapType,
> + sizeof BridgeCapType
> + );
> + if (EFI_ERROR (Status)) {
> + goto UninitCapList;
> + }
> + if (BridgeCapType ==
> QEMU_PCI_BRIDGE_CAPABILITY_TYPE_RESOURCE_RESERVATION) {
> - continue;
> + //
> + // We have a match.
> + //
> + break;
> }
> -
> - //
> - // Read the body of the reservation hint.
> - //
> - COMPLETE_CONFIG_SPACE_STRUCT (
> - HpcPciAddress,
> - &Offset,
> - ReservationHint,
> - BridgeHdr
> - );
> - return EFI_SUCCESS;
> }
>
> - return EFI_NOT_FOUND;
> + //
> + // Populate ReservationHint.
> + //
> + Status = PciCapRead (
> + PciDevice,
> + VendorCap,
> + 0, // SourceOffsetInCap
> + ReservationHint,
> + sizeof *ReservationHint
> + );
> +
> +UninitCapList:
> + PciCapListUninit (CapList);
> +
> +UninitPciDevice:
> + PciCapPciSegmentDeviceUninit (PciDevice);
> +
> + return Status;
> }
>
>
> @@ -870,6 +795,8 @@ DriverInitialize (
> {
> EFI_STATUS Status;
>
> + mPciExtConfSpaceSupported = (PcdGet16 (PcdOvmfHostBridgePciDevId) ==
> + INTEL_Q35_MCH_DEVICE_ID);
> mPciHotPlugInit.GetRootHpcList = GetRootHpcList;
> mPciHotPlugInit.InitializeRootHpc = InitializeRootHpc;
> mPciHotPlugInit.GetResourcePadding = GetResourcePadding;
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 7/7] OvmfPkg/Virtio10Dxe: convert to PciCapLib
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
` (5 preceding siblings ...)
2018-05-23 20:21 ` [PATCH v2 6/7] OvmfPkg/PciHotPlugInitDxe: convert to PciCapLib Laszlo Ersek
@ 2018-05-23 20:21 ` Laszlo Ersek
2018-05-24 8:16 ` Ard Biesheuvel
2018-05-24 20:04 ` [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
7 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-23 20:21 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
Replace the manual capability list parsing in OvmfPkg/Virtio10Dxe with
PciCapLib and PciCapPciIoLib API calls.
The VIRTIO_PCI_CAP_LINK structure type is now superfluous. (Well, it
always has been; we should have used EFI_PCI_CAPABILITY_HDR.)
Also, EFI_PCI_CAPABILITY_VENDOR_HDR is now included at the front of
VIRTIO_PCI_CAP. No driver other than Virtio10Dxe relies on VIRTIO_PCI_CAP.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- no changes
OvmfPkg/Virtio10Dxe/Virtio10.inf | 2 +
OvmfPkg/Include/IndustryStandard/Virtio10.h | 7 +-
OvmfPkg/Virtio10Dxe/Virtio10.c | 135 +++++++-------------
3 files changed, 52 insertions(+), 92 deletions(-)
diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.inf b/OvmfPkg/Virtio10Dxe/Virtio10.inf
index c4ef15d94bfc..db0cb1189a29 100644
--- a/OvmfPkg/Virtio10Dxe/Virtio10.inf
+++ b/OvmfPkg/Virtio10Dxe/Virtio10.inf
@@ -32,6 +32,8 @@ [LibraryClasses]
BaseMemoryLib
DebugLib
MemoryAllocationLib
+ PciCapLib
+ PciCapPciIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
diff --git a/OvmfPkg/Include/IndustryStandard/Virtio10.h b/OvmfPkg/Include/IndustryStandard/Virtio10.h
index c5efb5cfcb8a..7d51aa36b326 100644
--- a/OvmfPkg/Include/IndustryStandard/Virtio10.h
+++ b/OvmfPkg/Include/IndustryStandard/Virtio10.h
@@ -16,6 +16,7 @@
#ifndef _VIRTIO_1_0_H_
#define _VIRTIO_1_0_H_
+#include <IndustryStandard/Pci23.h>
#include <IndustryStandard/Virtio095.h>
//
@@ -29,11 +30,7 @@
//
#pragma pack (1)
typedef struct {
- UINT8 CapId; // Capability identifier (generic)
- UINT8 CapNext; // Link to next capability (generic)
-} VIRTIO_PCI_CAP_LINK;
-
-typedef struct {
+ EFI_PCI_CAPABILITY_VENDOR_HDR VendorHdr;
UINT8 ConfigType; // Identifies the specific VirtIo 1.0 config structure
UINT8 Bar; // The BAR that contains the structure
UINT8 Padding[3];
diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
index e9b50b6e437b..9ebb72c76bfd 100644
--- a/OvmfPkg/Virtio10Dxe/Virtio10.c
+++ b/OvmfPkg/Virtio10Dxe/Virtio10.c
@@ -21,6 +21,8 @@
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/PciCapLib.h>
+#include <Library/PciCapPciIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
@@ -184,48 +186,6 @@ GetBarType (
}
-/**
- Read a slice from PCI config space at the given offset, then advance the
- offset.
-
- @param [in] PciIo The EFI_PCI_IO_PROTOCOL instance that represents the
- device.
-
- @param [in,out] Offset On input, the offset in PCI config space to start
- reading from. On output, the offset of the first byte
- that was not read. On error, Offset is not modified.
-
- @param [in] Size The number of bytes to read.
-
- @param [out] Buffer On output, the bytes read from PCI config space are
- stored in this object.
-
- @retval EFI_SUCCESS Size bytes have been transferred from PCI config space
- (from Offset) to Buffer, and Offset has been incremented
- by Size.
-
- @return Error codes from PciIo->Pci.Read().
-**/
-STATIC
-EFI_STATUS
-ReadConfigSpace (
- IN EFI_PCI_IO_PROTOCOL *PciIo,
- IN OUT UINT32 *Offset,
- IN UINTN Size,
- OUT VOID *Buffer
- )
-{
- EFI_STATUS Status;
-
- Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint8, *Offset, Size, Buffer);
- if (EFI_ERROR (Status)) {
- return Status;
- }
- *Offset += (UINT32)Size;
- return EFI_SUCCESS;
-}
-
-
/*
Traverse the PCI capabilities list of a virtio-1.0 device, and capture the
locations of the interesting virtio-1.0 register blocks.
@@ -239,57 +199,51 @@ ReadConfigSpace (
will have been updated from the PCI
capabilities found.
- @param[in] CapabilityPtr The offset of the first capability in PCI
- config space, taken from the standard PCI
- device header.
-
@retval EFI_SUCCESS Traversal successful.
- @return Error codes from the ReadConfigSpace() and GetBarType()
- helper functions.
+ @return Error codes from PciCapPciIoLib, PciCapLib, and the
+ GetBarType() helper function.
*/
STATIC
EFI_STATUS
ParseCapabilities (
- IN OUT VIRTIO_1_0_DEV *Device,
- IN UINT8 CapabilityPtr
+ IN OUT VIRTIO_1_0_DEV *Device
)
{
- UINT32 Offset;
- VIRTIO_PCI_CAP_LINK CapLink;
+ EFI_STATUS Status;
+ PCI_CAP_DEV *PciDevice;
+ PCI_CAP_LIST *CapList;
+ UINT16 VendorInstance;
+ PCI_CAP *VendorCap;
- for (Offset = CapabilityPtr & 0xFC;
- Offset > 0;
- Offset = CapLink.CapNext & 0xFC
- ) {
- EFI_STATUS Status;
+ Status = PciCapPciIoDeviceInit (Device->PciIo, &PciDevice);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = PciCapListInit (PciDevice, &CapList);
+ if (EFI_ERROR (Status)) {
+ goto UninitPciDevice;
+ }
+
+ for (VendorInstance = 0;
+ !EFI_ERROR (PciCapListFindCap (CapList, PciCapNormal,
+ EFI_PCI_CAPABILITY_ID_VENDOR, VendorInstance,
+ &VendorCap));
+ VendorInstance++) {
UINT8 CapLen;
VIRTIO_PCI_CAP VirtIoCap;
VIRTIO_1_0_CONFIG *ParsedConfig;
- //
- // Read capability identifier and link to next capability.
- //
- Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof CapLink,
- &CapLink);
- if (EFI_ERROR (Status)) {
- return Status;
- }
- if (CapLink.CapId != 0x09) {
- //
- // Not a vendor-specific capability, move to the next one.
- //
- continue;
- }
-
//
// Big enough to accommodate a VIRTIO_PCI_CAP structure?
//
- Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof CapLen, &CapLen);
+ Status = PciCapRead (PciDevice, VendorCap,
+ OFFSET_OF (EFI_PCI_CAPABILITY_VENDOR_HDR, Length), &CapLen,
+ sizeof CapLen);
if (EFI_ERROR (Status)) {
- return Status;
+ goto UninitCapList;
}
- if (CapLen < sizeof CapLink + sizeof CapLen + sizeof VirtIoCap) {
+ if (CapLen < sizeof VirtIoCap) {
//
// Too small, move to next.
//
@@ -299,11 +253,11 @@ ParseCapabilities (
//
// Read interesting part of capability.
//
- Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof VirtIoCap,
- &VirtIoCap);
+ Status = PciCapRead (PciDevice, VendorCap, 0, &VirtIoCap, sizeof VirtIoCap);
if (EFI_ERROR (Status)) {
- return Status;
+ goto UninitCapList;
}
+
switch (VirtIoCap.ConfigType) {
case VIRTIO_PCI_CAP_COMMON_CFG:
ParsedConfig = &Device->CommonConfig;
@@ -326,7 +280,7 @@ ParseCapabilities (
//
Status = GetBarType (Device->PciIo, VirtIoCap.Bar, &ParsedConfig->BarType);
if (EFI_ERROR (Status)) {
- return Status;
+ goto UninitCapList;
}
ParsedConfig->Bar = VirtIoCap.Bar;
ParsedConfig->Offset = VirtIoCap.Offset;
@@ -337,19 +291,18 @@ ParseCapabilities (
// This capability has an additional field called NotifyOffsetMultiplier;
// parse it too.
//
- if (CapLen < sizeof CapLink + sizeof CapLen + sizeof VirtIoCap +
- sizeof Device->NotifyOffsetMultiplier) {
+ if (CapLen < sizeof VirtIoCap + sizeof Device->NotifyOffsetMultiplier) {
//
// Too small, move to next.
//
continue;
}
- Status = ReadConfigSpace (Device->PciIo, &Offset,
- sizeof Device->NotifyOffsetMultiplier,
- &Device->NotifyOffsetMultiplier);
+ Status = PciCapRead (PciDevice, VendorCap, sizeof VirtIoCap,
+ &Device->NotifyOffsetMultiplier,
+ sizeof Device->NotifyOffsetMultiplier);
if (EFI_ERROR (Status)) {
- return Status;
+ goto UninitCapList;
}
}
@@ -359,7 +312,15 @@ ParseCapabilities (
ParsedConfig->Exists = TRUE;
}
- return EFI_SUCCESS;
+ ASSERT_EFI_ERROR (Status);
+
+UninitCapList:
+ PciCapListUninit (CapList);
+
+UninitPciDevice:
+ PciCapPciIoDeviceUninit (PciDevice);
+
+ return Status;
}
@@ -1015,7 +976,7 @@ Virtio10BindingStart (
Device->VirtIo.SubSystemDeviceId = Pci.Hdr.DeviceId - 0x1040;
- Status = ParseCapabilities (Device, Pci.Device.CapabilityPtr);
+ Status = ParseCapabilities (Device);
if (EFI_ERROR (Status)) {
goto ClosePciIo;
}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 7/7] OvmfPkg/Virtio10Dxe: convert to PciCapLib
2018-05-23 20:21 ` [PATCH v2 7/7] OvmfPkg/Virtio10Dxe: " Laszlo Ersek
@ 2018-05-24 8:16 ` Ard Biesheuvel
2018-05-24 14:55 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2018-05-24 8:16 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
> Replace the manual capability list parsing in OvmfPkg/Virtio10Dxe with
> PciCapLib and PciCapPciIoLib API calls.
>
> The VIRTIO_PCI_CAP_LINK structure type is now superfluous. (Well, it
> always has been; we should have used EFI_PCI_CAPABILITY_HDR.)
>
> Also, EFI_PCI_CAPABILITY_VENDOR_HDR is now included at the front of
> VIRTIO_PCI_CAP. No driver other than Virtio10Dxe relies on VIRTIO_PCI_CAP.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Notes:
> v2:
> - no changes
>
> OvmfPkg/Virtio10Dxe/Virtio10.inf | 2 +
> OvmfPkg/Include/IndustryStandard/Virtio10.h | 7 +-
> OvmfPkg/Virtio10Dxe/Virtio10.c | 135 +++++++-------------
> 3 files changed, 52 insertions(+), 92 deletions(-)
>
> diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.inf b/OvmfPkg/Virtio10Dxe/Virtio10.inf
> index c4ef15d94bfc..db0cb1189a29 100644
> --- a/OvmfPkg/Virtio10Dxe/Virtio10.inf
> +++ b/OvmfPkg/Virtio10Dxe/Virtio10.inf
> @@ -32,6 +32,8 @@ [LibraryClasses]
> BaseMemoryLib
> DebugLib
> MemoryAllocationLib
> + PciCapLib
> + PciCapPciIoLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> UefiLib
> diff --git a/OvmfPkg/Include/IndustryStandard/Virtio10.h b/OvmfPkg/Include/IndustryStandard/Virtio10.h
> index c5efb5cfcb8a..7d51aa36b326 100644
> --- a/OvmfPkg/Include/IndustryStandard/Virtio10.h
> +++ b/OvmfPkg/Include/IndustryStandard/Virtio10.h
> @@ -16,6 +16,7 @@
> #ifndef _VIRTIO_1_0_H_
> #define _VIRTIO_1_0_H_
>
> +#include <IndustryStandard/Pci23.h>
> #include <IndustryStandard/Virtio095.h>
>
> //
> @@ -29,11 +30,7 @@
> //
> #pragma pack (1)
> typedef struct {
> - UINT8 CapId; // Capability identifier (generic)
> - UINT8 CapNext; // Link to next capability (generic)
> -} VIRTIO_PCI_CAP_LINK;
> -
> -typedef struct {
> + EFI_PCI_CAPABILITY_VENDOR_HDR VendorHdr;
> UINT8 ConfigType; // Identifies the specific VirtIo 1.0 config structure
> UINT8 Bar; // The BAR that contains the structure
> UINT8 Padding[3];
> diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
> index e9b50b6e437b..9ebb72c76bfd 100644
> --- a/OvmfPkg/Virtio10Dxe/Virtio10.c
> +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c
> @@ -21,6 +21,8 @@
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/PciCapLib.h>
> +#include <Library/PciCapPciIoLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
>
> @@ -184,48 +186,6 @@ GetBarType (
> }
>
>
> -/**
> - Read a slice from PCI config space at the given offset, then advance the
> - offset.
> -
> - @param [in] PciIo The EFI_PCI_IO_PROTOCOL instance that represents the
> - device.
> -
> - @param [in,out] Offset On input, the offset in PCI config space to start
> - reading from. On output, the offset of the first byte
> - that was not read. On error, Offset is not modified.
> -
> - @param [in] Size The number of bytes to read.
> -
> - @param [out] Buffer On output, the bytes read from PCI config space are
> - stored in this object.
> -
> - @retval EFI_SUCCESS Size bytes have been transferred from PCI config space
> - (from Offset) to Buffer, and Offset has been incremented
> - by Size.
> -
> - @return Error codes from PciIo->Pci.Read().
> -**/
> -STATIC
> -EFI_STATUS
> -ReadConfigSpace (
> - IN EFI_PCI_IO_PROTOCOL *PciIo,
> - IN OUT UINT32 *Offset,
> - IN UINTN Size,
> - OUT VOID *Buffer
> - )
> -{
> - EFI_STATUS Status;
> -
> - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint8, *Offset, Size, Buffer);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> - *Offset += (UINT32)Size;
> - return EFI_SUCCESS;
> -}
> -
> -
> /*
> Traverse the PCI capabilities list of a virtio-1.0 device, and capture the
> locations of the interesting virtio-1.0 register blocks.
> @@ -239,57 +199,51 @@ ReadConfigSpace (
> will have been updated from the PCI
> capabilities found.
>
> - @param[in] CapabilityPtr The offset of the first capability in PCI
> - config space, taken from the standard PCI
> - device header.
> -
> @retval EFI_SUCCESS Traversal successful.
>
> - @return Error codes from the ReadConfigSpace() and GetBarType()
> - helper functions.
> + @return Error codes from PciCapPciIoLib, PciCapLib, and the
> + GetBarType() helper function.
> */
> STATIC
> EFI_STATUS
> ParseCapabilities (
> - IN OUT VIRTIO_1_0_DEV *Device,
> - IN UINT8 CapabilityPtr
> + IN OUT VIRTIO_1_0_DEV *Device
> )
> {
> - UINT32 Offset;
> - VIRTIO_PCI_CAP_LINK CapLink;
> + EFI_STATUS Status;
> + PCI_CAP_DEV *PciDevice;
> + PCI_CAP_LIST *CapList;
> + UINT16 VendorInstance;
> + PCI_CAP *VendorCap;
>
> - for (Offset = CapabilityPtr & 0xFC;
> - Offset > 0;
> - Offset = CapLink.CapNext & 0xFC
> - ) {
> - EFI_STATUS Status;
> + Status = PciCapPciIoDeviceInit (Device->PciIo, &PciDevice);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = PciCapListInit (PciDevice, &CapList);
> + if (EFI_ERROR (Status)) {
> + goto UninitPciDevice;
> + }
> +
> + for (VendorInstance = 0;
> + !EFI_ERROR (PciCapListFindCap (CapList, PciCapNormal,
> + EFI_PCI_CAPABILITY_ID_VENDOR, VendorInstance,
> + &VendorCap));
> + VendorInstance++) {
> UINT8 CapLen;
> VIRTIO_PCI_CAP VirtIoCap;
> VIRTIO_1_0_CONFIG *ParsedConfig;
>
> - //
> - // Read capability identifier and link to next capability.
> - //
> - Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof CapLink,
> - &CapLink);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> - if (CapLink.CapId != 0x09) {
> - //
> - // Not a vendor-specific capability, move to the next one.
> - //
> - continue;
> - }
> -
> //
> // Big enough to accommodate a VIRTIO_PCI_CAP structure?
> //
> - Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof CapLen, &CapLen);
> + Status = PciCapRead (PciDevice, VendorCap,
> + OFFSET_OF (EFI_PCI_CAPABILITY_VENDOR_HDR, Length), &CapLen,
> + sizeof CapLen);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto UninitCapList;
> }
> - if (CapLen < sizeof CapLink + sizeof CapLen + sizeof VirtIoCap) {
> + if (CapLen < sizeof VirtIoCap) {
> //
> // Too small, move to next.
> //
> @@ -299,11 +253,11 @@ ParseCapabilities (
> //
> // Read interesting part of capability.
> //
> - Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof VirtIoCap,
> - &VirtIoCap);
> + Status = PciCapRead (PciDevice, VendorCap, 0, &VirtIoCap, sizeof VirtIoCap);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto UninitCapList;
> }
> +
> switch (VirtIoCap.ConfigType) {
> case VIRTIO_PCI_CAP_COMMON_CFG:
> ParsedConfig = &Device->CommonConfig;
> @@ -326,7 +280,7 @@ ParseCapabilities (
> //
> Status = GetBarType (Device->PciIo, VirtIoCap.Bar, &ParsedConfig->BarType);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto UninitCapList;
> }
> ParsedConfig->Bar = VirtIoCap.Bar;
> ParsedConfig->Offset = VirtIoCap.Offset;
> @@ -337,19 +291,18 @@ ParseCapabilities (
> // This capability has an additional field called NotifyOffsetMultiplier;
> // parse it too.
> //
> - if (CapLen < sizeof CapLink + sizeof CapLen + sizeof VirtIoCap +
> - sizeof Device->NotifyOffsetMultiplier) {
> + if (CapLen < sizeof VirtIoCap + sizeof Device->NotifyOffsetMultiplier) {
> //
> // Too small, move to next.
> //
> continue;
> }
>
> - Status = ReadConfigSpace (Device->PciIo, &Offset,
> - sizeof Device->NotifyOffsetMultiplier,
> - &Device->NotifyOffsetMultiplier);
> + Status = PciCapRead (PciDevice, VendorCap, sizeof VirtIoCap,
> + &Device->NotifyOffsetMultiplier,
> + sizeof Device->NotifyOffsetMultiplier);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto UninitCapList;
> }
> }
>
> @@ -359,7 +312,15 @@ ParseCapabilities (
> ParsedConfig->Exists = TRUE;
> }
>
> - return EFI_SUCCESS;
> + ASSERT_EFI_ERROR (Status);
> +
> +UninitCapList:
> + PciCapListUninit (CapList);
> +
> +UninitPciDevice:
> + PciCapPciIoDeviceUninit (PciDevice);
> +
> + return Status;
> }
>
>
> @@ -1015,7 +976,7 @@ Virtio10BindingStart (
>
> Device->VirtIo.SubSystemDeviceId = Pci.Hdr.DeviceId - 0x1040;
>
> - Status = ParseCapabilities (Device, Pci.Device.CapabilityPtr);
> + Status = ParseCapabilities (Device);
> if (EFI_ERROR (Status)) {
> goto ClosePciIo;
> }
> --
> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 7/7] OvmfPkg/Virtio10Dxe: convert to PciCapLib
2018-05-24 8:16 ` Ard Biesheuvel
@ 2018-05-24 14:55 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-24 14:55 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Jordan Justen
On 05/24/18 10:16, Ard Biesheuvel wrote:
> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>> Replace the manual capability list parsing in OvmfPkg/Virtio10Dxe with
>> PciCapLib and PciCapPciIoLib API calls.
>>
>> The VIRTIO_PCI_CAP_LINK structure type is now superfluous. (Well, it
>> always has been; we should have used EFI_PCI_CAPABILITY_HDR.)
>>
>> Also, EFI_PCI_CAPABILITY_VENDOR_HDR is now included at the front of
>> VIRTIO_PCI_CAP. No driver other than Virtio10Dxe relies on VIRTIO_PCI_CAP.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Thanks for the super quick review!
Can you please clarify your review of 2/7? You agreed with the commit
msg (thanks!) but didn't comment on the code in either direction.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library
2018-05-23 20:21 [PATCH v2 0/7] OvmfPkg, ArmVirtPkg: add and use PCI(E) Capabilities Library Laszlo Ersek
` (6 preceding siblings ...)
2018-05-23 20:21 ` [PATCH v2 7/7] OvmfPkg/Virtio10Dxe: " Laszlo Ersek
@ 2018-05-24 20:04 ` Laszlo Ersek
7 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2018-05-24 20:04 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen, Ard Biesheuvel
On 05/23/18 22:21, Laszlo Ersek wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: pci_cap_v2
>
> In v2, the new libs are initially introduced under OvmfPkg, rather
> than MdePkg. v1 was posted at
> <http://mid.mail-archive.com/20180504213637.11266-1-lersek@redhat.com>.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
I've addressed Ard's requests with the following (cumulative) diff:
> diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> index 6789359f0a54..c059264b322d 100644
> --- a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> +++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> @@ -874,6 +874,8 @@ PciCapGetInfo (
> {
> PCI_CAP *InstanceZero;
>
> + ASSERT (Info != NULL);
> +
> InstanceZero = (Cap->Key.Instance == 0 ? Cap :
> Cap->NumInstancesUnion.InstanceZero);
>
> diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> index 84369e4dc3a8..358d87f93103 100644
> --- a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c
> @@ -73,10 +73,10 @@ ProtoDevTransferConfig (
> // possible in one iteration of the loop. Otherwise, transfer only one
> // unit, to improve the alignment.
> //
> - if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
> + if (Size >= 4 && (ConfigOffset & 3) == 0) {
> Width = EfiPciIoWidthUint32;
> Count = Size >> Width;
> - } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
> + } else if (Size >= 2 && (ConfigOffset & 1) == 0) {
> Width = EfiPciIoWidthUint16;
> Count = 1;
> } else {
After retesting the series, I pushed it as commit range
4b8552d794e7..5685a243b6f8.
I've also filed <https://bugzilla.tianocore.org/show_bug.cgi?id=957>
about upstreaming the libraries from the first three patches to a more
central package.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread