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

[Vlad Zolotarov <vladz-RmZWMc9puTNJc61us3aD9laTQe2KTcn/@public.gmane.org>]

On 01/05/15 04:59, Ouyang, Changchun wrote:
>
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz-RmZWMc9puTNJc61us3aD9laTQe2KTcn/@public.gmane.org]
>> Sent: Sunday, January 4, 2015 4:39 PM
>> To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> ---
>>>    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.

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

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