[v2,06/13] net/txgbe: check length of Tx packets

Add checking of the Tx packet length to avoid TDM fatal error as far as
possible. Set the pkt_len=1518 for invalid packet in simple Tx code path,
and drop it directly in featured Tx code path.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/txgbe/txgbe_rxtx.c          | 33 +++++++++++++++++++++++++
 drivers/net/txgbe/txgbe_rxtx_vec_neon.c | 10 +++++---
 drivers/net/txgbe/txgbe_rxtx_vec_sse.c  | 11 ++++++---
 3 files changed, 48 insertions(+), 6 deletions(-)
  

On 10/28/2024 2:31 AM, Jiawen Wu wrote:
> Add checking of the Tx packet length to avoid TDM fatal error as far as
> possible. Set the pkt_len=1518 for invalid packet in simple Tx code path,
> and drop it directly in featured Tx code path.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/txgbe/txgbe_rxtx.c          | 33 +++++++++++++++++++++++++
>  drivers/net/txgbe/txgbe_rxtx_vec_neon.c | 10 +++++---
>  drivers/net/txgbe/txgbe_rxtx_vec_sse.c  | 11 ++++++---
>  3 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/txgbe/txgbe_rxtx.c b/drivers/net/txgbe/txgbe_rxtx.c
> index 2d2b437643..06acbd0881 100644
> --- a/drivers/net/txgbe/txgbe_rxtx.c
> +++ b/drivers/net/txgbe/txgbe_rxtx.c
> @@ -160,6 +160,8 @@ tx4(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
>  	for (i = 0; i < 4; ++i, ++txdp, ++pkts) {
>  		buf_dma_addr = rte_mbuf_data_iova(*pkts);
>  		pkt_len = (*pkts)->data_len;
> +		if (pkt_len < RTE_ETHER_HDR_LEN)
> +			pkt_len = TXGBE_FRAME_SIZE_DFT;
>  
>  		/* write data to descriptor */
>  		txdp->qw0 = rte_cpu_to_le_64(buf_dma_addr);
> @@ -180,6 +182,8 @@ tx1(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
>  
>  	buf_dma_addr = rte_mbuf_data_iova(*pkts);
>  	pkt_len = (*pkts)->data_len;
> +	if (pkt_len < RTE_ETHER_HDR_LEN)
> +		pkt_len = TXGBE_FRAME_SIZE_DFT;
>  
>  	/* write data to descriptor */
>  	txdp->qw0 = cpu_to_le64(buf_dma_addr);
> @@ -813,6 +817,30 @@ txgbe_parse_tun_ptid(struct rte_mbuf *tx_pkt, uint8_t tun_len)
>  	return ptid;
>  }
>  
> +static inline bool
> +txgbe_check_pkt_err(struct rte_mbuf *tx_pkt)
> +{
> +	uint32_t total_len = 0, nb_seg = 0;
> +	struct rte_mbuf *mseg;
> +
> +	mseg = tx_pkt;
> +	do {
> +		if (mseg->data_len == 0)
> +			return true;
> +		total_len += mseg->data_len;
> +		nb_seg++;
> +		mseg = mseg->next;
> +	} while (mseg != NULL);
> +
> +	if (tx_pkt->pkt_len != total_len || tx_pkt->pkt_len == 0)
> +		return true;
> +
> +	if (tx_pkt->nb_segs != nb_seg || tx_pkt->nb_segs > 64)
> +		return true;
> +
> +	return false;
> +}
> +
>

Hi Jiawen,

Above are generic checks, we may add this function to ethdev driver
header (ethdev_driver.h) so that any PMD can use it, what do you think?
  

On Fri, Nov 1, 2024 9:23 AM, Ferruh Yigit wrote:
> On 10/28/2024 2:31 AM, Jiawen Wu wrote:
> > Add checking of the Tx packet length to avoid TDM fatal error as far as
> > possible. Set the pkt_len=1518 for invalid packet in simple Tx code path,
> > and drop it directly in featured Tx code path.
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >  drivers/net/txgbe/txgbe_rxtx.c          | 33 +++++++++++++++++++++++++
> >  drivers/net/txgbe/txgbe_rxtx_vec_neon.c | 10 +++++---
> >  drivers/net/txgbe/txgbe_rxtx_vec_sse.c  | 11 ++++++---
> >  3 files changed, 48 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/txgbe/txgbe_rxtx.c b/drivers/net/txgbe/txgbe_rxtx.c
> > index 2d2b437643..06acbd0881 100644
> > --- a/drivers/net/txgbe/txgbe_rxtx.c
> > +++ b/drivers/net/txgbe/txgbe_rxtx.c
> > @@ -160,6 +160,8 @@ tx4(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
> >  	for (i = 0; i < 4; ++i, ++txdp, ++pkts) {
> >  		buf_dma_addr = rte_mbuf_data_iova(*pkts);
> >  		pkt_len = (*pkts)->data_len;
> > +		if (pkt_len < RTE_ETHER_HDR_LEN)
> > +			pkt_len = TXGBE_FRAME_SIZE_DFT;
> >
> >  		/* write data to descriptor */
> >  		txdp->qw0 = rte_cpu_to_le_64(buf_dma_addr);
> > @@ -180,6 +182,8 @@ tx1(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
> >
> >  	buf_dma_addr = rte_mbuf_data_iova(*pkts);
> >  	pkt_len = (*pkts)->data_len;
> > +	if (pkt_len < RTE_ETHER_HDR_LEN)
> > +		pkt_len = TXGBE_FRAME_SIZE_DFT;
> >
> >  	/* write data to descriptor */
> >  	txdp->qw0 = cpu_to_le64(buf_dma_addr);
> > @@ -813,6 +817,30 @@ txgbe_parse_tun_ptid(struct rte_mbuf *tx_pkt, uint8_t tun_len)
> >  	return ptid;
> >  }
> >
> > +static inline bool
> > +txgbe_check_pkt_err(struct rte_mbuf *tx_pkt)
> > +{
> > +	uint32_t total_len = 0, nb_seg = 0;
> > +	struct rte_mbuf *mseg;
> > +
> > +	mseg = tx_pkt;
> > +	do {
> > +		if (mseg->data_len == 0)
> > +			return true;
> > +		total_len += mseg->data_len;
> > +		nb_seg++;
> > +		mseg = mseg->next;
> > +	} while (mseg != NULL);
> > +
> > +	if (tx_pkt->pkt_len != total_len || tx_pkt->pkt_len == 0)
> > +		return true;
> > +
> > +	if (tx_pkt->nb_segs != nb_seg || tx_pkt->nb_segs > 64)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >
> 
> Hi Jiawen,
> 
> Above are generic checks, we may add this function to ethdev driver
> header (ethdev_driver.h) so that any PMD can use it, what do you think?

In fact, the limitation of tx_pkt->nb_segs is already implemented in
.tx_pkt_prepare. But users developing their own apps don't necessarily call
this function. So I'd like to add these in txgbe_xmit_pkts(). What are you
going to do about it?
  

On 11/1/2024 1:52 AM, Jiawen Wu wrote:
> On Fri, Nov 1, 2024 9:23 AM, Ferruh Yigit wrote:
>> On 10/28/2024 2:31 AM, Jiawen Wu wrote:
>>> Add checking of the Tx packet length to avoid TDM fatal error as far as
>>> possible. Set the pkt_len=1518 for invalid packet in simple Tx code path,
>>> and drop it directly in featured Tx code path.
>>>
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>> ---
>>>  drivers/net/txgbe/txgbe_rxtx.c          | 33 +++++++++++++++++++++++++
>>>  drivers/net/txgbe/txgbe_rxtx_vec_neon.c | 10 +++++---
>>>  drivers/net/txgbe/txgbe_rxtx_vec_sse.c  | 11 ++++++---
>>>  3 files changed, 48 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/txgbe/txgbe_rxtx.c b/drivers/net/txgbe/txgbe_rxtx.c
>>> index 2d2b437643..06acbd0881 100644
>>> --- a/drivers/net/txgbe/txgbe_rxtx.c
>>> +++ b/drivers/net/txgbe/txgbe_rxtx.c
>>> @@ -160,6 +160,8 @@ tx4(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
>>>  	for (i = 0; i < 4; ++i, ++txdp, ++pkts) {
>>>  		buf_dma_addr = rte_mbuf_data_iova(*pkts);
>>>  		pkt_len = (*pkts)->data_len;
>>> +		if (pkt_len < RTE_ETHER_HDR_LEN)
>>> +			pkt_len = TXGBE_FRAME_SIZE_DFT;
>>>
>>>  		/* write data to descriptor */
>>>  		txdp->qw0 = rte_cpu_to_le_64(buf_dma_addr);
>>> @@ -180,6 +182,8 @@ tx1(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
>>>
>>>  	buf_dma_addr = rte_mbuf_data_iova(*pkts);
>>>  	pkt_len = (*pkts)->data_len;
>>> +	if (pkt_len < RTE_ETHER_HDR_LEN)
>>> +		pkt_len = TXGBE_FRAME_SIZE_DFT;
>>>
>>>  	/* write data to descriptor */
>>>  	txdp->qw0 = cpu_to_le64(buf_dma_addr);
>>> @@ -813,6 +817,30 @@ txgbe_parse_tun_ptid(struct rte_mbuf *tx_pkt, uint8_t tun_len)
>>>  	return ptid;
>>>  }
>>>
>>> +static inline bool
>>> +txgbe_check_pkt_err(struct rte_mbuf *tx_pkt)
>>> +{
>>> +	uint32_t total_len = 0, nb_seg = 0;
>>> +	struct rte_mbuf *mseg;
>>> +
>>> +	mseg = tx_pkt;
>>> +	do {
>>> +		if (mseg->data_len == 0)
>>> +			return true;
>>> +		total_len += mseg->data_len;
>>> +		nb_seg++;
>>> +		mseg = mseg->next;
>>> +	} while (mseg != NULL);
>>> +
>>> +	if (tx_pkt->pkt_len != total_len || tx_pkt->pkt_len == 0)
>>> +		return true;
>>> +
>>> +	if (tx_pkt->nb_segs != nb_seg || tx_pkt->nb_segs > 64)
>>> +		return true;
>>> +
>>> +	return false;
>>> +}
>>> +
>>>
>>
>> Hi Jiawen,
>>
>> Above are generic checks, we may add this function to ethdev driver
>> header (ethdev_driver.h) so that any PMD can use it, what do you think?
> 
> In fact, the limitation of tx_pkt->nb_segs is already implemented in
> .tx_pkt_prepare. But users developing their own apps don't necessarily call
> this function. So I'd like to add these in txgbe_xmit_pkts(). What are you
> going to do about it?
> 
> 

I had the same thought, that is why I am not suggesting to add these
checks to .tx_pkt_prepare() function.

But still these generic checks can go into a new static inline function
for drivers to use, not for end users, and any other driver can call
this function. Your two drivers already using the same function.

This is not hard requirement, but if there is a common code for multiple
drivers, that can go into a helper function in ethdev_driver.h
  

On Fri, 1 Nov 2024 01:22:47 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> Hi Jiawen,
> 
> Above are generic checks, we may add this function to ethdev driver
> header (ethdev_driver.h) so that any PMD can use it, what do you think?

This is in the fastpath, and additional checks should not be added there.
Or at least put them under RTE_ETHDEV_DEBUG.
  

On 11/1/2024 2:56 AM, Stephen Hemminger wrote:
> On Fri, 1 Nov 2024 01:22:47 +0000
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> Hi Jiawen,
>>
>> Above are generic checks, we may add this function to ethdev driver
>> header (ethdev_driver.h) so that any PMD can use it, what do you think?
> 
> This is in the fastpath, and additional checks should not be added there.
> Or at least put them under RTE_ETHDEV_DEBUG.
>

It is in the datapath, and ideally there shouldn't be error checks in
datapath.
But if HW does not check for errors and doesn't recover from error
conditions, driver may prefer to get the hit and do the checks.

Not for all drivers, but for the drivers that intentionally wants do the
check, I thought having a common helper function helps but I can see it
can create confusion.

@Jiawen, please continue as it is in next version, I mean have the
checks function in the driver itself.