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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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.yigit@amd.com>
>> Sent: Thursday, October 20, 2022 05:01
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Guo, Junfeng
>> <junfeng.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
>> Jingjing <jingjing.wu@intel.com>
>> Cc: ferruh.yigit@xilinx.com; dev@dpdk.org; awogbemila@google.com;
>> Richardson, Bruce <bruce.richardson@intel.com>; Lin, Xueqin
>> <xueqin.lin@intel.com>; Wang, Haiyue <haiyue.wang@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.yigit@amd.com>
>>>> Sent: Wednesday, October 19, 2022 14:46
>>>> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
>>>> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
>>>> Cc: ferruh.yigit@xilinx.com; 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 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.wang@intel.com>
>>>>> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
>>>>> Signed-off-by: Junfeng Guo <junfeng.guo@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?
> 
> 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
>