Re: [dpdk-dev] [PATCH v4 3/6] ixgbe: Get VF queue number

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

> -----Original Message-----
> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> Sent: Sunday, January 4, 2015 4:39 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 3/6] ixgbe: Get VF queue number
> 
> 
> On 01/04/15 09:18, Ouyang Changchun wrote:
> > Get the available Rx and Tx queue number when receiving
> IXGBE_VF_GET_QUEUES message from VF.
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> >   lib/librte_pmd_ixgbe/ixgbe_pf.c | 35
> ++++++++++++++++++++++++++++++++++-
> >   1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > b/lib/librte_pmd_ixgbe/ixgbe_pf.c index 495aff5..cbb0145 100644
> > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c
> > @@ -53,6 +53,8 @@
> >   #include "ixgbe_ethdev.h"
> >
> >   #define IXGBE_MAX_VFTA     (128)
> > +#define IXGBE_VF_MSG_SIZE_DEFAULT 1
> > +#define IXGBE_VF_GET_QUEUE_MSG_SIZE 5
> >
> >   static inline uint16_t
> >   dev_num_vf(struct rte_eth_dev *eth_dev) @@ -491,9 +493,36 @@
> > ixgbe_negotiate_vf_api(struct rte_eth_dev *dev, uint32_t vf, uint32_t
> *msgbuf)
> >   }
> >
> >   static int
> > +ixgbe_get_vf_queues(struct rte_eth_dev *dev, uint32_t vf, uint32_t
> > +*msgbuf) {
> > +	struct ixgbe_vf_info *vfinfo =
> > +		*IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data-
> >dev_private);
> > +	uint32_t default_q = vf * RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
> > +
> > +	/* Verify if the PF supports the mbox APIs version or not */
> > +	switch (vfinfo[vf].api_version) {
> > +	case ixgbe_mbox_api_20:
> > +	case ixgbe_mbox_api_11:
> > +		break;
> > +	default:
> > +		return -1;
> > +	}
> > +
> > +	/* Notify VF of Rx and Tx queue number */
> > +	msgbuf[IXGBE_VF_RX_QUEUES] =
> RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
> > +	msgbuf[IXGBE_VF_TX_QUEUES] =
> RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
> > +
> > +	/* Notify VF of default queue */
> > +	msgbuf[IXGBE_VF_DEF_QUEUE] = default_q;
> 
> What about IXGBE_VF_TRANS_VLAN field?

This field is used for vlan strip or dcb case, which the vf rss don't need it.

> > +
> > +	return 0;
> > +}
> > +
> > +static int
> >   ixgbe_rcv_msg_from_vf(struct rte_eth_dev *dev, uint16_t vf)
> >   {
> >   	uint16_t mbx_size = IXGBE_VFMAILBOX_SIZE;
> > +	uint16_t msg_size = IXGBE_VF_MSG_SIZE_DEFAULT;
> >   	uint32_t msgbuf[IXGBE_VFMAILBOX_SIZE];
> >   	int32_t retval;
> >   	struct ixgbe_hw *hw =
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > @@ -537,6 +566,10 @@ ixgbe_rcv_msg_from_vf(struct rte_eth_dev *dev,
> uint16_t vf)
> >   	case IXGBE_VF_API_NEGOTIATE:
> >   		retval = ixgbe_negotiate_vf_api(dev, vf, msgbuf);
> >   		break;
> > +	case IXGBE_VF_GET_QUEUES:
> > +		retval = ixgbe_get_vf_queues(dev, vf, msgbuf);
> > +		msg_size = IXGBE_VF_GET_QUEUE_MSG_SIZE;
> 
> Although the msg_size semantics and motivation is clear, if u want to do then
> do it all the way - add it to all other cases too not just to
> IXGBE_VF_GET_QUEUES.
> For instance, why do u write all 16 DWORDS for API negotiation (only 2 are
> required) and only here u decided to get "greedy"? ;)
> 
> My point is: either drop it completely or fix all other places as well.

This is because the actual message size required by 2 different message(api-negotiation and vf-get-queue)
are different, the first one require only 4 bytes, the second one need 20 bytes.
If both use 4 bytes, then the second one will have incomplete message.
If both use 20 bytes, then the first one will contain garbage info which is not necessary at all.
So the code logic looks as above.

> > +		break;
> >   	default:
> >   		PMD_DRV_LOG(DEBUG, "Unhandled Msg %8.8x",
> (unsigned)msgbuf[0]);
> >   		retval = IXGBE_ERR_MBX;
> > @@ -551,7 +584,7 @@ ixgbe_rcv_msg_from_vf(struct rte_eth_dev *dev,
> > uint16_t vf)
> >
> >   	msgbuf[0] |= IXGBE_VT_MSGTYPE_CTS;
> >
> > -	ixgbe_write_mbx(hw, msgbuf, 1, vf);
> > +	ixgbe_write_mbx(hw, msgbuf, msg_size, vf);
> >
> >   	return retval;
> >   }