[v2,04/15] bus/pci: find PCI capability

  Introduce two helpers so that drivers stop reinventing the wheel when it
comes to finding capabilities in a device PCI configuration space.
Use it in existing drivers.

Note:
- base/ drivers code is left untouched, only some wrappers in cxgbe
  are touched,
- bnx2x maintained a per device cache of capabilities, this code has been
  reworked to only cache the capabilities used in this driver,

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
Changes since v1:
- updated commitlog,
- separated VFIO changes for using standard PCI helper in a separate
  patch,
- marked new experimental symbols with current version,
- reordered defines in rte_pci.h,

---
 drivers/bus/pci/linux/pci_vfio.c   |  74 ++++--------------
 drivers/bus/pci/pci_common.c       |  45 +++++++++++
 drivers/bus/pci/rte_bus_pci.h      |  31 ++++++++
 drivers/bus/pci/version.map        |   4 +
 drivers/crypto/virtio/virtio_pci.c |  57 +++++---------
 drivers/event/dlb2/pf/dlb2_main.c  |  42 +---------
 drivers/net/bnx2x/bnx2x.c          |  41 +++++-----
 drivers/net/cxgbe/base/adapter.h   |  28 +------
 drivers/net/gve/gve_ethdev.c       |  46 ++---------
 drivers/net/gve/gve_ethdev.h       |   4 -
 drivers/net/hns3/hns3_ethdev_vf.c  |  79 +++----------------
 drivers/net/virtio/virtio_pci.c    | 121 +++++------------------------
 lib/pci/rte_pci.h                  |  11 +++
 13 files changed, 186 insertions(+), 397 deletions(-)
  

Hi David,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, August 21, 2023 7:36 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@amd.com; Xia, Chenbo
> <chenbo.xia@intel.com>; nipun.gupta@amd.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
> Jay Zhou <jianjay.zhou@huawei.com>; McDaniel, Timothy
> <timothy.mcdaniel@intel.com>; Julien Aube <julien_dpdk@jaube.fr>; Rahul
> Lakkireddy <rahul.lakkireddy@chelsio.com>; Guo, Junfeng
> <junfeng.guo@intel.com>; Jeroen de Borst <jeroendb@google.com>; Rushil
> Gupta <rushilg@google.com>; Joshua Washington <joshwash@google.com>;
> Dongdong Liu <liudongdong3@huawei.com>; Yisen Zhuang
> <yisen.zhuang@huawei.com>; Maxime Coquelin <maxime.coquelin@redhat.com>;
> Gaetan Rivet <grive@u256.net>
> Subject: [PATCH v2 04/15] bus/pci: find PCI capability
> 
> Introduce two helpers so that drivers stop reinventing the wheel when it
> comes to finding capabilities in a device PCI configuration space.
> Use it in existing drivers.
> 
> Note:
> - base/ drivers code is left untouched, only some wrappers in cxgbe
>   are touched,
> - bnx2x maintained a per device cache of capabilities, this code has been
>   reworked to only cache the capabilities used in this driver,
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> Changes since v1:
> - updated commitlog,
> - separated VFIO changes for using standard PCI helper in a separate
>   patch,
> - marked new experimental symbols with current version,
> - reordered defines in rte_pci.h,
> 
> ---
>  drivers/bus/pci/linux/pci_vfio.c   |  74 ++++--------------
>  drivers/bus/pci/pci_common.c       |  45 +++++++++++
>  drivers/bus/pci/rte_bus_pci.h      |  31 ++++++++
>  drivers/bus/pci/version.map        |   4 +
>  drivers/crypto/virtio/virtio_pci.c |  57 +++++---------
>  drivers/event/dlb2/pf/dlb2_main.c  |  42 +---------
>  drivers/net/bnx2x/bnx2x.c          |  41 +++++-----
>  drivers/net/cxgbe/base/adapter.h   |  28 +------
>  drivers/net/gve/gve_ethdev.c       |  46 ++---------
>  drivers/net/gve/gve_ethdev.h       |   4 -
>  drivers/net/hns3/hns3_ethdev_vf.c  |  79 +++----------------
>  drivers/net/virtio/virtio_pci.c    | 121 +++++------------------------
>  lib/pci/rte_pci.h                  |  11 +++
>  13 files changed, 186 insertions(+), 397 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c
> b/drivers/bus/pci/linux/pci_vfio.c
> index 958f8b3b52..614ed5d696 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -110,74 +110,34 @@ static int
>  pci_vfio_get_msix_bar(const struct rte_pci_device *dev,
>  	struct pci_msix_table *msix_table)
>  {
> -	int ret;
> -	uint32_t reg;
> -	uint16_t flags;
> -	uint8_t cap_id, cap_offset;
> +	off_t cap_offset;
> 
> -	/* read PCI capability pointer from config space */
> -	ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> PCI_CAPABILITY_LIST);
> -	if (ret != sizeof(reg)) {
> -		RTE_LOG(ERR, EAL,
> -			"Cannot read capability pointer from PCI config
> space!\n");
> +	cap_offset = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);

I notice in some cases we use rte_pci_has_capability_list() to check first,
then looking for specific cap, in other cases we don't use
rte_pci_has_capability_list(). Since we define this API, should we always do
the check?


> +	if (cap_offset < 0)
>  		return -1;
> -	}
> 
> -	/* we need first byte */
> -	cap_offset = reg & 0xFF;
> +	if (cap_offset != 0) {
> +		uint16_t flags;
> +		uint32_t reg;
> 
> -	while (cap_offset) {
> -
> -		/* read PCI capability ID */
> -		ret = rte_pci_read_config(dev, &reg, sizeof(reg), cap_offset);
> -		if (ret != sizeof(reg)) {
> +		/* table offset resides in the next 4 bytes */
> +		if (rte_pci_read_config(dev, &reg, sizeof(reg), cap_offset + 4)
> < 0) {
>  			RTE_LOG(ERR, EAL,
> -				"Cannot read capability ID from PCI config
> space!\n");
> +				"Cannot read MSIX table from PCI config space!\n");
>  			return -1;
>  		}
> 
> -		/* we need first byte */
> -		cap_id = reg & 0xFF;
> -
> -		/* if we haven't reached MSI-X, check next capability */
> -		if (cap_id != PCI_CAP_ID_MSIX) {
> -			ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> cap_offset);
> -			if (ret != sizeof(reg)) {
> -				RTE_LOG(ERR, EAL,
> -					"Cannot read capability pointer from PCI
> config space!\n");
> -				return -1;
> -			}
> -
> -			/* we need second byte */
> -			cap_offset = (reg & 0xFF00) >> 8;
> -
> -			continue;
> +		if (rte_pci_read_config(dev, &flags, sizeof(flags), cap_offset
> + 2) < 0) {
> +			RTE_LOG(ERR, EAL,
> +				"Cannot read MSIX flags from PCI config space!\n");
> +			return -1;
>  		}
> -		/* else, read table offset */
> -		else {
> -			/* table offset resides in the next 4 bytes */
> -			ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> cap_offset + 4);
> -			if (ret != sizeof(reg)) {
> -				RTE_LOG(ERR, EAL,
> -					"Cannot read table offset from PCI config
> space!\n");
> -				return -1;
> -			}
> -
> -			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
> cap_offset + 2);
> -			if (ret != sizeof(flags)) {
> -				RTE_LOG(ERR, EAL,
> -					"Cannot read table flags from PCI config
> space!\n");
> -				return -1;
> -			}
> -
> -			msix_table->bar_index = reg & RTE_PCI_MSIX_TABLE_BIR;
> -			msix_table->offset = reg & RTE_PCI_MSIX_TABLE_OFFSET;
> -			msix_table->size =
> -				16 * (1 + (flags & RTE_PCI_MSIX_FLAGS_QSIZE));
> 
> -			return 0;
> -		}
> +		msix_table->bar_index = reg & RTE_PCI_MSIX_TABLE_BIR;
> +		msix_table->offset = reg & RTE_PCI_MSIX_TABLE_OFFSET;
> +		msix_table->size = 16 * (1 + (flags &
> RTE_PCI_MSIX_FLAGS_QSIZE));
>  	}
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index 382b0b8946..52272617eb 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -813,6 +813,51 @@ rte_pci_get_iommu_class(void)
>  	return iova_mode;
>  }
> 
> +bool
> +rte_pci_has_capability_list(const struct rte_pci_device *dev)
> +{
> +	uint16_t status;
> +
> +	if (rte_pci_read_config(dev, &status, sizeof(status),
> RTE_PCI_STATUS) != sizeof(status))
> +		return false;
> +
> +	return (status & RTE_PCI_STATUS_CAP_LIST) != 0;
> +}
> +
> +off_t
> +rte_pci_find_capability(const struct rte_pci_device *dev, uint8_t cap)
> +{
> +	off_t offset;
> +	uint8_t pos;
> +	int ttl;
> +
> +	offset = RTE_PCI_CAPABILITY_LIST;
> +	ttl = (RTE_PCI_CFG_SPACE_SIZE - RTE_PCI_STD_HEADER_SIZEOF) /
> RTE_PCI_CAP_SIZEOF;
> +
> +	if (rte_pci_read_config(dev, &pos, sizeof(pos), offset) < 0)
> +		return -1;
> +
> +	while (pos && ttl--) {
> +		uint16_t ent;
> +		uint8_t id;
> +
> +		offset = pos;
> +		if (rte_pci_read_config(dev, &ent, sizeof(ent), offset) < 0)
> +			return -1;
> +
> +		id = ent & 0xff;
> +		if (id == 0xff)
> +			break;
> +
> +		if (id == cap)
> +			return offset;
> +
> +		pos = (ent >> 8);
> +	}
> +
> +	return 0;
> +}
> +
>  off_t
>  rte_pci_find_ext_capability(const struct rte_pci_device *dev, uint32_t
> cap)
>  {
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index 75d0030eae..1ed33dbf3d 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -68,6 +68,37 @@ void rte_pci_unmap_device(struct rte_pci_device *dev);
>   */
>  void rte_pci_dump(FILE *f);
> 
> +/**
> + * Check whether this device has a PCI capability list.
> + *
> + *  @param dev
> + *    A pointer to rte_pci_device structure.
> + *
> + *  @return
> + *    true/false
> + */
> +__rte_experimental
> +bool rte_pci_has_capability_list(const struct rte_pci_device *dev);
> +
> +/**
> + * Find device's PCI capability.
> + *
> + *  @param dev
> + *    A pointer to rte_pci_device structure.
> + *
> + *  @param cap
> + *    Capability to be found, which can be any from
> + *    RTE_PCI_CAP_ID_*, defined in librte_pci.
> + *
> + *  @return
> + *  > 0: The offset of the next matching capability structure
> + *       within the device's PCI configuration space.
> + *  < 0: An error in PCI config space read.
> + *  = 0: Device does not support it.
> + */
> +__rte_experimental
> +off_t rte_pci_find_capability(const struct rte_pci_device *dev, uint8_t
> cap);
> +
>  /**
>   * Find device's extended PCI capability.
>   *
> diff --git a/drivers/bus/pci/version.map b/drivers/bus/pci/version.map
> index a0000f7938..2674f30235 100644
> --- a/drivers/bus/pci/version.map
> +++ b/drivers/bus/pci/version.map
> @@ -25,6 +25,10 @@ EXPERIMENTAL {
>  	# added in 23.07
>  	rte_pci_mmio_read;
>  	rte_pci_mmio_write;
> +
> +	# added in 23.11
> +	rte_pci_find_capability;
> +	rte_pci_has_capability_list;
>  };
> 
>  INTERNAL {
> diff --git a/drivers/crypto/virtio/virtio_pci.c
> b/drivers/crypto/virtio/virtio_pci.c
> index 95a43c8801..abc52b4701 100644
> --- a/drivers/crypto/virtio/virtio_pci.c
> +++ b/drivers/crypto/virtio/virtio_pci.c
> @@ -19,7 +19,6 @@
>   * we can't simply include that header here, as there is no such
>   * file for non-Linux platform.
>   */
> -#define PCI_CAPABILITY_LIST	0x34
>  #define PCI_CAP_ID_VNDR		0x09
>  #define PCI_CAP_ID_MSIX		0x11
> 
> @@ -343,8 +342,9 @@ get_cfg_addr(struct rte_pci_device *dev, struct
> virtio_pci_cap *cap)
>  static int
>  virtio_read_caps(struct rte_pci_device *dev, struct virtio_crypto_hw *hw)
>  {
> -	uint8_t pos;
>  	struct virtio_pci_cap cap;
> +	uint16_t flags;
> +	off_t pos;
>  	int ret;
> 
>  	if (rte_pci_map_device(dev)) {
> @@ -352,44 +352,26 @@ virtio_read_caps(struct rte_pci_device *dev, struct
> virtio_crypto_hw *hw)
>  		return -1;
>  	}
> 
> -	ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
> -	if (ret < 0) {
> -		VIRTIO_CRYPTO_INIT_LOG_DBG("failed to read pci capability
> list");
> -		return -1;
> +	/*
> +	 * Transitional devices would also have this capability,
> +	 * that's why we also check if msix is enabled.
> +	 */
> +	pos = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
> +	if (pos > 0 && rte_pci_read_config(dev, &flags, sizeof(flags),
> +			pos + 2) == sizeof(flags)) {
> +		if (flags & PCI_MSIX_ENABLE)
> +			hw->use_msix = VIRTIO_MSIX_ENABLED;
> +		else
> +			hw->use_msix = VIRTIO_MSIX_DISABLED;
> +	} else {
> +		hw->use_msix = VIRTIO_MSIX_NONE;
>  	}
> 
> -	while (pos) {
> -		ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos);
> -		if (ret < 0) {
> -			VIRTIO_CRYPTO_INIT_LOG_ERR(
> -				"failed to read pci cap at pos: %x", pos);
> -			break;
> -		}
> -
> -		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
> -			/* Transitional devices would also have this capability,
> -			 * that's why we also check if msix is enabled.
> -			 * 1st byte is cap ID; 2nd byte is the position of next
> -			 * cap; next two bytes are the flags.
> -			 */
> -			uint16_t flags = ((uint16_t *)&cap)[1];
> -
> -			if (flags & PCI_MSIX_ENABLE)
> -				hw->use_msix = VIRTIO_MSIX_ENABLED;
> -			else
> -				hw->use_msix = VIRTIO_MSIX_DISABLED;
> -		}
> -
> -		if (cap.cap_vndr != PCI_CAP_ID_VNDR) {
> -			VIRTIO_CRYPTO_INIT_LOG_DBG(
> -				"[%2x] skipping non VNDR cap id: %02x",
> -				pos, cap.cap_vndr);
> -			goto next;
> -		}
> -
> +	pos = rte_pci_find_capability(dev, PCI_CAP_ID_VNDR);

The logic of vendor cap init seems incorrect. Virtio devices have multiple
Vendor cap (different cfg type). But now the logic seems to only init the first
one.

> +	if (pos > 0 && rte_pci_read_config(dev, &cap, sizeof(cap), pos) ==
> sizeof(cap)) {
>  		VIRTIO_CRYPTO_INIT_LOG_DBG(
>  			"[%2x] cfg type: %u, bar: %u, offset: %04x, len: %u",
> -			pos, cap.cfg_type, cap.bar, cap.offset, cap.length);
> +			(unsigned int)pos, cap.cfg_type, cap.bar, cap.offset,
> cap.length);
> 
>  		switch (cap.cfg_type) {
>  		case VIRTIO_PCI_CAP_COMMON_CFG:
> @@ -411,9 +393,6 @@ virtio_read_caps(struct rte_pci_device *dev, struct
> virtio_crypto_hw *hw)
>  			hw->isr = get_cfg_addr(dev, &cap);
>  			break;
>  		}
> -
> -next:
> -		pos = cap.cap_next;
>  	}

...

> diff --git a/drivers/net/virtio/virtio_pci.c
> b/drivers/net/virtio/virtio_pci.c
> index 29eb739b04..9fd9db3e03 100644
> --- a/drivers/net/virtio/virtio_pci.c
> +++ b/drivers/net/virtio/virtio_pci.c
> @@ -20,7 +20,6 @@
>   * we can't simply include that header here, as there is no such
>   * file for non-Linux platform.
>   */
> -#define PCI_CAPABILITY_LIST	0x34
>  #define PCI_CAP_ID_VNDR		0x09
>  #define PCI_CAP_ID_MSIX		0x11
> 
> @@ -38,46 +37,16 @@ struct virtio_pci_internal
> virtio_pci_internal[RTE_MAX_ETHPORTS];
>  static enum virtio_msix_status
>  vtpci_msix_detect(struct rte_pci_device *dev)
>  {
> -	uint8_t pos;
> -	int ret;
> +	uint16_t flags;
> +	off_t pos;
> 
> -	ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
> -	if (ret != 1) {
> -		PMD_INIT_LOG(DEBUG,
> -			     "failed to read pci capability list, ret %d", ret);
> -		return VIRTIO_MSIX_NONE;
> -	}
> -
> -	while (pos) {
> -		uint8_t cap[2];
> -
> -		ret = rte_pci_read_config(dev, cap, sizeof(cap), pos);
> -		if (ret != sizeof(cap)) {
> -			PMD_INIT_LOG(DEBUG,
> -				     "failed to read pci cap at pos: %x ret %d",
> -				     pos, ret);
> -			break;
> -		}
> -
> -		if (cap[0] == PCI_CAP_ID_MSIX) {
> -			uint16_t flags;
> -
> -			ret = rte_pci_read_config(dev, &flags, sizeof(flags),
> -					pos + sizeof(cap));
> -			if (ret != sizeof(flags)) {
> -				PMD_INIT_LOG(DEBUG,
> -					     "failed to read pci cap at pos:"
> -					     " %x ret %d", pos + 2, ret);
> -				break;
> -			}
> -
> -			if (flags & PCI_MSIX_ENABLE)
> -				return VIRTIO_MSIX_ENABLED;
> -			else
> -				return VIRTIO_MSIX_DISABLED;
> -		}
> -
> -		pos = cap[1];
> +	pos = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
> +	if (pos > 0 && rte_pci_read_config(dev, &flags, sizeof(flags),
> +			pos + 2) == sizeof(flags)) {
> +		if (flags & PCI_MSIX_ENABLE)
> +			return VIRTIO_MSIX_ENABLED;
> +		else
> +			return VIRTIO_MSIX_DISABLED;
>  	}
> 
>  	return VIRTIO_MSIX_NONE;
> @@ -623,8 +592,8 @@ static int
>  virtio_read_caps(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
>  {
>  	struct virtio_pci_dev *dev = virtio_pci_get_dev(hw);
> -	uint8_t pos;
>  	struct virtio_pci_cap cap;
> +	off_t pos;
>  	int ret;
> 
>  	if (rte_pci_map_device(pci_dev)) {
> @@ -632,72 +601,25 @@ virtio_read_caps(struct rte_pci_device *pci_dev,
> struct virtio_hw *hw)
>  		return -1;
>  	}
> 
> -	ret = rte_pci_read_config(pci_dev, &pos, 1, PCI_CAPABILITY_LIST);
> -	if (ret != 1) {
> -		PMD_INIT_LOG(DEBUG,
> -			     "failed to read pci capability list, ret %d", ret);
> -		return -1;
> -	}
> -
> -	while (pos) {
> -		ret = rte_pci_read_config(pci_dev, &cap, 2, pos);
> -		if (ret != 2) {
> -			PMD_INIT_LOG(DEBUG,
> -				     "failed to read pci cap at pos: %x ret %d",
> -				     pos, ret);
> -			break;
> -		}
> -
> -		if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
> -			/* Transitional devices would also have this capability,
> -			 * that's why we also check if msix is enabled.
> -			 * 1st byte is cap ID; 2nd byte is the position of next
> -			 * cap; next two bytes are the flags.
> -			 */
> -			uint16_t flags;
> -
> -			ret = rte_pci_read_config(pci_dev, &flags, sizeof(flags),
> -					pos + 2);
> -			if (ret != sizeof(flags)) {
> -				PMD_INIT_LOG(DEBUG,
> -					     "failed to read pci cap at pos:"
> -					     " %x ret %d", pos + 2, ret);
> -				break;
> -			}
> -
> -			if (flags & PCI_MSIX_ENABLE)
> -				dev->msix_status = VIRTIO_MSIX_ENABLED;
> -			else
> -				dev->msix_status = VIRTIO_MSIX_DISABLED;
> -		}
> -
> -		if (cap.cap_vndr != PCI_CAP_ID_VNDR) {
> -			PMD_INIT_LOG(DEBUG,
> -				"[%2x] skipping non VNDR cap id: %02x",
> -				pos, cap.cap_vndr);
> -			goto next;
> -		}
> -
> -		ret = rte_pci_read_config(pci_dev, &cap, sizeof(cap), pos);
> -		if (ret != sizeof(cap)) {
> -			PMD_INIT_LOG(DEBUG,
> -				     "failed to read pci cap at pos: %x ret %d",
> -				     pos, ret);
> -			break;
> -		}
> +	/*
> +	 * Transitional devices would also have this capability,
> +	 * that's why we also check if msix is enabled.
> +	 */
> +	dev->msix_status = vtpci_msix_detect(pci_dev);
> 
> +	pos = rte_pci_find_capability(pci_dev, PCI_CAP_ID_VNDR);
> +	if (pos > 0 && rte_pci_read_config(pci_dev, &cap, sizeof(cap), pos)
> == sizeof(cap)) {
>  		PMD_INIT_LOG(DEBUG,
>  			"[%2x] cfg type: %u, bar: %u, offset: %04x, len: %u",
> -			pos, cap.cfg_type, cap.bar, cap.offset, cap.length);
> +			(unsigned int)pos, cap.cfg_type, cap.bar, cap.offset,
> cap.length);
> 

Same comment about the vendor cap.

Thanks,
Chenbo

>  		switch (cap.cfg_type) {
>  		case VIRTIO_PCI_CAP_COMMON_CFG:
>  			dev->common_cfg = get_cfg_addr(pci_dev, &cap);
>  			break;
>  		case VIRTIO_PCI_CAP_NOTIFY_CFG:
> -			ret = rte_pci_read_config(pci_dev,
> -					&dev->notify_off_multiplier,
> -					4, pos + sizeof(cap));
> +			ret = rte_pci_read_config(pci_dev, &dev-
> >notify_off_multiplier,
> +				4, pos + sizeof(cap));
>  			if (ret != 4)
>  				PMD_INIT_LOG(DEBUG,
>  					"failed to read notify_off_multiplier,
> ret %d",
> @@ -712,9 +634,6 @@ virtio_read_caps(struct rte_pci_device *pci_dev,
> struct virtio_hw *hw)
>  			dev->isr = get_cfg_addr(pci_dev, &cap);
>  			break;
>  		}
> -
> -next:
> -		pos = cap.cap_next;
>  	}
> 
>  	if (dev->common_cfg == NULL || dev->notify_base == NULL ||
> diff --git a/lib/pci/rte_pci.h b/lib/pci/rte_pci.h
> index aab761b918..49fd5b1d02 100644
> --- a/lib/pci/rte_pci.h
> +++ b/lib/pci/rte_pci.h
> @@ -28,13 +28,24 @@ extern "C" {
>  #define RTE_PCI_CFG_SPACE_SIZE		256
>  #define RTE_PCI_CFG_SPACE_EXP_SIZE	4096
> 
> +#define RTE_PCI_STD_HEADER_SIZEOF	64
> +
> +/* Standard register offsets in the PCI configuration space */
>  #define RTE_PCI_VENDOR_ID	0x00	/* 16 bits */
>  #define RTE_PCI_DEVICE_ID	0x02	/* 16 bits */
>  #define RTE_PCI_COMMAND		0x04	/* 16 bits */
> +#define RTE_PCI_STATUS		0x06	/* 16 bits */
> +#define RTE_PCI_CAPABILITY_LIST	0x34	/* 32 bits */
> 
>  /* PCI Command Register */
>  #define RTE_PCI_COMMAND_MASTER	0x4	/* Bus Master Enable */
> 
> +/* PCI Status Register (RTE_PCI_STATUS) */
> +#define RTE_PCI_STATUS_CAP_LIST		0x10	/* Support Capability List
> */
> +
> +/* Capability registers (RTE_PCI_CAPABILITY_LIST) */
> +#define RTE_PCI_CAP_SIZEOF		4
> +
>  /* PCI Express capability registers */
>  #define RTE_PCI_EXP_DEVCTL	8	/* Device Control */
> 
> --
> 2.41.0
  

Hello Chenbo,

On Thu, Sep 7, 2023 at 2:43 PM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > Introduce two helpers so that drivers stop reinventing the wheel when it
> > comes to finding capabilities in a device PCI configuration space.
> > Use it in existing drivers.
> >
> > Note:
> > - base/ drivers code is left untouched, only some wrappers in cxgbe
> >   are touched,
> > - bnx2x maintained a per device cache of capabilities, this code has been
> >   reworked to only cache the capabilities used in this driver,
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > Changes since v1:
> > - updated commitlog,
> > - separated VFIO changes for using standard PCI helper in a separate
> >   patch,
> > - marked new experimental symbols with current version,
> > - reordered defines in rte_pci.h,
> >
> > ---
> >  drivers/bus/pci/linux/pci_vfio.c   |  74 ++++--------------
> >  drivers/bus/pci/pci_common.c       |  45 +++++++++++
> >  drivers/bus/pci/rte_bus_pci.h      |  31 ++++++++
> >  drivers/bus/pci/version.map        |   4 +
> >  drivers/crypto/virtio/virtio_pci.c |  57 +++++---------
> >  drivers/event/dlb2/pf/dlb2_main.c  |  42 +---------
> >  drivers/net/bnx2x/bnx2x.c          |  41 +++++-----
> >  drivers/net/cxgbe/base/adapter.h   |  28 +------
> >  drivers/net/gve/gve_ethdev.c       |  46 ++---------
> >  drivers/net/gve/gve_ethdev.h       |   4 -
> >  drivers/net/hns3/hns3_ethdev_vf.c  |  79 +++----------------
> >  drivers/net/virtio/virtio_pci.c    | 121 +++++------------------------
> >  lib/pci/rte_pci.h                  |  11 +++
> >  13 files changed, 186 insertions(+), 397 deletions(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index 958f8b3b52..614ed5d696 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -110,74 +110,34 @@ static int
> >  pci_vfio_get_msix_bar(const struct rte_pci_device *dev,
> >       struct pci_msix_table *msix_table)
> >  {
> > -     int ret;
> > -     uint32_t reg;
> > -     uint16_t flags;
> > -     uint8_t cap_id, cap_offset;
> > +     off_t cap_offset;
> >
> > -     /* read PCI capability pointer from config space */
> > -     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > PCI_CAPABILITY_LIST);
> > -     if (ret != sizeof(reg)) {
> > -             RTE_LOG(ERR, EAL,
> > -                     "Cannot read capability pointer from PCI config
> > space!\n");
> > +     cap_offset = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
>
> I notice in some cases we use rte_pci_has_capability_list() to check first,
> then looking for specific cap, in other cases we don't use
> rte_pci_has_capability_list(). Since we define this API, should we always do
> the check?

Hum, I am not sure of what you suggest here.

I tried to do a 1:1 conversion of what existed.
Do you mean there are places where I missed converting some
rte_pci_read_config(PCI_CAPABILITY_LIST) to
rte_pci_has_capability_list()?
If so, could you point at them?

Or are you suggesting to have this check as part of rte_pci_find_capability() ?


I'll respin a v3 soon (to fix the nasty issue you pointed out below).
A v4 may be needed depending on your replies to above questions.


>
>
> > +     if (cap_offset < 0)
> >               return -1;
> > -     }
> >
> > -     /* we need first byte */
> > -     cap_offset = reg & 0xFF;
> > +     if (cap_offset != 0) {
> > +             uint16_t flags;
> > +             uint32_t reg;
> >
> > -     while (cap_offset) {
> > -
> > -             /* read PCI capability ID */
> > -             ret = rte_pci_read_config(dev, &reg, sizeof(reg), cap_offset);
> > -             if (ret != sizeof(reg)) {
> > +             /* table offset resides in the next 4 bytes */
> > +             if (rte_pci_read_config(dev, &reg, sizeof(reg), cap_offset + 4)
> > < 0) {
> >                       RTE_LOG(ERR, EAL,
> > -                             "Cannot read capability ID from PCI config
> > space!\n");
> > +                             "Cannot read MSIX table from PCI config space!\n");
> >                       return -1;
> >               }
> >
> > -             /* we need first byte */
> > -             cap_id = reg & 0xFF;
> > -
> > -             /* if we haven't reached MSI-X, check next capability */
> > -             if (cap_id != PCI_CAP_ID_MSIX) {
> > -                     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > cap_offset);
> > -                     if (ret != sizeof(reg)) {
> > -                             RTE_LOG(ERR, EAL,
> > -                                     "Cannot read capability pointer from PCI
> > config space!\n");
> > -                             return -1;
> > -                     }
> > -
> > -                     /* we need second byte */
> > -                     cap_offset = (reg & 0xFF00) >> 8;
> > -
> > -                     continue;
> > +             if (rte_pci_read_config(dev, &flags, sizeof(flags), cap_offset
> > + 2) < 0) {
> > +                     RTE_LOG(ERR, EAL,
> > +                             "Cannot read MSIX flags from PCI config space!\n");
> > +                     return -1;
> >               }
> > -             /* else, read table offset */
> > -             else {
> > -                     /* table offset resides in the next 4 bytes */
> > -                     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > cap_offset + 4);
> > -                     if (ret != sizeof(reg)) {
> > -                             RTE_LOG(ERR, EAL,
> > -                                     "Cannot read table offset from PCI config
> > space!\n");
> > -                             return -1;
> > -                     }
> > -
> > -                     ret = rte_pci_read_config(dev, &flags, sizeof(flags),
> > cap_offset + 2);
> > -                     if (ret != sizeof(flags)) {
> > -                             RTE_LOG(ERR, EAL,
> > -                                     "Cannot read table flags from PCI config
> > space!\n");
> > -                             return -1;
> > -                     }
> > -
> > -                     msix_table->bar_index = reg & RTE_PCI_MSIX_TABLE_BIR;
> > -                     msix_table->offset = reg & RTE_PCI_MSIX_TABLE_OFFSET;
> > -                     msix_table->size =
> > -                             16 * (1 + (flags & RTE_PCI_MSIX_FLAGS_QSIZE));
> >
> > -                     return 0;
> > -             }
> > +             msix_table->bar_index = reg & RTE_PCI_MSIX_TABLE_BIR;
> > +             msix_table->offset = reg & RTE_PCI_MSIX_TABLE_OFFSET;
> > +             msix_table->size = 16 * (1 + (flags &
> > RTE_PCI_MSIX_FLAGS_QSIZE));
> >       }
> > +
> >       return 0;
> >  }
> >
> > diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> > index 382b0b8946..52272617eb 100644
> > --- a/drivers/bus/pci/pci_common.c
> > +++ b/drivers/bus/pci/pci_common.c
> > @@ -813,6 +813,51 @@ rte_pci_get_iommu_class(void)
> >       return iova_mode;
> >  }
> >
> > +bool
> > +rte_pci_has_capability_list(const struct rte_pci_device *dev)
> > +{
> > +     uint16_t status;
> > +
> > +     if (rte_pci_read_config(dev, &status, sizeof(status),
> > RTE_PCI_STATUS) != sizeof(status))
> > +             return false;
> > +
> > +     return (status & RTE_PCI_STATUS_CAP_LIST) != 0;
> > +}
> > +
> > +off_t
> > +rte_pci_find_capability(const struct rte_pci_device *dev, uint8_t cap)
> > +{
> > +     off_t offset;
> > +     uint8_t pos;
> > +     int ttl;
> > +
> > +     offset = RTE_PCI_CAPABILITY_LIST;
> > +     ttl = (RTE_PCI_CFG_SPACE_SIZE - RTE_PCI_STD_HEADER_SIZEOF) /
> > RTE_PCI_CAP_SIZEOF;
> > +
> > +     if (rte_pci_read_config(dev, &pos, sizeof(pos), offset) < 0)
> > +             return -1;
> > +
> > +     while (pos && ttl--) {
> > +             uint16_t ent;
> > +             uint8_t id;
> > +
> > +             offset = pos;
> > +             if (rte_pci_read_config(dev, &ent, sizeof(ent), offset) < 0)
> > +                     return -1;
> > +
> > +             id = ent & 0xff;
> > +             if (id == 0xff)
> > +                     break;
> > +
> > +             if (id == cap)
> > +                     return offset;
> > +
> > +             pos = (ent >> 8);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  off_t
> >  rte_pci_find_ext_capability(const struct rte_pci_device *dev, uint32_t
> > cap)
> >  {
> > diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> > index 75d0030eae..1ed33dbf3d 100644
> > --- a/drivers/bus/pci/rte_bus_pci.h
> > +++ b/drivers/bus/pci/rte_bus_pci.h
> > @@ -68,6 +68,37 @@ void rte_pci_unmap_device(struct rte_pci_device *dev);
> >   */
> >  void rte_pci_dump(FILE *f);
> >
> > +/**
> > + * Check whether this device has a PCI capability list.
> > + *
> > + *  @param dev
> > + *    A pointer to rte_pci_device structure.
> > + *
> > + *  @return
> > + *    true/false
> > + */
> > +__rte_experimental
> > +bool rte_pci_has_capability_list(const struct rte_pci_device *dev);
> > +
> > +/**
> > + * Find device's PCI capability.
> > + *
> > + *  @param dev
> > + *    A pointer to rte_pci_device structure.
> > + *
> > + *  @param cap
> > + *    Capability to be found, which can be any from
> > + *    RTE_PCI_CAP_ID_*, defined in librte_pci.
> > + *
> > + *  @return
> > + *  > 0: The offset of the next matching capability structure
> > + *       within the device's PCI configuration space.
> > + *  < 0: An error in PCI config space read.
> > + *  = 0: Device does not support it.
> > + */
> > +__rte_experimental
> > +off_t rte_pci_find_capability(const struct rte_pci_device *dev, uint8_t
> > cap);
> > +
> >  /**
> >   * Find device's extended PCI capability.
> >   *
> > diff --git a/drivers/bus/pci/version.map b/drivers/bus/pci/version.map
> > index a0000f7938..2674f30235 100644
> > --- a/drivers/bus/pci/version.map
> > +++ b/drivers/bus/pci/version.map
> > @@ -25,6 +25,10 @@ EXPERIMENTAL {
> >       # added in 23.07
> >       rte_pci_mmio_read;
> >       rte_pci_mmio_write;
> > +
> > +     # added in 23.11
> > +     rte_pci_find_capability;
> > +     rte_pci_has_capability_list;
> >  };
> >
> >  INTERNAL {
> > diff --git a/drivers/crypto/virtio/virtio_pci.c
> > b/drivers/crypto/virtio/virtio_pci.c
> > index 95a43c8801..abc52b4701 100644
> > --- a/drivers/crypto/virtio/virtio_pci.c
> > +++ b/drivers/crypto/virtio/virtio_pci.c
> > @@ -19,7 +19,6 @@
> >   * we can't simply include that header here, as there is no such
> >   * file for non-Linux platform.
> >   */
> > -#define PCI_CAPABILITY_LIST  0x34
> >  #define PCI_CAP_ID_VNDR              0x09
> >  #define PCI_CAP_ID_MSIX              0x11
> >
> > @@ -343,8 +342,9 @@ get_cfg_addr(struct rte_pci_device *dev, struct
> > virtio_pci_cap *cap)
> >  static int
> >  virtio_read_caps(struct rte_pci_device *dev, struct virtio_crypto_hw *hw)
> >  {
> > -     uint8_t pos;
> >       struct virtio_pci_cap cap;
> > +     uint16_t flags;
> > +     off_t pos;
> >       int ret;
> >
> >       if (rte_pci_map_device(dev)) {
> > @@ -352,44 +352,26 @@ virtio_read_caps(struct rte_pci_device *dev, struct
> > virtio_crypto_hw *hw)
> >               return -1;
> >       }
> >
> > -     ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
> > -     if (ret < 0) {
> > -             VIRTIO_CRYPTO_INIT_LOG_DBG("failed to read pci capability
> > list");
> > -             return -1;
> > +     /*
> > +      * Transitional devices would also have this capability,
> > +      * that's why we also check if msix is enabled.
> > +      */
> > +     pos = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
> > +     if (pos > 0 && rte_pci_read_config(dev, &flags, sizeof(flags),
> > +                     pos + 2) == sizeof(flags)) {
> > +             if (flags & PCI_MSIX_ENABLE)
> > +                     hw->use_msix = VIRTIO_MSIX_ENABLED;
> > +             else
> > +                     hw->use_msix = VIRTIO_MSIX_DISABLED;
> > +     } else {
> > +             hw->use_msix = VIRTIO_MSIX_NONE;
> >       }
> >
> > -     while (pos) {
> > -             ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos);
> > -             if (ret < 0) {
> > -                     VIRTIO_CRYPTO_INIT_LOG_ERR(
> > -                             "failed to read pci cap at pos: %x", pos);
> > -                     break;
> > -             }
> > -
> > -             if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
> > -                     /* Transitional devices would also have this capability,
> > -                      * that's why we also check if msix is enabled.
> > -                      * 1st byte is cap ID; 2nd byte is the position of next
> > -                      * cap; next two bytes are the flags.
> > -                      */
> > -                     uint16_t flags = ((uint16_t *)&cap)[1];
> > -
> > -                     if (flags & PCI_MSIX_ENABLE)
> > -                             hw->use_msix = VIRTIO_MSIX_ENABLED;
> > -                     else
> > -                             hw->use_msix = VIRTIO_MSIX_DISABLED;
> > -             }
> > -
> > -             if (cap.cap_vndr != PCI_CAP_ID_VNDR) {
> > -                     VIRTIO_CRYPTO_INIT_LOG_DBG(
> > -                             "[%2x] skipping non VNDR cap id: %02x",
> > -                             pos, cap.cap_vndr);
> > -                     goto next;
> > -             }
> > -
> > +     pos = rte_pci_find_capability(dev, PCI_CAP_ID_VNDR);
>
> The logic of vendor cap init seems incorrect. Virtio devices have multiple
> Vendor cap (different cfg type). But now the logic seems to only init the first
> one.

Indeed, good catch!
It is sad that the CI did not catch this regression in virtio pmd init :-(.

I'll add a find_next_capability helper for v3.



>
> > +     if (pos > 0 && rte_pci_read_config(dev, &cap, sizeof(cap), pos) ==
> > sizeof(cap)) {
> >               VIRTIO_CRYPTO_INIT_LOG_DBG(
> >                       "[%2x] cfg type: %u, bar: %u, offset: %04x, len: %u",
> > -                     pos, cap.cfg_type, cap.bar, cap.offset, cap.length);
> > +                     (unsigned int)pos, cap.cfg_type, cap.bar, cap.offset,
> > cap.length);
> >
> >               switch (cap.cfg_type) {
> >               case VIRTIO_PCI_CAP_COMMON_CFG:
> > @@ -411,9 +393,6 @@ virtio_read_caps(struct rte_pci_device *dev, struct
> > virtio_crypto_hw *hw)
> >                       hw->isr = get_cfg_addr(dev, &cap);
> >                       break;
> >               }
> > -
> > -next:
> > -             pos = cap.cap_next;
> >       }
>
> ...
  

Hi David,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, September 14, 2023 8:29 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; ferruh.yigit@amd.com;
> nipun.gupta@amd.com; Richardson, Bruce <bruce.richardson@intel.com>;
> Burakov, Anatoly <anatoly.burakov@intel.com>; Jay Zhou
> <jianjay.zhou@huawei.com>; McDaniel, Timothy <timothy.mcdaniel@intel.com>;
> Julien Aube <julien_dpdk@jaube.fr>; Rahul Lakkireddy
> <rahul.lakkireddy@chelsio.com>; Guo, Junfeng <junfeng.guo@intel.com>;
> Jeroen de Borst <jeroendb@google.com>; Rushil Gupta <rushilg@google.com>;
> Joshua Washington <joshwash@google.com>; Dongdong Liu
> <liudongdong3@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>; Gaetan
> Rivet <grive@u256.net>
> Subject: Re: [PATCH v2 04/15] bus/pci: find PCI capability
> 
> Hello Chenbo,
> 
> On Thu, Sep 7, 2023 at 2:43 PM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > > Introduce two helpers so that drivers stop reinventing the wheel when
> it
> > > comes to finding capabilities in a device PCI configuration space.
> > > Use it in existing drivers.
> > >
> > > Note:
> > > - base/ drivers code is left untouched, only some wrappers in cxgbe
> > >   are touched,
> > > - bnx2x maintained a per device cache of capabilities, this code has
> been
> > >   reworked to only cache the capabilities used in this driver,
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > > Changes since v1:
> > > - updated commitlog,
> > > - separated VFIO changes for using standard PCI helper in a separate
> > >   patch,
> > > - marked new experimental symbols with current version,
> > > - reordered defines in rte_pci.h,
> > >
> > > ---
> > >  drivers/bus/pci/linux/pci_vfio.c   |  74 ++++--------------
> > >  drivers/bus/pci/pci_common.c       |  45 +++++++++++
> > >  drivers/bus/pci/rte_bus_pci.h      |  31 ++++++++
> > >  drivers/bus/pci/version.map        |   4 +
> > >  drivers/crypto/virtio/virtio_pci.c |  57 +++++---------
> > >  drivers/event/dlb2/pf/dlb2_main.c  |  42 +---------
> > >  drivers/net/bnx2x/bnx2x.c          |  41 +++++-----
> > >  drivers/net/cxgbe/base/adapter.h   |  28 +------
> > >  drivers/net/gve/gve_ethdev.c       |  46 ++---------
> > >  drivers/net/gve/gve_ethdev.h       |   4 -
> > >  drivers/net/hns3/hns3_ethdev_vf.c  |  79 +++----------------
> > >  drivers/net/virtio/virtio_pci.c    | 121 +++++-----------------------
> -
> > >  lib/pci/rte_pci.h                  |  11 +++
> > >  13 files changed, 186 insertions(+), 397 deletions(-)
> > >
> > > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > > b/drivers/bus/pci/linux/pci_vfio.c
> > > index 958f8b3b52..614ed5d696 100644
> > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > @@ -110,74 +110,34 @@ static int
> > >  pci_vfio_get_msix_bar(const struct rte_pci_device *dev,
> > >       struct pci_msix_table *msix_table)
> > >  {
> > > -     int ret;
> > > -     uint32_t reg;
> > > -     uint16_t flags;
> > > -     uint8_t cap_id, cap_offset;
> > > +     off_t cap_offset;
> > >
> > > -     /* read PCI capability pointer from config space */
> > > -     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > > PCI_CAPABILITY_LIST);
> > > -     if (ret != sizeof(reg)) {
> > > -             RTE_LOG(ERR, EAL,
> > > -                     "Cannot read capability pointer from PCI config
> > > space!\n");
> > > +     cap_offset = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
> >
> > I notice in some cases we use rte_pci_has_capability_list() to check
> first,
> > then looking for specific cap, in other cases we don't use
> > rte_pci_has_capability_list(). Since we define this API, should we
> always do
> > the check?
> 
> Hum, I am not sure of what you suggest here.
> 
> I tried to do a 1:1 conversion of what existed.
> Do you mean there are places where I missed converting some
> rte_pci_read_config(PCI_CAPABILITY_LIST) to
> rte_pci_has_capability_list()?
> If so, could you point at them?
> 
> Or are you suggesting to have this check as part of
> rte_pci_find_capability() ?

Your conversion is correct. I was saying about the usage of
rte_pci_has_capability_list/rte_pci_find_capability, logically should we
always call rte_pci_has_capability_list first before find capability?

I don't have strong opinion on this. You can choose to keep the same or
call rte_pci_has_capability_list more.

Thanks!
Chenbo

> 
> 
> I'll respin a v3 soon (to fix the nasty issue you pointed out below).
> A v4 may be needed depending on your replies to above questions.
> 
> 
> >
> >
> > > +     if (cap_offset < 0)
> > >               return -1;
> > > -     }
> > >
> > > -     /* we need first byte */
> > > -     cap_offset = reg & 0xFF;
> > > +     if (cap_offset != 0) {
> > > +             uint16_t flags;
> > > +             uint32_t reg;
> > >
> > > -     while (cap_offset) {
> > > -
> > > -             /* read PCI capability ID */
> > > -             ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> cap_offset);
> > > -             if (ret != sizeof(reg)) {
> > > +             /* table offset resides in the next 4 bytes */
> > > +             if (rte_pci_read_config(dev, &reg, sizeof(reg),
> cap_offset + 4)
> > > < 0) {
> > >                       RTE_LOG(ERR, EAL,
> > > -                             "Cannot read capability ID from PCI
> config
> > > space!\n");
> > > +                             "Cannot read MSIX table from PCI config
> space!\n");
> > >                       return -1;
> > >               }
> > >
> > > -             /* we need first byte */
> > > -             cap_id = reg & 0xFF;
> > > -
> > > -             /* if we haven't reached MSI-X, check next capability */
> > > -             if (cap_id != PCI_CAP_ID_MSIX) {
> > > -                     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > > cap_offset);
> > > -                     if (ret != sizeof(reg)) {
> > > -                             RTE_LOG(ERR, EAL,
> > > -                                     "Cannot read capability pointer
> from PCI
> > > config space!\n");
> > > -                             return -1;
> > > -                     }
> > > -
> > > -                     /* we need second byte */
> > > -                     cap_offset = (reg & 0xFF00) >> 8;
> > > -
> > > -                     continue;
> > > +             if (rte_pci_read_config(dev, &flags, sizeof(flags),
> cap_offset
> > > + 2) < 0) {
> > > +                     RTE_LOG(ERR, EAL,
> > > +                             "Cannot read MSIX flags from PCI config
> space!\n");
> > > +                     return -1;
> > >               }
> > > -             /* else, read table offset */
> > > -             else {
> > > -                     /* table offset resides in the next 4 bytes */
> > > -                     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > > cap_offset + 4);
> > > -                     if (ret != sizeof(reg)) {
> > > -                             RTE_LOG(ERR, EAL,
> > > -                                     "Cannot read table offset from
> PCI config
> > > space!\n");
> > > -                             return -1;
> > > -                     }
> > > -
> > > -                     ret = rte_pci_read_config(dev, &flags,
> sizeof(flags),
> > > cap_offset + 2);
> > > -                     if (ret != sizeof(flags)) {
> > > -                             RTE_LOG(ERR, EAL,
> > > -                                     "Cannot read table flags from
> PCI config
> > > space!\n");
> > > -                             return -1;
> > > -                     }
> > > -
> > > -                     msix_table->bar_index = reg &
> RTE_PCI_MSIX_TABLE_BIR;
> > > -                     msix_table->offset = reg &
> RTE_PCI_MSIX_TABLE_OFFSET;
> > > -                     msix_table->size =
> > > -                             16 * (1 + (flags &
> RTE_PCI_MSIX_FLAGS_QSIZE));
> > >
> > > -                     return 0;
> > > -             }
> > > +             msix_table->bar_index = reg & RTE_PCI_MSIX_TABLE_BIR;
> > > +             msix_table->offset = reg & RTE_PCI_MSIX_TABLE_OFFSET;
> > > +             msix_table->size = 16 * (1 + (flags &
> > > RTE_PCI_MSIX_FLAGS_QSIZE));
> > >       }
> > > +
> > >       return 0;
> > >  }
> > >
> > > diff --git a/drivers/bus/pci/pci_common.c
> b/drivers/bus/pci/pci_common.c
> > > index 382b0b8946..52272617eb 100644
> > > --- a/drivers/bus/pci/pci_common.c
> > > +++ b/drivers/bus/pci/pci_common.c
> > > @@ -813,6 +813,51 @@ rte_pci_get_iommu_class(void)
> > >       return iova_mode;
> > >  }
> > >
> > > +bool
> > > +rte_pci_has_capability_list(const struct rte_pci_device *dev)
> > > +{
> > > +     uint16_t status;
> > > +
> > > +     if (rte_pci_read_config(dev, &status, sizeof(status),
> > > RTE_PCI_STATUS) != sizeof(status))
> > > +             return false;
> > > +
> > > +     return (status & RTE_PCI_STATUS_CAP_LIST) != 0;
> > > +}
> > > +
> > > +off_t
> > > +rte_pci_find_capability(const struct rte_pci_device *dev, uint8_t cap)
> > > +{
> > > +     off_t offset;
> > > +     uint8_t pos;
> > > +     int ttl;
> > > +
> > > +     offset = RTE_PCI_CAPABILITY_LIST;
> > > +     ttl = (RTE_PCI_CFG_SPACE_SIZE - RTE_PCI_STD_HEADER_SIZEOF) /
> > > RTE_PCI_CAP_SIZEOF;
> > > +
> > > +     if (rte_pci_read_config(dev, &pos, sizeof(pos), offset) < 0)
> > > +             return -1;
> > > +
> > > +     while (pos && ttl--) {
> > > +             uint16_t ent;
> > > +             uint8_t id;
> > > +
> > > +             offset = pos;
> > > +             if (rte_pci_read_config(dev, &ent, sizeof(ent), offset)
> < 0)
> > > +                     return -1;
> > > +
> > > +             id = ent & 0xff;
> > > +             if (id == 0xff)
> > > +                     break;
> > > +
> > > +             if (id == cap)
> > > +                     return offset;
> > > +
> > > +             pos = (ent >> 8);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  off_t
> > >  rte_pci_find_ext_capability(const struct rte_pci_device *dev,
> uint32_t
> > > cap)
> > >  {
> > > diff --git a/drivers/bus/pci/rte_bus_pci.h
> b/drivers/bus/pci/rte_bus_pci.h
> > > index 75d0030eae..1ed33dbf3d 100644
> > > --- a/drivers/bus/pci/rte_bus_pci.h
> > > +++ b/drivers/bus/pci/rte_bus_pci.h
> > > @@ -68,6 +68,37 @@ void rte_pci_unmap_device(struct rte_pci_device
> *dev);
> > >   */
> > >  void rte_pci_dump(FILE *f);
> > >
> > > +/**
> > > + * Check whether this device has a PCI capability list.
> > > + *
> > > + *  @param dev
> > > + *    A pointer to rte_pci_device structure.
> > > + *
> > > + *  @return
> > > + *    true/false
> > > + */
> > > +__rte_experimental
> > > +bool rte_pci_has_capability_list(const struct rte_pci_device *dev);
> > > +
> > > +/**
> > > + * Find device's PCI capability.
> > > + *
> > > + *  @param dev
> > > + *    A pointer to rte_pci_device structure.
> > > + *
> > > + *  @param cap
> > > + *    Capability to be found, which can be any from
> > > + *    RTE_PCI_CAP_ID_*, defined in librte_pci.
> > > + *
> > > + *  @return
> > > + *  > 0: The offset of the next matching capability structure
> > > + *       within the device's PCI configuration space.
> > > + *  < 0: An error in PCI config space read.
> > > + *  = 0: Device does not support it.
> > > + */
> > > +__rte_experimental
> > > +off_t rte_pci_find_capability(const struct rte_pci_device *dev,
> uint8_t
> > > cap);
> > > +
> > >  /**
> > >   * Find device's extended PCI capability.
> > >   *
> > > diff --git a/drivers/bus/pci/version.map b/drivers/bus/pci/version.map
> > > index a0000f7938..2674f30235 100644
> > > --- a/drivers/bus/pci/version.map
> > > +++ b/drivers/bus/pci/version.map
> > > @@ -25,6 +25,10 @@ EXPERIMENTAL {
> > >       # added in 23.07
> > >       rte_pci_mmio_read;
> > >       rte_pci_mmio_write;
> > > +
> > > +     # added in 23.11
> > > +     rte_pci_find_capability;
> > > +     rte_pci_has_capability_list;
> > >  };
> > >
> > >  INTERNAL {
> > > diff --git a/drivers/crypto/virtio/virtio_pci.c
> > > b/drivers/crypto/virtio/virtio_pci.c
> > > index 95a43c8801..abc52b4701 100644
> > > --- a/drivers/crypto/virtio/virtio_pci.c
> > > +++ b/drivers/crypto/virtio/virtio_pci.c
> > > @@ -19,7 +19,6 @@
> > >   * we can't simply include that header here, as there is no such
> > >   * file for non-Linux platform.
> > >   */
> > > -#define PCI_CAPABILITY_LIST  0x34
> > >  #define PCI_CAP_ID_VNDR              0x09
> > >  #define PCI_CAP_ID_MSIX              0x11
> > >
> > > @@ -343,8 +342,9 @@ get_cfg_addr(struct rte_pci_device *dev, struct
> > > virtio_pci_cap *cap)
> > >  static int
> > >  virtio_read_caps(struct rte_pci_device *dev, struct virtio_crypto_hw
> *hw)
> > >  {
> > > -     uint8_t pos;
> > >       struct virtio_pci_cap cap;
> > > +     uint16_t flags;
> > > +     off_t pos;
> > >       int ret;
> > >
> > >       if (rte_pci_map_device(dev)) {
> > > @@ -352,44 +352,26 @@ virtio_read_caps(struct rte_pci_device *dev,
> struct
> > > virtio_crypto_hw *hw)
> > >               return -1;
> > >       }
> > >
> > > -     ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST);
> > > -     if (ret < 0) {
> > > -             VIRTIO_CRYPTO_INIT_LOG_DBG("failed to read pci
> capability
> > > list");
> > > -             return -1;
> > > +     /*
> > > +      * Transitional devices would also have this capability,
> > > +      * that's why we also check if msix is enabled.
> > > +      */
> > > +     pos = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
> > > +     if (pos > 0 && rte_pci_read_config(dev, &flags, sizeof(flags),
> > > +                     pos + 2) == sizeof(flags)) {
> > > +             if (flags & PCI_MSIX_ENABLE)
> > > +                     hw->use_msix = VIRTIO_MSIX_ENABLED;
> > > +             else
> > > +                     hw->use_msix = VIRTIO_MSIX_DISABLED;
> > > +     } else {
> > > +             hw->use_msix = VIRTIO_MSIX_NONE;
> > >       }
> > >
> > > -     while (pos) {
> > > -             ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos);
> > > -             if (ret < 0) {
> > > -                     VIRTIO_CRYPTO_INIT_LOG_ERR(
> > > -                             "failed to read pci cap at pos: %x",
> pos);
> > > -                     break;
> > > -             }
> > > -
> > > -             if (cap.cap_vndr == PCI_CAP_ID_MSIX) {
> > > -                     /* Transitional devices would also have this
> capability,
> > > -                      * that's why we also check if msix is enabled.
> > > -                      * 1st byte is cap ID; 2nd byte is the position
> of next
> > > -                      * cap; next two bytes are the flags.
> > > -                      */
> > > -                     uint16_t flags = ((uint16_t *)&cap)[1];
> > > -
> > > -                     if (flags & PCI_MSIX_ENABLE)
> > > -                             hw->use_msix = VIRTIO_MSIX_ENABLED;
> > > -                     else
> > > -                             hw->use_msix = VIRTIO_MSIX_DISABLED;
> > > -             }
> > > -
> > > -             if (cap.cap_vndr != PCI_CAP_ID_VNDR) {
> > > -                     VIRTIO_CRYPTO_INIT_LOG_DBG(
> > > -                             "[%2x] skipping non VNDR cap id: %02x",
> > > -                             pos, cap.cap_vndr);
> > > -                     goto next;
> > > -             }
> > > -
> > > +     pos = rte_pci_find_capability(dev, PCI_CAP_ID_VNDR);
> >
> > The logic of vendor cap init seems incorrect. Virtio devices have
> multiple
> > Vendor cap (different cfg type). But now the logic seems to only init
> the first
> > one.
> 
> Indeed, good catch!
> It is sad that the CI did not catch this regression in virtio pmd init :-(.
> 
> I'll add a find_next_capability helper for v3.
> 
> 
> 
> >
> > > +     if (pos > 0 && rte_pci_read_config(dev, &cap, sizeof(cap), pos)
> ==
> > > sizeof(cap)) {
> > >               VIRTIO_CRYPTO_INIT_LOG_DBG(
> > >                       "[%2x] cfg type: %u, bar: %u, offset: %04x,
> len: %u",
> > > -                     pos, cap.cfg_type, cap.bar, cap.offset,
> cap.length);
> > > +                     (unsigned int)pos, cap.cfg_type, cap.bar,
> cap.offset,
> > > cap.length);
> > >
> > >               switch (cap.cfg_type) {
> > >               case VIRTIO_PCI_CAP_COMMON_CFG:
> > > @@ -411,9 +393,6 @@ virtio_read_caps(struct rte_pci_device *dev,
> struct
> > > virtio_crypto_hw *hw)
> > >                       hw->isr = get_cfg_addr(dev, &cap);
> > >                       break;
> > >               }
> > > -
> > > -next:
> > > -             pos = cap.cap_next;
> > >       }
> >
> > ...
> 
> 
> --
> David Marchand
  

On Tue, Sep 19, 2023 at 4:19 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
>
> Hi David,
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, September 14, 2023 8:29 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net; ferruh.yigit@amd.com;
> > nipun.gupta@amd.com; Richardson, Bruce <bruce.richardson@intel.com>;
> > Burakov, Anatoly <anatoly.burakov@intel.com>; Jay Zhou
> > <jianjay.zhou@huawei.com>; McDaniel, Timothy <timothy.mcdaniel@intel.com>;
> > Julien Aube <julien_dpdk@jaube.fr>; Rahul Lakkireddy
> > <rahul.lakkireddy@chelsio.com>; Guo, Junfeng <junfeng.guo@intel.com>;
> > Jeroen de Borst <jeroendb@google.com>; Rushil Gupta <rushilg@google.com>;
> > Joshua Washington <joshwash@google.com>; Dongdong Liu
> > <liudongdong3@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>; Gaetan
> > Rivet <grive@u256.net>
> > Subject: Re: [PATCH v2 04/15] bus/pci: find PCI capability
> >
> > Hello Chenbo,
> >
> > On Thu, Sep 7, 2023 at 2:43 PM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > > > Introduce two helpers so that drivers stop reinventing the wheel when
> > it
> > > > comes to finding capabilities in a device PCI configuration space.
> > > > Use it in existing drivers.
> > > >
> > > > Note:
> > > > - base/ drivers code is left untouched, only some wrappers in cxgbe
> > > >   are touched,
> > > > - bnx2x maintained a per device cache of capabilities, this code has
> > been
> > > >   reworked to only cache the capabilities used in this driver,
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > ---
> > > > Changes since v1:
> > > > - updated commitlog,
> > > > - separated VFIO changes for using standard PCI helper in a separate
> > > >   patch,
> > > > - marked new experimental symbols with current version,
> > > > - reordered defines in rte_pci.h,
> > > >
> > > > ---
> > > >  drivers/bus/pci/linux/pci_vfio.c   |  74 ++++--------------
> > > >  drivers/bus/pci/pci_common.c       |  45 +++++++++++
> > > >  drivers/bus/pci/rte_bus_pci.h      |  31 ++++++++
> > > >  drivers/bus/pci/version.map        |   4 +
> > > >  drivers/crypto/virtio/virtio_pci.c |  57 +++++---------
> > > >  drivers/event/dlb2/pf/dlb2_main.c  |  42 +---------
> > > >  drivers/net/bnx2x/bnx2x.c          |  41 +++++-----
> > > >  drivers/net/cxgbe/base/adapter.h   |  28 +------
> > > >  drivers/net/gve/gve_ethdev.c       |  46 ++---------
> > > >  drivers/net/gve/gve_ethdev.h       |   4 -
> > > >  drivers/net/hns3/hns3_ethdev_vf.c  |  79 +++----------------
> > > >  drivers/net/virtio/virtio_pci.c    | 121 +++++-----------------------
> > -
> > > >  lib/pci/rte_pci.h                  |  11 +++
> > > >  13 files changed, 186 insertions(+), 397 deletions(-)
> > > >
> > > > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > > > b/drivers/bus/pci/linux/pci_vfio.c
> > > > index 958f8b3b52..614ed5d696 100644
> > > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > > @@ -110,74 +110,34 @@ static int
> > > >  pci_vfio_get_msix_bar(const struct rte_pci_device *dev,
> > > >       struct pci_msix_table *msix_table)
> > > >  {
> > > > -     int ret;
> > > > -     uint32_t reg;
> > > > -     uint16_t flags;
> > > > -     uint8_t cap_id, cap_offset;
> > > > +     off_t cap_offset;
> > > >
> > > > -     /* read PCI capability pointer from config space */
> > > > -     ret = rte_pci_read_config(dev, &reg, sizeof(reg),
> > > > PCI_CAPABILITY_LIST);
> > > > -     if (ret != sizeof(reg)) {
> > > > -             RTE_LOG(ERR, EAL,
> > > > -                     "Cannot read capability pointer from PCI config
> > > > space!\n");
> > > > +     cap_offset = rte_pci_find_capability(dev, PCI_CAP_ID_MSIX);
> > >
> > > I notice in some cases we use rte_pci_has_capability_list() to check
> > first,
> > > then looking for specific cap, in other cases we don't use
> > > rte_pci_has_capability_list(). Since we define this API, should we
> > always do
> > > the check?
> >
> > Hum, I am not sure of what you suggest here.
> >
> > I tried to do a 1:1 conversion of what existed.
> > Do you mean there are places where I missed converting some
> > rte_pci_read_config(PCI_CAPABILITY_LIST) to
> > rte_pci_has_capability_list()?
> > If so, could you point at them?
> >
> > Or are you suggesting to have this check as part of
> > rte_pci_find_capability() ?
>
> Your conversion is correct. I was saying about the usage of
> rte_pci_has_capability_list/rte_pci_find_capability, logically should we
> always call rte_pci_has_capability_list first before find capability?
>
> I don't have strong opinion on this. You can choose to keep the same or
> call rte_pci_has_capability_list more.

I prefer to keep it as is (the series is already big enough).
We can still add more checks in the future.

Thanks Chenbo.