| Message ID | 1408505611-6959-2-git-send-email-helin.zhang@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Helin, The title mention i40evf but the patch is related to PF (for VF communication). So I wonder wether it would be clearer to prefix it with i40e? Not sure. 2014-08-20 11:33, Helin Zhang: > To configure VSI queues for VF, Linux PF host supports > operation of I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES with > limited configurations. To support more configurations > (e.g configurable CRC stripping in VF), a new operation > of I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX has been > supported in DPDK PF host. This patch would be easier to read if you could split it in 3 patches: 1) renamings and line wrapping rework 2) introduction of new extended message/operation 3) crc config > - int ret = I40E_SUCCESS; > - struct i40e_virtchnl_vsi_queue_config_info *qconfig = > - (struct i40e_virtchnl_vsi_queue_config_info *)msg; > - int i; > - struct i40e_virtchnl_queue_pair_info *qpair; > - > - if (msg == NULL || msglen <= sizeof(*qconfig) || > - qconfig->num_queue_pairs > vsi->nb_qps) { > + struct i40e_virtchnl_vsi_queue_config_info *vc_vqci = > + (struct i40e_virtchnl_vsi_queue_config_info *)msg; > + struct i40e_virtchnl_queue_pair_info *vc_qpi; > + struct i40e_virtchnl_queue_pair_extra_info *vc_qpei = NULL; > + int i, ret = I40E_SUCCESS; > + > + if (msg == NULL || msglen <= sizeof(*vc_vqci) || > + vc_vqci->num_queue_pairs > vsi->nb_qps) { > PMD_DRV_LOG(ERR, "vsi_queue_config_info argument wrong\n"); > ret = I40E_ERR_PARAM; > goto send_msg; > } > > - qpair = qconfig->qpair; > - for (i = 0; i < qconfig->num_queue_pairs; i++) { > - if (qpair[i].rxq.queue_id > vsi->nb_qps - 1 || > - qpair[i].txq.queue_id > vsi->nb_qps - 1) { > + vc_qpi = vc_vqci->qpair; > + if (opcode == I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX) > + vc_qpei = (struct i40e_virtchnl_queue_pair_extra_info *) > + (((uint8_t *)vc_qpi) + > + (sizeof(struct i40e_virtchnl_queue_pair_info) * > + vc_vqci->num_queue_pairs)); > + > + for (i = 0; i < vc_vqci->num_queue_pairs; i++) { > + if (vc_qpi[i].rxq.queue_id > vsi->nb_qps - 1 || > + vc_qpi[i].txq.queue_id > vsi->nb_qps - 1) { > ret = I40E_ERR_PARAM; > goto send_msg; > } Mixing renaming and new feature makes it difficult to read. > case I40E_VIRTCHNL_OP_ENABLE_QUEUES: > PMD_DRV_LOG(INFO, "OP_ENABLE_QUEUES received\n"); > - i40e_pf_host_process_cmd_enable_queues(vf, > - msg, msglen); > + i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen); > break; > case I40E_VIRTCHNL_OP_DISABLE_QUEUES: > PMD_DRV_LOG(INFO, "OP_DISABLE_QUEUE received\n"); > - i40e_pf_host_process_cmd_disable_queues(vf, > - msg, msglen); > + i40e_pf_host_process_cmd_disable_queues(vf, msg, msglen); > break; > case I40E_VIRTCHNL_OP_ADD_ETHER_ADDRESS: > PMD_DRV_LOG(INFO, "OP_ADD_ETHER_ADDRESS received\n"); > - i40e_pf_host_process_cmd_add_ether_address(vf, > - msg, msglen); > + i40e_pf_host_process_cmd_add_ether_address(vf, msg, msglen); > break; > case I40E_VIRTCHNL_OP_DEL_ETHER_ADDRESS: > PMD_DRV_LOG(INFO, "OP_DEL_ETHER_ADDRESS received\n"); > - i40e_pf_host_process_cmd_del_ether_address(vf, > - msg, msglen); > + i40e_pf_host_process_cmd_del_ether_address(vf, msg, msglen); > break; > case I40E_VIRTCHNL_OP_ADD_VLAN: > PMD_DRV_LOG(INFO, "OP_ADD_VLAN received\n"); > @@ -932,10 +951,9 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev, > case I40E_VIRTCHNL_OP_FCOE: > PMD_DRV_LOG(ERR, "OP_FCOE received, not supported\n"); > default: > - PMD_DRV_LOG(ERR, "%u received, not supported\n", > - opcode); > - i40e_pf_host_send_msg_to_vf(vf, opcode, > - I40E_ERR_PARAM, NULL, 0); > + PMD_DRV_LOG(ERR, "%u received, not supported\n", opcode); > + i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM, > + NULL, 0); > break; > } Line wrapping should go in a cleanup patch (like renaming). Thanks
Hi Thomas > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Saturday, August 30, 2014 10:43 PM > To: Zhang, Helin > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/3] i40evf: support > I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in DPDK PF host > > Hi Helin, > > The title mention i40evf but the patch is related to PF (for VF communication). > So I wonder wether it would be clearer to prefix it with i40e? Not sure. > > 2014-08-20 11:33, Helin Zhang: > > To configure VSI queues for VF, Linux PF host supports operation of > > I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES with limited configurations. To > > support more configurations (e.g configurable CRC stripping in VF), a > > new operation of I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX has been > > supported in DPDK PF host. > > This patch would be easier to read if you could split it in 3 patches: > 1) renamings and line wrapping rework > 2) introduction of new extended message/operation > 3) crc config > > > - int ret = I40E_SUCCESS; > > - struct i40e_virtchnl_vsi_queue_config_info *qconfig = > > - (struct i40e_virtchnl_vsi_queue_config_info *)msg; > > - int i; > > - struct i40e_virtchnl_queue_pair_info *qpair; > > - > > - if (msg == NULL || msglen <= sizeof(*qconfig) || > > - qconfig->num_queue_pairs > vsi->nb_qps) { > > + struct i40e_virtchnl_vsi_queue_config_info *vc_vqci = > > + (struct i40e_virtchnl_vsi_queue_config_info *)msg; > > + struct i40e_virtchnl_queue_pair_info *vc_qpi; > > + struct i40e_virtchnl_queue_pair_extra_info *vc_qpei = NULL; > > + int i, ret = I40E_SUCCESS; > > + > > + if (msg == NULL || msglen <= sizeof(*vc_vqci) || > > + vc_vqci->num_queue_pairs > vsi->nb_qps) { > > PMD_DRV_LOG(ERR, "vsi_queue_config_info argument wrong\n"); > > ret = I40E_ERR_PARAM; > > goto send_msg; > > } > > > > - qpair = qconfig->qpair; > > - for (i = 0; i < qconfig->num_queue_pairs; i++) { > > - if (qpair[i].rxq.queue_id > vsi->nb_qps - 1 || > > - qpair[i].txq.queue_id > vsi->nb_qps - 1) { > > + vc_qpi = vc_vqci->qpair; > > + if (opcode == I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX) > > + vc_qpei = (struct i40e_virtchnl_queue_pair_extra_info *) > > + (((uint8_t *)vc_qpi) + > > + (sizeof(struct i40e_virtchnl_queue_pair_info) * > > + vc_vqci->num_queue_pairs)); > > + > > + for (i = 0; i < vc_vqci->num_queue_pairs; i++) { > > + if (vc_qpi[i].rxq.queue_id > vsi->nb_qps - 1 || > > + vc_qpi[i].txq.queue_id > vsi->nb_qps - 1) { > > ret = I40E_ERR_PARAM; > > goto send_msg; > > } > > Mixing renaming and new feature makes it difficult to read. > > > case I40E_VIRTCHNL_OP_ENABLE_QUEUES: > > PMD_DRV_LOG(INFO, "OP_ENABLE_QUEUES received\n"); > > - i40e_pf_host_process_cmd_enable_queues(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_DISABLE_QUEUES: > > PMD_DRV_LOG(INFO, "OP_DISABLE_QUEUE received\n"); > > - i40e_pf_host_process_cmd_disable_queues(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_disable_queues(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_ADD_ETHER_ADDRESS: > > PMD_DRV_LOG(INFO, "OP_ADD_ETHER_ADDRESS received\n"); > > - i40e_pf_host_process_cmd_add_ether_address(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_add_ether_address(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_DEL_ETHER_ADDRESS: > > PMD_DRV_LOG(INFO, "OP_DEL_ETHER_ADDRESS received\n"); > > - i40e_pf_host_process_cmd_del_ether_address(vf, > > - msg, msglen); > > + i40e_pf_host_process_cmd_del_ether_address(vf, msg, msglen); > > break; > > case I40E_VIRTCHNL_OP_ADD_VLAN: > > PMD_DRV_LOG(INFO, "OP_ADD_VLAN received\n"); @@ -932,10 > +951,9 @@ > > i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev, > > case I40E_VIRTCHNL_OP_FCOE: > > PMD_DRV_LOG(ERR, "OP_FCOE received, not supported\n"); > > default: > > - PMD_DRV_LOG(ERR, "%u received, not supported\n", > > - opcode); > > - i40e_pf_host_send_msg_to_vf(vf, opcode, > > - I40E_ERR_PARAM, NULL, 0); > > + PMD_DRV_LOG(ERR, "%u received, not supported\n", opcode); > > + i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM, > > + NULL, 0); > > break; > > } > > Line wrapping should go in a cleanup patch (like renaming). > > Thanks > -- > Thomas Thank you very much! I have reworked the patches and sent out again. Regards, Helin
diff --git a/lib/librte_pmd_i40e/i40e_pf.c b/lib/librte_pmd_i40e/i40e_pf.c index e8b154d..9a03e74 100644 --- a/lib/librte_pmd_i40e/i40e_pf.c +++ b/lib/librte_pmd_i40e/i40e_pf.c @@ -327,7 +327,8 @@ send_msg: static int i40e_pf_host_hmc_config_rxq(struct i40e_hw *hw, struct i40e_pf_vf *vf, - struct i40e_virtchnl_rxq_info *rxq) + struct i40e_virtchnl_rxq_info *rxq, + struct i40e_virtchnl_queue_pair_extra_info *qpei) { int err = I40E_SUCCESS; struct i40e_hmc_obj_rxq rx_ctx; @@ -356,7 +357,10 @@ i40e_pf_host_hmc_config_rxq(struct i40e_hw *hw, rx_ctx.tphdata_ena = 1; rx_ctx.tphhead_ena = 1; rx_ctx.lrxqthresh = 2; - rx_ctx.crcstrip = 1; + if (qpei) /* For DPDK PF host */ + rx_ctx.crcstrip = qpei->crcstrip ? 1 : 0; + else /* For Linux PF host */ + rx_ctx.crcstrip = 1; rx_ctx.l2tsel = 1; rx_ctx.prefena = 1; @@ -411,42 +415,56 @@ i40e_pf_host_hmc_config_txq(struct i40e_hw *hw, static int i40e_pf_host_process_cmd_config_vsi_queues(struct i40e_pf_vf *vf, uint8_t *msg, - uint16_t msglen) + uint16_t msglen, + int opcode) { struct i40e_hw *hw = I40E_PF_TO_HW(vf->pf); struct i40e_vsi *vsi = vf->vsi; - int ret = I40E_SUCCESS; - struct i40e_virtchnl_vsi_queue_config_info *qconfig = - (struct i40e_virtchnl_vsi_queue_config_info *)msg; - int i; - struct i40e_virtchnl_queue_pair_info *qpair; - - if (msg == NULL || msglen <= sizeof(*qconfig) || - qconfig->num_queue_pairs > vsi->nb_qps) { + struct i40e_virtchnl_vsi_queue_config_info *vc_vqci = + (struct i40e_virtchnl_vsi_queue_config_info *)msg; + struct i40e_virtchnl_queue_pair_info *vc_qpi; + struct i40e_virtchnl_queue_pair_extra_info *vc_qpei = NULL; + int i, ret = I40E_SUCCESS; + + if (msg == NULL || msglen <= sizeof(*vc_vqci) || + vc_vqci->num_queue_pairs > vsi->nb_qps) { PMD_DRV_LOG(ERR, "vsi_queue_config_info argument wrong\n"); ret = I40E_ERR_PARAM; goto send_msg; } - qpair = qconfig->qpair; - for (i = 0; i < qconfig->num_queue_pairs; i++) { - if (qpair[i].rxq.queue_id > vsi->nb_qps - 1 || - qpair[i].txq.queue_id > vsi->nb_qps - 1) { + vc_qpi = vc_vqci->qpair; + if (opcode == I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX) + vc_qpei = (struct i40e_virtchnl_queue_pair_extra_info *) + (((uint8_t *)vc_qpi) + + (sizeof(struct i40e_virtchnl_queue_pair_info) * + vc_vqci->num_queue_pairs)); + + for (i = 0; i < vc_vqci->num_queue_pairs; i++) { + if (vc_qpi[i].rxq.queue_id > vsi->nb_qps - 1 || + vc_qpi[i].txq.queue_id > vsi->nb_qps - 1) { ret = I40E_ERR_PARAM; goto send_msg; } - /* Apply VF RX queue setting to HMC */ - if (i40e_pf_host_hmc_config_rxq(hw, vf, &qpair[i].rxq) - != I40E_SUCCESS) { + /* + * Apply VF RX queue setting to HMC. + * If the opcode is I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX, + * then the extra information of + * 'struct i40e_virtchnl_queue_pair_extra_info' is needed, + * otherwise set the last parameter to NULL. + */ + if (i40e_pf_host_hmc_config_rxq(hw, vf, &vc_qpi[i].rxq, + (vc_qpei != NULL ? (&vc_qpei[i]) : NULL)) != + I40E_SUCCESS) { PMD_DRV_LOG(ERR, "Configure RX queue HMC failed"); ret = I40E_ERR_PARAM; goto send_msg; } /* Apply VF TX queue setting to HMC */ - if (i40e_pf_host_hmc_config_txq(hw, vf, &qpair[i].txq) - != I40E_SUCCESS) { + if (i40e_pf_host_hmc_config_txq(hw, vf, + &vc_qpi[i].txq) != I40E_SUCCESS) { PMD_DRV_LOG(ERR, "Configure TX queue HMC failed"); ret = I40E_ERR_PARAM; goto send_msg; @@ -454,8 +472,8 @@ i40e_pf_host_process_cmd_config_vsi_queues(struct i40e_pf_vf *vf, } send_msg: - i40e_pf_host_send_msg_to_vf(vf, I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES, - ret, NULL, 0); + i40e_pf_host_send_msg_to_vf(vf, opcode, ret, NULL, 0); + return ret; } @@ -871,8 +889,13 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev, break; case I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES: PMD_DRV_LOG(INFO, "OP_CONFIG_VSI_QUEUES received\n"); - i40e_pf_host_process_cmd_config_vsi_queues(vf, - msg, msglen); + i40e_pf_host_process_cmd_config_vsi_queues(vf, msg, + msglen, opcode); + break; + case I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX: + PMD_DRV_LOG(INFO, "OP_CONFIG_VSI_QUEUES_EX received\n"); + i40e_pf_host_process_cmd_config_vsi_queues(vf, msg, + msglen, opcode); break; case I40E_VIRTCHNL_OP_CONFIG_IRQ_MAP: PMD_DRV_LOG(INFO, "OP_CONFIG_IRQ_MAP received\n"); @@ -880,23 +903,19 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev, break; case I40E_VIRTCHNL_OP_ENABLE_QUEUES: PMD_DRV_LOG(INFO, "OP_ENABLE_QUEUES received\n"); - i40e_pf_host_process_cmd_enable_queues(vf, - msg, msglen); + i40e_pf_host_process_cmd_enable_queues(vf, msg, msglen); break; case I40E_VIRTCHNL_OP_DISABLE_QUEUES: PMD_DRV_LOG(INFO, "OP_DISABLE_QUEUE received\n"); - i40e_pf_host_process_cmd_disable_queues(vf, - msg, msglen); + i40e_pf_host_process_cmd_disable_queues(vf, msg, msglen); break; case I40E_VIRTCHNL_OP_ADD_ETHER_ADDRESS: PMD_DRV_LOG(INFO, "OP_ADD_ETHER_ADDRESS received\n"); - i40e_pf_host_process_cmd_add_ether_address(vf, - msg, msglen); + i40e_pf_host_process_cmd_add_ether_address(vf, msg, msglen); break; case I40E_VIRTCHNL_OP_DEL_ETHER_ADDRESS: PMD_DRV_LOG(INFO, "OP_DEL_ETHER_ADDRESS received\n"); - i40e_pf_host_process_cmd_del_ether_address(vf, - msg, msglen); + i40e_pf_host_process_cmd_del_ether_address(vf, msg, msglen); break; case I40E_VIRTCHNL_OP_ADD_VLAN: PMD_DRV_LOG(INFO, "OP_ADD_VLAN received\n"); @@ -932,10 +951,9 @@ i40e_pf_host_handle_vf_msg(struct rte_eth_dev *dev, case I40E_VIRTCHNL_OP_FCOE: PMD_DRV_LOG(ERR, "OP_FCOE received, not supported\n"); default: - PMD_DRV_LOG(ERR, "%u received, not supported\n", - opcode); - i40e_pf_host_send_msg_to_vf(vf, opcode, - I40E_ERR_PARAM, NULL, 0); + PMD_DRV_LOG(ERR, "%u received, not supported\n", opcode); + i40e_pf_host_send_msg_to_vf(vf, opcode, I40E_ERR_PARAM, + NULL, 0); break; } } diff --git a/lib/librte_pmd_i40e/i40e_pf.h b/lib/librte_pmd_i40e/i40e_pf.h index 41b6826..19e1379 100644 --- a/lib/librte_pmd_i40e/i40e_pf.h +++ b/lib/librte_pmd_i40e/i40e_pf.h @@ -57,7 +57,14 @@ enum i40e_virtchnl_ops_DPDK { I40E_VIRTCHNL_OP_GET_LINK_STAT = I40E_VIRTCHNL_OP_EVENT + 0x100, I40E_VIRTCHNL_OP_CFG_VLAN_OFFLOAD, I40E_VIRTCHNL_OP_CFG_VLAN_PVID, + I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX, }; + +/* DPDK extra configurations for queues */ +struct i40e_virtchnl_queue_pair_extra_info { + uint8_t crcstrip; +}; + struct i40e_virtchnl_vlan_offload_info { uint16_t vsi_id; uint8_t enable_vlan_strip;