Hello Chenbo,
On Thu, Sep 7, 2023 at 2:43 PM Xia, Chenbo <chenbo....@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.march...@redhat.com>
> > Acked-by: Bruce Richardson <bruce.richard...@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, ®, 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, ®, sizeof(reg), cap_offset);
> > - if (ret != sizeof(reg)) {
> > + /* table offset resides in the next 4 bytes */
> > + if (rte_pci_read_config(dev, ®, 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, ®, 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, ®, 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
