From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Zolotarov Subject: Re: [PATCH v4 3/6] ixgbe: Get VF queue number Date: Tue, 06 Jan 2015 13:26:49 +0200 Message-ID: <54ABC679.9070202@cloudius-systems.com> References: <1419398584-19520-1-git-send-email-changchun.ouyang@intel.com> <1420355937-18484-1-git-send-email-changchun.ouyang@intel.com> <1420355937-18484-4-git-send-email-changchun.ouyang@intel.com> <54A8FC16.40402@cloudius-systems.com> <54AA625E.9060607@cloudius-systems.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: "Ouyang, Changchun" , "dev-VfR2kkLFssw@public.gmane.org" Return-path: In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On 01/06/15 03:54, Ouyang, Changchun wrote: > >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz-RmZWMc9puTNJc61us3aD9laTQe2KTcn/@public.gmane.org] >> Sent: Monday, January 5, 2015 6:07 PM >> To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.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-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 >>>>> --- >>>>> 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. ;) > >>>>> + >>>>> + 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! ;) > >>>>> + 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; >>>>> }