[PATCH v2 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[PATCH v2 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[Maxime Coquelin]

This patch reverts commit 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding"),
as the checksum forwarding is expected to only perform
checksum and not also overwrites the source and destination
MAC addresses.

Doing so, we can test checksum offloading with real traffic
without breaking broadcast packets.

Fixes: 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Acked-by: Chenbo Xia <chenbo.xia@intel.com>
---
 app/test-pmd/csumonly.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 7df201e047..1a3fd9ce8a 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -916,10 +916,6 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		 * and inner headers */
 
 		eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
-		rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
-				&eth_hdr->dst_addr);
-		rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
-				&eth_hdr->src_addr);
 		parse_ethernet(eth_hdr, &info);
 		l3_hdr = (char *)eth_hdr + info.l2_len;
 
-- 
2.35.3

[PATCH v2 2/6] vhost: fix missing enqueue pseudo-header calculation

[Maxime Coquelin]

The Virtio specification requires that in case of checksum
offloading, the pseudo-header checksum must be set in the
L4 header.

When received from another Vhost-user port, the packet
checksum might already contain the pseudo-header checksum
but we have no way to know it. So we have no other choice
than doing the pseudo-header checksum systematically.

This patch handles this using the rte_net_intel_cksum_prepare()
helper.

Fixes: 859b480d5afd ("vhost: add guest offload setting")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
 lib/vhost/virtio_net.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 68a26eb17d..ce22e3ac79 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -596,6 +596,16 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
 		csum_l4 |= RTE_MBUF_F_TX_TCP_CKSUM;
 
 	if (csum_l4) {
+		/*
+		 * Pseudo-header checksum must be set as per Virtio spec.
+		 *
+		 * Note: We don't propagate rte_net_intel_cksum_prepare()
+		 * errors, as it would have an impact on performance, and an
+		 * error would mean the packet is dropped by the guest instead
+		 * of being dropped here.
+		 */
+		rte_net_intel_cksum_prepare(m_buf);
+
 		net_hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
 		net_hdr->csum_start = m_buf->l2_len + m_buf->l3_len;
 
-- 
2.35.3

Re: [PATCH v2 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[Singh, Aman Deep]

Hi Maxime,

On 6/8/2022 6:19 PM, Maxime Coquelin wrote:
> This patch reverts commit 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding"),
> as the checksum forwarding is expected to only perform
> checksum and not also overwrites the source and destination
> MAC addresses.
>
> Doing so, we can test checksum offloading with real traffic
> without breaking broadcast packets.
>
> Fixes: 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Acked-by: Chenbo Xia <chenbo.xia@intel.com>

Acked-by: Aman Singh<aman.deep.singh@intel.com>

> ---
>   app/test-pmd/csumonly.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 7df201e047..1a3fd9ce8a 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -916,10 +916,6 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>   		 * and inner headers */
>   
>   		eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
> -		rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
> -				&eth_hdr->dst_addr);
> -		rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
> -				&eth_hdr->src_addr);
>   		parse_ethernet(eth_hdr, &info);
>   		l3_hdr = (char *)eth_hdr + info.l2_len;
>   

LGTM, In principle csum mode should not modify the mac addresses.
This code has been there from start, so might break some TC's.

Re: [PATCH v2 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[Maxime Coquelin]

On 6/13/22 14:24, Singh, Aman Deep wrote:
> Hi Maxime,
> 
> On 6/8/2022 6:19 PM, Maxime Coquelin wrote:
>> This patch reverts commit 10f4620f02e1 ("app/testpmd: modify mac in 
>> csum forwarding"),
>> as the checksum forwarding is expected to only perform
>> checksum and not also overwrites the source and destination
>> MAC addresses.
>>
>> Doing so, we can test checksum offloading with real traffic
>> without breaking broadcast packets.
>>
>> Fixes: 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Acked-by: Chenbo Xia <chenbo.xia@intel.com>
> 
> Acked-by: Aman Singh<aman.deep.singh@intel.com>
> 
>> ---
>>   app/test-pmd/csumonly.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>> index 7df201e047..1a3fd9ce8a 100644
>> --- a/app/test-pmd/csumonly.c
>> +++ b/app/test-pmd/csumonly.c
>> @@ -916,10 +916,6 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>>            * and inner headers */
>>           eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
>> -        rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
>> -                &eth_hdr->dst_addr);
>> -        rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
>> -                &eth_hdr->src_addr);
>>           parse_ethernet(eth_hdr, &info);
>>           l3_hdr = (char *)eth_hdr + info.l2_len;
> 
> LGTM, In principle csum mode should not modify the mac addresses.
> This code has been there from start, so might break some TC's.
> 
> 

Agree, some tests will need to be adapted.
David showed me some tests in DTS were removing the MAC rewriting.

Thanks,
Maxime