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

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

> -----Original Message-----
> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> Sent: Friday, January 9, 2015 10:02 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 5/6] ixgbe: Config VF RSS
> 
> 
> On 01/09/15 08:07, Ouyang, Changchun wrote:
> >
> >> -----Original Message-----
> >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >> Sent: Thursday, January 8, 2015 5:43 PM
> >> To: Ouyang, Changchun; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v5 5/6] ixgbe: Config VF RSS
> >>
> >>
> >> On 01/07/15 08:32, 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>
> >>>
> >>> 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 | 103
> >> +++++++++++++++++++++++++++++++++-----
> >>>    2 files changed, 106 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..e83a9ab 100644
> >>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >>> @@ -3327,6 +3327,68 @@ 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:
> >>> +	case ETH_16_POOLS:
> >> Isn't ETH_16_POOLS mode is invalid for VF RSS? It's what both spec
> >> states and what u handle in this patch in ixgbe_pf_host_configure().
> >> IMHO it would be better to treat this mode value as an error here
> >> since if u get it here it indicates of a SW bug.
> > I think we discussed it before already,  return err here will break here in the
> case of max vf number is less than 16.
> > If doing that, This make the library seems can't support vf rss in the case of
> max vf num less than 16.
> > So we obviously don't hope it break here.
> 
> I don't remember we were discussing these specific lines. However I do
> remember we talked about the previous section of this patch.
> I'm afraid u are missing my point here: ixgbe_pf_host_configure() is called
> before ixgbe_config_vf_rss() in the ixgbe_dev_start() flow. This means that
> RTE_ETH_DEV_SRIOV(dev).active will already be adjusted by your (!!!) code
> in the ixgbe_pf_host_configure() when u get to
> ixgbe_config_vf_rss() and it should not be equal ETH_16_POOLS unless there
> is a bug in your code.
> 
> So, unless I've missed something here, don't u think an assert() would be
> appropriate if RTE_ETH_DEV_SRIOV(dev).active equals ETH_16_POOLS?

Ooh, thanks for identifying this. Here ETH_16_POOLS branch not necessary, as you said, I 
Have resolved it in function ixgbe_pf_host_configure.
Then I will fix it in v6.
Thanks again
Changchun