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 3E849A0543; Tue, 25 Oct 2022 10:57:04 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0F9EF40A89; Tue, 25 Oct 2022 10:57:04 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id A5666400D5 for ; Tue, 25 Oct 2022 10:57:02 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (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 E9B1F5E; Tue, 25 Oct 2022 11:57:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru E9B1F5E DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1666688222; bh=rjbIRTI05n03IMSmNoKUV4kHO1nrxtJNH+Vonb9ilGQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=WbSmppfI120bW42qbwp5luWFvQqrthd7lIk5CXjxq4U5g40CaaZH4yMt4vrqp9dNQ GxbXvMqsGXUJbds1QC78lzCla1+ZTILOx4YI1RbBgW5l42TQiLQaSIiwlECOFr0kkf rYxqaiAo2RB4coDpIVEFS300KQdQ8sUrIBe2QjIo= Message-ID: Date: Tue, 25 Oct 2022 11:57:01 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH v11 02/18] net/idpf: add support for device initialization Content-Language: en-US To: Junfeng Guo , qi.z.zhang@intel.com, jingjing.wu@intel.com, beilei.xing@intel.com Cc: dev@dpdk.org, Xiaoyun Li , Xiao Wang References: <20221024130134.1046536-2-junfeng.guo@intel.com> <20221024131227.1062446-1-junfeng.guo@intel.com> <20221024131227.1062446-3-junfeng.guo@intel.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20221024131227.1062446-3-junfeng.guo@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 10/24/22 16:12, Junfeng Guo wrote: > Support device init and add the following dev ops: > - dev_configure > - dev_close > - dev_infos_get > > Signed-off-by: Beilei Xing > Signed-off-by: Xiaoyun Li > Signed-off-by: Xiao Wang > Signed-off-by: Junfeng Guo [snip] > diff --git a/doc/guides/nics/features/idpf.ini b/doc/guides/nics/features/idpf.ini > new file mode 100644 > index 0000000000..7a44b8b5e4 > --- /dev/null > +++ b/doc/guides/nics/features/idpf.ini > @@ -0,0 +1,10 @@ > +; > +; Supported features of the 'idpf' network poll mode driver. > +; > +; Refer to default.ini for the full list of available PMD features. > +; > +; A feature with "P" indicates only be supported when non-vector path > +; is selected. The statement should be added when the first P appears. > +; > +[Features] > +Linux = Y I think you should add at least one architecture. I guess it should be x86_64 since otherwise it is unclear where to build... > diff --git a/drivers/net/idpf/idpf_ethdev.c b/drivers/net/idpf/idpf_ethdev.c > new file mode 100644 > index 0000000000..75695085f8 > --- /dev/null > +++ b/drivers/net/idpf/idpf_ethdev.c > @@ -0,0 +1,825 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Intel Corporation > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "idpf_ethdev.h" > + > +#define IDPF_TX_SINGLE_Q "tx_single" > +#define IDPF_RX_SINGLE_Q "rx_single" > +#define IDPF_VPORT "vport" > + > +rte_spinlock_t idpf_adapter_lock; > +/* A list for all adapters, one adapter matches one PCI device */ > +struct idpf_adapter_list idpf_adapter_list; > +bool idpf_adapter_list_init; > + > +static const char * const idpf_valid_args[] = { > + IDPF_TX_SINGLE_Q, > + IDPF_RX_SINGLE_Q, > + IDPF_VPORT, > + NULL > +}; > + > +static int idpf_dev_configure(struct rte_eth_dev *dev); > +static int idpf_dev_close(struct rte_eth_dev *dev); > +static int idpf_dev_info_get(struct rte_eth_dev *dev, > + struct rte_eth_dev_info *dev_info); > +static void idpf_adapter_rel(struct idpf_adapter *adapter); > + > +static const struct eth_dev_ops idpf_eth_dev_ops = { > + .dev_configure = idpf_dev_configure, > + .dev_close = idpf_dev_close, > + .dev_infos_get = idpf_dev_info_get, > +}; > + > +static int > +idpf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info) > +{ > + struct idpf_vport *vport = dev->data->dev_private; > + struct idpf_adapter *adapter = vport->adapter; > + > + dev_info->max_rx_queues = adapter->caps->max_rx_q; > + dev_info->max_tx_queues = adapter->caps->max_tx_q; > + dev_info->min_rx_bufsize = IDPF_MIN_BUF_SIZE; > + dev_info->max_rx_pktlen = IDPF_MAX_FRAME_SIZE; > + > + dev_info->max_mtu = dev_info->max_rx_pktlen - IDPF_ETH_OVERHEAD; > + dev_info->min_mtu = RTE_ETHER_MIN_MTU; > + > + dev_info->max_mac_addrs = IDPF_NUM_MACADDR_MAX; > + dev_info->dev_capa = 0; > + dev_info->rx_offload_capa = 0; > + > + dev_info->tx_offload_capa = 0; You don't need above zeros since the structure is memset to 0 in ethdev layer before driver callback invocation. > + > + return 0; > +} > + > +static int > +idpf_init_vport_req_info(struct rte_eth_dev *dev) > +{ > + struct idpf_vport *vport = dev->data->dev_private; > + struct idpf_adapter *adapter = vport->adapter; > + struct virtchnl2_create_vport *vport_info; > + uint16_t idx = adapter->cur_vport_idx; > + > + if (idx == IDPF_INVALID_VPORT_IDX) { > + PMD_INIT_LOG(ERR, "Invalid vport index."); > + return -1; > + } > + > + if (adapter->vport_req_info[idx] == NULL) { > + adapter->vport_req_info[idx] = rte_zmalloc(NULL, > + sizeof(struct virtchnl2_create_vport), 0); > + if (adapter->vport_req_info[idx] == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate vport_req_info"); > + return -1; > + } > + } > + > + vport_info = > + (struct virtchnl2_create_vport *)adapter->vport_req_info[idx]; > + > + vport_info->vport_type = rte_cpu_to_le_16(VIRTCHNL2_VPORT_TYPE_DEFAULT); > + if (adapter->txq_model == 0) { > + vport_info->txq_model = > + rte_cpu_to_le_16(VIRTCHNL2_QUEUE_MODEL_SPLIT); > + vport_info->num_tx_q = IDPF_DEFAULT_TXQ_NUM; > + vport_info->num_tx_complq = > + IDPF_DEFAULT_TXQ_NUM * IDPF_TX_COMPLQ_PER_GRP; > + } else { > + vport_info->txq_model = > + rte_cpu_to_le_16(VIRTCHNL2_QUEUE_MODEL_SINGLE); > + vport_info->num_tx_q = IDPF_DEFAULT_TXQ_NUM; > + vport_info->num_tx_complq = 0; > + } > + if (adapter->rxq_model == 0) { > + vport_info->rxq_model = > + rte_cpu_to_le_16(VIRTCHNL2_QUEUE_MODEL_SPLIT); > + vport_info->num_rx_q = IDPF_DEFAULT_RXQ_NUM; > + vport_info->num_rx_bufq = > + IDPF_DEFAULT_RXQ_NUM * IDPF_RX_BUFQ_PER_GRP; > + } else { > + vport_info->rxq_model = > + rte_cpu_to_le_16(VIRTCHNL2_QUEUE_MODEL_SINGLE); > + vport_info->num_rx_q = IDPF_DEFAULT_RXQ_NUM; > + vport_info->num_rx_bufq = 0; > + } > + > + return 0; > +} > + > +static uint16_t > +idpf_parse_devarg_id(char *name) > +{ > + uint16_t val; > + char *p; > + > + p = strstr(name, "vport_"); > + p += sizeof("vport_") - 1; > + > + val = strtoul(p, NULL, 10); There is a potential segfault here if there is no vport_ in the name. > + > + return val; > +} > + > +#define IDPF_RSS_KEY_LEN 52 The define is not used. It must be a part of the RSS support patch and it should be use to set key size in device information. > + > +static int > +idpf_init_vport(struct rte_eth_dev *dev) > +{ > + struct idpf_vport *vport = dev->data->dev_private; > + struct idpf_adapter *adapter = vport->adapter; > + uint16_t idx = adapter->cur_vport_idx; > + struct virtchnl2_create_vport *vport_info = > + (struct virtchnl2_create_vport *)adapter->vport_recv_info[idx]; > + int i; > + > + vport->vport_id = vport_info->vport_id; > + vport->txq_model = vport_info->txq_model; > + vport->rxq_model = vport_info->rxq_model; > + vport->num_tx_q = vport_info->num_tx_q; > + vport->num_tx_complq = vport_info->num_tx_complq; > + vport->num_rx_q = vport_info->num_rx_q; > + vport->num_rx_bufq = vport_info->num_rx_bufq; > + rte_memcpy(vport->default_mac_addr, > + vport_info->default_mac_addr, ETH_ALEN); > + vport->sw_idx = idx; > + > + for (i = 0; i < vport_info->chunks.num_chunks; i++) { > + if (vport_info->chunks.chunks[i].type == > + VIRTCHNL2_QUEUE_TYPE_TX) { switch/case will be more readable here > + vport->chunks_info.tx_start_qid = > + vport_info->chunks.chunks[i].start_queue_id; > + vport->chunks_info.tx_qtail_start = > + vport_info->chunks.chunks[i].qtail_reg_start; > + vport->chunks_info.tx_qtail_spacing = > + vport_info->chunks.chunks[i].qtail_reg_spacing; > + } else if (vport_info->chunks.chunks[i].type == > + VIRTCHNL2_QUEUE_TYPE_RX) { > + vport->chunks_info.rx_start_qid = > + vport_info->chunks.chunks[i].start_queue_id; > + vport->chunks_info.rx_qtail_start = > + vport_info->chunks.chunks[i].qtail_reg_start; > + vport->chunks_info.rx_qtail_spacing = > + vport_info->chunks.chunks[i].qtail_reg_spacing; > + } else if (vport_info->chunks.chunks[i].type == > + VIRTCHNL2_QUEUE_TYPE_TX_COMPLETION) { > + vport->chunks_info.tx_compl_start_qid = > + vport_info->chunks.chunks[i].start_queue_id; > + vport->chunks_info.tx_compl_qtail_start = > + vport_info->chunks.chunks[i].qtail_reg_start; > + vport->chunks_info.tx_compl_qtail_spacing = > + vport_info->chunks.chunks[i].qtail_reg_spacing; > + } else if (vport_info->chunks.chunks[i].type == > + VIRTCHNL2_QUEUE_TYPE_RX_BUFFER) { > + vport->chunks_info.rx_buf_start_qid = > + vport_info->chunks.chunks[i].start_queue_id; > + vport->chunks_info.rx_buf_qtail_start = > + vport_info->chunks.chunks[i].qtail_reg_start; > + vport->chunks_info.rx_buf_qtail_spacing = > + vport_info->chunks.chunks[i].qtail_reg_spacing; > + } > + } > + > + vport->devarg_id = idpf_parse_devarg_id(dev->data->name); > + vport->dev_data = dev->data; > + > + adapter->vports[idx] = vport; > + > + return 0; > +} > +static int > +idpf_dev_configure(struct rte_eth_dev *dev) > +{ > + if ((dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_RSS_FLAG) != 0) > + PMD_INIT_LOG(ERR, "RSS is not supported."); Do you think it is really enought? You ignore: - link_speeds - rxmode in the case of DCB-only and/or with VMDq - txmode in DCB and VMQq cases - txmode: pvid, hw_vlan* - lpbk_mode - dcb_capability_en - intr_conf > + > + return 0; > +} > + > +static int > +idpf_dev_close(struct rte_eth_dev *dev) > +{ > + struct idpf_vport *vport = dev->data->dev_private; > + struct idpf_adapter *adapter = vport->adapter; > + > + idpf_vc_destroy_vport(vport); > + > + > + adapter->cur_vports &= ~RTE_BIT32(vport->devarg_id); > + > + rte_free(vport); > + dev->data->dev_private = NULL; > + > + return 0; > +} > + > +static int > +insert_value(struct idpf_adapter *adapter, uint16_t id) > +{ > + uint16_t i; > + > + for (i = 0; i < adapter->req_vport_nb; i++) { > + if (adapter->req_vports[i] == id) > + return 0; > + } > + > + if (adapter->req_vport_nb >= RTE_DIM(adapter->req_vports)) { > + PMD_INIT_LOG(ERR, "Total vport number can't be > %d", > + IDPF_MAX_VPORT_NUM); > + return -1; > + } > + > + adapter->req_vports[adapter->req_vport_nb] = id; > + adapter->req_vport_nb++; > + > + return 0; > +} > + > +static const char * > +parse_range(const char *value, struct idpf_adapter *adapter) > +{ > + uint16_t lo, hi, i; > + int n = 0; > + int result; > + const char *pos = value; > + > + result = sscanf(value, "%hu%n-%hu%n", &lo, &n, &hi, &n); > + if (result == 1) { > + if (lo >= IDPF_MAX_VPORT_NUM) > + return NULL; > + if (insert_value(adapter, lo) != 0) > + return NULL; > + } else if (result == 2) { > + if (lo > hi || hi >= IDPF_MAX_VPORT_NUM) > + return NULL; > + for (i = lo; i <= hi; i++) { > + if (insert_value(adapter, i) != 0) > + return NULL; > + } > + } else { > + return NULL; > + } > + > + return pos + n; > +} > + > +static int > +parse_vport(const char *key, const char *value, void *args) > +{ > + struct idpf_adapter *adapter = (struct idpf_adapter *)args; type cast from void * is not required in C > + const char *pos = value; > + int i; > + > + adapter->req_vport_nb = 0; > + > + if (*pos == '[') > + pos++; > + > + while (1) { > + pos = parse_range(pos, adapter); > + if (pos == NULL) { > + PMD_INIT_LOG(ERR, "invalid value:\"%s\" for key:\"%s\", ", > + value, key); > + return -1; > + } > + if (*pos != ',') > + break; > + pos++; > + } > + > + if (*value == '[' && *pos != ']') { > + PMD_INIT_LOG(ERR, "invalid value:\"%s\" for key:\"%s\", ", > + value, key); > + return -1; > + } > + > + if (adapter->cur_vport_nb + adapter->req_vport_nb > > + IDPF_MAX_VPORT_NUM) { > + PMD_INIT_LOG(ERR, "Total vport number can't be > %d", > + IDPF_MAX_VPORT_NUM); > + return -1; > + } > + > + for (i = 0; i < adapter->req_vport_nb; i++) { > + if ((adapter->cur_vports & RTE_BIT32(adapter->req_vports[i])) == 0) { > + adapter->cur_vports |= RTE_BIT32(adapter->req_vports[i]); > + adapter->cur_vport_nb++; > + } else { > + PMD_INIT_LOG(ERR, "Vport %d has been created", > + adapter->req_vports[i]); > + return -1; > + } > + } > + > + return 0; > +} > + > +static int > +parse_bool(const char *key, const char *value, void *args) > +{ > + int *i = (int *)args; type cast from void * is not required in C > + char *end; > + int num; > + > + num = strtoul(value, &end, 10); man strtoul: The implementation may also set errno to EINVAL in case no conversion was performed (no digits seen, and 0 returned). > + > + if (num != 0 && num != 1) { So, 0 may be returned in the case of failure, but you treat it as OK here. > + PMD_INIT_LOG(ERR, "invalid value:\"%s\" for key:\"%s\", value must be 0 or 1", > + value, key); > + return -1; > + } > + > + *i = num; > + return 0; > +} > + > +static int > +idpf_parse_devargs(struct rte_pci_device *pci_dev, struct idpf_adapter *adapter) > +{ > + struct rte_devargs *devargs = pci_dev->device.devargs; > + struct rte_kvargs *kvlist; > + int ret; > + > + if (devargs == NULL) > + return 0; > + > + kvlist = rte_kvargs_parse(devargs->args, idpf_valid_args); > + if (kvlist == NULL) { > + PMD_INIT_LOG(ERR, "invalid kvargs key"); > + return -EINVAL; > + } > + > + ret = rte_kvargs_process(kvlist, IDPF_VPORT, &parse_vport, > + adapter); > + if (ret != 0) > + goto bail; > + > + ret = rte_kvargs_process(kvlist, IDPF_TX_SINGLE_Q, &parse_bool, > + &adapter->txq_model); > + if (ret != 0) > + goto bail; > + > + ret = rte_kvargs_process(kvlist, IDPF_RX_SINGLE_Q, &parse_bool, > + &adapter->rxq_model); > + if (ret != 0) > + goto bail; > + > +bail: > + rte_kvargs_free(kvlist); > + return ret; > +} > + > +static void > +idpf_reset_pf(struct idpf_hw *hw) > +{ > + uint32_t reg; > + > + reg = IDPF_READ_REG(hw, PFGEN_CTRL); > + IDPF_WRITE_REG(hw, PFGEN_CTRL, (reg | PFGEN_CTRL_PFSWR)); > +} > + > +#define IDPF_RESET_WAIT_CNT 100 > +static int > +idpf_check_pf_reset_done(struct idpf_hw *hw) > +{ > + uint32_t reg; > + int i; > + > + for (i = 0; i < IDPF_RESET_WAIT_CNT; i++) { > + reg = IDPF_READ_REG(hw, PFGEN_RSTAT); > + if (reg != 0xFFFFFFFF && (reg & PFGEN_RSTAT_PFR_STATE_M)) > + return 0; > + rte_delay_ms(1000); > + } > + > + PMD_INIT_LOG(ERR, "IDPF reset timeout"); > + return -EBUSY; > +} > + > +#define CTLQ_NUM 2 > +static int > +idpf_init_mbx(struct idpf_hw *hw) > +{ > + struct idpf_ctlq_create_info ctlq_info[CTLQ_NUM] = { > + { > + .type = IDPF_CTLQ_TYPE_MAILBOX_TX, > + .id = IDPF_CTLQ_ID, > + .len = IDPF_CTLQ_LEN, > + .buf_size = IDPF_DFLT_MBX_BUF_SIZE, > + .reg = { > + .head = PF_FW_ATQH, > + .tail = PF_FW_ATQT, > + .len = PF_FW_ATQLEN, > + .bah = PF_FW_ATQBAH, > + .bal = PF_FW_ATQBAL, > + .len_mask = PF_FW_ATQLEN_ATQLEN_M, > + .len_ena_mask = PF_FW_ATQLEN_ATQENABLE_M, > + .head_mask = PF_FW_ATQH_ATQH_M, > + } > + }, > + { > + .type = IDPF_CTLQ_TYPE_MAILBOX_RX, > + .id = IDPF_CTLQ_ID, > + .len = IDPF_CTLQ_LEN, > + .buf_size = IDPF_DFLT_MBX_BUF_SIZE, > + .reg = { > + .head = PF_FW_ARQH, > + .tail = PF_FW_ARQT, > + .len = PF_FW_ARQLEN, > + .bah = PF_FW_ARQBAH, > + .bal = PF_FW_ARQBAL, > + .len_mask = PF_FW_ARQLEN_ARQLEN_M, > + .len_ena_mask = PF_FW_ARQLEN_ARQENABLE_M, > + .head_mask = PF_FW_ARQH_ARQH_M, > + } > + } > + }; > + struct idpf_ctlq_info *ctlq; > + int ret; > + > + ret = idpf_ctlq_init(hw, CTLQ_NUM, ctlq_info); > + if (ret != 0) > + return ret; > + > + LIST_FOR_EACH_ENTRY_SAFE(ctlq, NULL, &hw->cq_list_head, > + struct idpf_ctlq_info, cq_list) { > + if (ctlq->q_id == IDPF_CTLQ_ID && > + ctlq->cq_type == IDPF_CTLQ_TYPE_MAILBOX_TX) > + hw->asq = ctlq; > + if (ctlq->q_id == IDPF_CTLQ_ID && > + ctlq->cq_type == IDPF_CTLQ_TYPE_MAILBOX_RX) > + hw->arq = ctlq; > + } > + > + if (hw->asq == NULL || hw->arq == NULL) { > + idpf_ctlq_deinit(hw); > + ret = -ENOENT; > + } > + > + return ret; > +} > + > +static int > +idpf_adapter_init(struct rte_pci_device *pci_dev, struct idpf_adapter *adapter) > +{ > + struct idpf_hw *hw = &adapter->hw; > + int ret = 0; > + > + hw->hw_addr = (void *)pci_dev->mem_resource[0].addr; > + hw->hw_addr_len = pci_dev->mem_resource[0].len; > + hw->back = adapter; > + hw->vendor_id = pci_dev->id.vendor_id; > + hw->device_id = pci_dev->id.device_id; > + hw->subsystem_vendor_id = pci_dev->id.subsystem_vendor_id; > + > + strncpy(adapter->name, pci_dev->device.name, PCI_PRI_STR_SIZE); > + > + idpf_reset_pf(hw); > + ret = idpf_check_pf_reset_done(hw); > + if (ret != 0) { > + PMD_INIT_LOG(ERR, "IDPF is still resetting"); > + goto err; > + } > + > + ret = idpf_init_mbx(hw); > + if (ret != 0) { > + PMD_INIT_LOG(ERR, "Failed to init mailbox"); > + goto err; > + } > + > + adapter->mbx_resp = rte_zmalloc("idpf_adapter_mbx_resp", > + IDPF_DFLT_MBX_BUF_SIZE, 0); > + if (adapter->mbx_resp == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate idpf_adapter_mbx_resp memory"); > + goto err_mbx; > + } > + > + if (idpf_vc_check_api_version(adapter) != 0) { > + PMD_INIT_LOG(ERR, "Failed to check api version"); > + goto err_api; > + } > + > + adapter->caps = rte_zmalloc("idpf_caps", > + sizeof(struct virtchnl2_get_capabilities), 0); > + if (adapter->caps == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate idpf_caps memory"); > + goto err_api; > + } > + > + if (idpf_vc_get_caps(adapter) != 0) { > + PMD_INIT_LOG(ERR, "Failed to get capabilities"); > + goto err_caps; > + } > + > + adapter->max_vport_nb = adapter->caps->max_vports; > + > + adapter->vport_req_info = rte_zmalloc("vport_req_info", > + adapter->max_vport_nb * > + sizeof(*adapter->vport_req_info), > + 0); > + if (adapter->vport_req_info == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate vport_req_info memory"); > + goto err_caps; > + } > + > + adapter->vport_recv_info = rte_zmalloc("vport_recv_info", > + adapter->max_vport_nb * > + sizeof(*adapter->vport_recv_info), > + 0); > + if (adapter->vport_recv_info == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate vport_recv_info memory"); > + goto err_vport_recv_info; > + } > + > + adapter->vports = rte_zmalloc("vports", > + adapter->max_vport_nb * > + sizeof(*adapter->vports), > + 0); > + if (adapter->vports == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate vports memory"); > + goto err_vports; > + } > + > + adapter->max_rxq_per_msg = (IDPF_DFLT_MBX_BUF_SIZE - > + sizeof(struct virtchnl2_config_rx_queues)) / > + sizeof(struct virtchnl2_rxq_info); > + adapter->max_txq_per_msg = (IDPF_DFLT_MBX_BUF_SIZE - > + sizeof(struct virtchnl2_config_tx_queues)) / > + sizeof(struct virtchnl2_txq_info); > + > + adapter->cur_vports = 0; > + adapter->cur_vport_nb = 0; > + > + return ret; > + > +err_vports: > + rte_free(adapter->vport_recv_info); > + adapter->vport_recv_info = NULL; > +err_vport_recv_info: > + rte_free(adapter->vport_req_info); > + adapter->vport_req_info = NULL; > +err_caps: > + rte_free(adapter->caps); > + adapter->caps = NULL; > +err_api: > + rte_free(adapter->mbx_resp); > + adapter->mbx_resp = NULL; > +err_mbx: > + idpf_ctlq_deinit(hw); > +err: > + return -1; > +} > + > +static uint16_t > +idpf_get_vport_idx(struct idpf_vport **vports, uint16_t max_vport_nb) > +{ > + uint16_t vport_idx; > + uint16_t i; > + > + for (i = 0; i < max_vport_nb; i++) { > + if (vports[i] == NULL) > + break; > + } > + > + if (i == max_vport_nb) > + vport_idx = IDPF_INVALID_VPORT_IDX; > + else > + vport_idx = i; > + > + return vport_idx; > +} > + > +static int > +idpf_dev_init(struct rte_eth_dev *dev, void *init_params) > +{ > + struct idpf_vport *vport = dev->data->dev_private; > + struct idpf_adapter *adapter = init_params; > + int ret = 0; > + > + dev->dev_ops = &idpf_eth_dev_ops; > + vport->adapter = adapter; > + > + ret = idpf_init_vport_req_info(dev); > + if (ret != 0) { > + PMD_INIT_LOG(ERR, "Failed to init vport req_info."); > + goto err; > + } > + > + ret = idpf_vc_create_vport(adapter); > + if (ret != 0) { > + PMD_INIT_LOG(ERR, "Failed to create vport."); > + goto err_create_vport; > + } > + > + ret = idpf_init_vport(dev); > + if (ret != 0) { > + PMD_INIT_LOG(ERR, "Failed to init vports."); > + goto err_init_vport; > + } > + > + adapter->cur_vport_idx = idpf_get_vport_idx(adapter->vports, > + adapter->max_vport_nb); > + > + dev->data->mac_addrs = rte_zmalloc(NULL, RTE_ETHER_ADDR_LEN, 0); > + if (dev->data->mac_addrs == NULL) { > + PMD_INIT_LOG(ERR, "Cannot allocate mac_addr memory."); > + ret = -ENOMEM; > + goto err_init_vport; > + } > + > + rte_ether_addr_copy((struct rte_ether_addr *)vport->default_mac_addr, > + &dev->data->mac_addrs[0]); > + > + return 0; > + > +err_init_vport: > + idpf_vc_destroy_vport(vport); > +err_create_vport: > + rte_free(vport->adapter->vport_req_info[vport->adapter->cur_vport_idx]); > +err: > + return ret; > +} > + > +static const struct rte_pci_id pci_id_idpf_map[] = { > + { RTE_PCI_DEVICE(IDPF_INTEL_VENDOR_ID, IDPF_DEV_ID_PF) }, > + { .vendor_id = 0, /* sentinel */ }, > +}; > + > +struct idpf_adapter * > +idpf_find_adapter(struct rte_pci_device *pci_dev) It looks like the function requires corresponding lock to be held. If yes, it should be documented and code fixed. If no, it should be explaiend why. > +{ > + struct idpf_adapter *adapter; > + > + TAILQ_FOREACH(adapter, &idpf_adapter_list, next) { > + if (strncmp(adapter->name, pci_dev->device.name, PCI_PRI_STR_SIZE) == 0) > + return adapter; > + } > + > + return NULL; > +} > + > +static int > +idpf_pci_probe(struct rte_pci_driver *pci_drv __rte_unused, > + struct rte_pci_device *pci_dev) > +{ > + struct idpf_adapter *adapter; > + char name[RTE_ETH_NAME_MAX_LEN]; > + int i, retval; > + bool first_probe = false; > + > + if (!idpf_adapter_list_init) { > + rte_spinlock_init(&idpf_adapter_lock); > + TAILQ_INIT(&idpf_adapter_list); > + idpf_adapter_list_init = true; > + } > + > + adapter = idpf_find_adapter(pci_dev); > + if (adapter == NULL) { > + first_probe = true; > + adapter = (struct idpf_adapter *)rte_zmalloc("idpf_adapter", > + sizeof(struct idpf_adapter), 0); > + if (adapter == NULL) { > + PMD_INIT_LOG(ERR, "Failed to allocate adapter."); > + return -1; > + } > + > + retval = idpf_adapter_init(pci_dev, adapter); > + if (retval != 0) { > + PMD_INIT_LOG(ERR, "Failed to init adapter."); > + return retval; > + } > + > + rte_spinlock_lock(&idpf_adapter_lock); > + TAILQ_INSERT_TAIL(&idpf_adapter_list, adapter, next); > + rte_spinlock_unlock(&idpf_adapter_lock); > + } > + > + retval = idpf_parse_devargs(pci_dev, adapter); > + if (retval != 0) { > + PMD_INIT_LOG(ERR, "Failed to parse private devargs"); > + goto err; > + } > + > + if (adapter->req_vport_nb == 0) { > + /* If no vport devarg, create vport 0 by default. */ > + snprintf(name, sizeof(name), "idpf_%s_vport_0", > + pci_dev->device.name); > + retval = rte_eth_dev_create(&pci_dev->device, name, > + sizeof(struct idpf_vport), > + NULL, NULL, idpf_dev_init, > + adapter); > + if (retval != 0) > + PMD_DRV_LOG(ERR, "Failed to create default vport 0"); > + adapter->cur_vports |= RTE_BIT32(0); > + adapter->cur_vport_nb++; > + } else { > + for (i = 0; i < adapter->req_vport_nb; i++) { > + snprintf(name, sizeof(name), "idpf_%s_vport_%d", > + pci_dev->device.name, > + adapter->req_vports[i]); > + retval = rte_eth_dev_create(&pci_dev->device, name, > + sizeof(struct idpf_vport), > + NULL, NULL, idpf_dev_init, > + adapter); > + if (retval != 0) > + PMD_DRV_LOG(ERR, "Failed to create vport %d", > + adapter->req_vports[i]); > + } > + } > + > + return 0; > + > +err: > + if (first_probe) { > + rte_spinlock_lock(&idpf_adapter_lock); > + TAILQ_REMOVE(&idpf_adapter_list, adapter, next); > + rte_spinlock_unlock(&idpf_adapter_lock); > + idpf_adapter_rel(adapter); > + rte_free(adapter); > + } > + return retval; > +} > + > +static void > +idpf_adapter_rel(struct idpf_adapter *adapter) > +{ > + struct idpf_hw *hw = &adapter->hw; > + int i; > + > + idpf_ctlq_deinit(hw); > + > + rte_free(adapter->caps); > + adapter->caps = NULL; > + > + rte_free(adapter->mbx_resp); > + adapter->mbx_resp = NULL; > + > + if (adapter->vport_req_info != NULL) { > + for (i = 0; i < adapter->max_vport_nb; i++) { > + rte_free(adapter->vport_req_info[i]); > + adapter->vport_req_info[i] = NULL; > + } > + rte_free(adapter->vport_req_info); > + adapter->vport_req_info = NULL; > + } > + > + if (adapter->vport_recv_info != NULL) { > + for (i = 0; i < adapter->max_vport_nb; i++) { > + rte_free(adapter->vport_recv_info[i]); > + adapter->vport_recv_info[i] = NULL; > + } > + rte_free(adapter->vport_recv_info); > + adapter->vport_recv_info = NULL; > + } > + > + rte_free(adapter->vports); > + adapter->vports = NULL; > +} > + > +static int > +idpf_pci_remove(struct rte_pci_device *pci_dev) > +{ > + struct idpf_adapter *adapter = idpf_find_adapter(pci_dev); > + uint16_t port_id; > + > + /* Ethdev created can be found RTE_ETH_FOREACH_DEV_OF through rte_device */ > + RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device) { > + rte_eth_dev_close(port_id); > + } > + > + rte_spinlock_lock(&idpf_adapter_lock); > + TAILQ_REMOVE(&idpf_adapter_list, adapter, next); > + rte_spinlock_unlock(&idpf_adapter_lock); > + idpf_adapter_rel(adapter); > + rte_free(adapter); > + > + return 0; > +} > + > +static struct rte_pci_driver rte_idpf_pmd = { > + .id_table = pci_id_idpf_map, > + .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_PROBE_AGAIN, > + .probe = idpf_pci_probe, > + .remove = idpf_pci_remove, > +}; > + > +/** > + * Driver initialization routine. > + * Invoked once at EAL init time. > + * Register itself as the [Poll Mode] Driver of PCI devices. > + */ > +RTE_PMD_REGISTER_PCI(net_idpf, rte_idpf_pmd); > +RTE_PMD_REGISTER_PCI_TABLE(net_idpf, pci_id_idpf_map); > +RTE_PMD_REGISTER_KMOD_DEP(net_ice, "* igb_uio | uio_pci_generic | vfio-pci"); > + > +RTE_LOG_REGISTER_SUFFIX(idpf_logtype_init, init, NOTICE); > +RTE_LOG_REGISTER_SUFFIX(idpf_logtype_driver, driver, NOTICE); > diff --git a/drivers/net/idpf/idpf_ethdev.h b/drivers/net/idpf/idpf_ethdev.h > new file mode 100644 > index 0000000000..c0ae801fd5 > --- /dev/null > +++ b/drivers/net/idpf/idpf_ethdev.h > @@ -0,0 +1,207 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2022 Intel Corporation > + */ > + > +#ifndef _IDPF_ETHDEV_H_ > +#define _IDPF_ETHDEV_H_ > + > +#include > +#include seems to be unused > +#include seems to be unused > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "idpf_logs.h" > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Are you sure that everything above used in the patch? > + > +#define IDPF_MAX_VPORT_NUM 8 > + > +#define IDPF_DEFAULT_RXQ_NUM 16 > +#define IDPF_DEFAULT_TXQ_NUM 16 > + > +#define IDPF_INVALID_VPORT_IDX 0xffff > +#define IDPF_TXQ_PER_GRP 1 unused > +#define IDPF_TX_COMPLQ_PER_GRP 1 > +#define IDPF_RXQ_PER_GRP 1 unused > +#define IDPF_RX_BUFQ_PER_GRP 2 > + > +#define IDPF_CTLQ_ID -1 > +#define IDPF_CTLQ_LEN 64 > +#define IDPF_DFLT_MBX_BUF_SIZE 4096 > + > +#define IDPF_MIN_BUF_SIZE 1024 > +#define IDPF_MAX_FRAME_SIZE 9728 > + > +#define IDPF_NUM_MACADDR_MAX 64 > + > +#define IDPF_VLAN_TAG_SIZE 4 > +#define IDPF_ETH_OVERHEAD \ > + (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + IDPF_VLAN_TAG_SIZE * 2) > + > +#ifndef ETH_ADDR_LEN > +#define ETH_ADDR_LEN 6 > +#endif Why do you need it? > + > +#define IDPF_ADAPTER_NAME_LEN (PCI_PRI_STR_SIZE + 1) > + > +/* Message type read in virtual channel from PF */ > +enum idpf_vc_result { > + IDPF_MSG_ERR = -1, /* Meet error when accessing admin queue */ > + IDPF_MSG_NON, /* Read nothing from admin queue */ > + IDPF_MSG_SYS, /* Read system msg from admin queue */ > + IDPF_MSG_CMD, /* Read async command result */ > +}; > + > +struct idpf_chunks_info { > + uint32_t tx_start_qid; > + uint32_t rx_start_qid; > + /* Valid only if split queue model */ > + uint32_t tx_compl_start_qid; > + uint32_t rx_buf_start_qid; > + > + uint64_t tx_qtail_start; > + uint32_t tx_qtail_spacing; > + uint64_t rx_qtail_start; > + uint32_t rx_qtail_spacing; > + uint64_t tx_compl_qtail_start; > + uint32_t tx_compl_qtail_spacing; > + uint64_t rx_buf_qtail_start; > + uint32_t rx_buf_qtail_spacing; > +}; > + > +struct idpf_vport { > + struct idpf_adapter *adapter; /* Backreference to associated adapter */ > + uint16_t vport_id; > + uint32_t txq_model; > + uint32_t rxq_model; > + uint16_t num_tx_q; Shouldn't it be set as the result of configure? data->nb_tx_queues which is the result of the configure is not used in the patch? > + /* valid only if txq_model is split Q */ > + uint16_t num_tx_complq; > + uint16_t num_rx_q; same here. > + /* valid only if rxq_model is split Q */ > + uint16_t num_rx_bufq; > + > + uint16_t max_mtu; unused > + uint8_t default_mac_addr[VIRTCHNL_ETH_LENGTH_OF_ADDRESS]; > + > + uint16_t sw_idx; /* SW idx */ > + > + struct rte_eth_dev_data *dev_data; /* Pointer to the device data */ > + uint16_t max_pkt_len; /* Maximum packet length */ > + > + /* Chunk info */ > + struct idpf_chunks_info chunks_info; > + > + uint16_t devarg_id; > +}; > + > +struct idpf_adapter { > + TAILQ_ENTRY(idpf_adapter) next; > + struct idpf_hw hw; > + char name[IDPF_ADAPTER_NAME_LEN]; > + > + struct virtchnl_version_info virtchnl_version; unused. I'm stopping unused check here. Please, do it yourself carefully after each patch to avoid more review iterations. > + struct virtchnl2_get_capabilities *caps; > + > + volatile enum virtchnl_ops pend_cmd; /* pending command not finished */ > + uint32_t cmd_retval; /* return value of the cmd response from ipf */ > + uint8_t *mbx_resp; /* buffer to store the mailbox response from ipf */ > + > + uint32_t txq_model; /* 0 - split queue model, non-0 - single queue model */ > + uint32_t rxq_model; /* 0 - split queue model, non-0 - single queue model */ > + > + /* Vport info */ > + uint8_t **vport_req_info; > + uint8_t **vport_recv_info; > + struct idpf_vport **vports; > + uint16_t max_vport_nb; > + uint16_t req_vports[IDPF_MAX_VPORT_NUM]; > + uint16_t req_vport_nb; > + uint16_t cur_vports; > + uint16_t cur_vport_nb; > + uint16_t cur_vport_idx; > + > + /* Max config queue number per VC message */ > + uint32_t max_rxq_per_msg; > + uint32_t max_txq_per_msg; > + > + bool stopped; > +}; > + > +TAILQ_HEAD(idpf_adapter_list, idpf_adapter); > +extern struct idpf_adapter_list adapter_list; there is no adapter_list variable. [snip] > diff --git a/drivers/net/idpf/idpf_vchnl.c b/drivers/net/idpf/idpf_vchnl.c > new file mode 100644 > index 0000000000..ec72a8bba7 > --- /dev/null > +++ b/drivers/net/idpf/idpf_vchnl.c [snip] > +static int > +idpf_send_vc_msg(struct idpf_adapter *adapter, enum virtchnl_ops op, > + uint16_t msg_size, uint8_t *msg) > +{ > + struct idpf_ctlq_msg *ctlq_msg; > + struct idpf_dma_mem *dma_mem; > + int err; > + > + err = idpf_vc_clean(adapter); > + if (err != 0) > + goto err; > + > + ctlq_msg = (struct idpf_ctlq_msg *)rte_zmalloc(NULL, > + sizeof(struct idpf_ctlq_msg), 0); Type cast from void * is not required in C. Fix it everywhere, please. [snip] > +int > +idpf_vc_get_caps(struct idpf_adapter *adapter) > +{ > + struct virtchnl2_get_capabilities caps_msg; > + struct idpf_cmd_info args; > + int err; > + > + memset(&caps_msg, 0, sizeof(struct virtchnl2_get_capabilities)); > + caps_msg.csum_caps = > + VIRTCHNL2_CAP_TX_CSUM_L3_IPV4 | > + VIRTCHNL2_CAP_TX_CSUM_L4_IPV4_TCP | > + VIRTCHNL2_CAP_TX_CSUM_L4_IPV4_UDP | > + VIRTCHNL2_CAP_TX_CSUM_L4_IPV4_SCTP | > + VIRTCHNL2_CAP_TX_CSUM_L4_IPV6_TCP | > + VIRTCHNL2_CAP_TX_CSUM_L4_IPV6_UDP | > + VIRTCHNL2_CAP_TX_CSUM_L4_IPV6_SCTP | > + VIRTCHNL2_CAP_TX_CSUM_GENERIC | > + VIRTCHNL2_CAP_RX_CSUM_L3_IPV4 | > + VIRTCHNL2_CAP_RX_CSUM_L4_IPV4_TCP | > + VIRTCHNL2_CAP_RX_CSUM_L4_IPV4_UDP | > + VIRTCHNL2_CAP_RX_CSUM_L4_IPV4_SCTP | > + VIRTCHNL2_CAP_RX_CSUM_L4_IPV6_TCP | > + VIRTCHNL2_CAP_RX_CSUM_L4_IPV6_UDP | > + VIRTCHNL2_CAP_RX_CSUM_L4_IPV6_SCTP | > + VIRTCHNL2_CAP_RX_CSUM_GENERIC; > + > + caps_msg.seg_caps = > + VIRTCHNL2_CAP_SEG_IPV4_TCP | > + VIRTCHNL2_CAP_SEG_IPV4_UDP | > + VIRTCHNL2_CAP_SEG_IPV4_SCTP | > + VIRTCHNL2_CAP_SEG_IPV6_TCP | > + VIRTCHNL2_CAP_SEG_IPV6_UDP | > + VIRTCHNL2_CAP_SEG_IPV6_SCTP | > + VIRTCHNL2_CAP_SEG_GENERIC; > + > + caps_msg.rss_caps = > + VIRTCHNL2_CAP_RSS_IPV4_TCP | > + VIRTCHNL2_CAP_RSS_IPV4_UDP | > + VIRTCHNL2_CAP_RSS_IPV4_SCTP | > + VIRTCHNL2_CAP_RSS_IPV4_OTHER | > + VIRTCHNL2_CAP_RSS_IPV6_TCP | > + VIRTCHNL2_CAP_RSS_IPV6_UDP | > + VIRTCHNL2_CAP_RSS_IPV6_SCTP | > + VIRTCHNL2_CAP_RSS_IPV6_OTHER | > + VIRTCHNL2_CAP_RSS_IPV4_AH | > + VIRTCHNL2_CAP_RSS_IPV4_ESP | > + VIRTCHNL2_CAP_RSS_IPV4_AH_ESP | > + VIRTCHNL2_CAP_RSS_IPV6_AH | > + VIRTCHNL2_CAP_RSS_IPV6_ESP | > + VIRTCHNL2_CAP_RSS_IPV6_AH_ESP; > + > + caps_msg.hsplit_caps = > + VIRTCHNL2_CAP_RX_HSPLIT_AT_L2 | > + VIRTCHNL2_CAP_RX_HSPLIT_AT_L3 | > + VIRTCHNL2_CAP_RX_HSPLIT_AT_L4V4 | > + VIRTCHNL2_CAP_RX_HSPLIT_AT_L4V6; > + > + caps_msg.rsc_caps = > + VIRTCHNL2_CAP_RSC_IPV4_TCP | > + VIRTCHNL2_CAP_RSC_IPV4_SCTP | > + VIRTCHNL2_CAP_RSC_IPV6_TCP | > + VIRTCHNL2_CAP_RSC_IPV6_SCTP; > + > + caps_msg.other_caps = > + VIRTCHNL2_CAP_RDMA | > + VIRTCHNL2_CAP_SRIOV | > + VIRTCHNL2_CAP_MACFILTER | > + VIRTCHNL2_CAP_FLOW_DIRECTOR | > + VIRTCHNL2_CAP_SPLITQ_QSCHED | > + VIRTCHNL2_CAP_CRC | > + VIRTCHNL2_CAP_WB_ON_ITR | > + VIRTCHNL2_CAP_PROMISC | > + VIRTCHNL2_CAP_LINK_SPEED | > + VIRTCHNL2_CAP_VLAN; I'm wondering why all above capabilities are mentioned in the patch? What does the API do? Do it is request it? Negotiage? > + > + args.ops = VIRTCHNL2_OP_GET_CAPS; > + args.in_args = (uint8_t *)&caps_msg; > + args.in_args_size = sizeof(caps_msg); > + args.out_buffer = adapter->mbx_resp; > + args.out_size = IDPF_DFLT_MBX_BUF_SIZE; > + > + err = idpf_execute_vc_cmd(adapter, &args); > + if (err != 0) { > + PMD_DRV_LOG(ERR, > + "Failed to execute command of VIRTCHNL2_OP_GET_CAPS"); > + return err; > + } > + > + rte_memcpy(adapter->caps, args.out_buffer, sizeof(caps_msg)); > + > + return 0; > +}