Re: [PATCH v5 3/8] net/gve: add support for device initialization

[Ferruh Yigit]

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(&reg_bar->driver_version);
+       /* Get max queues to alloc etherdev */
+       max_tx_queues = ioread32be(&reg_bar->max_tx_queues);
+       max_rx_queues = ioread32be(&reg_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?

<...>

+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.


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