On 10/20/2022 10:29 AM, Guo, Junfeng wrote:
CAUTION: This message has originated from an External Source. Please use proper
judgment and caution when opening attachments, clicking links, or responding to
this email.
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@amd.com>
Sent: Thursday, October 20, 2022 05:01
To: Li, Xiaoyun <xiaoyun...@intel.com>; Guo, Junfeng
<junfeng....@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>; Wu,
Jingjing <jingjing...@intel.com>
Cc: ferruh.yi...@xilinx.com; dev@dpdk.org; awogbem...@google.com;
Richardson, Bruce <bruce.richard...@intel.com>; Lin, Xueqin
<xueqin....@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>
Subject: Re: [PATCH v5 3/8] net/gve: add support for device initialization
On 10/19/2022 4:59 PM, Li, Xiaoyun wrote:
Hi
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@amd.com>
Sent: Wednesday, October 19, 2022 14:46
To: Guo, Junfeng <junfeng....@intel.com>; Zhang, Qi Z
<qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com>
Cc: ferruh.yi...@xilinx.com; dev@dpdk.org; Li, Xiaoyun
<xiaoyun...@intel.com>; awogbem...@google.com; Richardson, Bruce
<bruce.richard...@intel.com>; Lin, Xueqin <xueqin....@intel.com>;
Wang,
Haiyue <haiyue.w...@intel.com>
Subject: Re: [PATCH v5 3/8] net/gve: add support for device
initialization
On 10/10/2022 11:17 AM, Junfeng Guo wrote:
Support device init and add following devops skeleton:
- dev_configure
- dev_start
- dev_stop
- dev_close
Note that build system (including doc) is also added in this patch.
Signed-off-by: Haiyue Wang <haiyue.w...@intel.com>
Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com>
Signed-off-by: Junfeng Guo <junfeng....@intel.com>
<...>
diff --git a/doc/guides/rel_notes/release_22_11.rst
b/doc/guides/rel_notes/release_22_11.rst
index fbb575255f..c1162ea1a4 100644
--- a/doc/guides/rel_notes/release_22_11.rst
+++ b/doc/guides/rel_notes/release_22_11.rst
@@ -200,6 +200,11 @@ New Features
into single event containing ``rte_event_vector``
whose event type is ``RTE_EVENT_TYPE_CRYPTODEV_VECTOR``.
+* **Added GVE net PMD**
+
+ * Added the new ``gve`` net driver for Google Virtual Ethernet
devices.
+ * See the :doc:`../nics/gve` NIC guide for more details on this new
driver.
+
Can you please move the block amaong the other ethdev drivers, as
alphabetically sorted?
<...>
+static int
+gve_dev_init(struct rte_eth_dev *eth_dev) {
+ struct gve_priv *priv = eth_dev->data->dev_private;
+ int max_tx_queues, max_rx_queues;
+ struct rte_pci_device *pci_dev;
+ struct gve_registers *reg_bar;
+ rte_be32_t *db_bar;
+ int err;
+
+ eth_dev->dev_ops = &gve_eth_dev_ops;
+
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return 0;
+
+ pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
+
+ reg_bar = pci_dev->mem_resource[GVE_REG_BAR].addr;
+ if (!reg_bar) {
+ PMD_DRV_LOG(ERR, "Failed to map pci bar!");
+ return -ENOMEM;
+ }
+
+ db_bar = pci_dev->mem_resource[GVE_DB_BAR].addr;
+ if (!db_bar) {
+ PMD_DRV_LOG(ERR, "Failed to map doorbell bar!");
+ return -ENOMEM;
+ }
+
+ gve_write_version(®_bar->driver_version);
+ /* Get max queues to alloc etherdev */
+ max_tx_queues = ioread32be(®_bar->max_tx_queues);
+ max_rx_queues = ioread32be(®_bar->max_rx_queues);
+
+ priv->reg_bar0 = reg_bar;
+ priv->db_bar2 = db_bar;
+ priv->pci_dev = pci_dev;
+ priv->state_flags = 0x0;
+
+ priv->max_nb_txq = max_tx_queues;
+ priv->max_nb_rxq = max_rx_queues;
+
+ err = gve_init_priv(priv, false);
+ if (err)
+ return err;
+
+ eth_dev->data->mac_addrs = rte_zmalloc("gve_mac",
sizeof(struct
rte_ether_addr), 0);
+ if (!eth_dev->data->mac_addrs) {
+ PMD_DRV_LOG(ERR, "Failed to allocate memory to store
mac
address");
+ return -ENOMEM;
+ }
+ rte_ether_addr_copy(&priv->dev_addr,
+ eth_dev->data->mac_addrs);
+
Is anything assinged to 'priv->dev_addr' to copy?
Also since there is a 'priv->dev_addr' field, why not use it directly,
instead of
allocating memory for 'eth_dev->data->mac_addrs'?
I mean why not "eth_dev->data->mac_addrs = &priv->dev_addr"?
Makes sense. There's no need to allocate a new memory. @Guo,
Junfeng Can you update this?
Thanks Xiaoyun and Ferruh for the comments!
I tried to update the code as suggested but may get "Invalid Memory"
warning when quit the testpmd. I found it was caused at the function
rte_eth_dev_release_port with " rte_free(eth_dev->data->mac_addrs); ".
Seems that allocating memory for 'eth_dev->data->mac_addrs' is still
needed. Please help correct me if I misunderstood this. Thanks! I'll keep
this part unchanged for the coming patchset first.
No it is not needed, you need to set pointer to NULL on release path to
prevent common code free it (the problem you are getting). There are
samples in various PMDs, please check.
<...>
+struct gve_priv {
+ struct gve_irq_db *irq_dbs; /* array of num_ntfy_blks */
+ const struct rte_memzone *irq_dbs_mz;
+ uint32_t mgmt_msix_idx;
+ rte_be32_t *cnt_array; /* array of num_event_counters */
+ const struct rte_memzone *cnt_array_mz;
+
+ uint16_t num_event_counters;
+ uint16_t tx_desc_cnt; /* txq size */
+ uint16_t rx_desc_cnt; /* rxq size */
+ uint16_t tx_pages_per_qpl; /* tx buffer length */
+ uint16_t rx_data_slot_cnt; /* rx buffer length */
These fields are not used in this patch, I guess some will be used in
datapath
patch.
This is needed for base code gve_adminq.c not for datapath. Most of
the stuff in gve_priv is for gve_adminq.c.
The adminq will update this info which dpdk pmd will need later.
Compiler will complain if these don't exsit.
You are right they are used by 'gve_adminq.c', so OK to keep them, if
there are ones not used at this stage, can you add them whenever they
are used, or remove them if not used at all. If all used/required, no
change required.
Yes, we have already tried to move all the unused items to the corresponding
stages patch by patch. Thanks for reminding this!
thanks.
Can you please only add fields that is used in the patch? This way it will
be
clear in which functionality that field is used and enable to detect not
used
fields.
We are accepting batch updates for base code, but this is dpdk related
code,
lets only add things that are used when they are used.
Same for all data structures.
<...>
diff --git a/drivers/net/gve/version.map
b/drivers/net/gve/version.map
new file mode 100644 index 0000000000..c2e0723b4c
--- /dev/null
+++ b/drivers/net/gve/version.map
@@ -0,0 +1,3 @@
+DPDK_22 {
DPDK_23
