On 01/09/15 08:07, Ouyang, Changchun wrote: > >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com] >> Sent: Thursday, January 8, 2015 5:43 PM >> To: Ouyang, Changchun; dev at 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 at 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? thanks, vlad > >
