[PATCH 0/3] support of configurable CRC stripping in VF

[PATCH 0/3] support of configurable CRC stripping in VF

[Helin Zhang]

To support configurable CRC stripping in VF, a new
operation of I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX
is added to carry more configuration information from
VM to Intel(r) DPDK PF host, comparing to
I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES which is supported
by Linux PF host.

Helin Zhang (3):
  i40evf: support I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in DPDK PF host
  i40evf: support I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in i40e VF PMD
  config: remove useless config of
    CONFIG_RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC

 config/common_bsdapp                 |   1 -
 config/common_linuxapp               |   1 -
 lib/librte_pmd_i40e/i40e_ethdev_vf.c | 188 ++++++++++++++++++++++++-----------
 lib/librte_pmd_i40e/i40e_pf.c        |  90 ++++++++++-------
 lib/librte_pmd_i40e/i40e_pf.h        |   7 ++
 5 files changed, 191 insertions(+), 96 deletions(-)

-- 
1.8.1.4

[PATCH 1/3] i40evf: support I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in DPDK PF host

[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.

Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jing Chen <jing.d.chen-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_i40e/i40e_pf.c | 90 ++++++++++++++++++++++++++-----------------
 lib/librte_pmd_i40e/i40e_pf.h |  7 ++++
 2 files changed, 61 insertions(+), 36 deletions(-)

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;
-- 
1.8.1.4

Re: [PATCH 1/3] i40evf: support I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in DPDK PF host

[Thomas Monjalon]

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

Re: [PATCH 1/3] i40evf: support I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in DPDK PF host

[Zhang, Helin]

Hi Thomas

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Saturday, August 30, 2014 10:43 PM
> To: Zhang, Helin
> Cc: dev-VfR2kkLFssw@public.gmane.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

[PATCH 2/3] i40evf: support I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in i40e VF PMD

[Helin Zhang]

To support configurable CRC in VF, use operation of
I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX to carry more
information from VM to PF host, if the peer is DPDK
PF host. Otherwise assume it is Linux PF host and
just use operation of I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES.

Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jing Chen <jing.d.chen-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_i40e/i40e_ethdev_vf.c | 188 ++++++++++++++++++++++++-----------
 1 file changed, 130 insertions(+), 58 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev_vf.c b/lib/librte_pmd_i40e/i40e_ethdev_vf.c
index 2726bfb..97310ea 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev_vf.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev_vf.c
@@ -515,82 +515,154 @@ i40evf_config_vlan_pvid(struct rte_eth_dev *dev,
 	return err;
 }
 
+/* It configures VSI queues to co-work with Linux PF host */
 static int
-i40evf_configure_queues(struct rte_eth_dev *dev)
+i40evf_configure_vsi_queues(struct rte_eth_dev *dev)
 {
 	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
-	struct i40e_virtchnl_vsi_queue_config_info *queue_info;
-	struct i40e_virtchnl_queue_pair_info *queue_cfg;
 	struct i40e_rx_queue **rxq =
 		(struct i40e_rx_queue **)dev->data->rx_queues;
 	struct i40e_tx_queue **txq =
 		(struct i40e_tx_queue **)dev->data->tx_queues;
-	int i, len, nb_qpairs, num_rxq, num_txq;
-	int err;
+	struct i40e_virtchnl_vsi_queue_config_info *vc_vqci;
+	struct i40e_virtchnl_queue_pair_info *vc_qpi;
 	struct vf_cmd_info args;
-	struct rte_pktmbuf_pool_private *mbp_priv;
-
-	nb_qpairs = vf->num_queue_pairs;
-	len = sizeof(*queue_info) + sizeof(*queue_cfg) * nb_qpairs;
-	queue_info = rte_zmalloc("queue_info", len, 0);
-	if (queue_info == NULL) {
-		PMD_INIT_LOG(ERR, "failed alloc memory for queue_info\n");
-		return -1;
+	int size, i, nb_qp, ret;
+
+	nb_qp = vf->num_queue_pairs;
+	size = sizeof(struct i40e_virtchnl_vsi_queue_config_info) +
+		sizeof(struct i40e_virtchnl_queue_pair_info) * nb_qp;
+	vc_vqci = rte_zmalloc("queue_info", size, 0);
+	if (!vc_vqci) {
+		PMD_DRV_LOG(ERR, "Failed to allocate memory for VF "
+					"configuring queues\n");
+		return -ENOMEM;
 	}
-	queue_info->vsi_id = vf->vsi_res->vsi_id;
-	queue_info->num_queue_pairs = nb_qpairs;
-	queue_cfg = queue_info->qpair;
-
-	num_rxq = dev->data->nb_rx_queues;
-	num_txq = dev->data->nb_tx_queues;
-	/*
-	 * PF host driver required to configure queues in pairs, which means
-	 * rxq_num should equals to txq_num. The actual usage won't always
-	 * work that way. The solution is fills 0 with HW ring option in case
-	 * they are not equal.
-	 */
-	for (i = 0; i < nb_qpairs; i++) {
-		/*Fill TX info */
-		queue_cfg->txq.vsi_id = queue_info->vsi_id;
-		queue_cfg->txq.queue_id = i;
-		if (i < num_txq) {
-			queue_cfg->txq.ring_len = txq[i]->nb_tx_desc;
-			queue_cfg->txq.dma_ring_addr = txq[i]->tx_ring_phys_addr;
-		} else {
-			queue_cfg->txq.ring_len = 0;
-			queue_cfg->txq.dma_ring_addr = 0;
+	vc_vqci->vsi_id = vf->vsi_res->vsi_id;
+	vc_vqci->num_queue_pairs = nb_qp;
+
+	for (i = 0, vc_qpi = vc_vqci->qpair; i < nb_qp; i++, vc_qpi++) {
+		vc_qpi->txq.vsi_id = vc_vqci->vsi_id;
+		vc_qpi->txq.queue_id = i;
+		if (i < dev->data->nb_tx_queues) {
+			vc_qpi->txq.ring_len = txq[i]->nb_tx_desc;
+			vc_qpi->txq.dma_ring_addr = txq[i]->tx_ring_phys_addr;
 		}
 
-		/* Fill RX info */
-		queue_cfg->rxq.vsi_id = queue_info->vsi_id;
-		queue_cfg->rxq.queue_id = i;
-		queue_cfg->rxq.max_pkt_size = vf->max_pkt_len;
-		if (i < num_rxq) {
+		vc_qpi->rxq.vsi_id = vc_vqci->vsi_id;
+		vc_qpi->rxq.queue_id = i;
+		vc_qpi->rxq.max_pkt_size = vf->max_pkt_len;
+		if (i < dev->data->nb_rx_queues) {
+			struct rte_pktmbuf_pool_private *mbp_priv;
+
+			vc_qpi->rxq.ring_len = rxq[i]->nb_rx_desc;
+			vc_qpi->rxq.dma_ring_addr = rxq[i]->rx_ring_phys_addr;
 			mbp_priv = rte_mempool_get_priv(rxq[i]->mp);
-			queue_cfg->rxq.databuffer_size = mbp_priv->mbuf_data_room_size -
-						   RTE_PKTMBUF_HEADROOM;;
-			queue_cfg->rxq.ring_len = rxq[i]->nb_rx_desc;
-			queue_cfg->rxq.dma_ring_addr = rxq[i]->rx_ring_phys_addr;;
-		} else {
-			queue_cfg->rxq.ring_len = 0;
-			queue_cfg->rxq.dma_ring_addr = 0;
-			queue_cfg->rxq.databuffer_size = 0;
+			vc_qpi->rxq.databuffer_size =
+				mbp_priv->mbuf_data_room_size -
+					RTE_PKTMBUF_HEADROOM;
 		}
-		queue_cfg++;
 	}
-
+	memset(&args, 0, sizeof(args));
 	args.ops = I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES;
-	args.in_args = (u8 *)queue_info;
-	args.in_args_size = len;
+	args.in_args = (uint8_t *)vc_vqci;
+	args.in_args_size = size;
 	args.out_buffer = cmd_result_buffer;
 	args.out_size = I40E_AQ_BUF_SZ;
-	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
-		PMD_DRV_LOG(ERR, "fail to execute command "
-				"OP_CONFIG_VSI_QUEUES\n");
-	rte_free(queue_info);
+	ret = i40evf_execute_vf_cmd(dev, &args);
+	if (ret)
+		PMD_DRV_LOG(ERR, "Failed to execute command of "
+			"I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES\n");
+	rte_free(vc_vqci);
 
-	return err;
+	return ret;
+}
+
+/* It configures VSI queues to co-work with DPDK PF host */
+static int
+i40evf_configure_vsi_queues_ex(struct rte_eth_dev *dev)
+{
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
+	struct i40e_rx_queue **rxq =
+		(struct i40e_rx_queue **)dev->data->rx_queues;
+	struct i40e_tx_queue **txq =
+		(struct i40e_tx_queue **)dev->data->tx_queues;
+	struct i40e_virtchnl_vsi_queue_config_info *vc_vqci;
+	struct i40e_virtchnl_queue_pair_info *vc_qpi;
+	struct i40e_virtchnl_queue_pair_extra_info *vc_qpei;
+	struct vf_cmd_info args;
+	int size, i, nb_qp, ret;
+
+	nb_qp = vf->num_queue_pairs;
+	size = sizeof(struct i40e_virtchnl_vsi_queue_config_info) +
+		sizeof(struct i40e_virtchnl_queue_pair_info) * nb_qp +
+		sizeof(struct i40e_virtchnl_queue_pair_extra_info) * nb_qp;
+	vc_vqci = rte_zmalloc("queue_info", size, 0);
+	if (!vc_vqci) {
+		PMD_DRV_LOG(ERR, "Failed to allocate memory for VF "
+					"configuring queues\n");
+		return -ENOMEM;
+	}
+	vc_vqci->vsi_id = vf->vsi_res->vsi_id;
+	vc_vqci->num_queue_pairs = nb_qp;
+	vc_qpi = vc_vqci->qpair;
+	vc_qpei = (struct i40e_virtchnl_queue_pair_extra_info *)
+		(((uint8_t *)vc_vqci->qpair) +
+		sizeof(struct i40e_virtchnl_queue_pair_info) * nb_qp);
+
+	for (i = 0; i < nb_qp; i++, vc_qpi++, vc_qpei++) {
+		vc_qpi->txq.vsi_id = vc_vqci->vsi_id;
+		vc_qpi->txq.queue_id = i;
+		if (i < dev->data->nb_tx_queues) {
+			vc_qpi->txq.ring_len = txq[i]->nb_tx_desc;
+			vc_qpi->txq.dma_ring_addr = txq[i]->tx_ring_phys_addr;
+		}
+		vc_qpi->rxq.vsi_id = vc_vqci->vsi_id;
+		vc_qpi->rxq.queue_id = i;
+		vc_qpi->rxq.max_pkt_size = vf->max_pkt_len;
+		if (i < dev->data->nb_rx_queues) {
+			struct rte_pktmbuf_pool_private *mbp_priv;
+
+			vc_qpi->rxq.ring_len = rxq[i]->nb_rx_desc;
+			vc_qpi->rxq.dma_ring_addr = rxq[i]->rx_ring_phys_addr;
+			mbp_priv = rte_mempool_get_priv(rxq[i]->mp);
+			vc_qpi->rxq.databuffer_size =
+				mbp_priv->mbuf_data_room_size -
+					RTE_PKTMBUF_HEADROOM;
+			/*
+			 * It adds extra info for configuring VSI queues, which
+			 * is needed to enable the configurable crc stripping
+			 * in VF.
+			 */
+			vc_qpei->crcstrip =
+				dev->data->dev_conf.rxmode.hw_strip_crc;
+		}
+	}
+	memset(&args, 0, sizeof(args));
+	args.ops =
+		(enum i40e_virtchnl_ops)I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX;
+	args.in_args = (uint8_t *)vc_vqci;
+	args.in_args_size = size;
+	args.out_buffer = cmd_result_buffer;
+	args.out_size = I40E_AQ_BUF_SZ;
+	ret = i40evf_execute_vf_cmd(dev, &args);
+	if (ret)
+		PMD_DRV_LOG(ERR, "Failed to execute command of "
+			"I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX\n");
+	rte_free(vc_vqci);
+
+	return ret;
+}
+
+static int
+i40evf_configure_queues(struct rte_eth_dev *dev)
+{
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
+
+	if (vf->host_is_dpdk) /* To support DPDK PF host */
+		return i40evf_configure_vsi_queues_ex(dev);
+	else /* To support Linux PF host */
+		return i40evf_configure_vsi_queues(dev);
 }
 
 static int
-- 
1.8.1.4

Re: [PATCH 2/3] i40evf: support I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX in i40e VF PMD

[Thomas Monjalon]

2014-08-20 11:33, Helin Zhang:
> To support configurable CRC in VF, use operation of
> I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES_EX to carry more
> information from VM to PF host, if the peer is DPDK
> PF host. Otherwise assume it is Linux PF host and
> just use operation of I40E_VIRTCHNL_OP_CONFIG_VSI_QUEUES.
[...]
> +/* It configures VSI queues to co-work with Linux PF host */
>  static int
> -i40evf_configure_queues(struct rte_eth_dev *dev)
> +i40evf_configure_vsi_queues(struct rte_eth_dev *dev)
>  {
>  	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> -	struct i40e_virtchnl_vsi_queue_config_info *queue_info;
> -	struct i40e_virtchnl_queue_pair_info *queue_cfg;
>  	struct i40e_rx_queue **rxq =
>  		(struct i40e_rx_queue **)dev->data->rx_queues;
>  	struct i40e_tx_queue **txq =
>  		(struct i40e_tx_queue **)dev->data->tx_queues;
> -	int i, len, nb_qpairs, num_rxq, num_txq;
> -	int err;
> +	struct i40e_virtchnl_vsi_queue_config_info *vc_vqci;
> +	struct i40e_virtchnl_queue_pair_info *vc_qpi;
>  	struct vf_cmd_info args;
> -	struct rte_pktmbuf_pool_private *mbp_priv;
> -
> -	nb_qpairs = vf->num_queue_pairs;
> -	len = sizeof(*queue_info) + sizeof(*queue_cfg) * nb_qpairs;
> -	queue_info = rte_zmalloc("queue_info", len, 0);
> -	if (queue_info == NULL) {
> -		PMD_INIT_LOG(ERR, "failed alloc memory for queue_info\n");
> -		return -1;
> +	int size, i, nb_qp, ret;
> +
> +	nb_qp = vf->num_queue_pairs;
> +	size = sizeof(struct i40e_virtchnl_vsi_queue_config_info) +
> +		sizeof(struct i40e_virtchnl_queue_pair_info) * nb_qp;
> +	vc_vqci = rte_zmalloc("queue_info", size, 0);
> +	if (!vc_vqci) {
> +		PMD_DRV_LOG(ERR, "Failed to allocate memory for VF "
> +					"configuring queues\n");
> +		return -ENOMEM;
>  	}
> -	queue_info->vsi_id = vf->vsi_res->vsi_id;
> -	queue_info->num_queue_pairs = nb_qpairs;
> -	queue_cfg = queue_info->qpair;
> -
> -	num_rxq = dev->data->nb_rx_queues;
> -	num_txq = dev->data->nb_tx_queues;
> -	/*
> -	 * PF host driver required to configure queues in pairs, which means
> -	 * rxq_num should equals to txq_num. The actual usage won't always
> -	 * work that way. The solution is fills 0 with HW ring option in case
> -	 * they are not equal.
> -	 */
> -	for (i = 0; i < nb_qpairs; i++) {
> -		/*Fill TX info */
> -		queue_cfg->txq.vsi_id = queue_info->vsi_id;
> -		queue_cfg->txq.queue_id = i;
> -		if (i < num_txq) {
> -			queue_cfg->txq.ring_len = txq[i]->nb_tx_desc;
> -			queue_cfg->txq.dma_ring_addr = txq[i]->tx_ring_phys_addr;
> -		} else {
> -			queue_cfg->txq.ring_len = 0;
> -			queue_cfg->txq.dma_ring_addr = 0;
> +	vc_vqci->vsi_id = vf->vsi_res->vsi_id;
> +	vc_vqci->num_queue_pairs = nb_qp;
> +
> +	for (i = 0, vc_qpi = vc_vqci->qpair; i < nb_qp; i++, vc_qpi++) {
> +		vc_qpi->txq.vsi_id = vc_vqci->vsi_id;
> +		vc_qpi->txq.queue_id = i;
> +		if (i < dev->data->nb_tx_queues) {
> +			vc_qpi->txq.ring_len = txq[i]->nb_tx_desc;
> +			vc_qpi->txq.dma_ring_addr = txq[i]->tx_ring_phys_addr;
>  		}
>  
> -		/* Fill RX info */
> -		queue_cfg->rxq.vsi_id = queue_info->vsi_id;
> -		queue_cfg->rxq.queue_id = i;
> -		queue_cfg->rxq.max_pkt_size = vf->max_pkt_len;
> -		if (i < num_rxq) {
> +		vc_qpi->rxq.vsi_id = vc_vqci->vsi_id;
> +		vc_qpi->rxq.queue_id = i;
> +		vc_qpi->rxq.max_pkt_size = vf->max_pkt_len;
> +		if (i < dev->data->nb_rx_queues) {
> +			struct rte_pktmbuf_pool_private *mbp_priv;
> +
> +			vc_qpi->rxq.ring_len = rxq[i]->nb_rx_desc;
> +			vc_qpi->rxq.dma_ring_addr = rxq[i]->rx_ring_phys_addr;
>  			mbp_priv = rte_mempool_get_priv(rxq[i]->mp);
> -			queue_cfg->rxq.databuffer_size = mbp_priv->mbuf_data_room_size -
> -						   RTE_PKTMBUF_HEADROOM;;
> -			queue_cfg->rxq.ring_len = rxq[i]->nb_rx_desc;
> -			queue_cfg->rxq.dma_ring_addr = rxq[i]->rx_ring_phys_addr;;
> -		} else {
> -			queue_cfg->rxq.ring_len = 0;
> -			queue_cfg->rxq.dma_ring_addr = 0;
> -			queue_cfg->rxq.databuffer_size = 0;
> +			vc_qpi->rxq.databuffer_size =
> +				mbp_priv->mbuf_data_room_size -
> +					RTE_PKTMBUF_HEADROOM;
>  		}
> -		queue_cfg++;
>  	}

It's not clear why you reworked the legacy function.
Please explain it in the commit log and move renamings in a separated patch.

Thanks
-- 
Thomas

[PATCH 3/3] config: remove useless config of CONFIG_RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC

[Helin Zhang]

As i40e support configurable CRC stripping in VF,
the useless configuration of
CONFIG_RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC should
be removed.

Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jingjing Wu <jingjing.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jing Chen <jing.d.chen-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 config/common_bsdapp   | 1 -
 config/common_linuxapp | 1 -
 2 files changed, 2 deletions(-)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index bf6d8a0..c62388a 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -180,7 +180,6 @@ CONFIG_RTE_LIBRTE_I40E_DEBUG_RX=n
 CONFIG_RTE_LIBRTE_I40E_DEBUG_TX=n
 CONFIG_RTE_LIBRTE_I40E_DEBUG_TX_FREE=n
 CONFIG_RTE_LIBRTE_I40E_DEBUG_DRIVER=n
-CONFIG_RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC=y
 CONFIG_RTE_LIBRTE_I40E_RX_ALLOW_BULK_ALLOC=n
 CONFIG_RTE_LIBRTE_I40E_ALLOW_UNSUPPORTED_SFP=y
 CONFIG_RTE_LIBRTE_I40E_16BYTE_RX_DESC=n
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 9047975..c680a63 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -203,7 +203,6 @@ CONFIG_RTE_LIBRTE_I40E_DEBUG_RX=n
 CONFIG_RTE_LIBRTE_I40E_DEBUG_TX=n
 CONFIG_RTE_LIBRTE_I40E_DEBUG_TX_FREE=n
 CONFIG_RTE_LIBRTE_I40E_DEBUG_DRIVER=n
-CONFIG_RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC=n
 CONFIG_RTE_LIBRTE_I40E_RX_ALLOW_BULK_ALLOC=y
 CONFIG_RTE_LIBRTE_I40E_ALLOW_UNSUPPORTED_SFP=n
 CONFIG_RTE_LIBRTE_I40E_16BYTE_RX_DESC=n
-- 
1.8.1.4