From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 005CA42597; Thu, 14 Sep 2023 14:29:44 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7243840293; Thu, 14 Sep 2023 14:29:44 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 7960640289 for ; Thu, 14 Sep 2023 14:29:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1694694582; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=jsOlAcr1DZmFTPyFOdbSkE6YtbM/ulQ6jOlB+XisCKU=; b=UJ5uuG+Y61t+Ojmv+mSkocb/7oVQTSnESwn7/mnlhU+ghFznwiF+CtpWhJU5Rn0chkACTN yrVpPuVTZtcjqg0KPV4HxoBRcKjjNI32p7PPku0kecLdNxQINMgbOgK0suOZQYxabWbi8p /sMgHW5HaPg0nGzVJad9O7oYMFYs1M0= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-300-fwNZNxb-ORaFzSPO_YkB_Q-1; Thu, 14 Sep 2023 08:29:41 -0400 X-MC-Unique: fwNZNxb-ORaFzSPO_YkB_Q-1 Received: by mail-lj1-f197.google.com with SMTP id 38308e7fff4ca-2b8405aace3so10954511fa.3 for ; Thu, 14 Sep 2023 05:29:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694694580; x=1695299380; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jsOlAcr1DZmFTPyFOdbSkE6YtbM/ulQ6jOlB+XisCKU=; b=hA8z1p8MxiQuT13JtiEExxLb/KGvwa9fL8HDBkSN1JUv4AbwKQ6PLehZxHnX0ZWLBE NoYb9AML2RVcz7eMS2M8mtGAOVli7TR7/gkhEmyOsgZ/J8kd11rQ63ejaSNIAL9p+iHf R/Ql9ZSEK6RUk7FI0irCzvsmadSBMqp2i0VbdmCGRWVcFIXAkf0q2yqHZ8JaeBJXP3Oe OTxQl0HxmSU6GvZx38wy2X+HNJ4BCR7k/y2GMqeLFN2j8VsyeYidJNu4xtvQTm8lqbPJ FDWs5F6VvBDZJfgnIB2XJnxn5ZnExyW+ZV5N8kWXHybofq2b6u3xc5UYaYQES7LUXcjP 80FA== X-Gm-Message-State: AOJu0YzxjlXNXK7MhpNBQbWsqnFfyLx3to+q3p9Au61j3hpaD+t5x7r+ vu2wUG2jyk+pW6DB0GTZjvJsJezEjoOh4Lg3tpALdGz1CYqE4TYzKzFOfQp80gCSkKzG6i7IXWc atOYkCZCIhS9qLYVkS30= X-Received: by 2002:a05:651c:1257:b0:2bf:bffc:483d with SMTP id h23-20020a05651c125700b002bfbffc483dmr3649796ljh.42.1694694579801; Thu, 14 Sep 2023 05:29:39 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFHxpXSlg82vblSd8zzxXD/Y4T8UODui2tYcIAY5j/t1f+Vpazz9Xejs6MpxfVduyRSOg3+WddQClIrZ5HKD/4= X-Received: by 2002:a05:651c:1257:b0:2bf:bffc:483d with SMTP id h23-20020a05651c125700b002bfbffc483dmr3649765ljh.42.1694694579356; Thu, 14 Sep 2023 05:29:39 -0700 (PDT) MIME-Version: 1.0 References: <20230803075038.307012-1-david.marchand@redhat.com> <20230821113549.3191921-1-david.marchand@redhat.com> <20230821113549.3191921-5-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Thu, 14 Sep 2023 14:29:27 +0200 Message-ID: Subject: Re: [PATCH v2 04/15] bus/pci: find PCI capability To: "Xia, Chenbo" , Maxime Coquelin Cc: "dev@dpdk.org" , "thomas@monjalon.net" , "ferruh.yigit@amd.com" , "nipun.gupta@amd.com" , "Richardson, Bruce" , "Burakov, Anatoly" , Jay Zhou , "McDaniel, Timothy" , Julien Aube , Rahul Lakkireddy , "Guo, Junfeng" , Jeroen de Borst , Rushil Gupta , Joshua Washington , Dongdong Liu , Yisen Zhuang , Gaetan Rivet X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hello Chenbo, On Thu, Sep 7, 2023 at 2:43=E2=80=AFPM Xia, Chenbo w= rote: > > Introduce two helpers so that drivers stop reinventing the wheel when i= t > > 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 be= en > > reworked to only cache the capabilities used in this driver, > > > > Signed-off-by: David Marchand > > Acked-by: Bruce Richardson > > --- > > 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 =3D rte_pci_read_config(dev, ®, sizeof(reg), > > PCI_CAPABILITY_LIST); > > - if (ret !=3D sizeof(reg)) { > > - RTE_LOG(ERR, EAL, > > - "Cannot read capability pointer from PCI config > > space!\n"); > > + cap_offset =3D rte_pci_find_capability(dev, PCI_CAP_ID_MSIX); > > I notice in some cases we use rte_pci_has_capability_list() to check firs= t, > 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 =3D reg & 0xFF; > > + if (cap_offset !=3D 0) { > > + uint16_t flags; > > + uint32_t reg; > > > > - while (cap_offset) { > > - > > - /* read PCI capability ID */ > > - ret =3D rte_pci_read_config(dev, ®, sizeof(reg), cap_o= ffset); > > - if (ret !=3D sizeof(reg)) { > > + /* table offset resides in the next 4 bytes */ > > + if (rte_pci_read_config(dev, ®, sizeof(reg), cap_offse= t + 4) > > < 0) { > > RTE_LOG(ERR, EAL, > > - "Cannot read capability ID from PCI confi= g > > space!\n"); > > + "Cannot read MSIX table from PCI config s= pace!\n"); > > return -1; > > } > > > > - /* we need first byte */ > > - cap_id =3D reg & 0xFF; > > - > > - /* if we haven't reached MSI-X, check next capability */ > > - if (cap_id !=3D PCI_CAP_ID_MSIX) { > > - ret =3D rte_pci_read_config(dev, ®, sizeof(reg= ), > > cap_offset); > > - if (ret !=3D sizeof(reg)) { > > - RTE_LOG(ERR, EAL, > > - "Cannot read capability pointer f= rom PCI > > config space!\n"); > > - return -1; > > - } > > - > > - /* we need second byte */ > > - cap_offset =3D (reg & 0xFF00) >> 8; > > - > > - continue; > > + if (rte_pci_read_config(dev, &flags, sizeof(flags), cap_o= ffset > > + 2) < 0) { > > + RTE_LOG(ERR, EAL, > > + "Cannot read MSIX flags from PCI config s= pace!\n"); > > + return -1; > > } > > - /* else, read table offset */ > > - else { > > - /* table offset resides in the next 4 bytes */ > > - ret =3D rte_pci_read_config(dev, ®, sizeof(reg= ), > > cap_offset + 4); > > - if (ret !=3D sizeof(reg)) { > > - RTE_LOG(ERR, EAL, > > - "Cannot read table offset from PC= I config > > space!\n"); > > - return -1; > > - } > > - > > - ret =3D rte_pci_read_config(dev, &flags, sizeof(f= lags), > > cap_offset + 2); > > - if (ret !=3D sizeof(flags)) { > > - RTE_LOG(ERR, EAL, > > - "Cannot read table flags from PCI= config > > space!\n"); > > - return -1; > > - } > > - > > - msix_table->bar_index =3D reg & RTE_PCI_MSIX_TABL= E_BIR; > > - msix_table->offset =3D reg & RTE_PCI_MSIX_TABLE_O= FFSET; > > - msix_table->size =3D > > - 16 * (1 + (flags & RTE_PCI_MSIX_FLAGS_QSI= ZE)); > > > > - return 0; > > - } > > + msix_table->bar_index =3D reg & RTE_PCI_MSIX_TABLE_BIR; > > + msix_table->offset =3D reg & RTE_PCI_MSIX_TABLE_OFFSET; > > + msix_table->size =3D 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) !=3D sizeof(status)) > > + return false; > > + > > + return (status & RTE_PCI_STATUS_CAP_LIST) !=3D 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 =3D RTE_PCI_CAPABILITY_LIST; > > + ttl =3D (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 =3D pos; > > + if (rte_pci_read_config(dev, &ent, sizeof(ent), offset) <= 0) > > + return -1; > > + > > + id =3D ent & 0xff; > > + if (id =3D=3D 0xff) > > + break; > > + > > + if (id =3D=3D cap) > > + return offset; > > + > > + pos =3D (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_pc= i.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. > > + * =3D 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, stru= ct > > virtio_crypto_hw *hw) > > return -1; > > } > > > > - ret =3D 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 =3D rte_pci_find_capability(dev, PCI_CAP_ID_MSIX); > > + if (pos > 0 && rte_pci_read_config(dev, &flags, sizeof(flags), > > + pos + 2) =3D=3D sizeof(flags)) { > > + if (flags & PCI_MSIX_ENABLE) > > + hw->use_msix =3D VIRTIO_MSIX_ENABLED; > > + else > > + hw->use_msix =3D VIRTIO_MSIX_DISABLED; > > + } else { > > + hw->use_msix =3D VIRTIO_MSIX_NONE; > > } > > > > - while (pos) { > > - ret =3D 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 =3D=3D PCI_CAP_ID_MSIX) { > > - /* Transitional devices would also have this capa= bility, > > - * that's why we also check if msix is enabled. > > - * 1st byte is cap ID; 2nd byte is the position o= f next > > - * cap; next two bytes are the flags. > > - */ > > - uint16_t flags =3D ((uint16_t *)&cap)[1]; > > - > > - if (flags & PCI_MSIX_ENABLE) > > - hw->use_msix =3D VIRTIO_MSIX_ENABLED; > > - else > > - hw->use_msix =3D VIRTIO_MSIX_DISABLED; > > - } > > - > > - if (cap.cap_vndr !=3D PCI_CAP_ID_VNDR) { > > - VIRTIO_CRYPTO_INIT_LOG_DBG( > > - "[%2x] skipping non VNDR cap id: %02x", > > - pos, cap.cap_vndr); > > - goto next; > > - } > > - > > + pos =3D rte_pci_find_capability(dev, PCI_CAP_ID_VNDR); > > The logic of vendor cap init seems incorrect. Virtio devices have multipl= e > 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) = =3D=3D > > 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.lengt= h); > > + (unsigned int)pos, cap.cfg_type, cap.bar, cap.off= set, > > 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 =3D get_cfg_addr(dev, &cap); > > break; > > } > > - > > -next: > > - pos =3D cap.cap_next; > > } > > ... --=20 David Marchand