Hi Wenzhuo,
> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Wenzhuo Lu
> Sent: Friday, January 29, 2016 7:48 AM
> To: dev at 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 at 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
