diff mbox

[dpdk-dev,v4,3/6] ixgbe: Get VF queue number

Message ID 1420355937-18484-4-git-send-email-changchun.ouyang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Ouyang Changchun Jan. 4, 2015, 7:18 a.m. UTC 
  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(-)

Comments

Vladislav Zolotarov Jan. 4, 2015, 8:38 a.m. UTC | #1 
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?

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

> +		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;
>   }
Ouyang Changchun Jan. 5, 2015, 2:59 a.m. UTC | #2 
> -----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.

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

> > +		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;
> >   }
Vladislav Zolotarov Jan. 5, 2015, 10:07 a.m. UTC | #3 
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.

>
>>> +
>>> +	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;
>>>    }
Ouyang Changchun Jan. 6, 2015, 1:54 a.m. UTC | #4 
> -----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?
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;
> >>>    }
Vladislav Zolotarov Jan. 6, 2015, 11:26 a.m. UTC | #5 
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. ;)

>    
>>>>> +
>>>>> +	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;
>>>>>     }
Ouyang Changchun Jan. 7, 2015, 1:18 a.m. UTC | #6 
> -----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;
> >>>>>     }
diff mbox

Patch

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