> -----Original Message-----
> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
> Sent: Tuesday, January 6, 2015 7:27 PM
> To: Ouyang, Changchun; dev at 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 at cloudius-systems.com]
> >> Sent: Monday, January 5, 2015 6:07 PM
> >> To: Ouyang, Changchun; dev at 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 at cloudius-systems.com]
> >>>> Sent: Sunday, January 4, 2015 4:39 PM
> >>>> To: Ouyang, Changchun; dev at 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 at 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;
> >>>>> }
