[PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

When KEEP_CRC offload is enabled, some packets will be truncated and
the CRC is still be stripped in following cases:
1. For HIP08 hardware, the packet type is TCP and the length
   is less than or equal to 60B.
2. For other hardwares, the packet type is IP and the length
   is less than or equal to 60B.

So driver has to recaculate positively the CRC of these packets
on above cases.

In addition, to avoid impacting performance, KEEP_CRC is not
supported when NEON or SVE algorithm is used.

Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Signed-off-by: Jie Hai <haijie1@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c          | 116 ++++++++++++++++++++------
 drivers/net/hns3/hns3_rxtx.h          |   9 ++
 drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
 drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 -----
 drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
 5 files changed, 103 insertions(+), 47 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 7e636a0a2e99..6e9b6db843f9 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -11,6 +11,7 @@
 #include <rte_io.h>
 #include <rte_net.h>
 #include <rte_malloc.h>
+#include <rte_net_crc.h>
 #if defined(RTE_ARCH_ARM64)
 #include <rte_cpuflags.h>
 #include <rte_vect.h>
@@ -1732,8 +1733,9 @@ hns3_rx_buf_len_calc(struct rte_mempool *mp, uint16_t *rx_buf_len)
 }
 
 static int
-hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
-				uint16_t nb_desc)
+hns3_rxq_conf_runtime_check(struct hns3_hw *hw,
+			    const struct rte_eth_rxconf *conf,
+			    uint16_t buf_size, uint16_t nb_desc)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
 	eth_rx_burst_t pkt_burst = dev->rx_pkt_burst;
@@ -1766,6 +1768,14 @@ hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
 			return -EINVAL;
 		}
 	}
+
+	if ((conf->offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC) &&
+	    pkt_burst != hns3_recv_pkts_simple &&
+	    pkt_burst != hns3_recv_scattered_pkts) {
+		hns3_err(hw, "KEEP_CRC offload is not supported in the current rx function.");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -1802,7 +1812,7 @@ hns3_rx_queue_conf_check(struct hns3_hw *hw, const struct rte_eth_rxconf *conf,
 	}
 
 	if (hw->data->dev_started) {
-		ret = hns3_rxq_conf_runtime_check(hw, *buf_size, nb_desc);
+		ret = hns3_rxq_conf_runtime_check(hw, conf, *buf_size, nb_desc);
 		if (ret) {
 			hns3_err(hw, "Rx queue runtime setup fail.");
 			return ret;
@@ -1917,6 +1927,8 @@ hns3_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t nb_desc,
 	memset(&rxq->err_stats, 0, sizeof(struct hns3_rx_bd_errors_stats));
 	memset(&rxq->dfx_stats, 0, sizeof(struct hns3_rx_dfx_stats));
 
+	if (hw->revision < PCI_REVISION_ID_HIP09_A)
+		rxq->keep_crc_fail_only_tcp = true;
 	/* CRC len set here is used for amending packet length */
 	if (dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC)
 		rxq->crc_len = RTE_ETHER_CRC_LEN;
@@ -2400,6 +2412,47 @@ hns3_rx_ptp_timestamp_handle(struct hns3_rx_queue *rxq, struct rte_mbuf *mbuf,
 	pf->rx_timestamp = timestamp;
 }
 
+static inline bool
+hns3_need_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+	uint32_t ptype = m->packet_type;
+
+	if (m->pkt_len > HNS3_KEEP_CRC_OK_MIN_PKT_LEN)
+		return false;
+
+	if (!(RTE_ETH_IS_IPV4_HDR(ptype) || RTE_ETH_IS_IPV6_HDR(ptype)))
+		return false;
+
+	if (rxq->keep_crc_fail_only_tcp)
+		return (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP;
+
+	return true;
+}
+
+static inline void
+hns3_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+	char *append_data;
+	uint32_t crc;
+
+	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
+			       m->data_len, RTE_NET_CRC32_ETH);
+
+	/*
+	 * The hns3 driver requires that mbuf size must be at least 512B.
+	 * When CRC is stripped by hardware, the pkt_len must be less than
+	 * or equal to 60B. Therefore, the space of the mbuf is enough
+	 * to insert the CRC.
+	 *
+	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
+	 * do not contain the CRC length. Therefore, after CRC data is appended
+	 * by PMD again, both pkt_len and data_len add the CRC length.
+	 */
+	append_data = rte_pktmbuf_append(m, rxq->crc_len);
+	/* The CRC data is binary data and does not care about the byte order. */
+	rte_memcpy(append_data, (void *)&crc, rxq->crc_len);
+}
+
 uint16_t
 hns3_recv_pkts_simple(void *rx_queue,
 		      struct rte_mbuf **rx_pkts,
@@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
 		rxdp->rx.bd_base_info = 0;
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
-		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
-				rxq->crc_len;
+		rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
 		rxm->data_len = rxm->pkt_len;
 		rxm->port = rxq->port_id;
 		rxm->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
@@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
 			goto pkt_err;
 
 		rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info, ol_info);
-
 		if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
 
+		if (unlikely(rxq->crc_len > 0)) {
+			if (hns3_need_recalculate_crc(rxq, rxm))
+				hns3_recalculate_crc(rxq, rxm);
+			rxm->pkt_len -= rxq->crc_len;
+			rxm->data_len -= rxq->crc_len;
+		}
+
 		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
 
 		/* Increment bytes counter  */
@@ -2662,10 +2720,10 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
 		rxm->data_len = rte_le_to_cpu_16(rxd.rx.size);
+		rxm->next = NULL;
 
 		if (!(bd_base_info & BIT(HNS3_RXD_FE_B))) {
 			last_seg = rxm;
-			rxm->next = NULL;
 			continue;
 		}
 
@@ -2679,23 +2737,6 @@ hns3_recv_scattered_pkts(void *rx_queue,
 		 */
 		first_seg->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
 
-		/*
-		 * This is the last buffer of the received packet. If the CRC
-		 * is not stripped by the hardware:
-		 *  - Subtract the CRC length from the total packet length.
-		 *  - If the last buffer only contains the whole CRC or a part
-		 *  of it, free the mbuf associated to the last buffer. If part
-		 *  of the CRC is also contained in the previous mbuf, subtract
-		 *  the length of that CRC part from the data length of the
-		 *  previous mbuf.
-		 */
-		rxm->next = NULL;
-		if (unlikely(rxq->crc_len > 0)) {
-			first_seg->pkt_len -= rxq->crc_len;
-			recalculate_data_len(first_seg, last_seg, rxm, rxq,
-				rxm->data_len);
-		}
-
 		first_seg->port = rxq->port_id;
 		first_seg->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
 		first_seg->ol_flags = RTE_MBUF_F_RX_RSS_HASH;
@@ -2721,10 +2762,35 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		first_seg->packet_type = hns3_rx_calc_ptype(rxq,
 						l234_info, ol_info);
-
 		if (first_seg->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
 
+		/*
+		 * This is the last buffer of the received packet. If the CRC
+		 * is not stripped by the hardware:
+		 *  - Subtract the CRC length from the total packet length.
+		 *  - If the last buffer only contains the whole CRC or a part
+		 *  of it, free the mbuf associated to the last buffer. If part
+		 *  of the CRC is also contained in the previous mbuf, subtract
+		 *  the length of that CRC part from the data length of the
+		 *  previous mbuf.
+		 *
+		 * In addition, the CRC is still stripped for a kind of packets:
+		 * 1. All IP-TCP packet whose the length is less than and equal
+		 *    to 60 Byte (no CRC) on HIP08 hardware.
+		 * 2. All IP packet whose the length is less than and equal to
+		 *    60 Byte (no CRC) on other hardware.
+		 * In this case, the PMD calculates the CRC and appends it to
+		 * mbuf.
+		 */
+		if (unlikely(rxq->crc_len > 0)) {
+			if (hns3_need_recalculate_crc(rxq, first_seg))
+				hns3_recalculate_crc(rxq, first_seg);
+			first_seg->pkt_len -= rxq->crc_len;
+			recalculate_data_len(first_seg, last_seg, rxm, rxq,
+				rxm->data_len);
+		}
+
 		hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
 
 		/* Increment bytes counter */
diff --git a/drivers/net/hns3/hns3_rxtx.h b/drivers/net/hns3/hns3_rxtx.h
index e2ad42bb8e30..8b70a8238ae0 100644
--- a/drivers/net/hns3/hns3_rxtx.h
+++ b/drivers/net/hns3/hns3_rxtx.h
@@ -178,6 +178,8 @@
 		(HNS3_TXD_VLD_CMD | HNS3_TXD_FE_CMD | HNS3_TXD_DEFAULT_BDTYPE)
 #define HNS3_TXD_SEND_SIZE_SHIFT	16
 
+#define HNS3_KEEP_CRC_OK_MIN_PKT_LEN	60
+
 enum hns3_pkt_l2t_type {
 	HNS3_L2_TYPE_UNICAST,
 	HNS3_L2_TYPE_MULTICAST,
@@ -328,6 +330,13 @@ struct hns3_rx_queue {
 	/* 4 if RTE_ETH_RX_OFFLOAD_KEEP_CRC offload set, 0 otherwise */
 	uint8_t crc_len;
 
+	/*
+	 * Indicate the range of packet type that fail to keep CRC.
+	 * For HIP08 hardware, only the IP-TCP packet type is included.
+	 * For other hardwares, all IP packet type is included.
+	 */
+	uint8_t keep_crc_fail_only_tcp:1;
+
 	/*
 	 * Indicate whether ignore the outer VLAN field in the Rx BD reported
 	 * by the Hardware. Because the outer VLAN is the PVID if the PVID is
diff --git a/drivers/net/hns3/hns3_rxtx_vec.c b/drivers/net/hns3/hns3_rxtx_vec.c
index 9708ec614e02..bf37ce51b1ad 100644
--- a/drivers/net/hns3/hns3_rxtx_vec.c
+++ b/drivers/net/hns3/hns3_rxtx_vec.c
@@ -185,7 +185,8 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO |
 				 RTE_ETH_RX_OFFLOAD_VLAN |
-				 RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+				 RTE_ETH_RX_OFFLOAD_TIMESTAMP |
+				 RTE_ETH_RX_OFFLOAD_KEEP_CRC;
 
 	if (dev->data->scattered_rx)
 		return -ENOTSUP;
diff --git a/drivers/net/hns3/hns3_rxtx_vec_neon.h b/drivers/net/hns3/hns3_rxtx_vec_neon.h
index 0dc6b9f0a22d..60ec501a2ab6 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_neon.h
+++ b/drivers/net/hns3/hns3_rxtx_vec_neon.h
@@ -148,14 +148,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		8, 9, 10, 11,	         /* rx.rss_hash to rte_mbuf.hash.rss */
 	};
 
-	uint16x8_t crc_adjust = {
-		0, 0,         /* ignore pkt_type field */
-		rxq->crc_len, /* sub crc on pkt_len */
-		0,            /* ignore high-16bits of pkt_len */
-		rxq->crc_len, /* sub crc on data_len */
-		0, 0, 0,      /* ignore non-length fields */
-	};
-
 	/* compile-time verifies the shuffle mask */
 	RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, pkt_len) !=
 			 offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4);
@@ -171,7 +163,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		uint8x16_t pkt_mb1, pkt_mb2, pkt_mb3, pkt_mb4;
 		uint64x2_t mbp1, mbp2;
 		uint16x4_t bd_vld = {0};
-		uint16x8_t tmp;
 		uint64_t stat;
 
 		/* calc how many bd valid */
@@ -225,16 +216,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		pkt_mb3 = vqtbl2q_u8(pkt_mbuf3, shuf_desc_fields_msk);
 		pkt_mb4 = vqtbl2q_u8(pkt_mbuf4, shuf_desc_fields_msk);
 
-		/* 4 packets remove crc */
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb1), crc_adjust);
-		pkt_mb1 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb2), crc_adjust);
-		pkt_mb2 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb3), crc_adjust);
-		pkt_mb3 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb4), crc_adjust);
-		pkt_mb4 = vreinterpretq_u8_u16(tmp);
-
 		/* save packet info to rx_pkts mbuf */
 		vst1q_u8((void *)&sw_ring[pos + 0].mbuf->rx_descriptor_fields1,
 			 pkt_mb1);
diff --git a/drivers/net/hns3/hns3_rxtx_vec_sve.c b/drivers/net/hns3/hns3_rxtx_vec_sve.c
index 8aa4448558cf..67c87f570e8a 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_sve.c
+++ b/drivers/net/hns3/hns3_rxtx_vec_sve.c
@@ -36,8 +36,7 @@ hns3_desc_parse_field_sve(struct hns3_rx_queue *rxq,
 		/* init rte_mbuf.rearm_data last 64-bit */
 		rx_pkts[i]->ol_flags = RTE_MBUF_F_RX_RSS_HASH;
 		rx_pkts[i]->hash.rss = rxdp[i].rx.rss_hash;
-		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len) -
-					rxq->crc_len;
+		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len);
 		rx_pkts[i]->data_len = rx_pkts[i]->pkt_len;
 
 		l234_info = rxdp[i].rx.l234_info;
-- 
2.30.0

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Ferruh Yigit]

On 2/6/2024 1:10 AM, Jie Hai wrote:
> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> When KEEP_CRC offload is enabled, some packets will be truncated and
> the CRC is still be stripped in following cases:
> 1. For HIP08 hardware, the packet type is TCP and the length
>    is less than or equal to 60B.
> 2. For other hardwares, the packet type is IP and the length
>    is less than or equal to 60B.
> 

If a device doesn't support the offload by some packets, it can be
option to disable offload for that device, instead of calculating it in
software and append it.
Unless you have a specific usecase, or requirement to support the offload.

<...>

> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>  			goto pkt_err;
>  
>  		rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info, ol_info);
> -
>  		if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>  			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>  
> +		if (unlikely(rxq->crc_len > 0)) {
> +			if (hns3_need_recalculate_crc(rxq, rxm))
> +				hns3_recalculate_crc(rxq, rxm);
> +			rxm->pkt_len -= rxq->crc_len;
> +			rxm->data_len -= rxq->crc_len;
>

Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
practically same as stripping CRC.

We don't count CRC length in the statistics, but it should be accessible
in the payload by the user.

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Jie Hai]

Hi, Ferruh,

Thanks for your review.

On 2024/2/7 22:15, Ferruh Yigit wrote:
> On 2/6/2024 1:10 AM, Jie Hai wrote:
>> From: Dengdui Huang <huangdengdui@huawei.com>
>>
>> When KEEP_CRC offload is enabled, some packets will be truncated and
>> the CRC is still be stripped in following cases:
>> 1. For HIP08 hardware, the packet type is TCP and the length
>>     is less than or equal to 60B.
>> 2. For other hardwares, the packet type is IP and the length
>>     is less than or equal to 60B.
>>
> 
> If a device doesn't support the offload by some packets, it can be
> option to disable offload for that device, instead of calculating it in
> software and append it.

The KEEP CRC feature of hns3 is faulty only in the specific packet
type and small packet(<60B) case.
What's more, the small ethernet packet is not common.

> Unless you have a specific usecase, or requirement to support the offload.

Yes, some users of hns3 are already using this feature.
So we cannot drop this offload

> <...>
> 
>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>   			goto pkt_err;
>>   
>>   		rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info, ol_info);
>> -
>>   		if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>   			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>   
>> +		if (unlikely(rxq->crc_len > 0)) {
>> +			if (hns3_need_recalculate_crc(rxq, rxm))
>> +				hns3_recalculate_crc(rxq, rxm);
>> +			rxm->pkt_len -= rxq->crc_len;
>> +			rxm->data_len -= rxq->crc_len;
>>
> 
> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
> practically same as stripping CRC.
> 
> We don't count CRC length in the statistics, but it should be accessible
> in the payload by the user.
Our drivers are behaving exactly as you say.
> 
> .

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Ferruh Yigit]

On 2/20/2024 3:58 AM, Jie Hai wrote:
> Hi, Ferruh,
> 
> Thanks for your review.
> 
> On 2024/2/7 22:15, Ferruh Yigit wrote:
>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>
>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>> the CRC is still be stripped in following cases:
>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>     is less than or equal to 60B.
>>> 2. For other hardwares, the packet type is IP and the length
>>>     is less than or equal to 60B.
>>>
>>
>> If a device doesn't support the offload by some packets, it can be
>> option to disable offload for that device, instead of calculating it in
>> software and append it.
> 
> The KEEP CRC feature of hns3 is faulty only in the specific packet
> type and small packet(<60B) case.
> What's more, the small ethernet packet is not common.
> 
>> Unless you have a specific usecase, or requirement to support the
>> offload.
> 
> Yes, some users of hns3 are already using this feature.
> So we cannot drop this offload
> 
>> <...>
>>
>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>               goto pkt_err;
>>>             rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>> ol_info);
>>> -
>>>           if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>               rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>   +        if (unlikely(rxq->crc_len > 0)) {
>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>> +                hns3_recalculate_crc(rxq, rxm);
>>> +            rxm->pkt_len -= rxq->crc_len;
>>> +            rxm->data_len -= rxq->crc_len;
>>>
>>
>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>> practically same as stripping CRC.
>>
>> We don't count CRC length in the statistics, but it should be accessible
>> in the payload by the user.
> Our drivers are behaving exactly as you say.
>

If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
'rxq->crc_len', can you please explain what above lines does?

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Jie Hai]

On 2024/2/23 21:53, Ferruh Yigit wrote:
> On 2/20/2024 3:58 AM, Jie Hai wrote:
>> Hi, Ferruh,
>>
>> Thanks for your review.
>>
>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>
>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>> the CRC is still be stripped in following cases:
>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>      is less than or equal to 60B.
>>>> 2. For other hardwares, the packet type is IP and the length
>>>>      is less than or equal to 60B.
>>>>
>>>
>>> If a device doesn't support the offload by some packets, it can be
>>> option to disable offload for that device, instead of calculating it in
>>> software and append it.
>>
>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>> type and small packet(<60B) case.
>> What's more, the small ethernet packet is not common.
>>
>>> Unless you have a specific usecase, or requirement to support the
>>> offload.
>>
>> Yes, some users of hns3 are already using this feature.
>> So we cannot drop this offload
>>
>>> <...>
>>>
>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>                goto pkt_err;
>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>> ol_info);
>>>> -
>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>> +            rxm->data_len -= rxq->crc_len;
>>>>
>>>
>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>> practically same as stripping CRC.
>>>
>>> We don't count CRC length in the statistics, but it should be accessible
>>> in the payload by the user.
>> Our drivers are behaving exactly as you say.
>>
> 
> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
> 'rxq->crc_len', can you please explain what above lines does?
> 
> 
@@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
  		rxdp->rx.bd_base_info = 0;

  		rxm->data_off = RTE_PKTMBUF_HEADROOM;
-		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
-				rxq->crc_len;
+		rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);

In the previous code above, the 'pkt_len' is set to the length obtained
from the BD. the length obtained from the BD already contains CRC length.
But as you said above, the DPDK requires that the length of the mbuf
does not contain CRC length . So we subtract 'rxq->crc_len' from
mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
just moves the code around.
> .

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Ferruh Yigit]

On 2/26/2024 3:16 AM, Jie Hai wrote:
> On 2024/2/23 21:53, Ferruh Yigit wrote:
>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>> Hi, Ferruh,
>>>
>>> Thanks for your review.
>>>
>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>
>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>> the CRC is still be stripped in following cases:
>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>      is less than or equal to 60B.
>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>      is less than or equal to 60B.
>>>>>
>>>>
>>>> If a device doesn't support the offload by some packets, it can be
>>>> option to disable offload for that device, instead of calculating it in
>>>> software and append it.
>>>
>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>> type and small packet(<60B) case.
>>> What's more, the small ethernet packet is not common.
>>>
>>>> Unless you have a specific usecase, or requirement to support the
>>>> offload.
>>>
>>> Yes, some users of hns3 are already using this feature.
>>> So we cannot drop this offload
>>>
>>>> <...>
>>>>
>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>                goto pkt_err;
>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>> ol_info);
>>>>> -
>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>
>>>>
>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>> practically same as stripping CRC.
>>>>
>>>> We don't count CRC length in the statistics, but it should be
>>>> accessible
>>>> in the payload by the user.
>>> Our drivers are behaving exactly as you say.
>>>
>>
>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>> 'rxq->crc_len', can you please explain what above lines does?
>>
>>
> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>          rxdp->rx.bd_base_info = 0;
> 
>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
> -                rxq->crc_len;
> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
> 
> In the previous code above, the 'pkt_len' is set to the length obtained
> from the BD. the length obtained from the BD already contains CRC length.
> But as you said above, the DPDK requires that the length of the mbuf
> does not contain CRC length . So we subtract 'rxq->crc_len' from
> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
> just moves the code around.
>

Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
it is other way around and this is our confusion.

CRC length shouldn't be in the statistics, I mean in received bytes stats.
Assume that received packet is 128 bytes and we know it has the CRC,
Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)

But mbuf->data_len & mbuf->pkt_len should have full frame length,
including CRC.

As application explicitly requested to KEEP CRC, it will know last 4
bytes are CRC.
Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
you reduce 'mbuf->data_len' by CRC size, application can't know if 4
bytes after 'mbuf->data_len' is valid CRC or not.

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[huangdengdui]

On 2024/2/27 0:43, Ferruh Yigit wrote:
> On 2/26/2024 3:16 AM, Jie Hai wrote:
>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>> Hi, Ferruh,
>>>>
>>>> Thanks for your review.
>>>>
>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>
>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>> the CRC is still be stripped in following cases:
>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>      is less than or equal to 60B.
>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>      is less than or equal to 60B.
>>>>>>
>>>>>
>>>>> If a device doesn't support the offload by some packets, it can be
>>>>> option to disable offload for that device, instead of calculating it in
>>>>> software and append it.
>>>>
>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>> type and small packet(<60B) case.
>>>> What's more, the small ethernet packet is not common.
>>>>
>>>>> Unless you have a specific usecase, or requirement to support the
>>>>> offload.
>>>>
>>>> Yes, some users of hns3 are already using this feature.
>>>> So we cannot drop this offload
>>>>
>>>>> <...>
>>>>>
>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>                goto pkt_err;
>>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>> ol_info);
>>>>>> -
>>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>
>>>>>
>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>> practically same as stripping CRC.
>>>>>
>>>>> We don't count CRC length in the statistics, but it should be
>>>>> accessible
>>>>> in the payload by the user.
>>>> Our drivers are behaving exactly as you say.
>>>>
>>>
>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>> 'rxq->crc_len', can you please explain what above lines does?
>>>
>>>
>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>          rxdp->rx.bd_base_info = 0;
>>
>>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>> -                rxq->crc_len;
>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>
>> In the previous code above, the 'pkt_len' is set to the length obtained
>> from the BD. the length obtained from the BD already contains CRC length.
>> But as you said above, the DPDK requires that the length of the mbuf
>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>> just moves the code around.
>>
> 
> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
> it is other way around and this is our confusion.
> 
> CRC length shouldn't be in the statistics, I mean in received bytes stats.
> Assume that received packet is 128 bytes and we know it has the CRC,
> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
> 
> But mbuf->data_len & mbuf->pkt_len should have full frame length,
> including CRC.
> 
> As application explicitly requested to KEEP CRC, it will know last 4
> bytes are CRC.
> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
> bytes after 'mbuf->data_len' is valid CRC or not.
> 
I agree with you.

But the implementation of other PMDs supported KEEP_CRC is like this.
In addition, there are probably many users that are already using it.
If we modify it, it may cause applications incompatible.

what do you think?

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Ferruh Yigit]

On 2/28/2024 2:27 AM, huangdengdui wrote:
> 
> 
> On 2024/2/27 0:43, Ferruh Yigit wrote:
>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>> Hi, Ferruh,
>>>>>
>>>>> Thanks for your review.
>>>>>
>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>
>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>> the CRC is still be stripped in following cases:
>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>      is less than or equal to 60B.
>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>      is less than or equal to 60B.
>>>>>>>
>>>>>>
>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>> software and append it.
>>>>>
>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>> type and small packet(<60B) case.
>>>>> What's more, the small ethernet packet is not common.
>>>>>
>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>> offload.
>>>>>
>>>>> Yes, some users of hns3 are already using this feature.
>>>>> So we cannot drop this offload
>>>>>
>>>>>> <...>
>>>>>>
>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>                goto pkt_err;
>>>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>> ol_info);
>>>>>>> -
>>>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>
>>>>>>
>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>> practically same as stripping CRC.
>>>>>>
>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>> accessible
>>>>>> in the payload by the user.
>>>>> Our drivers are behaving exactly as you say.
>>>>>
>>>>
>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>
>>>>
>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>          rxdp->rx.bd_base_info = 0;
>>>
>>>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>> -                rxq->crc_len;
>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>
>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>> from the BD. the length obtained from the BD already contains CRC length.
>>> But as you said above, the DPDK requires that the length of the mbuf
>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>> just moves the code around.
>>>
>>
>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>> it is other way around and this is our confusion.
>>
>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>> Assume that received packet is 128 bytes and we know it has the CRC,
>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>
>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>> including CRC.
>>
>> As application explicitly requested to KEEP CRC, it will know last 4
>> bytes are CRC.
>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>> bytes after 'mbuf->data_len' is valid CRC or not.
>>
> I agree with you.
> 
> But the implementation of other PMDs supported KEEP_CRC is like this.
> In addition, there are probably many users that are already using it.
> If we modify it, it may cause applications incompatible.
> 
> what do you think?
> 
This is documented in the ethdev [1], better to follow the documentation
for all PMDs, can you please highlight the relevant driver code, we can
discuss it with their maintainers.

Alternatively we can document this additionally in the KEEP_CRC feature
document if it helps for the applications.


[1]
https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[huangdengdui]

On 2024/2/28 21:07, Ferruh Yigit wrote:
> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>
>>
>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>> Hi, Ferruh,
>>>>>>
>>>>>> Thanks for your review.
>>>>>>
>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>
>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>      is less than or equal to 60B.
>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>      is less than or equal to 60B.
>>>>>>>>
>>>>>>>
>>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>>> software and append it.
>>>>>>
>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>>> type and small packet(<60B) case.
>>>>>> What's more, the small ethernet packet is not common.
>>>>>>
>>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>>> offload.
>>>>>>
>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>> So we cannot drop this offload
>>>>>>
>>>>>>> <...>
>>>>>>>
>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>                goto pkt_err;
>>>>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>>> ol_info);
>>>>>>>> -
>>>>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>
>>>>>>>
>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>> practically same as stripping CRC.
>>>>>>>
>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>> accessible
>>>>>>> in the payload by the user.
>>>>>> Our drivers are behaving exactly as you say.
>>>>>>
>>>>>
>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>
>>>>>
>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>          rxdp->rx.bd_base_info = 0;
>>>>
>>>>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>> -                rxq->crc_len;
>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>
>>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>>> from the BD. the length obtained from the BD already contains CRC length.
>>>> But as you said above, the DPDK requires that the length of the mbuf
>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>>> just moves the code around.
>>>>
>>>
>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>>> it is other way around and this is our confusion.
>>>
>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>>> Assume that received packet is 128 bytes and we know it has the CRC,
>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>
>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>> including CRC.
>>>
>>> As application explicitly requested to KEEP CRC, it will know last 4
>>> bytes are CRC.
>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>
>> I agree with you.
>>
>> But the implementation of other PMDs supported KEEP_CRC is like this.
>> In addition, there are probably many users that are already using it.
>> If we modify it, it may cause applications incompatible.
>>
>> what do you think?
>>
> This is documented in the ethdev [1], better to follow the documentation
> for all PMDs, can you please highlight the relevant driver code, we can
> discuss it with their maintainers.
> 
> Alternatively we can document this additionally in the KEEP_CRC feature
> document if it helps for the applications.
> 
> 
> [1]
> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257

Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.

Do you mean that we add this description in the KEEP_CRC feature document
and notify all drivers that support KEEP_CRC to follow this documentation?

If so, can you merge this patch first?
Then we send a RFC to disscuss it with all PMDs maintainer.

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Ferruh Yigit]

On 2/29/2024 3:58 AM, huangdengdui wrote:
> 
> 
> On 2024/2/28 21:07, Ferruh Yigit wrote:
>> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>>
>>>
>>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>>> Hi, Ferruh,
>>>>>>>
>>>>>>> Thanks for your review.
>>>>>>>
>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>>
>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>>      is less than or equal to 60B.
>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>>      is less than or equal to 60B.
>>>>>>>>>
>>>>>>>>
>>>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>>>> software and append it.
>>>>>>>
>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>>>> type and small packet(<60B) case.
>>>>>>> What's more, the small ethernet packet is not common.
>>>>>>>
>>>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>>>> offload.
>>>>>>>
>>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>>> So we cannot drop this offload
>>>>>>>
>>>>>>>> <...>
>>>>>>>>
>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>                goto pkt_err;
>>>>>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>>>> ol_info);
>>>>>>>>> -
>>>>>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>>
>>>>>>>>
>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>>> practically same as stripping CRC.
>>>>>>>>
>>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>>> accessible
>>>>>>>> in the payload by the user.
>>>>>>> Our drivers are behaving exactly as you say.
>>>>>>>
>>>>>>
>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>>
>>>>>>
>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>          rxdp->rx.bd_base_info = 0;
>>>>>
>>>>>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>>> -                rxq->crc_len;
>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>>
>>>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>>>> from the BD. the length obtained from the BD already contains CRC length.
>>>>> But as you said above, the DPDK requires that the length of the mbuf
>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>>>> just moves the code around.
>>>>>
>>>>
>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>>>> it is other way around and this is our confusion.
>>>>
>>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>>>> Assume that received packet is 128 bytes and we know it has the CRC,
>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>>
>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>>> including CRC.
>>>>
>>>> As application explicitly requested to KEEP CRC, it will know last 4
>>>> bytes are CRC.
>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>>
>>> I agree with you.
>>>
>>> But the implementation of other PMDs supported KEEP_CRC is like this.
>>> In addition, there are probably many users that are already using it.
>>> If we modify it, it may cause applications incompatible.
>>>
>>> what do you think?
>>>
>> This is documented in the ethdev [1], better to follow the documentation
>> for all PMDs, can you please highlight the relevant driver code, we can
>> discuss it with their maintainers.
>>
>> Alternatively we can document this additionally in the KEEP_CRC feature
>> document if it helps for the applications.
>>
>>
>> [1]
>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257
> 
> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
> 

I think it is clear that pkt_len and data_len should contain crc_len, we
can ask for more comments.

> Do you mean that we add this description in the KEEP_CRC feature document
> and notify all drivers that support KEEP_CRC to follow this documentation?
> 
> If so, can you merge this patch first?
> Then we send a RFC to disscuss it with all PMDs maintainer.
>

Not for drivers, just a suggestion that if we should update feature
documentation with above information for users. So there is no
dependency to features document update.

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[huangdengdui]

On 2024/2/29 17:25, Ferruh Yigit wrote:
> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>
>>
>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>>>
>>>>
>>>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>>>> Hi, Ferruh,
>>>>>>>>
>>>>>>>> Thanks for your review.
>>>>>>>>
>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>>>
>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>>>      is less than or equal to 60B.
>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>>>      is less than or equal to 60B.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>>>>> software and append it.
>>>>>>>>
>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>>>>> type and small packet(<60B) case.
>>>>>>>> What's more, the small ethernet packet is not common.
>>>>>>>>
>>>>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>>>>> offload.
>>>>>>>>
>>>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>>>> So we cannot drop this offload
>>>>>>>>
>>>>>>>>> <...>
>>>>>>>>>
>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>>                goto pkt_err;
>>>>>>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>>>>> ol_info);
>>>>>>>>>> -
>>>>>>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>>>> practically same as stripping CRC.
>>>>>>>>>
>>>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>>>> accessible
>>>>>>>>> in the payload by the user.
>>>>>>>> Our drivers are behaving exactly as you say.
>>>>>>>>
>>>>>>>
>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>>>
>>>>>>>
>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>          rxdp->rx.bd_base_info = 0;
>>>>>>
>>>>>>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>>>> -                rxq->crc_len;
>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>>>
>>>>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>>>>> from the BD. the length obtained from the BD already contains CRC length.
>>>>>> But as you said above, the DPDK requires that the length of the mbuf
>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>>>>> just moves the code around.
>>>>>>
>>>>>
>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>>>>> it is other way around and this is our confusion.
>>>>>
>>>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>>>>> Assume that received packet is 128 bytes and we know it has the CRC,
>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>>>
>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>>>> including CRC.
>>>>>
>>>>> As application explicitly requested to KEEP CRC, it will know last 4
>>>>> bytes are CRC.
>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>>>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>>>
>>>> I agree with you.
>>>>
>>>> But the implementation of other PMDs supported KEEP_CRC is like this.
>>>> In addition, there are probably many users that are already using it.
>>>> If we modify it, it may cause applications incompatible.
>>>>
>>>> what do you think?
>>>>
>>> This is documented in the ethdev [1], better to follow the documentation
>>> for all PMDs, can you please highlight the relevant driver code, we can
>>> discuss it with their maintainers.
>>>
>>> Alternatively we can document this additionally in the KEEP_CRC feature
>>> document if it helps for the applications.
>>>
>>>
>>> [1]
>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257
>>
>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>
> 
> I think it is clear that pkt_len and data_len should contain crc_len, we
> can ask for more comments.
This patch doesn't change the logic for hns3 PMD and the implementation of
other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?

> 
>> Do you mean that we add this description in the KEEP_CRC feature document
>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>
>> If so, can you merge this patch first?
>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>
> 
> Not for drivers, just a suggestion that if we should update feature
> documentation with above information for users. So there is no
> dependency to features document update.
> 
> 
Sorry I'm more confused. What should we do next?

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Ferruh Yigit]

On 3/1/2024 6:55 AM, huangdengdui wrote:
> 
> 
> On 2024/2/29 17:25, Ferruh Yigit wrote:
>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>
>>>
>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>>>>
>>>>>
>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>>>>> Hi, Ferruh,
>>>>>>>>>
>>>>>>>>> Thanks for your review.
>>>>>>>>>
>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>>>>
>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>>>>      is less than or equal to 60B.
>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>>>>      is less than or equal to 60B.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>>>>>> software and append it.
>>>>>>>>>
>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>>>>>> type and small packet(<60B) case.
>>>>>>>>> What's more, the small ethernet packet is not common.
>>>>>>>>>
>>>>>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>>>>>> offload.
>>>>>>>>>
>>>>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>>>>> So we cannot drop this offload
>>>>>>>>>
>>>>>>>>>> <...>
>>>>>>>>>>
>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>>>                goto pkt_err;
>>>>>>>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>>>>>> ol_info);
>>>>>>>>>>> -
>>>>>>>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>>>>> practically same as stripping CRC.
>>>>>>>>>>
>>>>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>>>>> accessible
>>>>>>>>>> in the payload by the user.
>>>>>>>>> Our drivers are behaving exactly as you say.
>>>>>>>>>
>>>>>>>>
>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>>>>
>>>>>>>>
>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>          rxdp->rx.bd_base_info = 0;
>>>>>>>
>>>>>>>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>>>>> -                rxq->crc_len;
>>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>>>>
>>>>>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>>>>>> from the BD. the length obtained from the BD already contains CRC length.
>>>>>>> But as you said above, the DPDK requires that the length of the mbuf
>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>>>>>> just moves the code around.
>>>>>>>
>>>>>>
>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>>>>>> it is other way around and this is our confusion.
>>>>>>
>>>>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>>>>>> Assume that received packet is 128 bytes and we know it has the CRC,
>>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>>>>
>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>>>>> including CRC.
>>>>>>
>>>>>> As application explicitly requested to KEEP CRC, it will know last 4
>>>>>> bytes are CRC.
>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>>>>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>>>>
>>>>> I agree with you.
>>>>>
>>>>> But the implementation of other PMDs supported KEEP_CRC is like this.
>>>>> In addition, there are probably many users that are already using it.
>>>>> If we modify it, it may cause applications incompatible.
>>>>>
>>>>> what do you think?
>>>>>
>>>> This is documented in the ethdev [1], better to follow the documentation
>>>> for all PMDs, can you please highlight the relevant driver code, we can
>>>> discuss it with their maintainers.
>>>>
>>>> Alternatively we can document this additionally in the KEEP_CRC feature
>>>> document if it helps for the applications.
>>>>
>>>>
>>>> [1]
>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257
>>>
>>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>>
>>
>> I think it is clear that pkt_len and data_len should contain crc_len, we
>> can ask for more comments.
> This patch doesn't change the logic for hns3 PMD and the implementation of
> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
> 

If hns3 behaving against the documented behavior, I don't understand why
you are pushing for merging this patch, instead of fixing it.


Other drivers behavior is something else, not directly related to this
patch, but again if you can provide references we can discuss with their
maintainers.


>>
>>> Do you mean that we add this description in the KEEP_CRC feature document
>>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>>
>>> If so, can you merge this patch first?
>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>
>>
>> Not for drivers, just a suggestion that if we should update feature
>> documentation with above information for users. So there is no
>> dependency to features document update.
>>
>>
> Sorry I'm more confused. What should we do next?

There is already API documentation about KEEP_CRC, I think that is
already sufficient for driver developers.

I am just brainstorming if updating './doc/guides/nics/features.rst' can
help end user, but it is not an action or blocker for this patch.

Next step is to update this path.

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Jie Hai]

On 2024/3/1 19:10, Ferruh Yigit wrote:
> On 3/1/2024 6:55 AM, huangdengdui wrote:
>>
>>
>> On 2024/2/29 17:25, Ferruh Yigit wrote:
>>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>>
>>>>
>>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>>> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>>>>>
>>>>>>
>>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>>>>>> Hi, Ferruh,
>>>>>>>>>>
>>>>>>>>>> Thanks for your review.
>>>>>>>>>>
>>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>>>>>
>>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>>>>>       is less than or equal to 60B.
>>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>>>>>       is less than or equal to 60B.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>>>>>>> software and append it.
>>>>>>>>>>
>>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>>>>>>> type and small packet(<60B) case.
>>>>>>>>>> What's more, the small ethernet packet is not common.
>>>>>>>>>>
>>>>>>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>>>>>>> offload.
>>>>>>>>>>
>>>>>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>>>>>> So we cannot drop this offload
>>>>>>>>>>
>>>>>>>>>>> <...>
>>>>>>>>>>>
>>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>>>>                 goto pkt_err;
>>>>>>>>>>>>               rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>>>>>>> ol_info);
>>>>>>>>>>>> -
>>>>>>>>>>>>             if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>>>>>                 rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>>>>>     +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>>>>>> practically same as stripping CRC.
>>>>>>>>>>>
>>>>>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>>>>>> accessible
>>>>>>>>>>> in the payload by the user.
>>>>>>>>>> Our drivers are behaving exactly as you say.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>           rxdp->rx.bd_base_info = 0;
>>>>>>>>
>>>>>>>>           rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>>>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>>>>>> -                rxq->crc_len;
>>>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>>>>>
>>>>>>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>>>>>>> from the BD. the length obtained from the BD already contains CRC length.
>>>>>>>> But as you said above, the DPDK requires that the length of the mbuf
>>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>>>>>>> just moves the code around.
>>>>>>>>
>>>>>>>
>>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>>>>>>> it is other way around and this is our confusion.
>>>>>>>
>>>>>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>>>>>>> Assume that received packet is 128 bytes and we know it has the CRC,
>>>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>>>>>
>>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>>>>>> including CRC.
>>>>>>>
>>>>>>> As application explicitly requested to KEEP CRC, it will know last 4
>>>>>>> bytes are CRC.
>>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>>>>>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>>>>>
>>>>>> I agree with you.
>>>>>>
>>>>>> But the implementation of other PMDs supported KEEP_CRC is like this.
>>>>>> In addition, there are probably many users that are already using it.
>>>>>> If we modify it, it may cause applications incompatible.
>>>>>>
>>>>>> what do you think?
>>>>>>
>>>>> This is documented in the ethdev [1], better to follow the documentation
>>>>> for all PMDs, can you please highlight the relevant driver code, we can
>>>>> discuss it with their maintainers.
>>>>>
>>>>> Alternatively we can document this additionally in the KEEP_CRC feature
>>>>> document if it helps for the applications.
>>>>>
>>>>>
>>>>> [1]
>>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257
>>>>
>>>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>>>
>>>
>>> I think it is clear that pkt_len and data_len should contain crc_len, we
>>> can ask for more comments.
>> This patch doesn't change the logic for hns3 PMD and the implementation of
>> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
>>
> 
> If hns3 behaving against the documented behavior, I don't understand why
> you are pushing for merging this patch, instead of fixing it.
> 
> 
> Other drivers behavior is something else, not directly related to this
> patch, but again if you can provide references we can discuss with their
> maintainers.
> 
> 
>>>
>>>> Do you mean that we add this description in the KEEP_CRC feature document
>>>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>>>
>>>> If so, can you merge this patch first?
>>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>>
>>>
>>> Not for drivers, just a suggestion that if we should update feature
>>> documentation with above information for users. So there is no
>>> dependency to features document update.
>>>
>>>
>> Sorry I'm more confused. What should we do next?
> 
> There is already API documentation about KEEP_CRC, I think that is
> already sufficient for driver developers.
> 
> I am just brainstorming if updating './doc/guides/nics/features.rst' can
> help end user, but it is not an action or blocker for this patch.
> 
> Next step is to update this path.
> 


Hi, Ferruh,

Thanks for your attention.
I have the following suggestions for the email discussion.
Please take the time to see if they make sense.

For pkt_len and data_len, there is no clear document indicating
whether the length should include the CRC.
However, according to the usage of the driver and the APP, it
is obvious that almost all drivers do not include the CRC by default.

The issue you raised about pkt_len/data_len supposedly containing CRC
and users not being able to get CRC has been around for a long
time, at least dating back to DPDK 18.11 when there was no hns3
driver. And there is no clear solution to this problem for now.

This issue is not caused by the current patch and is not related
to the problem to be solved by the current patch.
Therefore, it is recommended that the problem corresponding to the current
patch should be fixed first.

The problem that the pkt_len/data_len should contain the CRC
and the user should access the CRC can be discussed later.

I have the following two options:

1. Modify the corresponding document to specify that pkt_len/data_len
should contain CRC, and modify all related drivers. This requires the
participation and discussion of other driver developers.

2. Users can use rte_pktmbuf_dump() to print packet data.
The length of the packet data to be printed can be specified.
However, the API restricts that the length of the data required is
less than data_len. Therefore, users cannot obtain the CRC value.
However, the result of the rte_pktmbuf_tailroom() API can tell how
many bytes after data_len are accessible. Users can use rte_pktmbuf_tailroom
and keep_crc offload to obtain the CRC value of packets.

> .

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Jie Hai]

Hi, Ferruh

Kindly ping for reply.

Thanks,
Jie Hai
On 2024/3/8 19:36, Jie Hai wrote:
> On 2024/3/1 19:10, Ferruh Yigit wrote:
>> On 3/1/2024 6:55 AM, huangdengdui wrote:
>>>
>>>
>>> On 2024/2/29 17:25, Ferruh Yigit wrote:
>>>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>>>
>>>>>
>>>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>>>> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>>>>>>> Hi, Ferruh,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for your review.
>>>>>>>>>>>
>>>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be 
>>>>>>>>>>>>> truncated and
>>>>>>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>>>>>>       is less than or equal to 60B.
>>>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>>>>>>       is less than or equal to 60B.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> If a device doesn't support the offload by some packets, it 
>>>>>>>>>>>> can be
>>>>>>>>>>>> option to disable offload for that device, instead of 
>>>>>>>>>>>> calculating it in
>>>>>>>>>>>> software and append it.
>>>>>>>>>>>
>>>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific 
>>>>>>>>>>> packet
>>>>>>>>>>> type and small packet(<60B) case.
>>>>>>>>>>> What's more, the small ethernet packet is not common.
>>>>>>>>>>>
>>>>>>>>>>>> Unless you have a specific usecase, or requirement to 
>>>>>>>>>>>> support the
>>>>>>>>>>>> offload.
>>>>>>>>>>>
>>>>>>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>>>>>>> So we cannot drop this offload
>>>>>>>>>>>
>>>>>>>>>>>> <...>
>>>>>>>>>>>>
>>>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>>>>>                 goto pkt_err;
>>>>>>>>>>>>>               rxm->packet_type = hns3_rx_calc_ptype(rxq, 
>>>>>>>>>>>>> l234_info,
>>>>>>>>>>>>> ol_info);
>>>>>>>>>>>>> -
>>>>>>>>>>>>>             if (rxm->packet_type == 
>>>>>>>>>>>>> RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>>>>>>                 rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>>>>>>     +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>>>>>>> practically same as stripping CRC.
>>>>>>>>>>>>
>>>>>>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>>>>>>> accessible
>>>>>>>>>>>> in the payload by the user.
>>>>>>>>>>> Our drivers are behaving exactly as you say.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>>>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>           rxdp->rx.bd_base_info = 0;
>>>>>>>>>
>>>>>>>>>           rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>>>>>>> -        rxm->pkt_len = 
>>>>>>>>> (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>>>>>>> -                rxq->crc_len;
>>>>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>>>>>>
>>>>>>>>> In the previous code above, the 'pkt_len' is set to the length 
>>>>>>>>> obtained
>>>>>>>>> from the BD. the length obtained from the BD already contains 
>>>>>>>>> CRC length.
>>>>>>>>> But as you said above, the DPDK requires that the length of the 
>>>>>>>>> mbuf
>>>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the 
>>>>>>>>> logic, it
>>>>>>>>> just moves the code around.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, 
>>>>>>>> indeed
>>>>>>>> it is other way around and this is our confusion.
>>>>>>>>
>>>>>>>> CRC length shouldn't be in the statistics, I mean in received 
>>>>>>>> bytes stats.
>>>>>>>> Assume that received packet is 128 bytes and we know it has the 
>>>>>>>> CRC,
>>>>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>>>>>>
>>>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>>>>>>> including CRC.
>>>>>>>>
>>>>>>>> As application explicitly requested to KEEP CRC, it will know 
>>>>>>>> last 4
>>>>>>>> bytes are CRC.
>>>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, 
>>>>>>>> so if
>>>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know 
>>>>>>>> if 4
>>>>>>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>>>>>>
>>>>>>> I agree with you.
>>>>>>>
>>>>>>> But the implementation of other PMDs supported KEEP_CRC is like 
>>>>>>> this.
>>>>>>> In addition, there are probably many users that are already using 
>>>>>>> it.
>>>>>>> If we modify it, it may cause applications incompatible.
>>>>>>>
>>>>>>> what do you think?
>>>>>>>
>>>>>> This is documented in the ethdev [1], better to follow the 
>>>>>> documentation
>>>>>> for all PMDs, can you please highlight the relevant driver code, 
>>>>>> we can
>>>>>> discuss it with their maintainers.
>>>>>>
>>>>>> Alternatively we can document this additionally in the KEEP_CRC 
>>>>>> feature
>>>>>> document if it helps for the applications.
>>>>>>
>>>>>>
>>>>>> [1]
>>>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257
>>>>>
>>>>> Currently，this documentation does not describe whether pkt_len and 
>>>>> data_len should contain crc_len.
>>>>>
>>>>
>>>> I think it is clear that pkt_len and data_len should contain 
>>>> crc_len, we
>>>> can ask for more comments.
>>> This patch doesn't change the logic for hns3 PMD and the 
>>> implementation of
>>> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this 
>>> patch first?
>>>
>>
>> If hns3 behaving against the documented behavior, I don't understand why
>> you are pushing for merging this patch, instead of fixing it.
>>
>>
>> Other drivers behavior is something else, not directly related to this
>> patch, but again if you can provide references we can discuss with their
>> maintainers.
>>
>>
>>>>
>>>>> Do you mean that we add this description in the KEEP_CRC feature 
>>>>> document
>>>>> and notify all drivers that support KEEP_CRC to follow this 
>>>>> documentation?
>>>>>
>>>>> If so, can you merge this patch first?
>>>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>>>
>>>>
>>>> Not for drivers, just a suggestion that if we should update feature
>>>> documentation with above information for users. So there is no
>>>> dependency to features document update.
>>>>
>>>>
>>> Sorry I'm more confused. What should we do next?
>>
>> There is already API documentation about KEEP_CRC, I think that is
>> already sufficient for driver developers.
>>
>> I am just brainstorming if updating './doc/guides/nics/features.rst' can
>> help end user, but it is not an action or blocker for this patch.
>>
>> Next step is to update this path.
>>
> 
> 
> Hi, Ferruh,
> 
> Thanks for your attention.
> I have the following suggestions for the email discussion.
> Please take the time to see if they make sense.
> 
> For pkt_len and data_len, there is no clear document indicating
> whether the length should include the CRC.
> However, according to the usage of the driver and the APP, it
> is obvious that almost all drivers do not include the CRC by default.
> 
> The issue you raised about pkt_len/data_len supposedly containing CRC
> and users not being able to get CRC has been around for a long
> time, at least dating back to DPDK 18.11 when there was no hns3
> driver. And there is no clear solution to this problem for now.
> 
> This issue is not caused by the current patch and is not related
> to the problem to be solved by the current patch.
> Therefore, it is recommended that the problem corresponding to the current
> patch should be fixed first.
> 
> The problem that the pkt_len/data_len should contain the CRC
> and the user should access the CRC can be discussed later.
> 
> I have the following two options:
> 
> 1. Modify the corresponding document to specify that pkt_len/data_len
> should contain CRC, and modify all related drivers. This requires the
> participation and discussion of other driver developers.
> 
> 2. Users can use rte_pktmbuf_dump() to print packet data.
> The length of the packet data to be printed can be specified.
> However, the API restricts that the length of the data required is
> less than data_len. Therefore, users cannot obtain the CRC value.
> However, the result of the rte_pktmbuf_tailroom() API can tell how
> many bytes after data_len are accessible. Users can use 
> rte_pktmbuf_tailroom
> and keep_crc offload to obtain the CRC value of packets.
> 
>> .

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Jie Hai]

On 2024/3/1 19:10, Ferruh Yigit wrote:
> On 3/1/2024 6:55 AM, huangdengdui wrote:
>>
>>
>> On 2024/2/29 17:25, Ferruh Yigit wrote:
>>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>>
>>>>
>>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>>> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>>>>>
>>>>>>
>>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>>>>>> Hi, Ferruh,
>>>>>>>>>>
>>>>>>>>>> Thanks for your review.
>>>>>>>>>>
>>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>>>>>
>>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>>>>>       is less than or equal to 60B.
>>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>>>>>       is less than or equal to 60B.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>>>>>>> software and append it.
>>>>>>>>>>
>>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>>>>>>> type and small packet(<60B) case.
>>>>>>>>>> What's more, the small ethernet packet is not common.
>>>>>>>>>>
>>>>>>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>>>>>>> offload.
>>>>>>>>>>
>>>>>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>>>>>> So we cannot drop this offload
>>>>>>>>>>
>>>>>>>>>>> <...>
>>>>>>>>>>>
>>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>>>>                 goto pkt_err;
>>>>>>>>>>>>               rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>>>>>>> ol_info);
>>>>>>>>>>>> -
>>>>>>>>>>>>             if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>>>>>                 rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>>>>>     +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>>>>>> practically same as stripping CRC.
>>>>>>>>>>>
>>>>>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>>>>>> accessible
>>>>>>>>>>> in the payload by the user.
>>>>>>>>>> Our drivers are behaving exactly as you say.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>           rxdp->rx.bd_base_info = 0;
>>>>>>>>
>>>>>>>>           rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>>>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>>>>>> -                rxq->crc_len;
>>>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>>>>>
>>>>>>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>>>>>>> from the BD. the length obtained from the BD already contains CRC length.
>>>>>>>> But as you said above, the DPDK requires that the length of the mbuf
>>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>>>>>>> just moves the code around.
>>>>>>>>
>>>>>>>
>>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>>>>>>> it is other way around and this is our confusion.
>>>>>>>
>>>>>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>>>>>>> Assume that received packet is 128 bytes and we know it has the CRC,
>>>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>>>>>
>>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>>>>>> including CRC.
>>>>>>>
>>>>>>> As application explicitly requested to KEEP CRC, it will know last 4
>>>>>>> bytes are CRC.
>>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>>>>>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>>>>>
>>>>>> I agree with you.
>>>>>>
>>>>>> But the implementation of other PMDs supported KEEP_CRC is like this.
>>>>>> In addition, there are probably many users that are already using it.
>>>>>> If we modify it, it may cause applications incompatible.
>>>>>>
>>>>>> what do you think?
>>>>>>
>>>>> This is documented in the ethdev [1], better to follow the documentation
>>>>> for all PMDs, can you please highlight the relevant driver code, we can
>>>>> discuss it with their maintainers.
>>>>>
>>>>> Alternatively we can document this additionally in the KEEP_CRC feature
>>>>> document if it helps for the applications.
>>>>>
>>>>>
>>>>> [1]
>>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257
>>>>
>>>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>>>
>>>
>>> I think it is clear that pkt_len and data_len should contain crc_len, we
>>> can ask for more comments.
>> This patch doesn't change the logic for hns3 PMD and the implementation of
>> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
>>
> 
> If hns3 behaving against the documented behavior, I don't understand why
> you are pushing for merging this patch, instead of fixing it.
> 

> 
> Other drivers behavior is something else, not directly related to this
> patch, but again if you can provide references we can discuss with their
> maintainers.
> 
Hi, all maintainers,
We need your opinions on whether pkt_len and data_len should contain CRC 
len. The KEEP CRC feature is related. As if it is enabled, most drivers
will substract CRC len from pkt_len and data_len. which means users
cannot read the CRC data through the DPDK framework interface.

Among the drivers that support keeping CRC, only the bnxt, cfpl, idpf,
qede and sfc get the pkt_len and data_len from the descriptor and not
subtract CRC len by drivers. I don't know if the length of these drivers 
includes the CRC len or not, please confirm that, thanks.


Back to the current patch.
Hi, Ferruh.
Obviously, if we need to give users access to the CRC data, we'll have
to modify the defination in ethdev and usage in most drivers.

I don't think this change will be backported. Am I wrong?

But this patch for hns3 bugfix, need to be backported.

That's why we can separate this patch from the confirmation of the
meaning of pkt_len and data_len.
So can this patch merge first?

Thanks,
Jie Hai

> 
>>>
>>>> Do you mean that we add this description in the KEEP_CRC feature document
>>>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>>>
>>>> If so, can you merge this patch first?
>>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>>
>>>
>>> Not for drivers, just a suggestion that if we should update feature
>>> documentation with above information for users. So there is no
>>> dependency to features document update.
>>>
>>>
>> Sorry I'm more confused. What should we do next?
> 
> There is already API documentation about KEEP_CRC, I think that is
> already sufficient for driver developers.
> 
> I am just brainstorming if updating './doc/guides/nics/features.rst' can
> help end user, but it is not an action or blocker for this patch.
> 
> Next step is to update this path.
> 
> .

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Stephen Hemminger]

On Mon, 3 Jun 2024 09:38:19 +0800
Jie Hai <haijie1@huawei.com> wrote:

> On 2024/3/1 19:10, Ferruh Yigit wrote:
> > On 3/1/2024 6:55 AM, huangdengdui wrote:  
> >>
> >>
> >> On 2024/2/29 17:25, Ferruh Yigit wrote:  
> >>> On 2/29/2024 3:58 AM, huangdengdui wrote:  
> >>>>
> >>>>
> >>>> On 2024/2/28 21:07, Ferruh Yigit wrote:  
> >>>>> On 2/28/2024 2:27 AM, huangdengdui wrote:  
> >>>>>>
> >>>>>>
> >>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote:  
> >>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:  
> >>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:  
> >>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:  
> >>>>>>>>>> Hi, Ferruh,
> >>>>>>>>>>
> >>>>>>>>>> Thanks for your review.
> >>>>>>>>>>
> >>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:  
> >>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:  
> >>>>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
> >>>>>>>>>>>>
> >>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
> >>>>>>>>>>>> the CRC is still be stripped in following cases:
> >>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
> >>>>>>>>>>>>       is less than or equal to 60B.
> >>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
> >>>>>>>>>>>>       is less than or equal to 60B.
> >>>>>>>>>>>>  
> >>>>>>>>>>>
> >>>>>>>>>>> If a device doesn't support the offload by some packets, it can be
> >>>>>>>>>>> option to disable offload for that device, instead of calculating it in
> >>>>>>>>>>> software and append it.  
> >>>>>>>>>>
> >>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
> >>>>>>>>>> type and small packet(<60B) case.
> >>>>>>>>>> What's more, the small ethernet packet is not common.
> >>>>>>>>>>  
> >>>>>>>>>>> Unless you have a specific usecase, or requirement to support the
> >>>>>>>>>>> offload.  
> >>>>>>>>>>
> >>>>>>>>>> Yes, some users of hns3 are already using this feature.
> >>>>>>>>>> So we cannot drop this offload
> >>>>>>>>>>  
> >>>>>>>>>>> <...>
> >>>>>>>>>>>  
> >>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
> >>>>>>>>>>>>                 goto pkt_err;
> >>>>>>>>>>>>               rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
> >>>>>>>>>>>> ol_info);
> >>>>>>>>>>>> -
> >>>>>>>>>>>>             if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
> >>>>>>>>>>>>                 rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
> >>>>>>>>>>>>     +        if (unlikely(rxq->crc_len > 0)) {
> >>>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
> >>>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
> >>>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
> >>>>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
> >>>>>>>>>>>>  
> >>>>>>>>>>>
> >>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
> >>>>>>>>>>> practically same as stripping CRC.
> >>>>>>>>>>>
> >>>>>>>>>>> We don't count CRC length in the statistics, but it should be
> >>>>>>>>>>> accessible
> >>>>>>>>>>> in the payload by the user.  
> >>>>>>>>>> Our drivers are behaving exactly as you say.
> >>>>>>>>>>  
> >>>>>>>>>
> >>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
> >>>>>>>>> 'rxq->crc_len', can you please explain what above lines does?
> >>>>>>>>>
> >>>>>>>>>  
> >>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
> >>>>>>>>           rxdp->rx.bd_base_info = 0;
> >>>>>>>>
> >>>>>>>>           rxm->data_off = RTE_PKTMBUF_HEADROOM;
> >>>>>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
> >>>>>>>> -                rxq->crc_len;
> >>>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
> >>>>>>>>
> >>>>>>>> In the previous code above, the 'pkt_len' is set to the length obtained
> >>>>>>>> from the BD. the length obtained from the BD already contains CRC length.
> >>>>>>>> But as you said above, the DPDK requires that the length of the mbuf
> >>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
> >>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
> >>>>>>>> just moves the code around.
> >>>>>>>>  
> >>>>>>>
> >>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
> >>>>>>> it is other way around and this is our confusion.
> >>>>>>>
> >>>>>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
> >>>>>>> Assume that received packet is 128 bytes and we know it has the CRC,
> >>>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
> >>>>>>>
> >>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
> >>>>>>> including CRC.
> >>>>>>>
> >>>>>>> As application explicitly requested to KEEP CRC, it will know last 4
> >>>>>>> bytes are CRC.
> >>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
> >>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
> >>>>>>> bytes after 'mbuf->data_len' is valid CRC or not.
> >>>>>>>  
> >>>>>> I agree with you.
> >>>>>>
> >>>>>> But the implementation of other PMDs supported KEEP_CRC is like this.
> >>>>>> In addition, there are probably many users that are already using it.
> >>>>>> If we modify it, it may cause applications incompatible.
> >>>>>>
> >>>>>> what do you think?
> >>>>>>  
> >>>>> This is documented in the ethdev [1], better to follow the documentation
> >>>>> for all PMDs, can you please highlight the relevant driver code, we can
> >>>>> discuss it with their maintainers.
> >>>>>
> >>>>> Alternatively we can document this additionally in the KEEP_CRC feature
> >>>>> document if it helps for the applications.
> >>>>>
> >>>>>
> >>>>> [1]
> >>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257  
> >>>>
> >>>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
> >>>>  
> >>>
> >>> I think it is clear that pkt_len and data_len should contain crc_len, we
> >>> can ask for more comments.  
> >> This patch doesn't change the logic for hns3 PMD and the implementation of
> >> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
> >>  
> > 
> > If hns3 behaving against the documented behavior, I don't understand why
> > you are pushing for merging this patch, instead of fixing it.
> >   
> 
> > 
> > Other drivers behavior is something else, not directly related to this
> > patch, but again if you can provide references we can discuss with their
> > maintainers.
> >   
> Hi, all maintainers,
> We need your opinions on whether pkt_len and data_len should contain CRC 
> len. The KEEP CRC feature is related. As if it is enabled, most drivers
> will substract CRC len from pkt_len and data_len. which means users
> cannot read the CRC data through the DPDK framework interface.
> 
> Among the drivers that support keeping CRC, only the bnxt, cfpl, idpf,
> qede and sfc get the pkt_len and data_len from the descriptor and not
> subtract CRC len by drivers. I don't know if the length of these drivers 
> includes the CRC len or not, please confirm that, thanks.
> 
> 
> Back to the current patch.
> Hi, Ferruh.
> Obviously, if we need to give users access to the CRC data, we'll have
> to modify the defination in ethdev and usage in most drivers.
> 
> I don't think this change will be backported. Am I wrong?
> 
> But this patch for hns3 bugfix, need to be backported.
> 
> That's why we can separate this patch from the confirmation of the
> meaning of pkt_len and data_len.
> So can this patch merge first?
> 
> Thanks,
> Jie Hai
> 
> >   
> >>>  
> >>>> Do you mean that we add this description in the KEEP_CRC feature document
> >>>> and notify all drivers that support KEEP_CRC to follow this documentation?
> >>>>
> >>>> If so, can you merge this patch first?
> >>>> Then we send a RFC to disscuss it with all PMDs maintainer.
> >>>>  
> >>>
> >>> Not for drivers, just a suggestion that if we should update feature
> >>> documentation with above information for users. So there is no
> >>> dependency to features document update.
> >>>
> >>>  
> >> Sorry I'm more confused. What should we do next?  
> > 
> > There is already API documentation about KEEP_CRC, I think that is
> > already sufficient for driver developers.
> > 
> > I am just brainstorming if updating './doc/guides/nics/features.rst' can
> > help end user, but it is not an action or blocker for this patch.
> > 
> > Next step is to update this path.

IMHO the only sane thing is:
  -if keep crc is enabled then pkt_len and data_len include the extra bytes for the CRC.
  -if keep crc is disabled, then pkt_len and data_len match the length of the packet without the CRC.


Other than driver testing, never saw much point to using keep crc.

RE: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Morten Brørup]

> IMHO the only sane thing is:
>   -if keep crc is enabled then pkt_len and data_len include the extra
> bytes for the CRC.

Agree.
Consider a segmented packet where only the CRC is in the last segment.
The length of that last segment would be zero if the CRC length was not included in pkt_len and data_len.
Or, if the last two bytes of the CRC is in the last segment; the length of that last segment would be -2 if not including the CRC length.

>   -if keep crc is disabled, then pkt_len and data_len match the length
> of the packet without the CRC.

Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Andrew Rybchenko]

On 6/3/24 05:33, Stephen Hemminger wrote:
> On Mon, 3 Jun 2024 09:38:19 +0800
> Jie Hai <haijie1@huawei.com> wrote:
> 
>> On 2024/3/1 19:10, Ferruh Yigit wrote:
>>> On 3/1/2024 6:55 AM, huangdengdui wrote:
>>>>
>>>>
>>>> On 2024/2/29 17:25, Ferruh Yigit wrote:
>>>>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>>>>
>>>>>>
>>>>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>>>>> On 2/28/2024 2:27 AM, huangdengdui wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote:
>>>>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>>>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>>>>>>>>> Hi, Ferruh,
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for your review.
>>>>>>>>>>>>
>>>>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>>>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>>>>>>>>> the CRC is still be stripped in following cases:
>>>>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>>>>>>>>        is less than or equal to 60B.
>>>>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>>>>>>>>        is less than or equal to 60B.
>>>>>>>>>>>>>>   
>>>>>>>>>>>>>
>>>>>>>>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>>>>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>>>>>>>>> software and append it.
>>>>>>>>>>>>
>>>>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>>>>>>>>> type and small packet(<60B) case.
>>>>>>>>>>>> What's more, the small ethernet packet is not common.
>>>>>>>>>>>>   
>>>>>>>>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>>>>>>>>> offload.
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, some users of hns3 are already using this feature.
>>>>>>>>>>>> So we cannot drop this offload
>>>>>>>>>>>>   
>>>>>>>>>>>>> <...>
>>>>>>>>>>>>>   
>>>>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>>>>>>                  goto pkt_err;
>>>>>>>>>>>>>>                rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>>>>>>>>> ol_info);
>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>              if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>>>>>>>>                  rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>>>>>>>>      +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>>>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>>>>>>>>   
>>>>>>>>>>>>>
>>>>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>>>>>>>>> practically same as stripping CRC.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>>>>>>>>> accessible
>>>>>>>>>>>>> in the payload by the user.
>>>>>>>>>>>> Our drivers are behaving exactly as you say.
>>>>>>>>>>>>   
>>>>>>>>>>>
>>>>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>>>>>>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>>>>>>>>
>>>>>>>>>>>   
>>>>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>>>>            rxdp->rx.bd_base_info = 0;
>>>>>>>>>>
>>>>>>>>>>            rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>>>>>>>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>>>>>>>>> -                rxq->crc_len;
>>>>>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>>>>>>>>
>>>>>>>>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>>>>>>>>> from the BD. the length obtained from the BD already contains CRC length.
>>>>>>>>>> But as you said above, the DPDK requires that the length of the mbuf
>>>>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>>>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>>>>>>>>> just moves the code around.
>>>>>>>>>>   
>>>>>>>>>
>>>>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>>>>>>>>> it is other way around and this is our confusion.
>>>>>>>>>
>>>>>>>>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>>>>>>>>> Assume that received packet is 128 bytes and we know it has the CRC,
>>>>>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>>>>>>>>
>>>>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>>>>>>>>> including CRC.
>>>>>>>>>
>>>>>>>>> As application explicitly requested to KEEP CRC, it will know last 4
>>>>>>>>> bytes are CRC.
>>>>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>>>>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>>>>>>>>> bytes after 'mbuf->data_len' is valid CRC or not.
>>>>>>>>>   
>>>>>>>> I agree with you.
>>>>>>>>
>>>>>>>> But the implementation of other PMDs supported KEEP_CRC is like this.
>>>>>>>> In addition, there are probably many users that are already using it.
>>>>>>>> If we modify it, it may cause applications incompatible.
>>>>>>>>
>>>>>>>> what do you think?
>>>>>>>>   
>>>>>>> This is documented in the ethdev [1], better to follow the documentation
>>>>>>> for all PMDs, can you please highlight the relevant driver code, we can
>>>>>>> discuss it with their maintainers.
>>>>>>>
>>>>>>> Alternatively we can document this additionally in the KEEP_CRC feature
>>>>>>> document if it helps for the applications.
>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257
>>>>>>
>>>>>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>>>>>   
>>>>>
>>>>> I think it is clear that pkt_len and data_len should contain crc_len, we
>>>>> can ask for more comments.
>>>> This patch doesn't change the logic for hns3 PMD and the implementation of
>>>> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
>>>>   
>>>
>>> If hns3 behaving against the documented behavior, I don't understand why
>>> you are pushing for merging this patch, instead of fixing it.
>>>    
>>
>>>
>>> Other drivers behavior is something else, not directly related to this
>>> patch, but again if you can provide references we can discuss with their
>>> maintainers.
>>>    
>> Hi, all maintainers,
>> We need your opinions on whether pkt_len and data_len should contain CRC
>> len. The KEEP CRC feature is related. As if it is enabled, most drivers
>> will substract CRC len from pkt_len and data_len. which means users
>> cannot read the CRC data through the DPDK framework interface.
>>
>> Among the drivers that support keeping CRC, only the bnxt, cfpl, idpf,
>> qede and sfc get the pkt_len and data_len from the descriptor and not
>> subtract CRC len by drivers. I don't know if the length of these drivers
>> includes the CRC len or not, please confirm that, thanks.
>>
>>
>> Back to the current patch.
>> Hi, Ferruh.
>> Obviously, if we need to give users access to the CRC data, we'll have
>> to modify the defination in ethdev and usage in most drivers.
>>
>> I don't think this change will be backported. Am I wrong?
>>
>> But this patch for hns3 bugfix, need to be backported.
>>
>> That's why we can separate this patch from the confirmation of the
>> meaning of pkt_len and data_len.
>> So can this patch merge first?
>>
>> Thanks,
>> Jie Hai
>>
>>>    
>>>>>   
>>>>>> Do you mean that we add this description in the KEEP_CRC feature document
>>>>>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>>>>>
>>>>>> If so, can you merge this patch first?
>>>>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>>>>   
>>>>>
>>>>> Not for drivers, just a suggestion that if we should update feature
>>>>> documentation with above information for users. So there is no
>>>>> dependency to features document update.
>>>>>
>>>>>   
>>>> Sorry I'm more confused. What should we do next?
>>>
>>> There is already API documentation about KEEP_CRC, I think that is
>>> already sufficient for driver developers.
>>>
>>> I am just brainstorming if updating './doc/guides/nics/features.rst' can
>>> help end user, but it is not an action or blocker for this patch.
>>>
>>> Next step is to update this path.
> 
> IMHO the only sane thing is:
>    -if keep crc is enabled then pkt_len and data_len include the extra bytes for the CRC.
>    -if keep crc is disabled, then pkt_len and data_len match the length of the packet without the CRC.
> 
> 
> Other than driver testing, never saw much point to using keep crc.

+1

[PATCH v2 0/3] bugfix about KEEP CRC offload

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is still be stripped in rare scenarios.
Some users of hns3 are already using this feature. So driver has to recaculate packet CRC.

In addition, in the mbuf, the data that exceeds the length specified by pkt_len is invalid.
Therefore, if the packet contains CRC data, pkt_len should contain the CRC data length.
However, almost of drivers supported KEEP CRC feature didn't add the CRC data length to pkt_len.
This patch adds description for KEEP CRC offload.

Dengdui Huang (3):
  ethdev: add description for KEEP CRC offload
  net/hns3: fix packet length do not contain CRC data length
  net/hns3: fix Rx packet without CRC data

 drivers/net/hns3/hns3_ethdev.c        |   5 +
 drivers/net/hns3/hns3_ethdev.h        |  23 +++++
 drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
 drivers/net/hns3/hns3_rxtx.h          |   3 +
 drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
 drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
 drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
 lib/ethdev/rte_ethdev.h               |   6 ++
 8 files changed, 124 insertions(+), 72 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] ethdev: add description for KEEP CRC offload

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

The data execeed the pkt_len in mbuf is inavailable for user.
When KEEP CRC offload is enabled, CRC field length should be
included pkt_len in mbuf. However, almost of drivers supported
KEEP CRC feature didn't add the CRC data length to pkt_len.
So it is very necessary to add a coments for this.

Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
---
 lib/ethdev/rte_ethdev.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 548fada1c7..3d44673161 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1550,6 +1550,12 @@ struct rte_eth_conf {
  */
 #define RTE_ETH_RX_OFFLOAD_TIMESTAMP        RTE_BIT64(14)
 #define RTE_ETH_RX_OFFLOAD_SECURITY         RTE_BIT64(15)
+/*
+ * Keep CRC data in packet.
+ *
+ * Note: If this offload is enabled, the pkt_len in mbuf should contain
+ * the CRC data length.
+ */
 #define RTE_ETH_RX_OFFLOAD_KEEP_CRC         RTE_BIT64(16)
 #define RTE_ETH_RX_OFFLOAD_SCTP_CKSUM       RTE_BIT64(17)
 #define RTE_ETH_RX_OFFLOAD_OUTER_UDP_CKSUM  RTE_BIT64(18)
-- 
2.33.0

RE: [PATCH v2 1/3] ethdev: add description for KEEP CRC offload

[Morten Brørup]

> From: Jie Hai [mailto:haijie1@huawei.com]
> Sent: Thursday, 18 July 2024 13.48
> 
> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> The data execeed the pkt_len in mbuf is inavailable for user.
> When KEEP CRC offload is enabled, CRC field length should be
> included pkt_len in mbuf. However, almost of drivers supported
> KEEP CRC feature didn't add the CRC data length to pkt_len.
> So it is very necessary to add a coments for this.
> 
> Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 548fada1c7..3d44673161 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1550,6 +1550,12 @@ struct rte_eth_conf {
>   */
>  #define RTE_ETH_RX_OFFLOAD_TIMESTAMP        RTE_BIT64(14)
>  #define RTE_ETH_RX_OFFLOAD_SECURITY         RTE_BIT64(15)
> +/*

Suggest making this a Doxygen comment: /**

> + * Keep CRC data in packet.
> + *
> + * Note: If this offload is enabled, the pkt_len in mbuf should contain

"should contain" -> "must include"

> + * the CRC data length.
> + */
>  #define RTE_ETH_RX_OFFLOAD_KEEP_CRC         RTE_BIT64(16)
>  #define RTE_ETH_RX_OFFLOAD_SCTP_CKSUM       RTE_BIT64(17)
>  #define RTE_ETH_RX_OFFLOAD_OUTER_UDP_CKSUM  RTE_BIT64(18)
> --
> 2.33.0

With above modifications,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v2 2/3] net/hns3: fix packet length do not contain CRC data length

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

In the HNS3 driver, pkt_len in mbuf do not contain the CRC length.
This patch fix it.

Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c          | 53 +++------------------------
 drivers/net/hns3/hns3_rxtx_vec_neon.h | 19 ----------
 drivers/net/hns3/hns3_rxtx_vec_sve.c  |  3 +-
 3 files changed, 7 insertions(+), 68 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 5941b966e0..39ba9080ea 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -1951,7 +1951,6 @@ hns3_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t nb_desc,
 	memset(&rxq->err_stats, 0, sizeof(struct hns3_rx_bd_errors_stats));
 	memset(&rxq->dfx_stats, 0, sizeof(struct hns3_rx_dfx_stats));
 
-	/* CRC len set here is used for amending packet length */
 	if (dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC)
 		rxq->crc_len = RTE_ETHER_CRC_LEN;
 	else
@@ -2383,23 +2382,6 @@ hns3_rxd_to_vlan_tci(struct hns3_rx_queue *rxq, struct rte_mbuf *mb,
 	}
 }
 
-static inline void
-recalculate_data_len(struct rte_mbuf *first_seg, struct rte_mbuf *last_seg,
-		    struct rte_mbuf *rxm, struct hns3_rx_queue *rxq,
-		    uint16_t data_len)
-{
-	uint8_t crc_len = rxq->crc_len;
-
-	if (data_len <= crc_len) {
-		rte_pktmbuf_free_seg(rxm);
-		first_seg->nb_segs--;
-		last_seg->data_len = (uint16_t)(last_seg->data_len -
-			(crc_len - data_len));
-		last_seg->next = NULL;
-	} else
-		rxm->data_len = (uint16_t)(data_len - crc_len);
-}
-
 static inline struct rte_mbuf *
 hns3_rx_alloc_buffer(struct hns3_rx_queue *rxq)
 {
@@ -2503,8 +2485,7 @@ hns3_recv_pkts_simple(void *rx_queue,
 		rxdp->rx.bd_base_info = 0;
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
-		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
-				rxq->crc_len;
+		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len));
 		rxm->data_len = rxm->pkt_len;
 		rxm->port = rxq->port_id;
 		rxm->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
@@ -2531,8 +2512,8 @@ hns3_recv_pkts_simple(void *rx_queue,
 
 		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
 
-		/* Increment bytes counter  */
-		rxq->basic_stats.bytes += rxm->pkt_len;
+		/* All byte-related statistics do not include Ethernet FCS */
+		rxq->basic_stats.bytes += rxm->pkt_len - rxq->crc_len;
 
 		rx_pkts[nb_rx++] = rxm;
 		continue;
@@ -2695,10 +2676,10 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
 		rxm->data_len = rte_le_to_cpu_16(rxd.rx.size);
+		rxm->next = NULL;
 
 		if (!(bd_base_info & BIT(HNS3_RXD_FE_B))) {
 			last_seg = rxm;
-			rxm->next = NULL;
 			continue;
 		}
 
@@ -2706,30 +2687,8 @@ hns3_recv_scattered_pkts(void *rx_queue,
 		if (unlikely(bd_base_info & BIT(HNS3_RXD_TS_VLD_B)))
 			hns3_rx_ptp_timestamp_handle(rxq, first_seg, timestamp);
 
-		/*
-		 * The last buffer of the received packet. packet len from
-		 * buffer description may contains CRC len, packet len should
-		 * subtract it, same as data len.
-		 */
 		first_seg->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
 
-		/*
-		 * This is the last buffer of the received packet. If the CRC
-		 * is not stripped by the hardware:
-		 *  - Subtract the CRC length from the total packet length.
-		 *  - If the last buffer only contains the whole CRC or a part
-		 *  of it, free the mbuf associated to the last buffer. If part
-		 *  of the CRC is also contained in the previous mbuf, subtract
-		 *  the length of that CRC part from the data length of the
-		 *  previous mbuf.
-		 */
-		rxm->next = NULL;
-		if (unlikely(rxq->crc_len > 0)) {
-			first_seg->pkt_len -= rxq->crc_len;
-			recalculate_data_len(first_seg, last_seg, rxm, rxq,
-				rxm->data_len);
-		}
-
 		first_seg->port = rxq->port_id;
 		first_seg->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
 		first_seg->ol_flags |= RTE_MBUF_F_RX_RSS_HASH;
@@ -2761,8 +2720,8 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
 
-		/* Increment bytes counter */
-		rxq->basic_stats.bytes += first_seg->pkt_len;
+		/* All byte-related statistics do not include Ethernet FCS */
+		rxq->basic_stats.bytes += first_seg->pkt_len - rxq->crc_len;
 
 		rx_pkts[nb_rx++] = first_seg;
 		first_seg = NULL;
diff --git a/drivers/net/hns3/hns3_rxtx_vec_neon.h b/drivers/net/hns3/hns3_rxtx_vec_neon.h
index 0dc6b9f0a2..60ec501a2a 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_neon.h
+++ b/drivers/net/hns3/hns3_rxtx_vec_neon.h
@@ -148,14 +148,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		8, 9, 10, 11,	         /* rx.rss_hash to rte_mbuf.hash.rss */
 	};
 
-	uint16x8_t crc_adjust = {
-		0, 0,         /* ignore pkt_type field */
-		rxq->crc_len, /* sub crc on pkt_len */
-		0,            /* ignore high-16bits of pkt_len */
-		rxq->crc_len, /* sub crc on data_len */
-		0, 0, 0,      /* ignore non-length fields */
-	};
-
 	/* compile-time verifies the shuffle mask */
 	RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, pkt_len) !=
 			 offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4);
@@ -171,7 +163,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		uint8x16_t pkt_mb1, pkt_mb2, pkt_mb3, pkt_mb4;
 		uint64x2_t mbp1, mbp2;
 		uint16x4_t bd_vld = {0};
-		uint16x8_t tmp;
 		uint64_t stat;
 
 		/* calc how many bd valid */
@@ -225,16 +216,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		pkt_mb3 = vqtbl2q_u8(pkt_mbuf3, shuf_desc_fields_msk);
 		pkt_mb4 = vqtbl2q_u8(pkt_mbuf4, shuf_desc_fields_msk);
 
-		/* 4 packets remove crc */
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb1), crc_adjust);
-		pkt_mb1 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb2), crc_adjust);
-		pkt_mb2 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb3), crc_adjust);
-		pkt_mb3 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb4), crc_adjust);
-		pkt_mb4 = vreinterpretq_u8_u16(tmp);
-
 		/* save packet info to rx_pkts mbuf */
 		vst1q_u8((void *)&sw_ring[pos + 0].mbuf->rx_descriptor_fields1,
 			 pkt_mb1);
diff --git a/drivers/net/hns3/hns3_rxtx_vec_sve.c b/drivers/net/hns3/hns3_rxtx_vec_sve.c
index 8aa4448558..67c87f570e 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_sve.c
+++ b/drivers/net/hns3/hns3_rxtx_vec_sve.c
@@ -36,8 +36,7 @@ hns3_desc_parse_field_sve(struct hns3_rx_queue *rxq,
 		/* init rte_mbuf.rearm_data last 64-bit */
 		rx_pkts[i]->ol_flags = RTE_MBUF_F_RX_RSS_HASH;
 		rx_pkts[i]->hash.rss = rxdp[i].rx.rss_hash;
-		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len) -
-					rxq->crc_len;
+		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len);
 		rx_pkts[i]->data_len = rx_pkts[i]->pkt_len;
 
 		l234_info = rxdp[i].rx.l234_info;
-- 
2.33.0

[PATCH v2 3/3] net/hns3: fix Rx packet without CRC data

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

When KEEP_CRC offload is enabled, the CRC data is still stripped
in following cases:
1. For HIP08 network engine, the packet type is TCP and the length
   is less than or equal to 60B.
2. For HIP09 network engine, the packet type is IP and the length
   is less than or equal to 60B.

So driver has to recaculate packet CRC for this rare scenarios.

In addition, to avoid impacting performance, KEEP_CRC is not
supported when NEON or SVE algorithm is used.

Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
---
 drivers/net/hns3/hns3_ethdev.c   |  5 ++
 drivers/net/hns3/hns3_ethdev.h   | 23 +++++++++
 drivers/net/hns3/hns3_rxtx.c     | 81 ++++++++++++++++++++++++++++++--
 drivers/net/hns3/hns3_rxtx.h     |  3 ++
 drivers/net/hns3/hns3_rxtx_vec.c |  3 +-
 5 files changed, 111 insertions(+), 4 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index ec1251cb7e..5fdabba547 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -2739,6 +2739,7 @@ hns3_get_capability(struct hns3_hw *hw)
 		hw->udp_cksum_mode = HNS3_SPECIAL_PORT_SW_CKSUM_MODE;
 		pf->support_multi_tc_pause = false;
 		hw->rx_dma_addr_align = HNS3_RX_DMA_ADDR_ALIGN_64;
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_TCP;
 		return 0;
 	}
 
@@ -2760,6 +2761,10 @@ hns3_get_capability(struct hns3_hw *hw)
 	hw->udp_cksum_mode = HNS3_SPECIAL_PORT_HW_CKSUM_MODE;
 	pf->support_multi_tc_pause = true;
 	hw->rx_dma_addr_align = HNS3_RX_DMA_ADDR_ALIGN_128;
+	if (hw->revision == PCI_REVISION_ID_HIP09_A)
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_IP;
+	else
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_NONE;
 
 	return 0;
 }
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index 799b61038a..801411d690 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -56,6 +56,10 @@
 #define HNS3_SPECIAL_PORT_SW_CKSUM_MODE         0
 #define HNS3_SPECIAL_PORT_HW_CKSUM_MODE         1
 
+#define HNS3_STRIP_CRC_PTYPE_NONE         0
+#define HNS3_STRIP_CRC_PTYPE_TCP          1
+#define HNS3_STRIP_CRC_PTYPE_IP           2
+
 #define HNS3_UC_MACADDR_NUM		128
 #define HNS3_VF_UC_MACADDR_NUM		48
 #define HNS3_MC_MACADDR_NUM		128
@@ -657,6 +661,25 @@ struct hns3_hw {
 	 */
 	uint8_t udp_cksum_mode;
 
+	/*
+	 * When KEEP_CRC offload is enabled, the CRC data of some type packets
+	 * whose length is less than or equal to HNS3_KEEP_CRC_OK_MIN_PKT_LEN
+	 * is still be stripped on some network engine. So here has to use this
+	 * field to distinguish they difference between different network engines.
+	 * value range:
+	 *  - HNS3_STRIP_CRC_PTYPE_TCP
+	 *     This value for HIP08 network engine.
+	 *     Indicates that only the IP-TCP packet type is stripped.
+	 *
+	 *  - HNS3_STRIP_CRC_PTYPE_IP
+	 *     This value for HIP09 network engine.
+	 *     Indicates that all IP packet types are stripped.
+	 *
+	 *  - HNS3_STRIP_CRC_PTYPE_NONE
+	 *     Indicates that all packet types are not stripped.
+	 */
+	uint8_t strip_crc_ptype;
+
 	struct hns3_port_base_vlan_config port_base_vlan_cfg;
 
 	pthread_mutex_t flows_lock; /* rte_flow ops lock */
diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 39ba9080ea..e936c0799f 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -11,6 +11,7 @@
 #include <rte_io.h>
 #include <rte_net.h>
 #include <rte_malloc.h>
+#include <rte_net_crc.h>
 #if defined(RTE_ARCH_ARM64)
 #include <rte_cpuflags.h>
 #include <rte_vect.h>
@@ -1766,8 +1767,9 @@ hns3_rx_buf_len_calc(struct rte_mempool *mp, uint16_t *rx_buf_len)
 }
 
 static int
-hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
-				uint16_t nb_desc)
+hns3_rxq_conf_runtime_check(struct hns3_hw *hw,
+			    const struct rte_eth_rxconf *conf,
+			    uint16_t buf_size, uint16_t nb_desc)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
 	eth_rx_burst_t pkt_burst = dev->rx_pkt_burst;
@@ -1800,6 +1802,14 @@ hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
 			return -EINVAL;
 		}
 	}
+
+	if ((conf->offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC) &&
+	    pkt_burst != hns3_recv_pkts_simple &&
+	    pkt_burst != hns3_recv_scattered_pkts) {
+		hns3_err(hw, "KEEP_CRC offload is not supported in the current rx function.");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -1836,7 +1846,7 @@ hns3_rx_queue_conf_check(struct hns3_hw *hw, const struct rte_eth_rxconf *conf,
 	}
 
 	if (hw->data->dev_started) {
-		ret = hns3_rxq_conf_runtime_check(hw, *buf_size, nb_desc);
+		ret = hns3_rxq_conf_runtime_check(hw, conf, *buf_size, nb_desc);
 		if (ret) {
 			hns3_err(hw, "Rx queue runtime setup fail.");
 			return ret;
@@ -1956,6 +1966,8 @@ hns3_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t nb_desc,
 	else
 		rxq->crc_len = 0;
 
+	rxq->keep_crc_fail_ptype = hw->strip_crc_ptype;
+
 	rxq->bulk_mbuf_num = 0;
 
 	rte_spinlock_lock(&hw->lock);
@@ -2415,6 +2427,50 @@ hns3_rx_ptp_timestamp_handle(struct hns3_rx_queue *rxq, struct rte_mbuf *mbuf,
 	pf->rx_timestamp = timestamp;
 }
 
+static inline bool
+hns3_need_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+	uint32_t ptype = m->packet_type;
+
+	if (rxq->keep_crc_fail_ptype == HNS3_STRIP_CRC_PTYPE_NONE)
+		return false;
+
+	if (m->pkt_len > HNS3_KEEP_CRC_OK_MIN_PKT_LEN)
+		return false;
+
+	if (!(RTE_ETH_IS_IPV4_HDR(ptype) || RTE_ETH_IS_IPV6_HDR(ptype)))
+		return false;
+
+	if (rxq->keep_crc_fail_ptype == HNS3_STRIP_CRC_PTYPE_TCP)
+		return (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP;
+
+	return true;
+}
+
+static inline void
+hns3_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+	char *append_data;
+	uint32_t crc;
+
+	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
+			       m->data_len, RTE_NET_CRC32_ETH);
+
+	/*
+	 * The hns3 driver requires that mbuf size must be at least 512B.
+	 * When CRC is stripped by hardware, the pkt_len must be less than
+	 * or equal to 60B. Therefore, the space of the mbuf is enough
+	 * to insert the CRC.
+	 *
+	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
+	 * do not contain the CRC length. Therefore, after CRC data is appended
+	 * by PMD again, both pkt_len and data_len add the CRC length.
+	 */
+	append_data = rte_pktmbuf_append(m, rxq->crc_len);
+	/* The CRC data is binary data and does not care about the byte order. */
+	rte_memcpy(append_data, (void *)&crc, rxq->crc_len);
+}
+
 uint16_t
 hns3_recv_pkts_simple(void *rx_queue,
 		      struct rte_mbuf **rx_pkts,
@@ -2510,6 +2566,10 @@ hns3_recv_pkts_simple(void *rx_queue,
 		if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
 
+		if (unlikely(rxq->crc_len > 0) &&
+		    hns3_need_recalculate_crc(rxq, rxm))
+			hns3_recalculate_crc(rxq, rxm);
+
 		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
 
 		/* All byte-related statistics do not include Ethernet FCS */
@@ -2718,6 +2778,21 @@ hns3_recv_scattered_pkts(void *rx_queue,
 		if (first_seg->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
 
+		/*
+		 * If KEEP_CRC offload is enabled, the network engine reserves
+		 * the CRC data in the packet. But the CRC is still stripped
+		 * for a kind of packets in hns3 NIC:
+		 * 1. All IP-TCP packet whose the length is less than and equal
+		 *    to 60 Byte (no CRC) on HIP08 network engine.
+		 * 2. All IP packet whose the length is less than and equal to
+		 *    60 Byte (no CRC) on HIP09 network engine.
+		 * In this case, the PMD calculates the CRC and appends it to
+		 * mbuf.
+		 */
+		if (unlikely(rxq->crc_len > 0) &&
+		    hns3_need_recalculate_crc(rxq, first_seg))
+			hns3_recalculate_crc(rxq, first_seg);
+
 		hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
 
 		/* All byte-related statistics do not include Ethernet FCS */
diff --git a/drivers/net/hns3/hns3_rxtx.h b/drivers/net/hns3/hns3_rxtx.h
index e975cd151a..0eb9796fe0 100644
--- a/drivers/net/hns3/hns3_rxtx.h
+++ b/drivers/net/hns3/hns3_rxtx.h
@@ -178,6 +178,8 @@
 		(HNS3_TXD_VLD_CMD | HNS3_TXD_FE_CMD | HNS3_TXD_DEFAULT_BDTYPE)
 #define HNS3_TXD_SEND_SIZE_SHIFT	16
 
+#define HNS3_KEEP_CRC_OK_MIN_PKT_LEN	60
+
 enum hns3_pkt_l2t_type {
 	HNS3_L2_TYPE_UNICAST,
 	HNS3_L2_TYPE_MULTICAST,
@@ -341,6 +343,7 @@ struct hns3_rx_queue {
 	 */
 	uint8_t pvid_sw_discard_en:1;
 	uint8_t ptype_en:1;          /* indicate if the ptype field enabled */
+	uint8_t keep_crc_fail_ptype:2;
 
 	uint64_t mbuf_initializer; /* value to init mbufs used with vector rx */
 	/* offset_table: used for vector, to solve execute re-order problem */
diff --git a/drivers/net/hns3/hns3_rxtx_vec.c b/drivers/net/hns3/hns3_rxtx_vec.c
index 9708ec614e..bf37ce51b1 100644
--- a/drivers/net/hns3/hns3_rxtx_vec.c
+++ b/drivers/net/hns3/hns3_rxtx_vec.c
@@ -185,7 +185,8 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO |
 				 RTE_ETH_RX_OFFLOAD_VLAN |
-				 RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+				 RTE_ETH_RX_OFFLOAD_TIMESTAMP |
+				 RTE_ETH_RX_OFFLOAD_KEEP_CRC;
 
 	if (dev->data->scattered_rx)
 		return -ENOTSUP;
-- 
2.33.0

[RFC] net/hns3: clarify handling of crc reinsert

[Stephen Hemminger]

Use a static assert rather than a comment to express why
there will be space in the buffer.  Use memcpy() rather
than rte_memcpy().

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/hns3/hns3_rxtx.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 861511547d..9272584d24 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -2449,6 +2449,15 @@ hns3_need_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
 	return true;
 }
 
+/*
+ * The hns3 driver requires that mbuf size must be at least 512B.
+ * When CRC is stripped by hardware, the pkt_len must be less than
+ * or equal to 60B. Therefore, the space of the mbuf is enough
+ * to insert the CRC.
+ */
+static_assert(HNS3_KEEP_CRC_OK_MIN_PKT_LEN < HNS3_MIN_BD_BUF_SIZE,
+	      "buffer size too small to insert CRC");
+
 static inline void
 hns3_recalculate_crc(struct rte_mbuf *m)
 {
@@ -2459,18 +2468,13 @@ hns3_recalculate_crc(struct rte_mbuf *m)
 			       m->data_len, RTE_NET_CRC32_ETH);
 
 	/*
-	 * The hns3 driver requires that mbuf size must be at least 512B.
-	 * When CRC is stripped by hardware, the pkt_len must be less than
-	 * or equal to 60B. Therefore, the space of the mbuf is enough
-	 * to insert the CRC.
-	 *
-	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
-	 * do not contain the CRC length. Therefore, after CRC data is appended
-	 * by PMD again, both pkt_len and data_len add the CRC length.
+	 * After CRC is stripped by hardware, pkt_len and data_len do not contain the CRC length.
+	 * Therefore, after CRC data is appended by PMD again.
 	 */
 	append_data = rte_pktmbuf_append(m, RTE_NET_CRC32_ETH);
-	/* The CRC data is binary data and does not care about the byte order. */
-	rte_memcpy(append_data, (void *)&crc, RTE_NET_CRC32_ETH);
+
+	/* CRC data is binary data and does not care about the byte order. */
+	memcpy(append_data, &crc, RTE_NET_CRC32_ETH);
 }
 
 uint16_t
-- 
2.45.2

Re: [PATCH v2 0/3] bugfix about KEEP CRC offload

[lihuisong (C)]

For series.
Acked-by: Huisong Li <lihuisong@huawei.com>


在 2024/7/18 19:48, Jie Hai 写道:
> From: Dengdui Huang <huangdengdui@huawei.com>
>
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is still be stripped in rare scenarios.
> Some users of hns3 are already using this feature. So driver has to recaculate packet CRC.
>
> In addition, in the mbuf, the data that exceeds the length specified by pkt_len is invalid.
> Therefore, if the packet contains CRC data, pkt_len should contain the CRC data length.
> However, almost of drivers supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> This patch adds description for KEEP CRC offload.
>
> Dengdui Huang (3):
>    ethdev: add description for KEEP CRC offload
>    net/hns3: fix packet length do not contain CRC data length
>    net/hns3: fix Rx packet without CRC data
>
>   drivers/net/hns3/hns3_ethdev.c        |   5 +
>   drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>   drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>   drivers/net/hns3/hns3_rxtx.h          |   3 +
>   drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>   drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>   drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>   lib/ethdev/rte_ethdev.h               |   6 ++
>   8 files changed, 124 insertions(+), 72 deletions(-)
>

Re: [PATCH v2 0/3] bugfix about KEEP CRC offload

[Stephen Hemminger]

On Thu, 18 Jul 2024 19:48:02 +0800
Jie Hai <haijie1@huawei.com> wrote:

> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is still be stripped in rare scenarios.
> Some users of hns3 are already using this feature. So driver has to recaculate packet CRC.
> 
> In addition, in the mbuf, the data that exceeds the length specified by pkt_len is invalid.
> Therefore, if the packet contains CRC data, pkt_len should contain the CRC data length.
> However, almost of drivers supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> This patch adds description for KEEP CRC offload.
> 
> Dengdui Huang (3):
>   ethdev: add description for KEEP CRC offload
>   net/hns3: fix packet length do not contain CRC data length
>   net/hns3: fix Rx packet without CRC data
> 
>  drivers/net/hns3/hns3_ethdev.c        |   5 +
>  drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>  drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>  drivers/net/hns3/hns3_rxtx.h          |   3 +
>  drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>  drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>  drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>  lib/ethdev/rte_ethdev.h               |   6 ++
>  8 files changed, 124 insertions(+), 72 deletions(-)
> 
> -- 

Reworded some of the commit message to improve readability.

Applied to next-net.

[PATCH v3 0/3] bugfix about KEEP CRC offload

[Jie Hai]

For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is
still be stripped in rare scenarios. Some users of hns3 are
already using this feature. So driver has to recaculate packet CRC.
In addition, in the mbuf, the data that exceeds the length specified
by pkt_len is invalid. Therefore, if the packet contains CRC data,
pkt_len should contain the CRC data length. However, almost of drivers
supported KEEP CRC feature didn't add the CRC data length to pkt_len.

This patch adds description for KEEP CRC offload.

Dengdui Huang (3):
  ethdev: add description for KEEP CRC offload
  net/hns3: fix packet length do not contain CRC data length
  net/hns3: fix Rx packet without CRC data

 drivers/net/hns3/hns3_ethdev.c        |   5 +
 drivers/net/hns3/hns3_ethdev.h        |  23 +++++
 drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
 drivers/net/hns3/hns3_rxtx.h          |   3 +
 drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
 drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
 drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
 lib/ethdev/rte_ethdev.h               |   6 ++
 8 files changed, 124 insertions(+), 72 deletions(-)

-- 
2.33.0

[PATCH v3 1/3] ethdev: add description for KEEP CRC offload

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

The data exceeds the pkt_len in mbuf is inavailable for user.
When KEEP CRC offload is enabled, CRC field length should be
included in the pkt_len in mbuf. However, almost of drivers
supported KEEP CRC feature didn't add the CRC data length to
pkt_len. So it is very necessary to add comments for this.

Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Jie Hai <haijie1@huawei.com>
---
 lib/ethdev/rte_ethdev.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 548fada1c7ad..1f7237c48af6 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1550,6 +1550,12 @@ struct rte_eth_conf {
  */
 #define RTE_ETH_RX_OFFLOAD_TIMESTAMP        RTE_BIT64(14)
 #define RTE_ETH_RX_OFFLOAD_SECURITY         RTE_BIT64(15)
+/**
+ * Keep CRC data in packet.
+ *
+ * Note: If this offload is enabled, the pkt_len in mbuf must include
+ * the CRC data length.
+ */
 #define RTE_ETH_RX_OFFLOAD_KEEP_CRC         RTE_BIT64(16)
 #define RTE_ETH_RX_OFFLOAD_SCTP_CKSUM       RTE_BIT64(17)
 #define RTE_ETH_RX_OFFLOAD_OUTER_UDP_CKSUM  RTE_BIT64(18)
-- 
2.33.0

Re: [PATCH v3 1/3] ethdev: add description for KEEP CRC offload

[Andrew Rybchenko]

On 7/19/24 12:04, Jie Hai wrote:
> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> The data exceeds the pkt_len in mbuf is inavailable for user.
> When KEEP CRC offload is enabled, CRC field length should be
> included in the pkt_len in mbuf. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to
> pkt_len. So it is very necessary to add comments for this.
> 
> Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Jie Hai <haijie1@huawei.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [PATCH v3 1/3] ethdev: add description for KEEP CRC offload

[Stephen Hemminger]

On Fri, 19 Jul 2024 17:04:13 +0800
Jie Hai <haijie1@huawei.com> wrote:

> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> The data exceeds the pkt_len in mbuf is inavailable for user.
                                          unavailable

> When KEEP CRC offload is enabled, CRC field length should be
> included in the pkt_len in mbuf. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to
> pkt_len. So it is very necessary to add comments for this.

All drivers must do the same thing, or this is a serious bug
in the drivers. Just changing a comment is not going to be helpful.

To fix this right:
 1. Do a test with one of the original drivers in DPDK that has this
    feature. I would suggest ixgbe, mlx5 or bnxt.

 2. Add a test to the PMD tests that validates this (if there is not
    one already).

 3. Put the documentation in a place where it shows up in user documentation.
    Either in doxygen comment or in doc/guides/nics

 4. Verify that all devices conform to the desired behavior

I can help, but only have some old mlx5 cards to test here.
Just putting comment in ethdev.h is not enough.

Re: [PATCH v3 1/3] ethdev: add description for KEEP CRC offload

[Jie Hai]

Hi, Stephen Hemminger

Thanks for your review.
I will add doc and fix on drivers in the next version.
The test will be done later.

On 2024/11/23 1:10, Stephen Hemminger wrote:
> On Fri, 19 Jul 2024 17:04:13 +0800
> Jie Hai <haijie1@huawei.com> wrote:
> 
>> From: Dengdui Huang <huangdengdui@huawei.com>
>>
>> The data exceeds the pkt_len in mbuf is inavailable for user.
>                                            unavailable
> 
>> When KEEP CRC offload is enabled, CRC field length should be
>> included in the pkt_len in mbuf. However, almost of drivers
>> supported KEEP CRC feature didn't add the CRC data length to
>> pkt_len. So it is very necessary to add comments for this.
> 
> All drivers must do the same thing, or this is a serious bug
> in the drivers. Just changing a comment is not going to be helpful.
> 
> To fix this right:
>   1. Do a test with one of the original drivers in DPDK that has this
>      feature. I would suggest ixgbe, mlx5 or bnxt.
> I can test it on ixgbe and mlx5.
>   2. Add a test to the PMD tests that validates this (if there is not
>      one already).
> 
Maybe later and not come with this patchset.
>   3. Put the documentation in a place where it shows up in user documentation.
>      Either in doxygen comment or in doc/guides/nics
> 
Will add in the next version.
>   4. Verify that all devices conform to the desired behavior
> 
> I can help, but only have some old mlx5 cards to test here.
Thanks.
> Just putting comment in ethdev.h is not enough.
> 

Thanks,
Jie Hai

> 

Re: [PATCH v3 1/3] ethdev: add description for KEEP CRC offload

[Stephen Hemminger]

On Tue, 26 Nov 2024 15:47:32 +0800
Jie Hai <haijie1@huawei.com> wrote:

> Hi, Stephen Hemminger
> 
> Thanks for your review.
> I will add doc and fix on drivers in the next version.
> The test will be done later.
> 
> On 2024/11/23 1:10, Stephen Hemminger wrote:
> > On Fri, 19 Jul 2024 17:04:13 +0800
> > Jie Hai <haijie1@huawei.com> wrote:
> >   
> >> From: Dengdui Huang <huangdengdui@huawei.com>
> >>
> >> The data exceeds the pkt_len in mbuf is inavailable for user.  
> >                                            unavailable
> >   
> >> When KEEP CRC offload is enabled, CRC field length should be
> >> included in the pkt_len in mbuf. However, almost of drivers
> >> supported KEEP CRC feature didn't add the CRC data length to
> >> pkt_len. So it is very necessary to add comments for this.  
> > 
> > All drivers must do the same thing, or this is a serious bug
> > in the drivers. Just changing a comment is not going to be helpful.
> > 
> > To fix this right:
> >   1. Do a test with one of the original drivers in DPDK that has this
> >      feature. I would suggest ixgbe, mlx5 or bnxt.
> > I can test it on ixgbe and mlx5.
> >   2. Add a test to the PMD tests that validates this (if there is not
> >      one already).
> >   
> Maybe later and not come with this patchset.
> >   3. Put the documentation in a place where it shows up in user documentation.
> >      Either in doxygen comment or in doc/guides/nics
> >   
> Will add in the next version.
> >   4. Verify that all devices conform to the desired behavior
> > 
> > I can help, but only have some old mlx5 cards to test here.  
> Thanks.
> > Just putting comment in ethdev.h is not enough.
> >   
> 
> Thanks,
> Jie Hai
> 
> >   

After more discussion, decided to not include this patch (only the two bugfixes).
The proper place to put it is in the programmer's guide, and also you have
discovered a messy place in the driver API. In 10 minutes, already found several
issues in other drivers around this. Like checking wrong bits, dead code, etc.

Re: [PATCH v3 1/3] ethdev: add description for KEEP CRC offload

[Stephen Hemminger]

On Fri, 19 Jul 2024 17:04:13 +0800
Jie Hai <haijie1@huawei.com> wrote:

> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> The data exceeds the pkt_len in mbuf is inavailable for user.
> When KEEP CRC offload is enabled, CRC field length should be
> included in the pkt_len in mbuf. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to
> pkt_len. So it is very necessary to add comments for this.
> 
> Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Jie Hai <haijie1@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

If you put the information in doc, users would see it.
Something like this:

diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index 0508f118fe..63b0331b06 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -470,8 +470,9 @@ protocol operations. See security library and PMD documentation for more details
 CRC offload
 -----------
 
-Supports CRC stripping by hardware.
-A PMD assumed to support CRC stripping by default. PMD should advertise if it supports keeping CRC.
+Supports including the CRC in the received packet.
+A PMD is assumed to support CRC stripping by default,
+PMD should only advertise if it supports keeping CRC.
 
 * **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:RTE_ETH_RX_OFFLOAD_KEEP_CRC``.
 
diff --git a/doc/guides/prog_guide/mbuf_lib.rst b/doc/guides/prog_guide/mbuf_lib.rst
index 4ad2a21f3f..bea9111ba4 100644
--- a/doc/guides/prog_guide/mbuf_lib.rst
+++ b/doc/guides/prog_guide/mbuf_lib.rst
@@ -207,6 +207,18 @@ The list of flags and their precise meaning is described in the mbuf API
 documentation (rte_mbuf.h). Also refer to the testpmd source code
 (specifically the csumonly.c file) for details.
 
+CRC offload
+~~~~~~~~~~~
+
+Normally the Ethernet Cyclic Redundancy Check (CRC) is *not* included in the mbuf.
+Some Poll Mode Driver's support keeping the received CRC in the mbuf.
+If a packet is received with keep CRC offload setting:
+- the CRC is in included in the mbuf pkt_len and data_len
+- the CRC is present but not checked
+- the mbuf should not be directly transmitted or the received CRC will be include
+  in the transmit
+
+
 Dynamic fields and flags
 ~~~~~~~~~~~~~~~~~~~~~~~~
 

[PATCH v3 2/3] net/hns3: fix packet length do not contain CRC data length

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

In the HNS3 driver, pkt_len in mbuf do not contain the CRC length.
This patch fix it.

Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Acked-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Jie Hai <haijie1@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c          | 53 +++------------------------
 drivers/net/hns3/hns3_rxtx_vec_neon.h | 19 ----------
 drivers/net/hns3/hns3_rxtx_vec_sve.c  |  3 +-
 3 files changed, 7 insertions(+), 68 deletions(-)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 5941b966e094..39ba9080ea15 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -1951,7 +1951,6 @@ hns3_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t nb_desc,
 	memset(&rxq->err_stats, 0, sizeof(struct hns3_rx_bd_errors_stats));
 	memset(&rxq->dfx_stats, 0, sizeof(struct hns3_rx_dfx_stats));
 
-	/* CRC len set here is used for amending packet length */
 	if (dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC)
 		rxq->crc_len = RTE_ETHER_CRC_LEN;
 	else
@@ -2383,23 +2382,6 @@ hns3_rxd_to_vlan_tci(struct hns3_rx_queue *rxq, struct rte_mbuf *mb,
 	}
 }
 
-static inline void
-recalculate_data_len(struct rte_mbuf *first_seg, struct rte_mbuf *last_seg,
-		    struct rte_mbuf *rxm, struct hns3_rx_queue *rxq,
-		    uint16_t data_len)
-{
-	uint8_t crc_len = rxq->crc_len;
-
-	if (data_len <= crc_len) {
-		rte_pktmbuf_free_seg(rxm);
-		first_seg->nb_segs--;
-		last_seg->data_len = (uint16_t)(last_seg->data_len -
-			(crc_len - data_len));
-		last_seg->next = NULL;
-	} else
-		rxm->data_len = (uint16_t)(data_len - crc_len);
-}
-
 static inline struct rte_mbuf *
 hns3_rx_alloc_buffer(struct hns3_rx_queue *rxq)
 {
@@ -2503,8 +2485,7 @@ hns3_recv_pkts_simple(void *rx_queue,
 		rxdp->rx.bd_base_info = 0;
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
-		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
-				rxq->crc_len;
+		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len));
 		rxm->data_len = rxm->pkt_len;
 		rxm->port = rxq->port_id;
 		rxm->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
@@ -2531,8 +2512,8 @@ hns3_recv_pkts_simple(void *rx_queue,
 
 		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
 
-		/* Increment bytes counter  */
-		rxq->basic_stats.bytes += rxm->pkt_len;
+		/* All byte-related statistics do not include Ethernet FCS */
+		rxq->basic_stats.bytes += rxm->pkt_len - rxq->crc_len;
 
 		rx_pkts[nb_rx++] = rxm;
 		continue;
@@ -2695,10 +2676,10 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
 		rxm->data_len = rte_le_to_cpu_16(rxd.rx.size);
+		rxm->next = NULL;
 
 		if (!(bd_base_info & BIT(HNS3_RXD_FE_B))) {
 			last_seg = rxm;
-			rxm->next = NULL;
 			continue;
 		}
 
@@ -2706,30 +2687,8 @@ hns3_recv_scattered_pkts(void *rx_queue,
 		if (unlikely(bd_base_info & BIT(HNS3_RXD_TS_VLD_B)))
 			hns3_rx_ptp_timestamp_handle(rxq, first_seg, timestamp);
 
-		/*
-		 * The last buffer of the received packet. packet len from
-		 * buffer description may contains CRC len, packet len should
-		 * subtract it, same as data len.
-		 */
 		first_seg->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
 
-		/*
-		 * This is the last buffer of the received packet. If the CRC
-		 * is not stripped by the hardware:
-		 *  - Subtract the CRC length from the total packet length.
-		 *  - If the last buffer only contains the whole CRC or a part
-		 *  of it, free the mbuf associated to the last buffer. If part
-		 *  of the CRC is also contained in the previous mbuf, subtract
-		 *  the length of that CRC part from the data length of the
-		 *  previous mbuf.
-		 */
-		rxm->next = NULL;
-		if (unlikely(rxq->crc_len > 0)) {
-			first_seg->pkt_len -= rxq->crc_len;
-			recalculate_data_len(first_seg, last_seg, rxm, rxq,
-				rxm->data_len);
-		}
-
 		first_seg->port = rxq->port_id;
 		first_seg->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
 		first_seg->ol_flags |= RTE_MBUF_F_RX_RSS_HASH;
@@ -2761,8 +2720,8 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
 
-		/* Increment bytes counter */
-		rxq->basic_stats.bytes += first_seg->pkt_len;
+		/* All byte-related statistics do not include Ethernet FCS */
+		rxq->basic_stats.bytes += first_seg->pkt_len - rxq->crc_len;
 
 		rx_pkts[nb_rx++] = first_seg;
 		first_seg = NULL;
diff --git a/drivers/net/hns3/hns3_rxtx_vec_neon.h b/drivers/net/hns3/hns3_rxtx_vec_neon.h
index 0dc6b9f0a22d..60ec501a2ab6 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_neon.h
+++ b/drivers/net/hns3/hns3_rxtx_vec_neon.h
@@ -148,14 +148,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		8, 9, 10, 11,	         /* rx.rss_hash to rte_mbuf.hash.rss */
 	};
 
-	uint16x8_t crc_adjust = {
-		0, 0,         /* ignore pkt_type field */
-		rxq->crc_len, /* sub crc on pkt_len */
-		0,            /* ignore high-16bits of pkt_len */
-		rxq->crc_len, /* sub crc on data_len */
-		0, 0, 0,      /* ignore non-length fields */
-	};
-
 	/* compile-time verifies the shuffle mask */
 	RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, pkt_len) !=
 			 offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4);
@@ -171,7 +163,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		uint8x16_t pkt_mb1, pkt_mb2, pkt_mb3, pkt_mb4;
 		uint64x2_t mbp1, mbp2;
 		uint16x4_t bd_vld = {0};
-		uint16x8_t tmp;
 		uint64_t stat;
 
 		/* calc how many bd valid */
@@ -225,16 +216,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		pkt_mb3 = vqtbl2q_u8(pkt_mbuf3, shuf_desc_fields_msk);
 		pkt_mb4 = vqtbl2q_u8(pkt_mbuf4, shuf_desc_fields_msk);
 
-		/* 4 packets remove crc */
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb1), crc_adjust);
-		pkt_mb1 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb2), crc_adjust);
-		pkt_mb2 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb3), crc_adjust);
-		pkt_mb3 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb4), crc_adjust);
-		pkt_mb4 = vreinterpretq_u8_u16(tmp);
-
 		/* save packet info to rx_pkts mbuf */
 		vst1q_u8((void *)&sw_ring[pos + 0].mbuf->rx_descriptor_fields1,
 			 pkt_mb1);
diff --git a/drivers/net/hns3/hns3_rxtx_vec_sve.c b/drivers/net/hns3/hns3_rxtx_vec_sve.c
index 8aa4448558cf..67c87f570e8a 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_sve.c
+++ b/drivers/net/hns3/hns3_rxtx_vec_sve.c
@@ -36,8 +36,7 @@ hns3_desc_parse_field_sve(struct hns3_rx_queue *rxq,
 		/* init rte_mbuf.rearm_data last 64-bit */
 		rx_pkts[i]->ol_flags = RTE_MBUF_F_RX_RSS_HASH;
 		rx_pkts[i]->hash.rss = rxdp[i].rx.rss_hash;
-		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len) -
-					rxq->crc_len;
+		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len);
 		rx_pkts[i]->data_len = rx_pkts[i]->pkt_len;
 
 		l234_info = rxdp[i].rx.l234_info;
-- 
2.33.0

[PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

When KEEP_CRC offload is enabled, the CRC data is still stripped
in following cases:
1. For HIP08 network engine, the packet type is TCP and the length
   is less than or equal to 60B.
2. For HIP09 network engine, the packet type is IP and the length
   is less than or equal to 60B.

So driver has to recaculate packet CRC for this rare scenarios.

In addition, to avoid impacting performance, KEEP_CRC is not
supported when NEON or SVE algorithm is used.

Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Acked-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Jie Hai <haijie1@huawei.com>
---
 drivers/net/hns3/hns3_ethdev.c   |  5 ++
 drivers/net/hns3/hns3_ethdev.h   | 23 +++++++++
 drivers/net/hns3/hns3_rxtx.c     | 81 ++++++++++++++++++++++++++++++--
 drivers/net/hns3/hns3_rxtx.h     |  3 ++
 drivers/net/hns3/hns3_rxtx_vec.c |  3 +-
 5 files changed, 111 insertions(+), 4 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index ec1251cb7ead..5fdabba54794 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -2739,6 +2739,7 @@ hns3_get_capability(struct hns3_hw *hw)
 		hw->udp_cksum_mode = HNS3_SPECIAL_PORT_SW_CKSUM_MODE;
 		pf->support_multi_tc_pause = false;
 		hw->rx_dma_addr_align = HNS3_RX_DMA_ADDR_ALIGN_64;
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_TCP;
 		return 0;
 	}
 
@@ -2760,6 +2761,10 @@ hns3_get_capability(struct hns3_hw *hw)
 	hw->udp_cksum_mode = HNS3_SPECIAL_PORT_HW_CKSUM_MODE;
 	pf->support_multi_tc_pause = true;
 	hw->rx_dma_addr_align = HNS3_RX_DMA_ADDR_ALIGN_128;
+	if (hw->revision == PCI_REVISION_ID_HIP09_A)
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_IP;
+	else
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_NONE;
 
 	return 0;
 }
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index 799b61038a06..ef1f5ebd584c 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -56,6 +56,10 @@
 #define HNS3_SPECIAL_PORT_SW_CKSUM_MODE         0
 #define HNS3_SPECIAL_PORT_HW_CKSUM_MODE         1
 
+#define HNS3_STRIP_CRC_PTYPE_NONE         0
+#define HNS3_STRIP_CRC_PTYPE_TCP          1
+#define HNS3_STRIP_CRC_PTYPE_IP           2
+
 #define HNS3_UC_MACADDR_NUM		128
 #define HNS3_VF_UC_MACADDR_NUM		48
 #define HNS3_MC_MACADDR_NUM		128
@@ -657,6 +661,25 @@ struct hns3_hw {
 	 */
 	uint8_t udp_cksum_mode;
 
+	/*
+	 * When KEEP_CRC offload is enabled, the CRC data of some type packets
+	 * whose length is less than or equal to HNS3_KEEP_CRC_OK_MIN_PKT_LEN
+	 * is still be stripped on some network engine. So here has to use this
+	 * field to distinguish the difference between different network engines.
+	 * value range:
+	 *  - HNS3_STRIP_CRC_PTYPE_TCP
+	 *     This value for HIP08 network engine.
+	 *     Indicates that only the IP-TCP packet type is stripped.
+	 *
+	 *  - HNS3_STRIP_CRC_PTYPE_IP
+	 *     This value for HIP09 network engine.
+	 *     Indicates that all IP packet types are stripped.
+	 *
+	 *  - HNS3_STRIP_CRC_PTYPE_NONE
+	 *     Indicates that all packet types are not stripped.
+	 */
+	uint8_t strip_crc_ptype;
+
 	struct hns3_port_base_vlan_config port_base_vlan_cfg;
 
 	pthread_mutex_t flows_lock; /* rte_flow ops lock */
diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 39ba9080ea15..d12463d0ec56 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -11,6 +11,7 @@
 #include <rte_io.h>
 #include <rte_net.h>
 #include <rte_malloc.h>
+#include <rte_net_crc.h>
 #if defined(RTE_ARCH_ARM64)
 #include <rte_cpuflags.h>
 #include <rte_vect.h>
@@ -1766,8 +1767,9 @@ hns3_rx_buf_len_calc(struct rte_mempool *mp, uint16_t *rx_buf_len)
 }
 
 static int
-hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
-				uint16_t nb_desc)
+hns3_rxq_conf_runtime_check(struct hns3_hw *hw,
+			    const struct rte_eth_rxconf *conf,
+			    uint16_t buf_size, uint16_t nb_desc)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
 	eth_rx_burst_t pkt_burst = dev->rx_pkt_burst;
@@ -1800,6 +1802,14 @@ hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
 			return -EINVAL;
 		}
 	}
+
+	if ((conf->offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC) &&
+	    pkt_burst != hns3_recv_pkts_simple &&
+	    pkt_burst != hns3_recv_scattered_pkts) {
+		hns3_err(hw, "KEEP_CRC offload is not supported in the current rx function.");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -1836,7 +1846,7 @@ hns3_rx_queue_conf_check(struct hns3_hw *hw, const struct rte_eth_rxconf *conf,
 	}
 
 	if (hw->data->dev_started) {
-		ret = hns3_rxq_conf_runtime_check(hw, *buf_size, nb_desc);
+		ret = hns3_rxq_conf_runtime_check(hw, conf, *buf_size, nb_desc);
 		if (ret) {
 			hns3_err(hw, "Rx queue runtime setup fail.");
 			return ret;
@@ -1956,6 +1966,8 @@ hns3_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t nb_desc,
 	else
 		rxq->crc_len = 0;
 
+	rxq->keep_crc_fail_ptype = hw->strip_crc_ptype;
+
 	rxq->bulk_mbuf_num = 0;
 
 	rte_spinlock_lock(&hw->lock);
@@ -2415,6 +2427,50 @@ hns3_rx_ptp_timestamp_handle(struct hns3_rx_queue *rxq, struct rte_mbuf *mbuf,
 	pf->rx_timestamp = timestamp;
 }
 
+static inline bool
+hns3_need_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+	uint32_t ptype = m->packet_type;
+
+	if (rxq->keep_crc_fail_ptype == HNS3_STRIP_CRC_PTYPE_NONE)
+		return false;
+
+	if (m->pkt_len > HNS3_KEEP_CRC_OK_MIN_PKT_LEN)
+		return false;
+
+	if (!(RTE_ETH_IS_IPV4_HDR(ptype) || RTE_ETH_IS_IPV6_HDR(ptype)))
+		return false;
+
+	if (rxq->keep_crc_fail_ptype == HNS3_STRIP_CRC_PTYPE_TCP)
+		return (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP;
+
+	return true;
+}
+
+static inline void
+hns3_recalculate_crc(struct rte_mbuf *m)
+{
+	char *append_data;
+	uint32_t crc;
+
+	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
+			       m->data_len, RTE_NET_CRC32_ETH);
+
+	/*
+	 * The hns3 driver requires that mbuf size must be at least 512B.
+	 * When CRC is stripped by hardware, the pkt_len must be less than
+	 * or equal to 60B. Therefore, the space of the mbuf is enough
+	 * to insert the CRC.
+	 *
+	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
+	 * do not contain the CRC length. Therefore, after CRC data is appended
+	 * by PMD again, both pkt_len and data_len add the CRC length.
+	 */
+	append_data = rte_pktmbuf_append(m, RTE_NET_CRC32_ETH);
+	/* The CRC data is binary data and does not care about the byte order. */
+	rte_memcpy(append_data, (void *)&crc, RTE_NET_CRC32_ETH);
+}
+
 uint16_t
 hns3_recv_pkts_simple(void *rx_queue,
 		      struct rte_mbuf **rx_pkts,
@@ -2510,6 +2566,10 @@ hns3_recv_pkts_simple(void *rx_queue,
 		if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
 
+		if (unlikely(rxq->crc_len > 0) &&
+		    hns3_need_recalculate_crc(rxq, rxm))
+			hns3_recalculate_crc(rxm);
+
 		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
 
 		/* All byte-related statistics do not include Ethernet FCS */
@@ -2718,6 +2778,21 @@ hns3_recv_scattered_pkts(void *rx_queue,
 		if (first_seg->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
 
+		/*
+		 * If KEEP_CRC offload is enabled, the network engine reserves
+		 * the CRC data in the packet. But the CRC is still stripped
+		 * for a kind of packets in hns3 NIC:
+		 * 1. All IP-TCP packet whose the length is less than and equal
+		 *    to 60 Byte (no CRC) on HIP08 network engine.
+		 * 2. All IP packet whose the length is less than and equal to
+		 *    60 Byte (no CRC) on HIP09 network engine.
+		 * In this case, the PMD calculates the CRC and appends it to
+		 * mbuf.
+		 */
+		if (unlikely(rxq->crc_len > 0) &&
+		    hns3_need_recalculate_crc(rxq, first_seg))
+			hns3_recalculate_crc(first_seg);
+
 		hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
 
 		/* All byte-related statistics do not include Ethernet FCS */
diff --git a/drivers/net/hns3/hns3_rxtx.h b/drivers/net/hns3/hns3_rxtx.h
index e975cd151a7e..0eb9796fe053 100644
--- a/drivers/net/hns3/hns3_rxtx.h
+++ b/drivers/net/hns3/hns3_rxtx.h
@@ -178,6 +178,8 @@
 		(HNS3_TXD_VLD_CMD | HNS3_TXD_FE_CMD | HNS3_TXD_DEFAULT_BDTYPE)
 #define HNS3_TXD_SEND_SIZE_SHIFT	16
 
+#define HNS3_KEEP_CRC_OK_MIN_PKT_LEN	60
+
 enum hns3_pkt_l2t_type {
 	HNS3_L2_TYPE_UNICAST,
 	HNS3_L2_TYPE_MULTICAST,
@@ -341,6 +343,7 @@ struct hns3_rx_queue {
 	 */
 	uint8_t pvid_sw_discard_en:1;
 	uint8_t ptype_en:1;          /* indicate if the ptype field enabled */
+	uint8_t keep_crc_fail_ptype:2;
 
 	uint64_t mbuf_initializer; /* value to init mbufs used with vector rx */
 	/* offset_table: used for vector, to solve execute re-order problem */
diff --git a/drivers/net/hns3/hns3_rxtx_vec.c b/drivers/net/hns3/hns3_rxtx_vec.c
index 9708ec614e02..bf37ce51b1ad 100644
--- a/drivers/net/hns3/hns3_rxtx_vec.c
+++ b/drivers/net/hns3/hns3_rxtx_vec.c
@@ -185,7 +185,8 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO |
 				 RTE_ETH_RX_OFFLOAD_VLAN |
-				 RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+				 RTE_ETH_RX_OFFLOAD_TIMESTAMP |
+				 RTE_ETH_RX_OFFLOAD_KEEP_CRC;
 
 	if (dev->data->scattered_rx)
 		return -ENOTSUP;
-- 
2.33.0

Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Stephen Hemminger]

On Fri, 19 Jul 2024 17:04:15 +0800
Jie Hai <haijie1@huawei.com> wrote:

> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> When KEEP_CRC offload is enabled, the CRC data is still stripped
> in following cases:
> 1. For HIP08 network engine, the packet type is TCP and the length
>    is less than or equal to 60B.
> 2. For HIP09 network engine, the packet type is IP and the length
>    is less than or equal to 60B.
> 
> So driver has to recaculate packet CRC for this rare scenarios.
> 
> In addition, to avoid impacting performance, KEEP_CRC is not
> supported when NEON or SVE algorithm is used.
> 
> Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>

> +static inline void
> +hns3_recalculate_crc(struct rte_mbuf *m)
> +{
> +	char *append_data;
> +	uint32_t crc;
> +
> +	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
> +			       m->data_len, RTE_NET_CRC32_ETH);
> +
> +	/*
> +	 * The hns3 driver requires that mbuf size must be at least 512B.
> +	 * When CRC is stripped by hardware, the pkt_len must be less than
> +	 * or equal to 60B. Therefore, the space of the mbuf is enough
> +	 * to insert the CRC.
> +	 *
> +	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
> +	 * do not contain the CRC length. Therefore, after CRC data is appended
> +	 * by PMD again, both pkt_len and data_len add the CRC length.
> +	 */
> +	append_data = rte_pktmbuf_append(m, RTE_NET_CRC32_ETH);
> +	/* The CRC data is binary data and does not care about the byte order. */
> +	rte_memcpy(append_data, (void *)&crc, RTE_NET_CRC32_ETH);
> +}

Ok, but minor nits for future.
Trying to phase out use of rte_memcpy(), rte_memcpy() has no advantage except
in certain cases of unaligned variable length copies mostly on older non x86 distros.
And regular memcpy() enables compiler to do more checking.

And you don't need a void * cast there, in C there is implicit cast to void * there.

Applied to next-net

Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Stephen Hemminger]

On Fri, 19 Jul 2024 17:04:15 +0800
Jie Hai <haijie1@huawei.com> wrote:

> +static inline void
> +hns3_recalculate_crc(struct rte_mbuf *m)
> +{
> +	char *append_data;
> +	uint32_t crc;
> +
> +	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
> +			       m->data_len, RTE_NET_CRC32_ETH);
> +
> +	/*
> +	 * The hns3 driver requires that mbuf size must be at least 512B.
> +	 * When CRC is stripped by hardware, the pkt_len must be less than
> +	 * or equal to 60B. Therefore, the space of the mbuf is enough
> +	 * to insert the CRC.
> +	 *
> +	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
> +	 * do not contain the CRC length. Therefore, after CRC data is appended
> +	 * by PMD again, both pkt_len and data_len add the CRC length.
> +	 */
> +	append_data = rte_pktmbuf_append(m, RTE_NET_CRC32_ETH);
> +	/* The CRC data is binary data and does not care about the byte order. */
> +	rte_memcpy(append_data, (void *)&crc, RTE_NET_CRC32_ETH);
> +}
> +

There is a bug here. If there is no space at end of mbuf (tailroom) then
rte_pktmbuf_append will return NULL. This would only happen if mbuf pool
was sized such that there was space of a full size packet without CRC
and then the code to add kept CRC was executed.

You need to check for NULL return and figure out some kind of unwind
path such as making it a receive error and dropping the packet.

Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[huangdengdui]

On 2024/11/26 1:45, Stephen Hemminger wrote:
> On Fri, 19 Jul 2024 17:04:15 +0800
> Jie Hai <haijie1@huawei.com> wrote:
> 
>> +static inline void
>> +hns3_recalculate_crc(struct rte_mbuf *m)
>> +{
>> +	char *append_data;
>> +	uint32_t crc;
>> +
>> +	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
>> +			       m->data_len, RTE_NET_CRC32_ETH);
>> +
>> +	/*
>> +	 * The hns3 driver requires that mbuf size must be at least 512B.
>> +	 * When CRC is stripped by hardware, the pkt_len must be less than
>> +	 * or equal to 60B. Therefore, the space of the mbuf is enough
>> +	 * to insert the CRC.
>> +	 *
>> +	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
>> +	 * do not contain the CRC length. Therefore, after CRC data is appended
>> +	 * by PMD again, both pkt_len and data_len add the CRC length.
>> +	 */
>> +	append_data = rte_pktmbuf_append(m, RTE_NET_CRC32_ETH);
>> +	/* The CRC data is binary data and does not care about the byte order. */
>> +	rte_memcpy(append_data, (void *)&crc, RTE_NET_CRC32_ETH);
>> +}
>> +
> 
> There is a bug here. If there is no space at end of mbuf (tailroom) then
> rte_pktmbuf_append will return NULL. This would only happen if mbuf pool
> was sized such that there was space of a full size packet without CRC
> and then the code to add kept CRC was executed.
> 
> You need to check for NULL return and figure out some kind of unwind
> path such as making it a receive error and dropping the packet.

This situation has been described in the comments.
The hns3 driver requires that mbuf size must be at least 512B.[1]
When CRC needs to be recalculated, the packet length must be less than 60.
So the space of the mbuf must be enough to insert the CRC.
[1]:https://elixir.bootlin.com/dpdk/v23.11/source/drivers/net/hns3/hns3_rxtx.c#L1723

Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Stephen Hemminger]

On Tue, 26 Nov 2024 10:40:27 +0800
huangdengdui <huangdengdui@huawei.com> wrote:

> On 2024/11/26 1:45, Stephen Hemminger wrote:
> > On Fri, 19 Jul 2024 17:04:15 +0800
> > Jie Hai <haijie1@huawei.com> wrote:
> >   
> >> +static inline void
> >> +hns3_recalculate_crc(struct rte_mbuf *m)
> >> +{
> >> +	char *append_data;
> >> +	uint32_t crc;
> >> +
> >> +	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
> >> +			       m->data_len, RTE_NET_CRC32_ETH);
> >> +
> >> +	/*
> >> +	 * The hns3 driver requires that mbuf size must be at least 512B.
> >> +	 * When CRC is stripped by hardware, the pkt_len must be less than
> >> +	 * or equal to 60B. Therefore, the space of the mbuf is enough
> >> +	 * to insert the CRC.
> >> +	 *
> >> +	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
> >> +	 * do not contain the CRC length. Therefore, after CRC data is appended
> >> +	 * by PMD again, both pkt_len and data_len add the CRC length.
> >> +	 */
> >> +	append_data = rte_pktmbuf_append(m, RTE_NET_CRC32_ETH);
> >> +	/* The CRC data is binary data and does not care about the byte order. */
> >> +	rte_memcpy(append_data, (void *)&crc, RTE_NET_CRC32_ETH);
> >> +}
> >> +  
> > 
> > There is a bug here. If there is no space at end of mbuf (tailroom) then
> > rte_pktmbuf_append will return NULL. This would only happen if mbuf pool
> > was sized such that there was space of a full size packet without CRC
> > and then the code to add kept CRC was executed.
> > 
> > You need to check for NULL return and figure out some kind of unwind
> > path such as making it a receive error and dropping the packet.  
> 
> This situation has been described in the comments.
> The hns3 driver requires that mbuf size must be at least 512B.[1]
> When CRC needs to be recalculated, the packet length must be less than 60.
> So the space of the mbuf must be enough to insert the CRC.
> [1]:https://elixir.bootlin.com/dpdk/v23.11/source/drivers/net/hns3/hns3_rxtx.c#L1723

Ok, but maybe add an RTE_ASSERT() to be safe.

Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Stephen Hemminger]

On Fri, 19 Jul 2024 17:04:15 +0800
Jie Hai <haijie1@huawei.com> wrote:

> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> When KEEP_CRC offload is enabled, the CRC data is still stripped
> in following cases:
> 1. For HIP08 network engine, the packet type is TCP and the length
>    is less than or equal to 60B.
> 2. For HIP09 network engine, the packet type is IP and the length
>    is less than or equal to 60B.
> 
> So driver has to recaculate packet CRC for this rare scenarios.
> 
> In addition, to avoid impacting performance, KEEP_CRC is not
> supported when NEON or SVE algorithm is used.
> 
> Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Jie Hai <haijie1@huawei.com>

Changed my mind on these patches after digging deeper into
what other drivers are doing. The proposed patches for hns3 do
the opposite of what the consensus of drivers is.

When looking at internals, all other drivers do not include the CRC
in the packet length calculation. It is hard to go back and determine
the rational for this, but my assumption is that if a packet is received
(with KEEP_CRC enabled), the application will likely want to send that
packet to another location, and the transmit side doesn't want the CRC.

There are a couple of related driver bugs in some drivers in handling
of the flag as well. One driver (idpf) thinks the CRC should count for the byte
statistics. This should be clarified and fixed.

One driver (atlantic) adds a check but doesn't implement the flag; the check for
valid offload flags is already handled by ethdev API.

Please resubmit for a later release, and can be picked up then by 24.11 stable.

You have found an area of DPDK which is poorly documented. Will raise an
agenda at next techboard to get a final agreement, then put that into
the programmer's guide.

Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Jie Hai]

On 2024/11/27 8:16, Stephen Hemminger wrote:
> On Fri, 19 Jul 2024 17:04:15 +0800
> Jie Hai <haijie1@huawei.com> wrote:
> 
>> From: Dengdui Huang <huangdengdui@huawei.com>
>>
>> When KEEP_CRC offload is enabled, the CRC data is still stripped
>> in following cases:
>> 1. For HIP08 network engine, the packet type is TCP and the length
>>     is less than or equal to 60B.
>> 2. For HIP09 network engine, the packet type is IP and the length
>>     is less than or equal to 60B.
>>
>> So driver has to recaculate packet CRC for this rare scenarios.
>>
>> In addition, to avoid impacting performance, KEEP_CRC is not
>> supported when NEON or SVE algorithm is used.
>>
>> Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>> Acked-by: Huisong Li <lihuisong@huawei.com>
>> Acked-by: Jie Hai <haijie1@huawei.com>
> 
> Changed my mind on these patches after digging deeper into
> what other drivers are doing. The proposed patches for hns3 do
> the opposite of what the consensus of drivers is.
> 
> When looking at internals, all other drivers do not include the CRC
> in the packet length calculation. It is hard to go back and determine
> the rational for this, but my assumption is that if a packet is received
> (with KEEP_CRC enabled), the application will likely want to send that
> packet to another location, and the transmit side doesn't want the CRC.
> 
> There are a couple of related driver bugs in some drivers in handling
> of the flag as well. One driver (idpf) thinks the CRC should count for the byte
> statistics. This should be clarified and fixed.
> 
> One driver (atlantic) adds a check but doesn't implement the flag; the check for
> valid offload flags is already handled by ethdev API.
> 
> Please resubmit for a later release, and can be picked up then by 24.11 stable.
> 
There is indeed much work to be done to clarify the relationship between 
keep crc and fields in mbuf.
In the current patchset, patch 1 and patch 2 are used for this purpose, 
but a bug occurs.
If CRC is not processed in the TX direction, CRC is forwarded as packet 
data.
As a result, the packet length is increased by 4.
I agree that this part should be carefully investigated before a 
decision is made.

But for patch 3,  this is a serious bug of the hns3 driver and  and is 
irrelevant to the preceding questions.

Could you please apply the patch 3 first to fix the bug of hns3 ?
I can send a single patch of hns3 driver for the next version.

> You have found an area of DPDK which is poorly documented. Will raise an
> agenda at next techboard to get a final agreement, then put that into
> the programmer's guide.
> .

Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Stephen Hemminger]

On Wed, 27 Nov 2024 10:32:24 +0800
Jie Hai <haijie1@huawei.com> wrote:

> On 2024/11/27 8:16, Stephen Hemminger wrote:
> > On Fri, 19 Jul 2024 17:04:15 +0800
> > Jie Hai <haijie1@huawei.com> wrote:
> >   
> >> From: Dengdui Huang <huangdengdui@huawei.com>
> >>
> >> When KEEP_CRC offload is enabled, the CRC data is still stripped
> >> in following cases:
> >> 1. For HIP08 network engine, the packet type is TCP and the length
> >>     is less than or equal to 60B.
> >> 2. For HIP09 network engine, the packet type is IP and the length
> >>     is less than or equal to 60B.
> >>
> >> So driver has to recaculate packet CRC for this rare scenarios.
> >>
> >> In addition, to avoid impacting performance, KEEP_CRC is not
> >> supported when NEON or SVE algorithm is used.
> >>
> >> Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> >> Acked-by: Huisong Li <lihuisong@huawei.com>
> >> Acked-by: Jie Hai <haijie1@huawei.com>  
> > 
> > Changed my mind on these patches after digging deeper into
> > what other drivers are doing. The proposed patches for hns3 do
> > the opposite of what the consensus of drivers is.
> > 
> > When looking at internals, all other drivers do not include the CRC
> > in the packet length calculation. It is hard to go back and determine
> > the rational for this, but my assumption is that if a packet is received
> > (with KEEP_CRC enabled), the application will likely want to send that
> > packet to another location, and the transmit side doesn't want the CRC.
> > 
> > There are a couple of related driver bugs in some drivers in handling
> > of the flag as well. One driver (idpf) thinks the CRC should count for the byte
> > statistics. This should be clarified and fixed.
> > 
> > One driver (atlantic) adds a check but doesn't implement the flag; the check for
> > valid offload flags is already handled by ethdev API.
> > 
> > Please resubmit for a later release, and can be picked up then by 24.11 stable.
> >   
> There is indeed much work to be done to clarify the relationship between 
> keep crc and fields in mbuf.
> In the current patchset, patch 1 and patch 2 are used for this purpose, 
> but a bug occurs.
> If CRC is not processed in the TX direction, CRC is forwarded as packet 
> data.
> As a result, the packet length is increased by 4.
> I agree that this part should be carefully investigated before a 
> decision is made.
> 
> But for patch 3,  this is a serious bug of the hns3 driver and  and is 
> irrelevant to the preceding questions.
> 
> Could you please apply the patch 3 first to fix the bug of hns3 ?
> I can send a single patch of hns3 driver for the next version.
> 
> > You have found an area of DPDK which is poorly documented. Will raise an
> > agenda at next techboard to get a final agreement, then put that into
> > the programmer's guide.
> > .  

Yes a bugfix that makes hns3 behave like other drivers for now
would be best approach.

Re: [PATCH v3 0/3] bugfix about KEEP CRC offload

[fengchengwen]

Series-reviewed-by: Chengwen Feng <fengchengwen@huawei.com>

On 2024/7/19 17:04, Jie Hai wrote:
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is
> still be stripped in rare scenarios. Some users of hns3 are
> already using this feature. So driver has to recaculate packet CRC.
> In addition, in the mbuf, the data that exceeds the length specified
> by pkt_len is invalid. Therefore, if the packet contains CRC data,
> pkt_len should contain the CRC data length. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> 
> This patch adds description for KEEP CRC offload.
> 
> Dengdui Huang (3):
>   ethdev: add description for KEEP CRC offload
>   net/hns3: fix packet length do not contain CRC data length
>   net/hns3: fix Rx packet without CRC data
> 
>  drivers/net/hns3/hns3_ethdev.c        |   5 +
>  drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>  drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>  drivers/net/hns3/hns3_rxtx.h          |   3 +
>  drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>  drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>  drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>  lib/ethdev/rte_ethdev.h               |   6 ++
>  8 files changed, 124 insertions(+), 72 deletions(-)
> 

Re: [PATCH v3 0/3] bugfix about KEEP CRC offload

[Jie Hai]

All maintainers,

Hi, kindly ping for review.

Thanks,
Jie Hai

On 2024/7/19 17:04, Jie Hai wrote:
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is
> still be stripped in rare scenarios. Some users of hns3 are
> already using this feature. So driver has to recaculate packet CRC.
> In addition, in the mbuf, the data that exceeds the length specified
> by pkt_len is invalid. Therefore, if the packet contains CRC data,
> pkt_len should contain the CRC data length. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> 
> This patch adds description for KEEP CRC offload.
> 
> Dengdui Huang (3):
>    ethdev: add description for KEEP CRC offload
>    net/hns3: fix packet length do not contain CRC data length
>    net/hns3: fix Rx packet without CRC data
> 
>   drivers/net/hns3/hns3_ethdev.c        |   5 +
>   drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>   drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>   drivers/net/hns3/hns3_rxtx.h          |   3 +
>   drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>   drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>   drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>   lib/ethdev/rte_ethdev.h               |   6 ++
>   8 files changed, 124 insertions(+), 72 deletions(-)
> 

Re: [PATCH v3 0/3] bugfix about KEEP CRC offload

[Jie Hai]

Hi, all maintainers,

Kindly ping for review.

Thanks,
Jie Hai

On 2024/7/19 17:04, Jie Hai wrote:
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is
> still be stripped in rare scenarios. Some users of hns3 are
> already using this feature. So driver has to recaculate packet CRC.
> In addition, in the mbuf, the data that exceeds the length specified
> by pkt_len is invalid. Therefore, if the packet contains CRC data,
> pkt_len should contain the CRC data length. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> 
> This patch adds description for KEEP CRC offload.
> 
> Dengdui Huang (3):
>    ethdev: add description for KEEP CRC offload
>    net/hns3: fix packet length do not contain CRC data length
>    net/hns3: fix Rx packet without CRC data
> 
>   drivers/net/hns3/hns3_ethdev.c        |   5 +
>   drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>   drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>   drivers/net/hns3/hns3_rxtx.h          |   3 +
>   drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>   drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>   drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>   lib/ethdev/rte_ethdev.h               |   6 ++
>   8 files changed, 124 insertions(+), 72 deletions(-)
> 

Re: [PATCH v3 0/3] bugfix about KEEP CRC offload

[Jie Hai]

Hi, all maintainers,

Kindly ping for review.

Thanks,
Jie Hai

On 2024/7/19 17:04, Jie Hai wrote:
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is
> still be stripped in rare scenarios. Some users of hns3 are
> already using this feature. So driver has to recaculate packet CRC.
> In addition, in the mbuf, the data that exceeds the length specified
> by pkt_len is invalid. Therefore, if the packet contains CRC data,
> pkt_len should contain the CRC data length. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> 
> This patch adds description for KEEP CRC offload.
> 
> Dengdui Huang (3):
>    ethdev: add description for KEEP CRC offload
>    net/hns3: fix packet length do not contain CRC data length
>    net/hns3: fix Rx packet without CRC data
> 
>   drivers/net/hns3/hns3_ethdev.c        |   5 +
>   drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>   drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>   drivers/net/hns3/hns3_rxtx.h          |   3 +
>   drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>   drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>   drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>   lib/ethdev/rte_ethdev.h               |   6 ++
>   8 files changed, 124 insertions(+), 72 deletions(-)
> 

Re: [PATCH v3 0/3] bugfix about KEEP CRC offload

[Jie Hai]

Hi, all maintainers,

Kindly ping for reviews.

Thanks,
Jie Hai

On 2024/7/19 17:04, Jie Hai wrote:
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is
> still be stripped in rare scenarios. Some users of hns3 are
> already using this feature. So driver has to recaculate packet CRC.
> In addition, in the mbuf, the data that exceeds the length specified
> by pkt_len is invalid. Therefore, if the packet contains CRC data,
> pkt_len should contain the CRC data length. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> 
> This patch adds description for KEEP CRC offload.
> 
> Dengdui Huang (3):
>    ethdev: add description for KEEP CRC offload
>    net/hns3: fix packet length do not contain CRC data length
>    net/hns3: fix Rx packet without CRC data
> 
>   drivers/net/hns3/hns3_ethdev.c        |   5 +
>   drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>   drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>   drivers/net/hns3/hns3_rxtx.h          |   3 +
>   drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>   drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>   drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>   lib/ethdev/rte_ethdev.h               |   6 ++
>   8 files changed, 124 insertions(+), 72 deletions(-)
> 

Re: [PATCH v3 0/3] bugfix about KEEP CRC offload

[Jie Hai]

Hi, stephen, ferruh, thomas,

This group of patches does not modify the API and
is a problem for hns3 driver.
Can the patches be applied in the current window?

Best regards,
Jie Hai

On 2024/7/19 17:04, Jie Hai wrote:
> For hns3 NIC, when KEEP_CRC offload is enabled, the CRC data is
> still be stripped in rare scenarios. Some users of hns3 are
> already using this feature. So driver has to recaculate packet CRC.
> In addition, in the mbuf, the data that exceeds the length specified
> by pkt_len is invalid. Therefore, if the packet contains CRC data,
> pkt_len should contain the CRC data length. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to pkt_len.
> 
> This patch adds description for KEEP CRC offload.
> 
> Dengdui Huang (3):
>    ethdev: add description for KEEP CRC offload
>    net/hns3: fix packet length do not contain CRC data length
>    net/hns3: fix Rx packet without CRC data
> 
>   drivers/net/hns3/hns3_ethdev.c        |   5 +
>   drivers/net/hns3/hns3_ethdev.h        |  23 +++++
>   drivers/net/hns3/hns3_rxtx.c          | 134 ++++++++++++++++----------
>   drivers/net/hns3/hns3_rxtx.h          |   3 +
>   drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
>   drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
>   drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
>   lib/ethdev/rte_ethdev.h               |   6 ++
>   8 files changed, 124 insertions(+), 72 deletions(-)
> 

[PATCH v4] net/hns3: fix Rx packet without CRC data

[Jie Hai]

From: Dengdui Huang <huangdengdui@huawei.com>

When KEEP_CRC offload is enabled, the CRC data is still stripped
in following cases:
1. For HIP08 network engine, the packet type is TCP and the length
   is less than or equal to 60B.
2. For HIP09 network engine, the packet type is IP and the length
   is less than or equal to 60B.

So driver has to recaculate packet CRC for this rare scenarios.

In addition, to avoid impacting performance, KEEP_CRC is not
supported when NEON or SVE algorithm is used.

Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Acked-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Jie Hai <haijie1@huawei.com>
---
 drivers/net/hns3/hns3_ethdev.c        |   5 ++
 drivers/net/hns3/hns3_ethdev.h        |  23 +++++
 drivers/net/hns3/hns3_rxtx.c          | 121 +++++++++++++++++++++-----
 drivers/net/hns3/hns3_rxtx.h          |   3 +
 drivers/net/hns3/hns3_rxtx_vec.c      |   3 +-
 drivers/net/hns3/hns3_rxtx_vec_neon.h |  19 ----
 drivers/net/hns3/hns3_rxtx_vec_sve.c  |   3 +-
 7 files changed, 132 insertions(+), 45 deletions(-)

diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c
index 72d1c30a7b2e..b3bd439d0dd5 100644
--- a/drivers/net/hns3/hns3_ethdev.c
+++ b/drivers/net/hns3/hns3_ethdev.c
@@ -2739,6 +2739,7 @@ hns3_get_capability(struct hns3_hw *hw)
 		hw->udp_cksum_mode = HNS3_SPECIAL_PORT_SW_CKSUM_MODE;
 		pf->support_multi_tc_pause = false;
 		hw->rx_dma_addr_align = HNS3_RX_DMA_ADDR_ALIGN_64;
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_TCP;
 		return 0;
 	}
 
@@ -2760,6 +2761,10 @@ hns3_get_capability(struct hns3_hw *hw)
 	hw->udp_cksum_mode = HNS3_SPECIAL_PORT_HW_CKSUM_MODE;
 	pf->support_multi_tc_pause = true;
 	hw->rx_dma_addr_align = HNS3_RX_DMA_ADDR_ALIGN_128;
+	if (hw->revision == PCI_REVISION_ID_HIP09_A)
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_IP;
+	else
+		hw->strip_crc_ptype = HNS3_STRIP_CRC_PTYPE_NONE;
 
 	return 0;
 }
diff --git a/drivers/net/hns3/hns3_ethdev.h b/drivers/net/hns3/hns3_ethdev.h
index 7824503bb89f..01d473fd2e66 100644
--- a/drivers/net/hns3/hns3_ethdev.h
+++ b/drivers/net/hns3/hns3_ethdev.h
@@ -54,6 +54,10 @@
 #define HNS3_SPECIAL_PORT_SW_CKSUM_MODE         0
 #define HNS3_SPECIAL_PORT_HW_CKSUM_MODE         1
 
+#define HNS3_STRIP_CRC_PTYPE_NONE         0
+#define HNS3_STRIP_CRC_PTYPE_TCP          1
+#define HNS3_STRIP_CRC_PTYPE_IP           2
+
 #define HNS3_UC_MACADDR_NUM		128
 #define HNS3_VF_UC_MACADDR_NUM		48
 #define HNS3_MC_MACADDR_NUM		128
@@ -655,6 +659,25 @@ struct hns3_hw {
 	 */
 	uint8_t udp_cksum_mode;
 
+	/*
+	 * When KEEP_CRC offload is enabled, the CRC data of some type packets
+	 * whose length is less than or equal to HNS3_KEEP_CRC_OK_MIN_PKT_LEN
+	 * is still be stripped on some network engine. So here has to use this
+	 * field to distinguish the difference between different network engines.
+	 * value range:
+	 *  - HNS3_STRIP_CRC_PTYPE_TCP
+	 *     This value for HIP08 network engine.
+	 *     Indicates that only the IP-TCP packet type is stripped.
+	 *
+	 *  - HNS3_STRIP_CRC_PTYPE_IP
+	 *     This value for HIP09 network engine.
+	 *     Indicates that all IP packet types are stripped.
+	 *
+	 *  - HNS3_STRIP_CRC_PTYPE_NONE
+	 *     Indicates that all packet types are not stripped.
+	 */
+	uint8_t strip_crc_ptype;
+
 	struct hns3_port_base_vlan_config port_base_vlan_cfg;
 
 	pthread_mutex_t flows_lock; /* rte_flow ops lock */
diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 03bbbc435fac..75fd4f55e73a 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -11,6 +11,7 @@
 #include <rte_io.h>
 #include <rte_net.h>
 #include <rte_malloc.h>
+#include <rte_net_crc.h>
 #if defined(RTE_ARCH_ARM64)
 #include <rte_cpuflags.h>
 #include <rte_vect.h>
@@ -1768,8 +1769,9 @@ hns3_rx_buf_len_calc(struct rte_mempool *mp, uint16_t *rx_buf_len)
 }
 
 static int
-hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
-				uint16_t nb_desc)
+hns3_rxq_conf_runtime_check(struct hns3_hw *hw,
+			    const struct rte_eth_rxconf *conf,
+			    uint16_t buf_size, uint16_t nb_desc)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
 	eth_rx_burst_t pkt_burst = dev->rx_pkt_burst;
@@ -1802,6 +1804,14 @@ hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
 			return -EINVAL;
 		}
 	}
+
+	if ((conf->offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC) &&
+	    pkt_burst != hns3_recv_pkts_simple &&
+	    pkt_burst != hns3_recv_scattered_pkts) {
+		hns3_err(hw, "KEEP_CRC offload is not supported in the current rx function.");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -1838,7 +1848,7 @@ hns3_rx_queue_conf_check(struct hns3_hw *hw, const struct rte_eth_rxconf *conf,
 	}
 
 	if (hw->data->dev_started) {
-		ret = hns3_rxq_conf_runtime_check(hw, *buf_size, nb_desc);
+		ret = hns3_rxq_conf_runtime_check(hw, conf, *buf_size, nb_desc);
 		if (ret) {
 			hns3_err(hw, "Rx queue runtime setup fail.");
 			return ret;
@@ -1959,6 +1969,8 @@ hns3_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t nb_desc,
 	else
 		rxq->crc_len = 0;
 
+	rxq->keep_crc_fail_ptype = hw->strip_crc_ptype;
+
 	rxq->bulk_mbuf_num = 0;
 
 	rte_spinlock_lock(&hw->lock);
@@ -2435,6 +2447,55 @@ hns3_rx_ptp_timestamp_handle(struct hns3_rx_queue *rxq, struct rte_mbuf *mbuf,
 	pf->rx_timestamp = timestamp;
 }
 
+static inline bool
+hns3_need_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+	uint32_t ptype = m->packet_type;
+
+	if (rxq->keep_crc_fail_ptype == HNS3_STRIP_CRC_PTYPE_NONE)
+		return false;
+
+	if (m->pkt_len > HNS3_KEEP_CRC_OK_MIN_PKT_LEN)
+		return false;
+
+	if (!(RTE_ETH_IS_IPV4_HDR(ptype) || RTE_ETH_IS_IPV6_HDR(ptype)))
+		return false;
+
+	if (rxq->keep_crc_fail_ptype == HNS3_STRIP_CRC_PTYPE_TCP)
+		return (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP;
+
+	return true;
+}
+
+/*
+ * The hns3 driver requires that mbuf size must be at least 512B.
+ * When CRC is stripped by hardware, the pkt_len must be less than
+ * or equal to 60B. Therefore, the space of the mbuf is enough
+ * to insert the CRC.
+ */
+static_assert(HNS3_KEEP_CRC_OK_MIN_PKT_LEN < HNS3_MIN_BD_BUF_SIZE,
+	      "buffer size too small to insert CRC");
+
+static inline void
+hns3_recalculate_crc(struct rte_mbuf *m)
+{
+	char *append_data;
+	uint32_t crc;
+
+	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
+			       m->data_len, RTE_NET_CRC32_ETH);
+
+	/*
+	 * After CRC is stripped by hardware, pkt_len and data_len do not
+	 * contain the CRC length. Therefore, after CRC data is appended
+	 * by PMD again.
+	 */
+	append_data = rte_pktmbuf_append(m, RTE_ETHER_CRC_LEN);
+
+	/* CRC data is binary data and does not care about the byte order. */
+	memcpy(append_data, &crc, RTE_ETHER_CRC_LEN);
+}
+
 uint16_t
 hns3_recv_pkts_simple(void *rx_queue,
 		      struct rte_mbuf **rx_pkts,
@@ -2505,8 +2566,7 @@ hns3_recv_pkts_simple(void *rx_queue,
 		rxdp->rx.bd_base_info = 0;
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
-		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
-				rxq->crc_len;
+		rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len));
 		rxm->data_len = rxm->pkt_len;
 		rxm->port = rxq->port_id;
 		rxm->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
@@ -2531,6 +2591,12 @@ hns3_recv_pkts_simple(void *rx_queue,
 		if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
 
+		if (unlikely(rxq->crc_len > 0) &&
+		    hns3_need_recalculate_crc(rxq, rxm))
+			hns3_recalculate_crc(rxm);
+		rxm->pkt_len -= rxq->crc_len;
+		rxm->data_len -= rxq->crc_len;
+
 		hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
 
 		/* Increment bytes counter  */
@@ -2697,10 +2763,10 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
 		rxm->data_len = rte_le_to_cpu_16(rxd.rx.size);
+		rxm->next = NULL;
 
 		if (!(bd_base_info & BIT(HNS3_RXD_FE_B))) {
 			last_seg = rxm;
-			rxm->next = NULL;
 			continue;
 		}
 
@@ -2715,23 +2781,6 @@ hns3_recv_scattered_pkts(void *rx_queue,
 		 */
 		first_seg->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
 
-		/*
-		 * This is the last buffer of the received packet. If the CRC
-		 * is not stripped by the hardware:
-		 *  - Subtract the CRC length from the total packet length.
-		 *  - If the last buffer only contains the whole CRC or a part
-		 *  of it, free the mbuf associated to the last buffer. If part
-		 *  of the CRC is also contained in the previous mbuf, subtract
-		 *  the length of that CRC part from the data length of the
-		 *  previous mbuf.
-		 */
-		rxm->next = NULL;
-		if (unlikely(rxq->crc_len > 0)) {
-			first_seg->pkt_len -= rxq->crc_len;
-			recalculate_data_len(first_seg, last_seg, rxm, rxq,
-				rxm->data_len);
-		}
-
 		first_seg->port = rxq->port_id;
 		first_seg->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
 		first_seg->ol_flags |= RTE_MBUF_F_RX_RSS_HASH;
@@ -2760,6 +2809,32 @@ hns3_recv_scattered_pkts(void *rx_queue,
 
 		if (first_seg->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
 			rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
+		/*
+		 * This is the last buffer of the received packet. If the CRC
+		 * is not stripped by the hardware:
+		 *  - Subtract the CRC length from the total packet length.
+		 *  - If the last buffer only contains the whole CRC or a part
+		 *  of it, free the mbuf associated to the last buffer. If part
+		 *  of the CRC is also contained in the previous mbuf, subtract
+		 *  the length of that CRC part from the data length of the
+		 *  previous mbuf.
+		 *
+		 * In addition, the CRC is still stripped for a kind of packets
+		 * in hns3 NIC:
+		 * 1. All IP-TCP packet whose the length is less than and equal
+		 *    to 60 Byte (no CRC) on HIP08 network engine.
+		 * 2. All IP packet whose the length is less than and equal to
+		 *    60 Byte (no CRC) on HIP09 network engine.
+		 * In this case, the PMD calculates the CRC and appends it to
+		 * mbuf.
+		 */
+		if (unlikely(rxq->crc_len > 0)) {
+			if (hns3_need_recalculate_crc(rxq, first_seg))
+				hns3_recalculate_crc(first_seg);
+			first_seg->pkt_len -= rxq->crc_len;
+			recalculate_data_len(first_seg, last_seg, rxm, rxq,
+				rxm->data_len);
+		}
 
 		hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
 
diff --git a/drivers/net/hns3/hns3_rxtx.h b/drivers/net/hns3/hns3_rxtx.h
index e975cd151a7e..0eb9796fe053 100644
--- a/drivers/net/hns3/hns3_rxtx.h
+++ b/drivers/net/hns3/hns3_rxtx.h
@@ -178,6 +178,8 @@
 		(HNS3_TXD_VLD_CMD | HNS3_TXD_FE_CMD | HNS3_TXD_DEFAULT_BDTYPE)
 #define HNS3_TXD_SEND_SIZE_SHIFT	16
 
+#define HNS3_KEEP_CRC_OK_MIN_PKT_LEN	60
+
 enum hns3_pkt_l2t_type {
 	HNS3_L2_TYPE_UNICAST,
 	HNS3_L2_TYPE_MULTICAST,
@@ -341,6 +343,7 @@ struct hns3_rx_queue {
 	 */
 	uint8_t pvid_sw_discard_en:1;
 	uint8_t ptype_en:1;          /* indicate if the ptype field enabled */
+	uint8_t keep_crc_fail_ptype:2;
 
 	uint64_t mbuf_initializer; /* value to init mbufs used with vector rx */
 	/* offset_table: used for vector, to solve execute re-order problem */
diff --git a/drivers/net/hns3/hns3_rxtx_vec.c b/drivers/net/hns3/hns3_rxtx_vec.c
index 9708ec614e02..bf37ce51b1ad 100644
--- a/drivers/net/hns3/hns3_rxtx_vec.c
+++ b/drivers/net/hns3/hns3_rxtx_vec.c
@@ -185,7 +185,8 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
 	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO |
 				 RTE_ETH_RX_OFFLOAD_VLAN |
-				 RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+				 RTE_ETH_RX_OFFLOAD_TIMESTAMP |
+				 RTE_ETH_RX_OFFLOAD_KEEP_CRC;
 
 	if (dev->data->scattered_rx)
 		return -ENOTSUP;
diff --git a/drivers/net/hns3/hns3_rxtx_vec_neon.h b/drivers/net/hns3/hns3_rxtx_vec_neon.h
index bbb5478015dd..86063a8def12 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_neon.h
+++ b/drivers/net/hns3/hns3_rxtx_vec_neon.h
@@ -150,14 +150,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		8, 9, 10, 11,	         /* rx.rss_hash to rte_mbuf.hash.rss */
 	};
 
-	uint16x8_t crc_adjust = {
-		0, 0,         /* ignore pkt_type field */
-		rxq->crc_len, /* sub crc on pkt_len */
-		0,            /* ignore high-16bits of pkt_len */
-		rxq->crc_len, /* sub crc on data_len */
-		0, 0, 0,      /* ignore non-length fields */
-	};
-
 	/* compile-time verifies the shuffle mask */
 	RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, pkt_len) !=
 			 offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4);
@@ -173,7 +165,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		uint8x16_t pkt_mb1, pkt_mb2, pkt_mb3, pkt_mb4;
 		uint64x2_t mbp1, mbp2;
 		uint16x4_t bd_vld = {0};
-		uint16x8_t tmp;
 		uint64_t stat;
 
 		/* calc how many bd valid */
@@ -227,16 +218,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
 		pkt_mb3 = vqtbl2q_u8(pkt_mbuf3, shuf_desc_fields_msk);
 		pkt_mb4 = vqtbl2q_u8(pkt_mbuf4, shuf_desc_fields_msk);
 
-		/* 4 packets remove crc */
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb1), crc_adjust);
-		pkt_mb1 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb2), crc_adjust);
-		pkt_mb2 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb3), crc_adjust);
-		pkt_mb3 = vreinterpretq_u8_u16(tmp);
-		tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb4), crc_adjust);
-		pkt_mb4 = vreinterpretq_u8_u16(tmp);
-
 		/* save packet info to rx_pkts mbuf */
 		vst1q_u8((void *)&sw_ring[pos + 0].mbuf->rx_descriptor_fields1,
 			 pkt_mb1);
diff --git a/drivers/net/hns3/hns3_rxtx_vec_sve.c b/drivers/net/hns3/hns3_rxtx_vec_sve.c
index 8aa4448558cf..67c87f570e8a 100644
--- a/drivers/net/hns3/hns3_rxtx_vec_sve.c
+++ b/drivers/net/hns3/hns3_rxtx_vec_sve.c
@@ -36,8 +36,7 @@ hns3_desc_parse_field_sve(struct hns3_rx_queue *rxq,
 		/* init rte_mbuf.rearm_data last 64-bit */
 		rx_pkts[i]->ol_flags = RTE_MBUF_F_RX_RSS_HASH;
 		rx_pkts[i]->hash.rss = rxdp[i].rx.rss_hash;
-		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len) -
-					rxq->crc_len;
+		rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len);
 		rx_pkts[i]->data_len = rx_pkts[i]->pkt_len;
 
 		l234_info = rxdp[i].rx.l234_info;
-- 
2.33.0