Re: [dpdk-dev] [PATCH] ixgbe: fix x550 VF tx issue

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Wenzhuo,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> Sent: Friday, January 29, 2016 7:48 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] ixgbe: fix x550 VF tx issue
> 
> On x550, the basic tx function may not work if we use DPDK VF based on
> kernel PF. The reason is kernel PF enables a new feature "malicious driver
> detection". According to this feature, driver should create TX context
> descriptor and set the "Check Context" bit in advanced data descriptor.
> This patch add the support of "malicious driver detection" to let kernel PF
> based DPDK VF work.

Is there any way for VF to determine is MDD is enabled on PF or not?
Mailbox message or something?
If not, then I suppose for x550 VF we always have to set default txq_flags in
dev_info to PKT_TX_MALICIOUS_DRIVER_DETECT?
Which means that for X550 VF simple TX path by default will be disabled, correct?
As alternative we probably can change tx_queue_start() for x550 VF to always
setup a dummy TCD and then make changes in TX simple path to always setup
CC bit for x550 VF.
That would allow to keep using TX simple for x550 VF , no matter is MDD is enabled  
by PF or not.

Another thing - there could be a potential slowdown with that feature:
As now TX has to occupy one extra HW context, even if no HW offload is requested by SW,
 correct?
Let say if app sends packets of 3 types:
- TCP with TX HW csum offload
- UDP with TX HW csum offload
- packets with no HW offload requested   
Then on 82599 VF no PMD would need to setup TCD only fist 2 times.
But on X550 VF will need to setup new TCD for every packet.
But I suppose that is unavoidable.

> 
> Although CC bit should be set in IOV mode, as tested, it's no harm to set
> CC bit in non-IVO mode. So, as PF and VF share the same code, will set CC
> bit anyway.

I don't think it totally harmless.
As I understand, if CC bit is set HW would try to apply related TC to the packet.
If this TC was never setup - you don't know what values it will contain.

> 
> Please aware there's another way, disabling MDD in PF kernel driver.
> Like this,
> >insmod ixgbe.ko MDD=0,0
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  doc/guides/rel_notes/release_2_3.rst | 12 ++++++++++++
>  drivers/net/ixgbe/ixgbe_rxtx.c       | 16 ++++++++++++++--
>  drivers/net/ixgbe/ixgbe_rxtx.h       |  3 ++-
>  lib/librte_ether/rte_ethdev.h        |  1 +
>  lib/librte_mbuf/rte_mbuf.h           |  3 ++-
>  5 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_2_3.rst b/doc/guides/rel_notes/release_2_3.rst
> index 99de186..90f81e0 100644
> --- a/doc/guides/rel_notes/release_2_3.rst
> +++ b/doc/guides/rel_notes/release_2_3.rst
> @@ -15,6 +15,18 @@ EAL
>  Drivers
>  ~~~~~~~
> 
> +* **ixgbe: Fixed x550 VF tx issue.**
> +
> +  On x550, the basic tx function may not work if we use DPDK VF based on
> +  kernel PF. The reason is kernel PF enables a new feature "malicious driver
> +  detection". According to this feature, driver should create TX context
> +  descriptor and set the "Check Context" bit in advanced data descriptor.
> +  This patch add the support of "malicious driver detection" to let kernel PF
> +  based DPDK VF work.
> +
> +  Please aware there's another way, disabling MDD in PF kernel driver.
> +  Like this,
> +  >insmod ixgbe.ko MDD=0,0
> 
>  Libraries
>  ~~~~~~~~~
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 52a263c..c8a7740 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -85,7 +85,8 @@
>  		PKT_TX_VLAN_PKT |		 \
>  		PKT_TX_IP_CKSUM |		 \
>  		PKT_TX_L4_MASK |		 \
> -		PKT_TX_TCP_SEG)
> +		PKT_TX_TCP_SEG |		 \
> +		PKT_TX_MALICIOUS_DRIVER_DETECT)
> 
>  static inline struct rte_mbuf *
>  rte_rxmbuf_alloc(struct rte_mempool *mp)
> @@ -564,6 +565,8 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq)
>  	return (0);
>  }
> 
> +#define DEFAULT_CTX_L2_LEN 14

Use sizeof(strutct eth_hdr) or something.

> +
>  uint16_t
>  ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  		uint16_t nb_pkts)
> @@ -614,11 +617,19 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  		 * are needed for offload functionality.
>  		 */
>  		ol_flags = tx_pkt->ol_flags;
> +		if (!(txq->txq_flags & ETH_TXQ_FLAGS_NOMDD))
> +			ol_flags |= PKT_TX_MALICIOUS_DRIVER_DETECT;

Probably a bit better introduce new filed inside txq: deaul_ol_flags or something,
Then set it up depending on HW type, and then just:

ol_flags = tx_pkt->ol_flags | txq->default_ol_flags;


> 
>  		/* If hardware offload required */
>  		tx_ol_req = ol_flags & IXGBE_TX_OFFLOAD_MASK;
>  		if (tx_ol_req) {
> -			tx_offload.l2_len = tx_pkt->l2_len;
> +			/* if l2 len isn't provided by the caller, give it a
> +			 * default value.
> +			 */
> +			if (tx_pkt->l2_len == 0)
> +				tx_offload.l2_len = DEFAULT_CTX_L2_LEN;
> +			else
> +				tx_offload.l2_len = tx_pkt->l2_len;

So with l2_len==0 what would happen?
PF would stop the queue?


>  			tx_offload.l3_len = tx_pkt->l3_len;
>  			tx_offload.l4_len = tx_pkt->l4_len;
>  			tx_offload.vlan_tci = tx_pkt->vlan_tci;
> @@ -801,6 +812,7 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  		}
> 
>  		olinfo_status |= (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
> +		olinfo_status |= IXGBE_ADVTXD_CC;

As I said, probably safer to set CC bit  only if (tx_ol_req != 0).  

> 
>  		m_seg = tx_pkt;
>  		do {
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
> index 475a800..bcde785 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.h
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.h
> @@ -255,7 +255,8 @@ struct ixgbe_txq_ops {
>   * RTE_PMD_IXGBE_TX_MAX_BURST.
>   */
>  #define IXGBE_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
> -			    ETH_TXQ_FLAGS_NOOFFLOADS)
> +			    ETH_TXQ_FLAGS_NOOFFLOADS | \
> +			    ETH_TXQ_FLAGS_NOMDD)
> 
>  /*
>   * Populate descriptors with the following info:
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index bada8ad..a2e32d0 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -637,6 +637,7 @@ struct rte_eth_rxconf {
>  #define ETH_TXQ_FLAGS_NOXSUMSCTP 0x0200 /**< disable SCTP checksum offload */
>  #define ETH_TXQ_FLAGS_NOXSUMUDP  0x0400 /**< disable UDP checksum offload */
>  #define ETH_TXQ_FLAGS_NOXSUMTCP  0x0800 /**< disable TCP checksum offload */
> +#define ETH_TXQ_FLAGS_NOMDD      0x1000 /**< disable malicious driver detection */
>  #define ETH_TXQ_FLAGS_NOOFFLOADS \
>  		(ETH_TXQ_FLAGS_NOVLANOFFL | ETH_TXQ_FLAGS_NOXSUMSCTP | \
>  		 ETH_TXQ_FLAGS_NOXSUMUDP  | ETH_TXQ_FLAGS_NOXSUMTCP)
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index f234ac9..b5ed25c 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -102,6 +102,7 @@ extern "C" {
> 
>  /* add new TX flags here */
> 
> +#define PKT_TX_MALICIOUS_DRIVER_DETECT (1ULL << 48)
>  /**
>   * Second VLAN insertion (QinQ) flag.
>   */
> @@ -824,7 +825,7 @@ struct rte_mbuf {
>  	struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */
>  	struct rte_mbuf *next;    /**< Next segment of scattered packet. */
> 
> -	/* fields to support TX offloads */
> +	/* fields to support malicious driver detection or TX offloads */

No fields are really updated, so probably no need to update comments here.

>  	union {
>  		uint64_t tx_offload;       /**< combined for easy fetch */
>  		struct {
> --
> 1.9.3