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: Monday, January 5, 2015 6:07 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 3/6] ixgbe: Get VF queue number
> 
> 
> On 01/05/15 04:59, Ouyang, Changchun wrote:
> >
> >> -----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.
> 
> But VFs do support VLAN stripping and u don't add it to just RSS. If VFs do not
> support VLAN stripping in the DPDK yet they should and then we will need
> this field.

If I don't miss your point, you also agree it is not related to vf rss itself, right?
As for Vlan stripping, it need another patch to support 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.
> 
> I understood the motivation at the first place but as I've explained
> above we already bring the garbage for some opcodes like API
> negotiation. So, u should either fix it for all opcodes like u did for
> GET_QUEUES or just drop it in GET_QUEUES and fix it for all opcodes in a
> different patch.

Here maybe I miss your point, my understanding is that  4 bytes are enough for all other opcode except for get_queue opcode,
 get_queues is the only one that need 20  bytes currently.
So I don't quite understand why I need fix any codes which we both think they are right.   

> >
> >>> +		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;
> >>>    }