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 E477BA00C4; Fri, 5 Aug 2022 12:49:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C451740C35; Fri, 5 Aug 2022 12:49:07 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 39085400D5 for ; Fri, 5 Aug 2022 12:49:06 +0200 (CEST) Received: from [192.168.1.39] (unknown [188.170.75.116]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 7DCDDA5; Fri, 5 Aug 2022 13:49:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 7DCDDA5 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1659696545; bh=78UsgvyAPsvvsFRf2KMvyZXReEM+H6puoiN2Ld4IGjg=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=PFdubr91fRFPL4aOInYl7YxsUPgjjva7CqQKwtME3EVvO0Qh4eiSDlVxEQOysPnnN WEGeyRav5cbxYwXboqZuOXNJD7phisSiyJyZmbNPk3w9+ImPS1H+4414E0K7p/iXI9 oRC2VkFiyKxtqRElaRHDfRLyAkDh4GZcqsH40bf8= Message-ID: <3ea40ed7-3c0a-de21-df58-51fbf7c59cc9@oktetlabs.ru> Date: Fri, 5 Aug 2022 13:49:04 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v5 01/12] net/nfp: move app specific attributes to own struct Content-Language: en-US To: Chaoyong He , dev@dpdk.org Cc: niklas.soderlund@corigine.com, Heinrich Kuhn References: <1659681155-16525-1-git-send-email-chaoyong.he@corigine.com> <1659681155-16525-2-git-send-email-chaoyong.he@corigine.com> From: Andrew Rybchenko In-Reply-To: <1659681155-16525-2-git-send-email-chaoyong.he@corigine.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 On 8/5/22 09:32, Chaoyong He wrote: > The NFP Card can load different firmware applications. Currently > only the CoreNIC application is supported. This commit makes > needed infrastructure changes in order to support other firmware > applications too. > > Clearer separation is made between the PF device and any application > specific concepts. The PF struct is now generic regardless of the > application loaded. A new struct is also made for the CoreNIC > application. Future additions to support other applications should > also add an applications specific struct. > > Signed-off-by: Chaoyong He > Signed-off-by: Heinrich Kuhn > Reviewed-by: Niklas Söderlund > --- > drivers/net/nfp/nfp_common.h | 33 +++++++- > drivers/net/nfp/nfp_ethdev.c | 196 +++++++++++++++++++++++++++---------------- > 2 files changed, 154 insertions(+), 75 deletions(-) > > diff --git a/drivers/net/nfp/nfp_common.h b/drivers/net/nfp/nfp_common.h > index 6d917e4..2aaf1d6 100644 > --- a/drivers/net/nfp/nfp_common.h > +++ b/drivers/net/nfp/nfp_common.h > @@ -111,6 +111,14 @@ > #include > #include > > +/* Firmware application ID's */ > +enum nfp_app_id { > + NFP_APP_CORE_NIC = 0x1, [snip] > + NFP_APP_BPF_NIC = 0x2, > + NFP_APP_FLOWER_NIC = 0x3, > + NFP_APP_ACTIVE_BUFFER_MGMT_NIC = 0x4, Above three defines are dead in the patch. I think it would make subsequent patches more selfcontained if corresponding defines are added later when they are really required and used. > +}; > + > /* nfp_qcp_ptr - Read or Write Pointer of a queue */ > enum nfp_qcp_ptr { > NFP_QCP_READ_PTR = 0, > @@ -121,8 +129,10 @@ struct nfp_pf_dev { > /* Backpointer to associated pci device */ > struct rte_pci_device *pci_dev; > > - /* Array of physical ports belonging to this PF */ > - struct nfp_net_hw *ports[NFP_MAX_PHYPORTS]; > + enum nfp_app_id app_id; > + > + /* Pointer to the app running on the PF */ > + void *app_priv; > > /* Current values for control */ > uint32_t ctrl; > @@ -151,8 +161,6 @@ struct nfp_pf_dev { > struct nfp_cpp_area *msix_area; > > uint8_t *hw_queues; > - uint8_t total_phyports; > - bool multiport; > > union eth_table_entry *eth_table; > > @@ -161,6 +169,20 @@ struct nfp_pf_dev { > uint32_t nfp_cpp_service_id; > }; > > +struct nfp_app_nic { > + /* Backpointer to the PF device */ > + struct nfp_pf_dev *pf_dev; > + > + /* > + * Array of physical ports belonging to the this CoreNIC app > + * This is really a list of vNIC's. One for each physical port > + */ > + struct nfp_net_hw *ports[NFP_MAX_PHYPORTS]; > + > + bool multiport; > + uint8_t total_phyports; > +}; > + > struct nfp_net_hw { > /* Backpointer to the PF this port belongs to */ > struct nfp_pf_dev *pf_dev; > @@ -424,6 +446,9 @@ int nfp_net_rss_hash_conf_get(struct rte_eth_dev *dev, > #define NFP_NET_DEV_PRIVATE_TO_PF(dev_priv)\ > (((struct nfp_net_hw *)dev_priv)->pf_dev) > > +#define NFP_APP_PRIV_TO_APP_NIC(app_priv)\ > + ((struct nfp_app_nic *)app_priv) > + Wouldn't it be better if tiny function is used instead. It should accept struct nfp_pf_dev pointer as an input argument. It would allow to validate that pf_dev->app_id is NFP_APP_CORE_NIC and make code more robust. > #endif /* _NFP_COMMON_H_ */ > /* > * Local variables: > diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c > index 5cdd34e..3c4b0ac 100644 > --- a/drivers/net/nfp/nfp_ethdev.c > +++ b/drivers/net/nfp/nfp_ethdev.c > @@ -39,15 +39,15 @@ > #include "nfp_cpp_bridge.h" > > static int > -nfp_net_pf_read_mac(struct nfp_pf_dev *pf_dev, int port) > +nfp_net_pf_read_mac(struct nfp_app_nic *app_nic, int port) > { > struct nfp_eth_table *nfp_eth_table; > struct nfp_net_hw *hw = NULL; > > /* Grab a pointer to the correct physical port */ > - hw = pf_dev->ports[port]; > + hw = app_nic->ports[port]; > > - nfp_eth_table = nfp_eth_read_ports(pf_dev->cpp); > + nfp_eth_table = nfp_eth_read_ports(app_nic->pf_dev->cpp); > > nfp_eth_copy_mac((uint8_t *)&hw->mac_addr, > (uint8_t *)&nfp_eth_table->ports[port].mac_addr); > @@ -64,6 +64,7 @@ > uint32_t new_ctrl, update = 0; > struct nfp_net_hw *hw; > struct nfp_pf_dev *pf_dev; > + struct nfp_app_nic *app_nic; > struct rte_eth_conf *dev_conf; > struct rte_eth_rxmode *rxmode; > uint32_t intr_vector; > @@ -71,6 +72,7 @@ > > hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private); > pf_dev = NFP_NET_DEV_PRIVATE_TO_PF(dev->data->dev_private); > + app_nic = NFP_APP_PRIV_TO_APP_NIC(pf_dev->app_priv); > > PMD_INIT_LOG(DEBUG, "Start"); > > @@ -82,7 +84,7 @@ > > /* check and configure queue intr-vector mapping */ > if (dev->data->dev_conf.intr_conf.rxq != 0) { > - if (pf_dev->multiport) { > + if (app_nic->multiport) { > PMD_INIT_LOG(ERR, "PMD rx interrupt is not supported " > "with NFP multiport PF"); > return -EINVAL; > @@ -250,6 +252,7 @@ > struct nfp_net_hw *hw; > struct rte_pci_device *pci_dev; > struct nfp_pf_dev *pf_dev; > + struct nfp_app_nic *app_nic; > int i; > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) > @@ -260,6 +263,7 @@ > pf_dev = NFP_NET_DEV_PRIVATE_TO_PF(dev->data->dev_private); > hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private); > pci_dev = RTE_ETH_DEV_TO_PCI(dev); > + app_nic = NFP_APP_PRIV_TO_APP_NIC(pf_dev->app_priv); > > /* > * We assume that the DPDK application is stopping all the > @@ -280,12 +284,12 @@ > /* Only free PF resources after all physical ports have been closed */ > /* Mark this port as unused and free device priv resources*/ > nn_cfg_writeb(hw, NFP_NET_CFG_LSC, 0xff); > - pf_dev->ports[hw->idx] = NULL; > + app_nic->ports[hw->idx] = NULL; > rte_eth_dev_release_port(dev); > > - for (i = 0; i < pf_dev->total_phyports; i++) { > + for (i = 0; i < app_nic->total_phyports; i++) { > /* Check to see if ports are still in use */ > - if (pf_dev->ports[i]) > + if (app_nic->ports[i]) > return 0; > } > > @@ -296,6 +300,7 @@ > free(pf_dev->hwinfo); > free(pf_dev->sym_tbl); > nfp_cpp_free(pf_dev->cpp); > + rte_free(app_nic); > rte_free(pf_dev); > > rte_intr_disable(pci_dev->intr_handle); > @@ -404,6 +409,7 @@ > { > struct rte_pci_device *pci_dev; > struct nfp_pf_dev *pf_dev; > + struct nfp_app_nic *app_nic; > struct nfp_net_hw *hw; > struct rte_ether_addr *tmp_ether_addr; > uint64_t rx_bar_off = 0; > @@ -420,6 +426,9 @@ > /* Use backpointer here to the PF of this eth_dev */ > pf_dev = NFP_NET_DEV_PRIVATE_TO_PF(eth_dev->data->dev_private); > > + /* Use backpointer to the CoreNIC app struct */ > + app_nic = NFP_APP_PRIV_TO_APP_NIC(pf_dev->app_priv); > + > /* NFP can not handle DMA addresses requiring more than 40 bits */ > if (rte_mem_check_dma_mask(40)) { > RTE_LOG(ERR, PMD, > @@ -438,7 +447,7 @@ > * Use PF array of physical ports to get pointer to > * this specific port > */ > - hw = pf_dev->ports[port]; > + hw = app_nic->ports[port]; > > PMD_INIT_LOG(DEBUG, "Working with physical port number: %d, " > "NFP internal port number: %d", port, hw->nfp_idx); > @@ -568,7 +577,7 @@ > goto dev_err_queues_map; > } > > - nfp_net_pf_read_mac(pf_dev, port); > + nfp_net_pf_read_mac(app_nic, port); > nfp_net_write_mac(hw, (uint8_t *)&hw->mac_addr); > > tmp_ether_addr = (struct rte_ether_addr *)&hw->mac_addr; > @@ -718,25 +727,67 @@ > } > > static int > -nfp_init_phyports(struct nfp_pf_dev *pf_dev) > +nfp_init_app_nic(struct nfp_pf_dev *pf_dev, > + struct nfp_eth_table *nfp_eth_table) > { > int i; > - int ret = 0; > + int ret; > + int err = 0; > + int total_vnics; > struct nfp_net_hw *hw; > + unsigned int numa_node; > struct rte_eth_dev *eth_dev; > - struct nfp_eth_table *nfp_eth_table; > + struct nfp_app_nic *app_nic; > + char port_name[RTE_ETH_NAME_MAX_LEN]; > > - nfp_eth_table = nfp_eth_read_ports(pf_dev->cpp); > - if (nfp_eth_table == NULL) { > - PMD_INIT_LOG(ERR, "Error reading NFP ethernet table"); > - return -EIO; > + PMD_INIT_LOG(INFO, "Total physical ports: %d", nfp_eth_table->count); > + > + /* Allocate memory for the CoreNIC app */ > + app_nic = rte_zmalloc("nfp_app_nic", sizeof(*app_nic), 0); > + if (app_nic == NULL) > + return -ENOMEM; > + > + /* Point the app_priv pointer in the PF to the coreNIC app */ > + pf_dev->app_priv = app_nic; > + > + /* Read the number of vNIC's created for the PF */ > + total_vnics = nfp_rtsym_read_le(pf_dev->sym_tbl, "nfd_cfg_pf0_num_ports", &err); > + if (err || total_vnics <= 0 || total_vnics > 8) { DPDK coding style says to compare integers with 0 explicitly. Since both ways are already present in net/nfp code and there is no clear preference in the driver itself, please, following DPDK coding style in a new code. > + PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value"); > + ret = -ENODEV; > + goto app_cleanup; > } > > - /* Loop through all physical ports on PF */ > - for (i = 0; i < pf_dev->total_phyports; i++) { > - const unsigned int numa_node = rte_socket_id(); > - char port_name[RTE_ETH_NAME_MAX_LEN]; > + /* > + * For coreNIC the number of vNICs exposed should be the same as the > + * number of physical ports > + */ > + if (total_vnics != (int)nfp_eth_table->count) { > + PMD_INIT_LOG(ERR, "Total physical ports do not match number of vNICs"); > + ret = -ENODEV; > + goto app_cleanup; > + } > > + /* Populate coreNIC app properties*/ > + app_nic->total_phyports = total_vnics; > + app_nic->pf_dev = pf_dev; > + if (total_vnics > 1) > + app_nic->multiport = true; > + > + /* Map the symbol table */ > + pf_dev->ctrl_bar = nfp_rtsym_map(pf_dev->sym_tbl, "_pf0_net_bar0", > + app_nic->total_phyports * 32768, &pf_dev->ctrl_area); > + if (pf_dev->ctrl_bar == NULL) { > + PMD_INIT_LOG(ERR, "nfp_rtsym_map fails for _pf0_net_ctrl_bar"); > + ret = -EIO; > + goto app_cleanup; > + } > + > + PMD_INIT_LOG(DEBUG, "ctrl bar: %p", pf_dev->ctrl_bar); > + > + /* Loop through all physical ports on PF */ > + numa_node = rte_socket_id(); > + for (i = 0; i < app_nic->total_phyports; i++) { > snprintf(port_name, sizeof(port_name), "%s_port%d", > pf_dev->pci_dev->device.name, i); > > @@ -760,7 +811,7 @@ > hw = NFP_NET_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private); > > /* Add this device to the PF's array of physical ports */ > - pf_dev->ports[i] = hw; > + app_nic->ports[i] = hw; > > hw->pf_dev = pf_dev; > hw->cpp = pf_dev->cpp; > @@ -783,20 +834,21 @@ > rte_eth_dev_probing_finish(eth_dev); > > } /* End loop, all ports on this PF */ > - ret = 0; > - goto eth_table_cleanup; > + > + return 0; > > port_cleanup: > - for (i = 0; i < pf_dev->total_phyports; i++) { > - if (pf_dev->ports[i] && pf_dev->ports[i]->eth_dev) { > + for (i = 0; i < app_nic->total_phyports; i++) { > + if (app_nic->ports[i] && app_nic->ports[i]->eth_dev) { > struct rte_eth_dev *tmp_dev; > - tmp_dev = pf_dev->ports[i]->eth_dev; > + tmp_dev = app_nic->ports[i]->eth_dev; > rte_eth_dev_release_port(tmp_dev); > - pf_dev->ports[i] = NULL; > + app_nic->ports[i] = NULL; > } > } > -eth_table_cleanup: > - free(nfp_eth_table); > + nfp_cpp_area_free(pf_dev->ctrl_area); > +app_cleanup: > + rte_free(app_nic); > > return ret; > } > @@ -804,11 +856,11 @@ > static int > nfp_pf_init(struct rte_pci_device *pci_dev) > { > - int err; > - int ret = 0; > + int ret; > + int err = 0; > uint64_t addr; > - int total_ports; > struct nfp_cpp *cpp; > + enum nfp_app_id app_id; > struct nfp_pf_dev *pf_dev; > struct nfp_hwinfo *hwinfo; > char name[RTE_ETH_NAME_MAX_LEN]; > @@ -840,9 +892,10 @@ > if (hwinfo == NULL) { > PMD_INIT_LOG(ERR, "Error reading hwinfo table"); > ret = -EIO; > - goto error; > + goto cpp_cleanup; > } > > + /* Read the number of physical ports from hardware */ > nfp_eth_table = nfp_eth_read_ports(cpp); > if (nfp_eth_table == NULL) { > PMD_INIT_LOG(ERR, "Error reading NFP ethernet table"); > @@ -865,20 +918,14 @@ > goto eth_table_cleanup; > } > > - total_ports = nfp_rtsym_read_le(sym_tbl, "nfd_cfg_pf0_num_ports", &err); > - if (total_ports != (int)nfp_eth_table->count) { > - PMD_DRV_LOG(ERR, "Inconsistent number of ports"); > + /* Read the app ID of the firmware loaded */ > + app_id = nfp_rtsym_read_le(sym_tbl, "_pf0_net_app_id", &err); > + if (err) { Compare vs 0 > + PMD_INIT_LOG(ERR, "Couldn't read app_id from fw"); > ret = -EIO; > goto sym_tbl_cleanup; > } > > - PMD_INIT_LOG(INFO, "Total physical ports: %d", total_ports); > - > - if (total_ports <= 0 || total_ports > 8) { > - PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value"); > - ret = -ENODEV; > - goto sym_tbl_cleanup; > - } > /* Allocate memory for the PF "device" */ > snprintf(name, sizeof(name), "nfp_pf%d", 0); > pf_dev = rte_zmalloc(name, sizeof(*pf_dev), 0); > @@ -888,27 +935,12 @@ > } > > /* Populate the newly created PF device */ > + pf_dev->app_id = app_id; > pf_dev->cpp = cpp; > pf_dev->hwinfo = hwinfo; > pf_dev->sym_tbl = sym_tbl; > - pf_dev->total_phyports = total_ports; > - > - if (total_ports > 1) > - pf_dev->multiport = true; > - > pf_dev->pci_dev = pci_dev; > > - /* Map the symbol table */ > - pf_dev->ctrl_bar = nfp_rtsym_map(pf_dev->sym_tbl, "_pf0_net_bar0", > - pf_dev->total_phyports * 32768, &pf_dev->ctrl_area); > - if (pf_dev->ctrl_bar == NULL) { > - PMD_INIT_LOG(ERR, "nfp_rtsym_map fails for _pf0_net_ctrl_bar"); > - ret = -EIO; > - goto pf_cleanup; > - } > - > - PMD_INIT_LOG(DEBUG, "ctrl bar: %p", pf_dev->ctrl_bar); > - > /* configure access to tx/rx vNIC BARs */ > switch (pci_dev->id.device_id) { > case PCI_DEVICE_ID_NFP3800_PF_NIC: > @@ -923,7 +955,7 @@ > default: > PMD_INIT_LOG(ERR, "nfp_net: no device ID matching"); > err = -ENODEV; > - goto ctrl_area_cleanup; > + goto pf_cleanup; > } > > pf_dev->hw_queues = nfp_cpp_map_area(pf_dev->cpp, 0, 0, > @@ -932,18 +964,27 @@ > if (pf_dev->hw_queues == NULL) { > PMD_INIT_LOG(ERR, "nfp_rtsym_map fails for net.qc"); > ret = -EIO; > - goto ctrl_area_cleanup; > + goto pf_cleanup; > } > > PMD_INIT_LOG(DEBUG, "tx/rx bar address: 0x%p", pf_dev->hw_queues); > > /* > - * Initialize and prep physical ports now > - * This will loop through all physical ports > + * PF initialization has been done at this point. Call app specific > + * init code now > */ > - ret = nfp_init_phyports(pf_dev); > - if (ret) { > - PMD_INIT_LOG(ERR, "Could not create physical ports"); > + switch (pf_dev->app_id) { > + case NFP_APP_CORE_NIC: > + PMD_INIT_LOG(INFO, "Initializing coreNIC"); > + ret = nfp_init_app_nic(pf_dev, nfp_eth_table); > + if (ret) { Compare vs 0 > + PMD_INIT_LOG(ERR, "Could not initialize coreNIC!"); > + goto hwqueues_cleanup; > + } > + break; > + default: > + PMD_INIT_LOG(ERR, "Unsupported Firmware loaded"); > + ret = -EINVAL; > goto hwqueues_cleanup; > } > > @@ -954,8 +995,6 @@ > > hwqueues_cleanup: > nfp_cpp_area_free(pf_dev->hwqueues_area); > -ctrl_area_cleanup: > - nfp_cpp_area_free(pf_dev->ctrl_area); > pf_cleanup: > rte_free(pf_dev); > sym_tbl_cleanup: > @@ -964,6 +1003,8 @@ > free(nfp_eth_table); > hwinfo_cleanup: > free(hwinfo); > +cpp_cleanup: > + nfp_cpp_free(cpp); > error: > return ret; > } > @@ -977,7 +1018,8 @@ > nfp_pf_secondary_init(struct rte_pci_device *pci_dev) > { > int i; > - int err; > + int err = 0; > + int ret = 0; > int total_ports; > struct nfp_cpp *cpp; > struct nfp_net_hw *hw; > @@ -1015,6 +1057,11 @@ > } > > total_ports = nfp_rtsym_read_le(sym_tbl, "nfd_cfg_pf0_num_ports", &err); > + if (err || total_ports <= 0 || total_ports > 8) { Compare err vs 0 > + PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value"); > + ret = -ENODEV; > + goto sym_tbl_cleanup; > + } > > for (i = 0; i < total_ports; i++) { > struct rte_eth_dev *eth_dev; > @@ -1028,7 +1075,8 @@ > if (eth_dev == NULL) { > RTE_LOG(ERR, EAL, > "secondary process attach failed, ethdev doesn't exist"); > - return -ENODEV; > + ret = -ENODEV; > + break; > } > > hw = NFP_NET_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private); > @@ -1041,10 +1089,16 @@ > rte_eth_dev_probing_finish(eth_dev); > } > > + if (ret) Compare vs 0 > + goto sym_tbl_cleanup; > + > /* Register the CPP bridge service for the secondary too */ > nfp_register_cpp_service(cpp); > > - return 0; > +sym_tbl_cleanup: > + free(sym_tbl); > + > + return ret; > } > > static int