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: Tuesday, January 6, 2015 7:27 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 3/6] ixgbe: Get VF queue number
> 
> 
> On 01/06/15 03:54, Ouyang, Changchun wrote:
> >
> >> -----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?
> 
> Right.
> 
> > As for Vlan stripping, it need another patch to support it.
> 
> Well, at least put some fat comment in bold there that some the fields in the
> command is not filled and why. ;)

OK, I will put more comments to explain it in v5.

> >
> >>>>> +
> >>>>> +	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.
> 
> Ooops. I missed the default value msg_size is 1 - I've confused its initialization
> with mbx_size initialization. So, u are right - your code is perfectly fine. My
> apologies! ;)

Never mind :-)
 
> >
> >>>>> +		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;
> >>>>>     }