> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Thursday, October 6, 2022 22:20
> 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 v4 1/9] net/gve/base: introduce GVE PMD base code
>
> On 9/27/2022 8:32 AM, Junfeng Guo wrote:
>
> >
> > The following base code is based on Google Virtual Ethernet (gve)
> > driver v1.3.0 under MIT license.
> > - gve_adminq.c
> > - gve_adminq.h
> > - gve_desc.h
> > - gve_desc_dqo.h
> > - gve_register.h
> > - gve.h
> >
> > The original code is in:
> > https://github.com/GoogleCloudPlatform/compute-virtual-ethernet-
> linux/\
> > tree/v1.3.0/google/gve
> >
> > Note that these code are not Intel files and they come from the kernel
> > community. The base code there has the statement of
> > SPDX-License-Identifier: (GPL-2.0 OR MIT). Here we just follow the
> > required MIT license as an exception to DPDK.
> >
> > Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com>
> > Signed-off-by: Haiyue Wang <haiyue.w...@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng....@intel.com>
>
> <...>
>
> > +/* Process all device options for a given describe device call. */
> > +static int
> > +gve_process_device_options(struct gve_priv *priv,
> > + struct gve_device_descriptor *descriptor,
> > + struct gve_device_option_gqi_rda
> > **dev_op_gqi_rda,
> > + struct gve_device_option_gqi_qpl
> > **dev_op_gqi_qpl,
> > + struct gve_device_option_dqo_rda
> > **dev_op_dqo_rda,
> > + struct gve_device_option_jumbo_frames
> **dev_op_jumbo_frames)
> > +{
> > + const int num_options = be16_to_cpu(descriptor-
> >num_device_options);
> > + struct gve_device_option *dev_opt;
> > + int i;
> > +
> > + /* The options struct directly follows the device descriptor. */
> > + dev_opt = RTE_PTR_ADD(descriptor, sizeof(*descriptor));
> > + for (i = 0; i < num_options; i++) {
> > + struct gve_device_option *next_opt;
> > +
> > + next_opt = gve_get_next_option(descriptor, dev_opt);
> > + if (!next_opt) {
> > + PMD_DRV_LOG(ERR,
> > + "options exceed device_descriptor's
> > total length.");
> > + return -EINVAL;
> > + }
> > +
> > + gve_parse_device_option(priv, dev_opt,
> > + dev_op_gqi_rda, dev_op_gqi_qpl,
> > + dev_op_dqo_rda,
> > dev_op_jumbo_frames);
> > + dev_opt = next_opt;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int gve_adminq_alloc(struct gve_priv *priv)
>
> Can you please be consistent in the syntax, at least within same file,
> if this file has slightly different syntax because it is base file, keep
> the file syntax instead of mixing with DPDK syntax,
> like return type of function should be on separate line.
>
> A generic comment for all base files.
Thanks for reminding!
Will keep the same syntax with kernel code for the files within the base folder.
And use DPDK syntax for the files in the upper folder of the base.
For the base files, it would be better to keep most of the code with the
origins.
>
> <...>
>
> > +int gve_adminq_describe_device(struct gve_priv *priv)
> > +{
> > + struct gve_device_option_jumbo_frames *dev_op_jumbo_frames
> = NULL;
> > + struct gve_device_option_gqi_rda *dev_op_gqi_rda = NULL;
> > + struct gve_device_option_gqi_qpl *dev_op_gqi_qpl = NULL;
> > + struct gve_device_option_dqo_rda *dev_op_dqo_rda = NULL;
> > + struct gve_device_descriptor *descriptor;
> > + struct gve_dma_mem descriptor_dma_mem;
> > + u32 supported_features_mask = 0;
> > + union gve_adminq_command cmd;
> > + int err = 0;
> > + u8 *mac;
> > + u16 mtu;
> > +
> > + memset(&cmd, 0, sizeof(cmd));
> > + descriptor = gve_alloc_dma_mem(&descriptor_dma_mem,
> PAGE_SIZE);
> > + if (!descriptor)
> > + return -ENOMEM;
> > + cmd.opcode = cpu_to_be32(GVE_ADMINQ_DESCRIBE_DEVICE);
> > + cmd.describe_device.device_descriptor_addr =
> > + cpu_to_be64(descriptor_dma_mem.pa);
> > + cmd.describe_device.device_descriptor_version =
> > +
> cpu_to_be32(GVE_ADMINQ_DEVICE_DESCRIPTOR_VERSION);
> > + cmd.describe_device.available_length = cpu_to_be32(PAGE_SIZE);
> > +
> > + err = gve_adminq_execute_cmd(priv, &cmd);
> > + if (err)
> > + goto free_device_descriptor;
> > +
> > + err = gve_process_device_options(priv, descriptor,
> &dev_op_gqi_rda,
> > + &dev_op_gqi_qpl, &dev_op_dqo_rda,
> > + &dev_op_jumbo_frames);
> > + if (err)
> > + goto free_device_descriptor;
> > +
> > + /* If the GQI_RAW_ADDRESSING option is not enabled and the
> queue format
> > + * is not set to GqiRda, choose the queue format in a priority
> > order:
> > + * DqoRda, GqiRda, GqiQpl. Use GqiQpl as default.
> > + */
> > + if (dev_op_dqo_rda) {
> > + priv->queue_format = GVE_DQO_RDA_FORMAT;
> > + PMD_DRV_LOG(INFO, "Driver is running with DQO RDA queue
> format.");
> > + supported_features_mask =
> > + be32_to_cpu(dev_op_dqo_rda-
> >supported_features_mask);
> > + } else if (dev_op_gqi_rda) {
> > + priv->queue_format = GVE_GQI_RDA_FORMAT;
> > + PMD_DRV_LOG(INFO, "Driver is running with GQI RDA queue
> format.");
> > + supported_features_mask =
> > + be32_to_cpu(dev_op_gqi_rda-
> >supported_features_mask);
> > + } else if (priv->queue_format == GVE_GQI_RDA_FORMAT) {
> > + PMD_DRV_LOG(INFO, "Driver is running with GQI RDA queue
> format.");
> > + } else {
> > + priv->queue_format = GVE_GQI_QPL_FORMAT;
> > + if (dev_op_gqi_qpl)
> > + supported_features_mask =
> > + be32_to_cpu(dev_op_gqi_qpl-
> >supported_features_mask);
> > + PMD_DRV_LOG(INFO, "Driver is running with GQI QPL queue
> format.");
> > + }
> > + if (gve_is_gqi(priv)) {
> > + err = gve_set_desc_cnt(priv, descriptor);
> > + } else {
> > + /* DQO supports LRO. */
> > + err = gve_set_desc_cnt_dqo(priv, descriptor,
> dev_op_dqo_rda);
> > + }
> > + if (err)
> > + goto free_device_descriptor;
> > +
> > + priv->max_registered_pages =
> > +
> > be64_to_cpu(descriptor->max_registered_pages);
> > + mtu = be16_to_cpu(descriptor->mtu);
> > + if (mtu < ETH_MIN_MTU) {
> > + PMD_DRV_LOG(ERR, "MTU %d below minimum MTU", mtu);
> > + err = -EINVAL;
> > + goto free_device_descriptor;
> > + }
> > + priv->max_mtu = mtu;
> > + priv->num_event_counters = be16_to_cpu(descriptor->counters);
> > + rte_memcpy(priv->dev_addr.addr_bytes, descriptor->mac,
> ETH_ALEN);
> > + mac = descriptor->mac;
> > + PMD_DRV_LOG(INFO, "MAC
> addr: %02x:%02x:%02x:%02x:%02x:%02x",
> > + mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> > + priv->tx_pages_per_qpl = be16_to_cpu(descriptor-
> >tx_pages_per_qpl);
> > + priv->rx_data_slot_cnt = be16_to_cpu(descriptor-
> >rx_pages_per_qpl);
> > +
> > + if (gve_is_gqi(priv) && priv->rx_data_slot_cnt < priv->rx_desc_cnt)
> {
> > + PMD_DRV_LOG(ERR, "rx_data_slot_cnt cannot be smaller
> than rx_desc_cnt, setting rx_desc_cnt down to %d",
>
> Can you try to reduce the line length as;
> PMD_DRV_LOG(ERR,
> "rx_data_slot_cnt cannot be smaller than rx_desc_cnt, setting
> rx_desc_cnt down to %d",
Sure, it could be. But it would still reach the 100 characters limit even
started
with a new line... Good news is that it is only log that won't trigger warnings
while running the build.
