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