[dpdk-dev,v6,5/6] ixgbe: Config VF RSS

  It needs config RSS and IXGBE_MRQC and IXGBE_VFPSRTYPE to enable VF RSS.

The psrtype will determine how many queues the received packets will distribute to,
and the value of psrtype should depends on both facet: max VF rxq number which
has been negotiated with PF, and the number of rxq specified in config on guest.

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>

Changes in v6:
  - Raise an error for the case of ETH_16_POOLS in config vf rss, as the previous 
    logic have changed it into: ETH_32_POOLS.

Changes in v4:
 - The number of rxq from config should be power of 2 and should not bigger than
    max VF rxq number(negotiated between guest and host).

---
 lib/librte_pmd_ixgbe/ixgbe_pf.c   |  15 ++++++
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 102 +++++++++++++++++++++++++++++++++-----
 2 files changed, 105 insertions(+), 12 deletions(-)
  

On 01/12/15 07:59, Ouyang Changchun wrote:
> It needs config RSS and IXGBE_MRQC and IXGBE_VFPSRTYPE to enable VF RSS.
>
> The psrtype will determine how many queues the received packets will distribute to,
> and the value of psrtype should depends on both facet: max VF rxq number which
> has been negotiated with PF, and the number of rxq specified in config on guest.
>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>

Reviewed-by: Vlad Zolotarov <vladz@cloudius-systems.com>

>
> Changes in v6:
>    - Raise an error for the case of ETH_16_POOLS in config vf rss, as the previous
>      logic have changed it into: ETH_32_POOLS.
>
> Changes in v4:
>   - The number of rxq from config should be power of 2 and should not bigger than
>      max VF rxq number(negotiated between guest and host).
>
> ---
>   lib/librte_pmd_ixgbe/ixgbe_pf.c   |  15 ++++++
>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 102 +++++++++++++++++++++++++++++++++-----
>   2 files changed, 105 insertions(+), 12 deletions(-)
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c b/lib/librte_pmd_ixgbe/ixgbe_pf.c
> index dbda9b5..93f6e43 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c
> @@ -187,6 +187,21 @@ int ixgbe_pf_host_configure(struct rte_eth_dev *eth_dev)
>   	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_LO(hw->mac.num_rar_entries), 0);
>   	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_HI(hw->mac.num_rar_entries), 0);
>   
> +	/*
> +	 * VF RSS can support at most 4 queues for each VF, even if
> +	 * 8 queues are available for each VF, it need refine to 4
> +	 * queues here due to this limitation, otherwise no queue
> +	 * will receive any packet even RSS is enabled.
> +	 */
> +	if (eth_dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_RSS) {
> +		if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool == 8) {
> +			RTE_ETH_DEV_SRIOV(eth_dev).active = ETH_32_POOLS;
> +			RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = 4;
> +			RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx =
> +				dev_num_vf(eth_dev) * 4;
> +		}
> +	}
> +
>   	/* set VMDq map to default PF pool */
>   	hw->mac.ops.set_vmdq(hw, 0, RTE_ETH_DEV_SRIOV(eth_dev).def_vmdq_idx);
>   
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index f69abda..20627df 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -3327,6 +3327,67 @@ ixgbe_alloc_rx_queue_mbufs(struct igb_rx_queue *rxq)
>   }
>   
>   static int
> +ixgbe_config_vf_rss(struct rte_eth_dev *dev)
> +{
> +	struct ixgbe_hw *hw;
> +	uint32_t mrqc;
> +
> +	ixgbe_rss_configure(dev);
> +
> +	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	/* MRQC: enable VF RSS */
> +	mrqc = IXGBE_READ_REG(hw, IXGBE_MRQC);
> +	mrqc &= ~IXGBE_MRQC_MRQE_MASK;
> +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> +	case ETH_64_POOLS:
> +		mrqc |= IXGBE_MRQC_VMDQRSS64EN;
> +		break;
> +
> +	case ETH_32_POOLS:
> +		mrqc |= IXGBE_MRQC_VMDQRSS32EN;
> +		break;
> +
> +	default:
> +		PMD_INIT_LOG(ERR, "Invalid pool number in IOV mode with VMDQ RSS");
> +		return -EINVAL;
> +	}
> +
> +	IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> +
> +	return 0;
> +}
> +
> +static int
> +ixgbe_config_vf_default(struct rte_eth_dev *dev)
> +{
> +	struct ixgbe_hw *hw =
> +		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> +	case ETH_64_POOLS:
> +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> +			IXGBE_MRQC_VMDQEN);
> +		break;
> +
> +	case ETH_32_POOLS:
> +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> +			IXGBE_MRQC_VMDQRT4TCEN);
> +		break;
> +
> +	case ETH_16_POOLS:
> +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> +			IXGBE_MRQC_VMDQRT8TCEN);
> +		break;
> +	default:
> +		PMD_INIT_LOG(ERR,
> +			"invalid pool number in IOV mode");
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int
>   ixgbe_dev_mq_rx_configure(struct rte_eth_dev *dev)
>   {
>   	struct ixgbe_hw *hw =
> @@ -3358,24 +3419,25 @@ ixgbe_dev_mq_rx_configure(struct rte_eth_dev *dev)
>   			default: ixgbe_rss_disable(dev);
>   		}
>   	} else {
> -		switch (RTE_ETH_DEV_SRIOV(dev).active) {
>   		/*
>   		 * SRIOV active scheme
> -		 * FIXME if support DCB/RSS together with VMDq & SRIOV
> +		 * Support RSS together with VMDq & SRIOV
>   		 */
> -		case ETH_64_POOLS:
> -			IXGBE_WRITE_REG(hw, IXGBE_MRQC, IXGBE_MRQC_VMDQEN);
> -			break;
> -
> -		case ETH_32_POOLS:
> -			IXGBE_WRITE_REG(hw, IXGBE_MRQC, IXGBE_MRQC_VMDQRT4TCEN);
> +		switch (dev->data->dev_conf.rxmode.mq_mode) {
> +		case ETH_MQ_RX_RSS:
> +		case ETH_MQ_RX_VMDQ_RSS:
> +			ixgbe_config_vf_rss(dev);
>   			break;
>   
> -		case ETH_16_POOLS:
> -			IXGBE_WRITE_REG(hw, IXGBE_MRQC, IXGBE_MRQC_VMDQRT8TCEN);
> -			break;
> +		/* FIXME if support DCB/RSS together with VMDq & SRIOV */
> +		case ETH_MQ_RX_VMDQ_DCB:
> +		case ETH_MQ_RX_VMDQ_DCB_RSS:
> +			PMD_INIT_LOG(ERR,
> +				"Could not support DCB with VMDq & SRIOV");
> +			return -1;
>   		default:
> -			PMD_INIT_LOG(ERR, "invalid pool number in IOV mode");
> +			ixgbe_config_vf_default(dev);
> +			break;
>   		}
>   	}
>   
> @@ -3993,6 +4055,19 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
>   	PMD_INIT_FUNC_TRACE();
>   	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>   
> +	if (rte_is_power_of_2(dev->data->nb_rx_queues) == 0) {
> +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> +			"it should be power of 2");
> +		return -1;
> +	}
> +
> +	if (dev->data->nb_rx_queues > hw->mac.max_rx_queues) {
> +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> +			"it should be equal to or less than %d",
> +			hw->mac.max_rx_queues);
> +		return -1;
> +	}
> +
>   	/*
>   	 * When the VF driver issues a IXGBE_VF_RESET request, the PF driver
>   	 * disables the VF receipt of packets if the PF MTU is > 1500.
> @@ -4094,6 +4169,9 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
>   			IXGBE_PSRTYPE_IPV6HDR;
>   #endif
>   
> +	/* Set RQPL for VF RSS according to max Rx queue */
> +	psrtype |= (dev->data->nb_rx_queues >> 1) <<
> +		IXGBE_PSRTYPE_RQPL_SHIFT;
>   	IXGBE_WRITE_REG(hw, IXGBE_VFPSRTYPE, psrtype);
>   
>   	if (dev->data->dev_conf.rxmode.enable_scatter) {
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun
> Sent: Monday, January 12, 2015 6:59 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
> 
> It needs config RSS and IXGBE_MRQC and IXGBE_VFPSRTYPE to enable VF RSS.
> 
> The psrtype will determine how many queues the received packets will distribute
> to,
> and the value of psrtype should depends on both facet: max VF rxq number
> which
> has been negotiated with PF, and the number of rxq specified in config on guest.
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> 
> Changes in v6:
>   - Raise an error for the case of ETH_16_POOLS in config vf rss, as the previous
>     logic have changed it into: ETH_32_POOLS.
> 
> Changes in v4:
>  - The number of rxq from config should be power of 2 and should not bigger
> than
>     max VF rxq number(negotiated between guest and host).
> 
> ---
>  lib/librte_pmd_ixgbe/ixgbe_pf.c   |  15 ++++++
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 102
> +++++++++++++++++++++++++++++++++-----
>  2 files changed, 105 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c b/lib/librte_pmd_ixgbe/ixgbe_pf.c
> index dbda9b5..93f6e43 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c
> @@ -187,6 +187,21 @@ int ixgbe_pf_host_configure(struct rte_eth_dev
> *eth_dev)
>  	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_LO(hw->mac.num_rar_entries),
> 0);
>  	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_HI(hw->mac.num_rar_entries),
> 0);
> 
> +	/*
> +	 * VF RSS can support at most 4 queues for each VF, even if
> +	 * 8 queues are available for each VF, it need refine to 4
> +	 * queues here due to this limitation, otherwise no queue
> +	 * will receive any packet even RSS is enabled.
> +	 */
> +	if (eth_dev->data->dev_conf.rxmode.mq_mode ==
> ETH_MQ_RX_VMDQ_RSS) {
> +		if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool == 8) {
> +			RTE_ETH_DEV_SRIOV(eth_dev).active =
> ETH_32_POOLS;
> +			RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = 4;
> +			RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx =
> +				dev_num_vf(eth_dev) * 4;
> +		}
> +	}
> +

I did not looked before at your patches but I think you are messing with things that should not be changed:

Why you are changing those values. They are set up during ixgbe_pf_host_init(). Limitation you are
describing is only RSS related. If there will be reconfiguration from 
ETH_MQ_RX_VMDQ_RSS to other mode those value need to be re-evaluated. If you find this
kind of limitation you should handle it during RSS part configuration. Or if your way is the right way
you should explicitly make separate function that will re-evaluate those parameters each time.

Second issue with this code is that the nb_q_per_pool is changed from:
ixgbe_pf_host_configure() -> ixgbe_dev_start() -> rte_eth_dev_start()
and
rte_eth_dev_check_vf_rss_rxq_num() -> rte_eth_dev_check_mq_mode() -> rte_eth_dev_configure()

Which one is the right one? If both, why they are calculated twice?

I don't think that rte_eth_dev_data::sriov field should be changed at all - it holds current SRIOV capabilities.
If this will change during runtime it no point to have this field at all and should be some kind of "siov_get()"
function that will calculate and return those parameters dynamically.

Please refer also to <F6F2A6264E145F47A18AB6DF8E87425D12B89B02@IRSMSX102.ger.corp.intel.com>
for further issues.

I think this patchset should not be applied.

>  	/* set VMDq map to default PF pool */
>  	hw->mac.ops.set_vmdq(hw, 0,
> RTE_ETH_DEV_SRIOV(eth_dev).def_vmdq_idx);
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index f69abda..20627df 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -3327,6 +3327,67 @@ ixgbe_alloc_rx_queue_mbufs(struct igb_rx_queue
> *rxq)
>  }
> 
>  static int
> +ixgbe_config_vf_rss(struct rte_eth_dev *dev)
> +{
> +	struct ixgbe_hw *hw;
> +	uint32_t mrqc;
> +
> +	ixgbe_rss_configure(dev);
> +
> +	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	/* MRQC: enable VF RSS */
> +	mrqc = IXGBE_READ_REG(hw, IXGBE_MRQC);
> +	mrqc &= ~IXGBE_MRQC_MRQE_MASK;
> +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> +	case ETH_64_POOLS:
> +		mrqc |= IXGBE_MRQC_VMDQRSS64EN;
> +		break;
> +
> +	case ETH_32_POOLS:
> +		mrqc |= IXGBE_MRQC_VMDQRSS32EN;
> +		break;
> +
> +	default:
> +		PMD_INIT_LOG(ERR, "Invalid pool number in IOV mode with
> VMDQ RSS");
> +		return -EINVAL;
> +	}
> +
> +	IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> +
> +	return 0;
> +}
> +
> +static int
> +ixgbe_config_vf_default(struct rte_eth_dev *dev)
> +{
> +	struct ixgbe_hw *hw =
> +		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> +	case ETH_64_POOLS:
> +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> +			IXGBE_MRQC_VMDQEN);
> +		break;
> +
> +	case ETH_32_POOLS:
> +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> +			IXGBE_MRQC_VMDQRT4TCEN);
> +		break;
> +
> +	case ETH_16_POOLS:
> +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> +			IXGBE_MRQC_VMDQRT8TCEN);
> +		break;
> +	default:
> +		PMD_INIT_LOG(ERR,
> +			"invalid pool number in IOV mode");
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int
>  ixgbe_dev_mq_rx_configure(struct rte_eth_dev *dev)
>  {
>  	struct ixgbe_hw *hw =
> @@ -3358,24 +3419,25 @@ ixgbe_dev_mq_rx_configure(struct rte_eth_dev
> *dev)
>  			default: ixgbe_rss_disable(dev);
>  		}
>  	} else {
> -		switch (RTE_ETH_DEV_SRIOV(dev).active) {
>  		/*
>  		 * SRIOV active scheme
> -		 * FIXME if support DCB/RSS together with VMDq & SRIOV
> +		 * Support RSS together with VMDq & SRIOV
>  		 */
> -		case ETH_64_POOLS:
> -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> IXGBE_MRQC_VMDQEN);
> -			break;
> -
> -		case ETH_32_POOLS:
> -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> IXGBE_MRQC_VMDQRT4TCEN);
> +		switch (dev->data->dev_conf.rxmode.mq_mode) {
> +		case ETH_MQ_RX_RSS:
> +		case ETH_MQ_RX_VMDQ_RSS:
> +			ixgbe_config_vf_rss(dev);
>  			break;
> 
> -		case ETH_16_POOLS:
> -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> IXGBE_MRQC_VMDQRT8TCEN);
> -			break;
> +		/* FIXME if support DCB/RSS together with VMDq & SRIOV */
> +		case ETH_MQ_RX_VMDQ_DCB:
> +		case ETH_MQ_RX_VMDQ_DCB_RSS:
> +			PMD_INIT_LOG(ERR,
> +				"Could not support DCB with VMDq & SRIOV");
> +			return -1;
>  		default:
> -			PMD_INIT_LOG(ERR, "invalid pool number in IOV
> mode");
> +			ixgbe_config_vf_default(dev);
> +			break;
>  		}
>  	}
> 
> @@ -3993,6 +4055,19 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
>  	PMD_INIT_FUNC_TRACE();
>  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> 
> +	if (rte_is_power_of_2(dev->data->nb_rx_queues) == 0) {
> +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> +			"it should be power of 2");
> +		return -1;
> +	}
> +
> +	if (dev->data->nb_rx_queues > hw->mac.max_rx_queues) {
> +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> +			"it should be equal to or less than %d",
> +			hw->mac.max_rx_queues);
> +		return -1;
> +	}
> +
>  	/*
>  	 * When the VF driver issues a IXGBE_VF_RESET request, the PF driver
>  	 * disables the VF receipt of packets if the PF MTU is > 1500.
> @@ -4094,6 +4169,9 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
>  			IXGBE_PSRTYPE_IPV6HDR;
>  #endif
> 
> +	/* Set RQPL for VF RSS according to max Rx queue */
> +	psrtype |= (dev->data->nb_rx_queues >> 1) <<
> +		IXGBE_PSRTYPE_RQPL_SHIFT;
>  	IXGBE_WRITE_REG(hw, IXGBE_VFPSRTYPE, psrtype);
> 
>  	if (dev->data->dev_conf.rxmode.enable_scatter) {
> --
> 1.8.4.2
  

> -----Original Message-----
> From: Wodkowski, PawelX
> Sent: Tuesday, January 20, 2015 5:35 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> Changchun
> > Sent: Monday, January 12, 2015 6:59 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
> >
> > It needs config RSS and IXGBE_MRQC and IXGBE_VFPSRTYPE to enable VF
> RSS.
> >
> > The psrtype will determine how many queues the received packets will
> > distribute to, and the value of psrtype should depends on both facet:
> > max VF rxq number which has been negotiated with PF, and the number of
> > rxq specified in config on guest.
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> >
> > Changes in v6:
> >   - Raise an error for the case of ETH_16_POOLS in config vf rss, as the
> previous
> >     logic have changed it into: ETH_32_POOLS.
> >
> > Changes in v4:
> >  - The number of rxq from config should be power of 2 and should not
> > bigger than
> >     max VF rxq number(negotiated between guest and host).
> >
> > ---
> >  lib/librte_pmd_ixgbe/ixgbe_pf.c   |  15 ++++++
> >  lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 102
> > +++++++++++++++++++++++++++++++++-----
> >  2 files changed, 105 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index dbda9b5..93f6e43 100644
> > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > @@ -187,6 +187,21 @@ int ixgbe_pf_host_configure(struct rte_eth_dev
> > *eth_dev)
> >  	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_LO(hw-
> >mac.num_rar_entries),
> > 0);
> >  	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_HI(hw-
> >mac.num_rar_entries),
> > 0);
> >
> > +	/*
> > +	 * VF RSS can support at most 4 queues for each VF, even if
> > +	 * 8 queues are available for each VF, it need refine to 4
> > +	 * queues here due to this limitation, otherwise no queue
> > +	 * will receive any packet even RSS is enabled.
> > +	 */
> > +	if (eth_dev->data->dev_conf.rxmode.mq_mode ==
> > ETH_MQ_RX_VMDQ_RSS) {
> > +		if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool == 8) {
> > +			RTE_ETH_DEV_SRIOV(eth_dev).active =
> > ETH_32_POOLS;
> > +			RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = 4;
> > +			RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx =
> > +				dev_num_vf(eth_dev) * 4;
> > +		}
> > +	}
> > +
> 
> I did not looked before at your patches but I think you are messing with
> things that should not be changed:
> 
> Why you are changing those values. They are set up during
> ixgbe_pf_host_init(). Limitation you are describing is only RSS related. If
> there will be reconfiguration from ETH_MQ_RX_VMDQ_RSS to other mode
> those value need to be re-evaluated. If you find this kind of limitation you
> should handle it during RSS part configuration. Or if your way is the right way
> you should explicitly make separate function that will re-evaluate those
> parameters each time.
> 
> Second issue with this code is that the nb_q_per_pool is changed from:
> ixgbe_pf_host_configure() -> ixgbe_dev_start() -> rte_eth_dev_start() and
> rte_eth_dev_check_vf_rss_rxq_num() -> rte_eth_dev_check_mq_mode() ->
> rte_eth_dev_configure()
> 
> Which one is the right one? If both, why they are calculated twice?
> 
> I don't think that rte_eth_dev_data::sriov field should be changed at all - it
> holds current SRIOV capabilities.
> If this will change during runtime it no point to have this field at all and should
> be some kind of "siov_get()"
> function that will calculate and return those parameters dynamically.
> 
> Please refer also to
> <F6F2A6264E145F47A18AB6DF8E87425D12B89B02@IRSMSX102.ger.corp.intel
> .com>
> for further issues.
> 
> I think this patchset should not be applied.

The better way should be either raise your comments before this patch is merged into mainline, or
You send out a patch to fix it.
I agree on part of what you said, the check is not necessary for vf rss in pf_host_configure because
Check_mq_mode has already check the queue number, I will send out a patch to fix it by removing this check.

On the other hand, I disagree with you on " rte_eth_dev_data::sriov field should be changed at all ",
The reason we need refine those value, is that those value get in pf_init, which is called on dev probe stage,
And those value are not accurate, they should vary according to mq mode, the mq mode could be determined only after
Dev is configured.

> 
> >  	/* set VMDq map to default PF pool */
> >  	hw->mac.ops.set_vmdq(hw, 0,
> > RTE_ETH_DEV_SRIOV(eth_dev).def_vmdq_idx);
> >
> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > index f69abda..20627df 100644
> > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > @@ -3327,6 +3327,67 @@ ixgbe_alloc_rx_queue_mbufs(struct
> igb_rx_queue
> > *rxq)
> >  }
> >
> >  static int
> > +ixgbe_config_vf_rss(struct rte_eth_dev *dev) {
> > +	struct ixgbe_hw *hw;
> > +	uint32_t mrqc;
> > +
> > +	ixgbe_rss_configure(dev);
> > +
> > +	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +
> > +	/* MRQC: enable VF RSS */
> > +	mrqc = IXGBE_READ_REG(hw, IXGBE_MRQC);
> > +	mrqc &= ~IXGBE_MRQC_MRQE_MASK;
> > +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> > +	case ETH_64_POOLS:
> > +		mrqc |= IXGBE_MRQC_VMDQRSS64EN;
> > +		break;
> > +
> > +	case ETH_32_POOLS:
> > +		mrqc |= IXGBE_MRQC_VMDQRSS32EN;
> > +		break;
> > +
> > +	default:
> > +		PMD_INIT_LOG(ERR, "Invalid pool number in IOV mode with
> > VMDQ RSS");
> > +		return -EINVAL;
> > +	}
> > +
> > +	IXGBE_WRITE_REG(hw, IXGBE_MRQC, mrqc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +ixgbe_config_vf_default(struct rte_eth_dev *dev) {
> > +	struct ixgbe_hw *hw =
> > +		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +
> > +	switch (RTE_ETH_DEV_SRIOV(dev).active) {
> > +	case ETH_64_POOLS:
> > +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > +			IXGBE_MRQC_VMDQEN);
> > +		break;
> > +
> > +	case ETH_32_POOLS:
> > +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > +			IXGBE_MRQC_VMDQRT4TCEN);
> > +		break;
> > +
> > +	case ETH_16_POOLS:
> > +		IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > +			IXGBE_MRQC_VMDQRT8TCEN);
> > +		break;
> > +	default:
> > +		PMD_INIT_LOG(ERR,
> > +			"invalid pool number in IOV mode");
> > +		break;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int
> >  ixgbe_dev_mq_rx_configure(struct rte_eth_dev *dev)  {
> >  	struct ixgbe_hw *hw =
> > @@ -3358,24 +3419,25 @@ ixgbe_dev_mq_rx_configure(struct
> rte_eth_dev
> > *dev)
> >  			default: ixgbe_rss_disable(dev);
> >  		}
> >  	} else {
> > -		switch (RTE_ETH_DEV_SRIOV(dev).active) {
> >  		/*
> >  		 * SRIOV active scheme
> > -		 * FIXME if support DCB/RSS together with VMDq & SRIOV
> > +		 * Support RSS together with VMDq & SRIOV
> >  		 */
> > -		case ETH_64_POOLS:
> > -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > IXGBE_MRQC_VMDQEN);
> > -			break;
> > -
> > -		case ETH_32_POOLS:
> > -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > IXGBE_MRQC_VMDQRT4TCEN);
> > +		switch (dev->data->dev_conf.rxmode.mq_mode) {
> > +		case ETH_MQ_RX_RSS:
> > +		case ETH_MQ_RX_VMDQ_RSS:
> > +			ixgbe_config_vf_rss(dev);
> >  			break;
> >
> > -		case ETH_16_POOLS:
> > -			IXGBE_WRITE_REG(hw, IXGBE_MRQC,
> > IXGBE_MRQC_VMDQRT8TCEN);
> > -			break;
> > +		/* FIXME if support DCB/RSS together with VMDq & SRIOV */
> > +		case ETH_MQ_RX_VMDQ_DCB:
> > +		case ETH_MQ_RX_VMDQ_DCB_RSS:
> > +			PMD_INIT_LOG(ERR,
> > +				"Could not support DCB with VMDq &
> SRIOV");
> > +			return -1;
> >  		default:
> > -			PMD_INIT_LOG(ERR, "invalid pool number in IOV
> > mode");
> > +			ixgbe_config_vf_default(dev);
> > +			break;
> >  		}
> >  	}
> >
> > @@ -3993,6 +4055,19 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
> >  	PMD_INIT_FUNC_TRACE();
> >  	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> > +	if (rte_is_power_of_2(dev->data->nb_rx_queues) == 0) {
> > +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> > +			"it should be power of 2");
> > +		return -1;
> > +	}
> > +
> > +	if (dev->data->nb_rx_queues > hw->mac.max_rx_queues) {
> > +		PMD_INIT_LOG(ERR, "The number of Rx queue invalid, "
> > +			"it should be equal to or less than %d",
> > +			hw->mac.max_rx_queues);
> > +		return -1;
> > +	}
> > +
> >  	/*
> >  	 * When the VF driver issues a IXGBE_VF_RESET request, the PF
> driver
> >  	 * disables the VF receipt of packets if the PF MTU is > 1500.
> > @@ -4094,6 +4169,9 @@ ixgbevf_dev_rx_init(struct rte_eth_dev *dev)
> >  			IXGBE_PSRTYPE_IPV6HDR;
> >  #endif
> >
> > +	/* Set RQPL for VF RSS according to max Rx queue */
> > +	psrtype |= (dev->data->nb_rx_queues >> 1) <<
> > +		IXGBE_PSRTYPE_RQPL_SHIFT;
> >  	IXGBE_WRITE_REG(hw, IXGBE_VFPSRTYPE, psrtype);
> >
> >  	if (dev->data->dev_conf.rxmode.enable_scatter) {
> > --
> > 1.8.4.2
  

> -----Original Message-----
> From: Ouyang, Changchun
> Sent: Wednesday, January 21, 2015 3:44 AM
> To: Wodkowski, PawelX; dev@dpdk.org
> Cc: Ouyang, Changchun
> Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
> 
> 
> 
> > -----Original Message-----
> > From: Wodkowski, PawelX
> > Sent: Tuesday, January 20, 2015 5:35 PM
> > To: Ouyang, Changchun; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> > Changchun
> > > Sent: Monday, January 12, 2015 6:59 AM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
> > >
> > > It needs config RSS and IXGBE_MRQC and IXGBE_VFPSRTYPE to enable VF
> > RSS.
> > >
> > > The psrtype will determine how many queues the received packets will
> > > distribute to, and the value of psrtype should depends on both facet:
> > > max VF rxq number which has been negotiated with PF, and the number of
> > > rxq specified in config on guest.
> > >
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > >
> > > Changes in v6:
> > >   - Raise an error for the case of ETH_16_POOLS in config vf rss, as the
> > previous
> > >     logic have changed it into: ETH_32_POOLS.
> > >
> > > Changes in v4:
> > >  - The number of rxq from config should be power of 2 and should not
> > > bigger than
> > >     max VF rxq number(negotiated between guest and host).
> > >
> > > ---
> > >  lib/librte_pmd_ixgbe/ixgbe_pf.c   |  15 ++++++
> > >  lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 102
> > > +++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 105 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index dbda9b5..93f6e43 100644
> > > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > > @@ -187,6 +187,21 @@ int ixgbe_pf_host_configure(struct rte_eth_dev
> > > *eth_dev)
> > >  	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_LO(hw-
> > >mac.num_rar_entries),
> > > 0);
> > >  	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_HI(hw-
> > >mac.num_rar_entries),
> > > 0);
> > >
> > > +	/*
> > > +	 * VF RSS can support at most 4 queues for each VF, even if
> > > +	 * 8 queues are available for each VF, it need refine to 4
> > > +	 * queues here due to this limitation, otherwise no queue
> > > +	 * will receive any packet even RSS is enabled.
> > > +	 */
> > > +	if (eth_dev->data->dev_conf.rxmode.mq_mode ==
> > > ETH_MQ_RX_VMDQ_RSS) {
> > > +		if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool == 8) {
> > > +			RTE_ETH_DEV_SRIOV(eth_dev).active =
> > > ETH_32_POOLS;
> > > +			RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = 4;
> > > +			RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx =
> > > +				dev_num_vf(eth_dev) * 4;
> > > +		}
> > > +	}
> > > +
> >
> > I did not looked before at your patches but I think you are messing with
> > things that should not be changed:
> >
> > Why you are changing those values. They are set up during
> > ixgbe_pf_host_init(). Limitation you are describing is only RSS related. If
> > there will be reconfiguration from ETH_MQ_RX_VMDQ_RSS to other mode
> > those value need to be re-evaluated. If you find this kind of limitation you
> > should handle it during RSS part configuration. Or if your way is the right way
> > you should explicitly make separate function that will re-evaluate those
> > parameters each time.
> >
> > Second issue with this code is that the nb_q_per_pool is changed from:
> > ixgbe_pf_host_configure() -> ixgbe_dev_start() -> rte_eth_dev_start() and
> > rte_eth_dev_check_vf_rss_rxq_num() -> rte_eth_dev_check_mq_mode() ->
> > rte_eth_dev_configure()
> >
> > Which one is the right one? If both, why they are calculated twice?
> >
> > I don't think that rte_eth_dev_data::sriov field should be changed at all - it
> > holds current SRIOV capabilities.
> > If this will change during runtime it no point to have this field at all and should
> > be some kind of "siov_get()"
> > function that will calculate and return those parameters dynamically.
> >
> > Please refer also to
> >
> <F6F2A6264E145F47A18AB6DF8E87425D12B89B02@IRSMSX102.ger.corp.intel
> > .com>
> > for further issues.
> >
> > I think this patchset should not be applied.
> 
> The better way should be either raise your comments before this patch is
> merged into mainline, or

Yes, I should but I trusted that Vlad review was covering this part. Does no matter
my, fault.

> You send out a patch to fix it.
> I agree on part of what you said, the check is not necessary for vf rss in
> pf_host_configure because
> Check_mq_mode has already check the queue number, I will send out a patch to
> fix it by removing this check.
> 
> On the other hand, I disagree with you on " rte_eth_dev_data::sriov field should
> be changed at all ",

This is my private opinion, but either way, recalculating those values or not,
it should be consistent and for feature development well documented when it is 
evaluated. Changing something in function that's name is calculated
"rte_eth_dev_check_mq_mode()" is not so very obvious.

> The reason we need refine those value, is that those value get in pf_init, which is
> called on dev probe stage,
> And those value are not accurate, they should vary according to mq mode, the
> mq mode could be determined only after
> Dev is configured.

If you think they are "not accurate" you should not calculate them because they are
invalid and make VF behavior undefined. VF can probe those values before you
make them "accurate" in port configuration phase. What then? It is a race condition
bug, and it definitely should be fixed in your next patch.

You should also fix port reconfiguration bug as I mention before (for VFs > 0 testpmd
is unable to start port after commnad 'port config all rxq X', X > 1 after RSS VF 
patches).

Pawel
  

On 01/21/15 10:44, Wodkowski, PawelX wrote:
>
>> -----Original Message-----
>> From: Ouyang, Changchun
>> Sent: Wednesday, January 21, 2015 3:44 AM
>> To: Wodkowski, PawelX; dev@dpdk.org
>> Cc: Ouyang, Changchun
>> Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
>>
>>
>>
>>> -----Original Message-----
>>> From: Wodkowski, PawelX
>>> Sent: Tuesday, January 20, 2015 5:35 PM
>>> To: Ouyang, Changchun; dev@dpdk.org
>>> Subject: RE: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
>>> Changchun
>>>> Sent: Monday, January 12, 2015 6:59 AM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v6 5/6] ixgbe: Config VF RSS
>>>>
>>>> It needs config RSS and IXGBE_MRQC and IXGBE_VFPSRTYPE to enable VF
>>> RSS.
>>>> The psrtype will determine how many queues the received packets will
>>>> distribute to, and the value of psrtype should depends on both facet:
>>>> max VF rxq number which has been negotiated with PF, and the number of
>>>> rxq specified in config on guest.
>>>>
>>>> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
>>>>
>>>> Changes in v6:
>>>>    - Raise an error for the case of ETH_16_POOLS in config vf rss, as the
>>> previous
>>>>      logic have changed it into: ETH_32_POOLS.
>>>>
>>>> Changes in v4:
>>>>   - The number of rxq from config should be power of 2 and should not
>>>> bigger than
>>>>      max VF rxq number(negotiated between guest and host).
>>>>
>>>> ---
>>>>   lib/librte_pmd_ixgbe/ixgbe_pf.c   |  15 ++++++
>>>>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 102
>>>> +++++++++++++++++++++++++++++++++-----
>>>>   2 files changed, 105 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c
>>>> b/lib/librte_pmd_ixgbe/ixgbe_pf.c index dbda9b5..93f6e43 100644
>>>> --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c
>>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c
>>>> @@ -187,6 +187,21 @@ int ixgbe_pf_host_configure(struct rte_eth_dev
>>>> *eth_dev)
>>>>   	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_LO(hw-
>>>> mac.num_rar_entries),
>>>> 0);
>>>>   	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_HI(hw-
>>>> mac.num_rar_entries),
>>>> 0);
>>>>
>>>> +	/*
>>>> +	 * VF RSS can support at most 4 queues for each VF, even if
>>>> +	 * 8 queues are available for each VF, it need refine to 4
>>>> +	 * queues here due to this limitation, otherwise no queue
>>>> +	 * will receive any packet even RSS is enabled.
>>>> +	 */
>>>> +	if (eth_dev->data->dev_conf.rxmode.mq_mode ==
>>>> ETH_MQ_RX_VMDQ_RSS) {
>>>> +		if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool == 8) {
>>>> +			RTE_ETH_DEV_SRIOV(eth_dev).active =
>>>> ETH_32_POOLS;
>>>> +			RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = 4;
>>>> +			RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx =
>>>> +				dev_num_vf(eth_dev) * 4;
>>>> +		}
>>>> +	}
>>>> +
>>> I did not looked before at your patches but I think you are messing with
>>> things that should not be changed:
>>>
>>> Why you are changing those values. They are set up during
>>> ixgbe_pf_host_init(). Limitation you are describing is only RSS related. If
>>> there will be reconfiguration from ETH_MQ_RX_VMDQ_RSS to other mode
>>> those value need to be re-evaluated. If you find this kind of limitation you
>>> should handle it during RSS part configuration. Or if your way is the right way
>>> you should explicitly make separate function that will re-evaluate those
>>> parameters each time.
>>>
>>> Second issue with this code is that the nb_q_per_pool is changed from:
>>> ixgbe_pf_host_configure() -> ixgbe_dev_start() -> rte_eth_dev_start() and
>>> rte_eth_dev_check_vf_rss_rxq_num() -> rte_eth_dev_check_mq_mode() ->
>>> rte_eth_dev_configure()
>>>
>>> Which one is the right one? If both, why they are calculated twice?
>>>
>>> I don't think that rte_eth_dev_data::sriov field should be changed at all - it
>>> holds current SRIOV capabilities.
>>> If this will change during runtime it no point to have this field at all and should
>>> be some kind of "siov_get()"
>>> function that will calculate and return those parameters dynamically.
>>>
>>> Please refer also to
>>>
>> <F6F2A6264E145F47A18AB6DF8E87425D12B89B02@IRSMSX102.ger.corp.intel
>>> .com>
>>> for further issues.
>>>
>>> I think this patchset should not be applied.
>> The better way should be either raise your comments before this patch is
>> merged into mainline, or
> Yes, I should but I trusted that Vlad review was covering this part.

I'm new on the list and my experience with DPDK is about two months so, 
pls., don't judge me too harsh... ;)
I tried to cover the obvious things and actually learned the code while 
reviewing. The things u say, Pavel(X?) make sense and I obviously missed 
that.
But as Changchun mentioned there is nothing that can't be fixed with a 
followup patches... ;)


> Does no matter
> my, fault.
>
>> You send out a patch to fix it.
>> I agree on part of what you said, the check is not necessary for vf rss in
>> pf_host_configure because
>> Check_mq_mode has already check the queue number, I will send out a patch to
>> fix it by removing this check.
>>
>> On the other hand, I disagree with you on " rte_eth_dev_data::sriov field should
>> be changed at all ",
> This is my private opinion, but either way, recalculating those values or not,
> it should be consistent and for feature development well documented when it is
> evaluated. Changing something in function that's name is calculated
> "rte_eth_dev_check_mq_mode()" is not so very obvious.
>
>> The reason we need refine those value, is that those value get in pf_init, which is
>> called on dev probe stage,
>> And those value are not accurate, they should vary according to mq mode, the
>> mq mode could be determined only after
>> Dev is configured.
> If you think they are "not accurate" you should not calculate them because they are
> invalid and make VF behavior undefined. VF can probe those values before you
> make them "accurate" in port configuration phase. What then? It is a race condition
> bug, and it definitely should be fixed in your next patch.
>
> You should also fix port reconfiguration bug as I mention before (for VFs > 0 testpmd
> is unable to start port after commnad 'port config all rxq X', X > 1 after RSS VF
> patches).
>
> Pawel
>
>
  

> 
> I'm new on the list and my experience with DPDK is about two months so,
> pls., don't judge me too harsh... ;)
> I tried to cover the obvious things and actually learned the code while
> reviewing. The things u say, Pavel(X?) make sense and I obviously missed

I am really puzzled about mail client I have to use. It is really stubborn 
about using my correct name :P

> that.
> But as Changchun mentioned there is nothing that can't be fixed with a
> followup patches... ;)
> 
Roger that :P 
No judging, I should also look those patches before they were acked.

Waiting for fixes.

Pawel