[PATCH v5 6/8] net/gve: add support for dev info get and dev configure

[Ferruh Yigit]

On 10/20/2022 10:29 AM, Guo, Junfeng wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit at amd.com>
>> Sent: Wednesday, October 19, 2022 21:49
>> To: Guo, Junfeng <junfeng.guo at intel.com>; Zhang, Qi Z
>> <qi.z.zhang at intel.com>; Wu, Jingjing <jingjing.wu at intel.com>
>> Cc: ferruh.yigit at xilinx.com; dev at dpdk.org; Li, Xiaoyun
>> <xiaoyun.li at intel.com>; awogbemila at google.com; Richardson, Bruce
>> <bruce.richardson at intel.com>; Lin, Xueqin <xueqin.lin at intel.com>
>> Subject: Re: [PATCH v5 6/8] net/gve: add support for dev info get and dev
>> configure
>>
>> On 10/10/2022 11:17 AM, Junfeng Guo wrote:
>>
>>>
>>> Add dev_ops dev_infos_get.
>>> Complete dev_configure with RX offloads configuration.
>>>
>>> Signed-off-by: Xiaoyun Li <xiaoyun.li at intel.com>
>>> Signed-off-by: Junfeng Guo <junfeng.guo at intel.com>
>>
>> <...>
>>
>>> --- a/drivers/net/gve/gve_ethdev.c
>>> +++ b/drivers/net/gve/gve_ethdev.c
>>> @@ -29,8 +29,16 @@ gve_write_version(uint8_t
>> *driver_version_register)
>>>    }
>>>
>>>    static int
>>> -gve_dev_configure(__rte_unused struct rte_eth_dev *dev)
>>> +gve_dev_configure(struct rte_eth_dev *dev)
>>>    {
>>> +       struct gve_priv *priv = dev->data->dev_private;
>>> +
>>> +       if (dev->data->dev_conf.rxmode.mq_mode &
>> RTE_ETH_MQ_RX_RSS_FLAG)
>>> +               dev->data->dev_conf.rxmode.offloads |=
>> RTE_ETH_RX_OFFLOAD_RSS_HASH;
>>> +
>>
>> This is force enabling the feature, we are doing this for PMDs that has
>> the hash value anyway and no additional work or performance loss
>> observed to enable this offload. Otherwise drivers shouldn't update
>> 'dev_conf.rxmode'.
>>
>> Can you please confirm this PMD fits above description? And can you
>> please add a coment that says force enabling the feature?
> 
> Yes, it seems force enabling this offloading is not quite reasonable here.
> This may just follow previous PMD convention, so we decided to remove
> this part in the coming version. Thanks!
> 
>>
>>> +       if (dev->data->dev_conf.rxmode.offloads &
>> RTE_ETH_RX_OFFLOAD_TCP_LRO)
>>> +               priv->enable_rsc = 1;
>>> +
>>>           return 0;
>>>    }
>>>
>>> @@ -94,6 +102,60 @@ gve_dev_close(struct rte_eth_dev *dev)
>>>           return err;
>>>    }
>>>
>>> +static int
>>> +gve_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info
>> *dev_info)
>>> +{
>>> +       struct gve_priv *priv = dev->data->dev_private;
>>> +
>>> +       dev_info->device = dev->device;
>>> +       dev_info->max_mac_addrs = 1;
>>> +       dev_info->max_rx_queues = priv->max_nb_rxq;
>>> +       dev_info->max_tx_queues = priv->max_nb_txq;
>>> +       dev_info->min_rx_bufsize = GVE_MIN_BUF_SIZE;
>>> +       dev_info->max_rx_pktlen = GVE_MAX_RX_PKTLEN;
>>> +       dev_info->max_mtu = RTE_ETHER_MTU;
>>
>> Can you please confirm max MTU this PMD supports is 1500? Meaning it
>> doesn't support jumbo frames etc...
> 
> Actually here is just a workaround solution for the max_mtu info...
> We can only get the max_mtu value via adminq message from the backend.
> But the real one (i.e., priv->max_mtu) we get is 1460, which is less than 1500
> Seems it is the GCP bug or something.
> If we use "dev_info->max_mtu = priv->max_mtu", the testpmd cannot even
> be launched successfully...
> I'll keep this part unchanged with some comments here if no other solutions.
> Please help correct me if you have any other idea. Thanks a lot!
> 

Getting actual value from device is correct thing to do, but it seems 
received value is not good, so OK to keep as it is.
Can you please follow this with GVE?

>>
>>> +       dev_info->min_mtu = RTE_ETHER_MIN_MTU;
>>> +
>>> +       dev_info->rx_offload_capa = 0;
>>> +       dev_info->tx_offload_capa =
>>> +               RTE_ETH_TX_OFFLOAD_MULTI_SEGS   |
>>> +               RTE_ETH_TX_OFFLOAD_IPV4_CKSUM   |
>>> +               RTE_ETH_TX_OFFLOAD_UDP_CKSUM    |
>>> +               RTE_ETH_TX_OFFLOAD_TCP_CKSUM    |
>>> +               RTE_ETH_TX_OFFLOAD_SCTP_CKSUM   |
>>> +               RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>
>> Can you adverstise these capabilities in the patch that implements them?
> 
> Will move this to the corresponding patch, thanks!
>