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

["Wodkowski, PawelX" <pawelx.wodkowski@intel.com>]

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