RE: [PATCH v4 3/9] net/gve: add support for device initialization

["Guo, Junfeng" <junfeng.guo@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, October 6, 2022 22:23
> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
> awogbemila@google.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Lin, Xueqin <xueqin.lin@intel.com>;
> Wang, Haiyue <haiyue.wang@intel.com>
> Subject: Re: [PATCH v4 3/9] net/gve: add support for device initialization
> 
> On 9/27/2022 8:32 AM, Junfeng Guo wrote:
> 
> >
> > Support device init and the fowllowing devops:
> 
> s/fowllowing/following/
> 
> > - dev_configure
> > - dev_start
> > - dev_stop
> > - dev_close
> 
> At this stage most of above are empty functions and not implemented yet,
> instead can you document in the commit log that build system and device
> initialization is added?

Agreed, will add this in the coming version. Thanks!

> 
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> 
> <...>
> 
> > --- /dev/null
> > +++ b/doc/guides/nics/gve.rst
> > @@ -0,0 +1,69 @@
> > +..  SPDX-License-Identifier: BSD-3-Clause
> > +    Copyright(C) 2022 Intel Corporation.
> > +
> > +GVE poll mode driver
> > +=======================
> > +
> > +The GVE PMD (**librte_net_gve**) provides poll mode driver support
> for
> > +Google Virtual Ethernet device.
> > +
> > +Please refer to
> https://cloud.google.com/compute/docs/networking/using-gvnic
> > +for the device description.
> > +
> 
> This seems another virtual interface, similar to iavf/virtio/idpf ...
> 
> Can you please briefly describe here the motivation to add yet another
> virtual interface, and again briefly describe cons/pros of the interface?

Sure. According to the official gVNIC description, gVNIC is an alternative
to the virtio driver that can provide higher network bandwidths.
Will add the brief descriptions of the cons/pros in the coming version.
Thanks!

> 
> > +The base code is under MIT license and based on GVE kernel driver
> v1.3.0.
> > +GVE base code files are:
> > +
> > +- gve_adminq.h
> > +- gve_adminq.c
> > +- gve_desc.h
> > +- gve_desc_dqo.h
> > +- gve_register.h
> > +- gve.h
> > +
> > +Please refer to https://github.com/GoogleCloudPlatform/compute-
> virtual-ethernet-linux/tree/v1.3.0/google/gve
> > +to find the original base code.
> > +
> > +GVE has 3 queue formats:
> > +
> > +- GQI_QPL - GQI with queue page list
> > +- GQI_RDA - GQI with raw DMA addressing
> > +- DQO_RDA - DQO with raw DMA addressing
> > +
> > +GQI_QPL queue format is queue page list mode. Driver needs to
> allocate
> > +memory and register this memory as a Queue Page List (QPL) in
> hardware
> > +(Google Hypervisor/GVE Backend) first. Each queue has its own QPL.
> > +Then Tx needs to copy packets to QPL memory and put this packet's
> offset
> > +in the QPL memory into hardware descriptors so that hardware can get
> the
> > +packets data. And Rx needs to read descriptors of offset in QPL to get
> > +QPL address and copy packets from the address to get real packets
> data.
> > +
> > +GQI_RDA queue format works like usual NICs that driver can put
> packets'
> > +physical address into hardware descriptors.
> > +
> > +DQO_RDA queue format has submission and completion queue pair for
> each
> > +Tx/Rx queue. And similar as GQI_RDA, driver can put packets' physical
> > +address into hardware descriptors.
> > +
> > +Please refer to
> https://www.kernel.org/doc/html/latest/networking/device_drivers/ethe
> rnet/google/gve.html
> > +to get more information about GVE queue formats.
> > +
> > +Features and Limitations
> > +------------------------
> > +
> > +In this release, the GVE PMD provides the basic functionality of packet
> > +reception and transmission.
> > +Supported features of the GVE PMD are:
> > +
> > +- Multiple queues for TX and RX
> > +- Receiver Side Scaling (RSS)
> > +- TSO offload
> > +- Port hardware statistics
> > +- Link state information
> > +- TX multi-segments (Scatter TX)
> > +- Tx UDP/TCP/SCTP Checksum
> > +
> 
> Can you build this list gradully, by adding relavent item in each patch
> that adds it?
> That way mapping with the code and documented feature becomes more
> obvious.

Sure... Will add the items of this list gradually in the coming version. Thanks!

> 
> <...>
> 
> > +static int
> > +gve_dev_uninit(struct rte_eth_dev *eth_dev)
> > +{
> > +       struct gve_priv *priv = eth_dev->data->dev_private;
> > +
> > +       eth_dev->data->mac_addrs = NULL;
> > +
> 
> At this stage 'mac_addrs' is not freed, setting it to NULL prevents it
> to be freed.

Thanks for the catch! Will improve this. Thanks!

> 
> <...>
> 
> > +
> > +static struct rte_pci_driver rte_gve_pmd = {
> > +       .id_table = pci_id_gve_map,
> > +       .drv_flags = RTE_PCI_DRV_NEED_MAPPING |
> RTE_PCI_DRV_INTR_LSC,
> 
> As far as I can see LSC interrupt is not supported, if that is correct
> should we drop the flag?

Sure, seems this flag is not used in current GCP env.
Will remove it in the coming version. Thanks!

> 
> <...>
> 
> > +
> > +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 */
> > +
> > +       /* Only valid for DQO_RDA queue format */
> > +       uint16_t tx_compq_size; /* tx completion queue size */
> > +       uint16_t rx_bufq_size; /* rx buff queue size */
> > +
> > +       uint64_t max_registered_pages;
> > +       uint64_t num_registered_pages; /* num pages registered with NIC
> */
> > +       uint16_t default_num_queues; /* default num queues to set up */
> > +       enum gve_queue_format queue_format; /* see enum
> gve_queue_format */
> > +       uint8_t enable_rsc;
> > +
> > +       uint16_t max_nb_txq;
> > +       uint16_t max_nb_rxq;
> > +       uint32_t num_ntfy_blks; /* spilt between TX and RX so must be
> even */
> > +
> > +       struct gve_registers __iomem *reg_bar0; /* see gve_register.h */
> > +       rte_be32_t __iomem *db_bar2; /* "array" of doorbells */
> > +       struct rte_pci_device *pci_dev;
> > +
> > +       /* Admin queue - see gve_adminq.h*/
> > +       union gve_adminq_command *adminq;
> > +       struct gve_dma_mem adminq_dma_mem;
> > +       uint32_t adminq_mask; /* masks prod_cnt to adminq size */
> > +       uint32_t adminq_prod_cnt; /* free-running count of AQ cmds
> executed */
> > +       uint32_t adminq_cmd_fail; /* free-running count of AQ cmds failed
> */
> > +       uint32_t adminq_timeouts; /* free-running count of AQ cmds
> timeouts */
> > +       /* free-running count of per AQ cmd executed */
> > +       uint32_t adminq_describe_device_cnt;
> > +       uint32_t adminq_cfg_device_resources_cnt;
> > +       uint32_t adminq_register_page_list_cnt;
> > +       uint32_t adminq_unregister_page_list_cnt;
> > +       uint32_t adminq_create_tx_queue_cnt;
> > +       uint32_t adminq_create_rx_queue_cnt;
> > +       uint32_t adminq_destroy_tx_queue_cnt;
> > +       uint32_t adminq_destroy_rx_queue_cnt;
> > +       uint32_t adminq_dcfg_device_resources_cnt;
> > +       uint32_t adminq_set_driver_parameter_cnt;
> > +       uint32_t adminq_report_stats_cnt;
> > +       uint32_t adminq_report_link_speed_cnt;
> > +       uint32_t adminq_get_ptype_map_cnt;
> > +
> > +       volatile uint32_t state_flags;
> > +
> > +       /* Gvnic device link speed from hypervisor. */
> > +       uint64_t link_speed;
> > +
> > +       uint16_t max_mtu;
> > +       struct rte_ether_addr dev_addr; /* mac address */
> > +
> > +       struct gve_queue_page_list *qpl;
> > +
> > +       struct gve_tx_queue **txqs;
> > +       struct gve_rx_queue **rxqs;
> > +};
> > +
> 
> Similar to previous comment, can you construct the headers by only
> adding used fields in that patch?
> 
> When batch copied an existing struct, it is very easy to add unused code
> and very hard to detect it. So if you only add what you need, that
> becomes easy to be sure all fields are used.
> 
> Also it makes more obvious which fields related to which feature.

Sure... Will try best to construct the header structure items gradually.
Thanks!

> 
> <...>
> 
> > new file mode 100644
> > index 0000000000..c2e0723b4c
> > --- /dev/null
> > +++ b/drivers/net/gve/version.map
> > @@ -0,0 +1,3 @@
> > +DPDK_22 {
> > +       local: *;
> > +};
> 
> it is 'DPDK_23' now, hopefully we will have an update to get rid of
> empty map files, feel free to review:
> https://patches.dpdk.org/project/dpdk/list/?series=25002

Sure, it looks much better. Thanks!

>