[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Yuanhan Liu]

On Wed, Oct 05, 2016 at 03:27:47PM +0200, Maxime Coquelin wrote:
> >/* Update offload features */
> >-   if (virtio_rx_offload(rxm, hdr) < 0) {
> >+   if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
> s/VIRTIO_NET_F_GUEST_CSUM/(1u << VIRTIO_NET_F_GUEST_CSUM)/

There is a helper function for that: vtpci_with_feature.

--yliu

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Olivier MATZ]

On 10/12/2016 03:02 PM, Yuanhan Liu wrote:
> On Wed, Oct 05, 2016 at 03:27:47PM +0200, Maxime Coquelin wrote:
>>> /* Update offload features */
>>> -   if (virtio_rx_offload(rxm, hdr) < 0) {
>>> +   if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
>> s/VIRTIO_NET_F_GUEST_CSUM/(1u << VIRTIO_NET_F_GUEST_CSUM)/
>
> There is a helper function for that: vtpci_with_feature.

Ok, will use it.

Thanks,
Olivier

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Olivier MATZ]

On 10/11/2016 04:36 PM, Maxime Coquelin wrote:
>
>
> On 10/11/2016 04:29 PM, Olivier MATZ wrote:
>>
>>
>> On 10/11/2016 04:04 PM, Maxime Coquelin wrote:
 +/* Optionally fill offload information in structure */
 +static int
 +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 +{
 +struct rte_net_hdr_lens hdr_lens;
 +uint32_t hdrlen, ptype;
 +int l4_supported = 0;
 +
 +/* nothing to do */
 +if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
 +return 0;
 +
 +m->ol_flags |= PKT_RX_IP_CKSUM_UNKNOWN;
 +
 +ptype = rte_net_get_ptype(m, _lens, RTE_PTYPE_ALL_MASK);
 +m->packet_type = ptype;
 +if ((ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP ||
 +(ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_UDP ||
 +(ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_SCTP)
 +l4_supported = 1;
 +
 +if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
 +hdrlen = hdr_lens.l2_len + hdr_lens.l3_len + hdr_lens.l4_len;
 +if (hdr->csum_start <= hdrlen && l4_supported) {
 +m->ol_flags |= PKT_RX_L4_CKSUM_NONE;
 +} else {
 +/* Unknown proto or tunnel, do sw cksum. We can assume
 + * the cksum field is in the first segment since the
 + * buffers we provided to the host are large enough.
 + * In case of SCTP, this will be wrong since it's a CRC
 + * but there's nothing we can do.
 + */
 +uint16_t csum, off;
 +
 +csum = rte_raw_cksum_mbuf(m, hdr->csum_start,
 +rte_pktmbuf_pkt_len(m) - hdr->csum_start);
 +if (csum != 0x)
>>> Why don't we do the 1-complement if 0x?
>>
>> This was modified after a comment from Xiao.
>>
>> In checksum arithmetic (ones' complement), there are 2 equivalent ways
>> to say the checksum is 0: 0x (0-), and 0x (0+).
>> Some protocols like UDP use this to differentiate between 0x (packet
>> checksum is 0) and 0x (packet checksum is not calculated).
>>
>> Here, we want to avoid to set a checksum to 0, in case it would mean no
>> checksum for UDP packets. Instead, it is set to 0x, which is also a
>> valid checksum for this packet.
>
> Ha ok, I wasn't aware of this.
> Thanks for the explanation!
>
> Maybe not a big deal, but we could add likely around the test?

Yep, good idea.

Thanks!
Olivier

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Maxime Coquelin]

On 10/11/2016 04:29 PM, Olivier MATZ wrote:
>
>
> On 10/11/2016 04:04 PM, Maxime Coquelin wrote:
>>> +/* Optionally fill offload information in structure */
>>> +static int
>>> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>>> +{
>>> +struct rte_net_hdr_lens hdr_lens;
>>> +uint32_t hdrlen, ptype;
>>> +int l4_supported = 0;
>>> +
>>> +/* nothing to do */
>>> +if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
>>> +return 0;
>>> +
>>> +m->ol_flags |= PKT_RX_IP_CKSUM_UNKNOWN;
>>> +
>>> +ptype = rte_net_get_ptype(m, _lens, RTE_PTYPE_ALL_MASK);
>>> +m->packet_type = ptype;
>>> +if ((ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP ||
>>> +(ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_UDP ||
>>> +(ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_SCTP)
>>> +l4_supported = 1;
>>> +
>>> +if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
>>> +hdrlen = hdr_lens.l2_len + hdr_lens.l3_len + hdr_lens.l4_len;
>>> +if (hdr->csum_start <= hdrlen && l4_supported) {
>>> +m->ol_flags |= PKT_RX_L4_CKSUM_NONE;
>>> +} else {
>>> +/* Unknown proto or tunnel, do sw cksum. We can assume
>>> + * the cksum field is in the first segment since the
>>> + * buffers we provided to the host are large enough.
>>> + * In case of SCTP, this will be wrong since it's a CRC
>>> + * but there's nothing we can do.
>>> + */
>>> +uint16_t csum, off;
>>> +
>>> +csum = rte_raw_cksum_mbuf(m, hdr->csum_start,
>>> +rte_pktmbuf_pkt_len(m) - hdr->csum_start);
>>> +if (csum != 0x)
>> Why don't we do the 1-complement if 0x?
>
> This was modified after a comment from Xiao.
>
> In checksum arithmetic (ones' complement), there are 2 equivalent ways
> to say the checksum is 0: 0x (0-), and 0x (0+).
> Some protocols like UDP use this to differentiate between 0x (packet
> checksum is 0) and 0x (packet checksum is not calculated).
>
> Here, we want to avoid to set a checksum to 0, in case it would mean no
> checksum for UDP packets. Instead, it is set to 0x, which is also a
> valid checksum for this packet.

Ha ok, I wasn't aware of this.
Thanks for the explanation!

Maybe not a big deal, but we could add likely around the test?

Maxime
>
> Regards,
> Olivier

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Olivier MATZ]

On 10/11/2016 04:04 PM, Maxime Coquelin wrote:
>> +/* Optionally fill offload information in structure */
>> +static int
>> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>> +{
>> +struct rte_net_hdr_lens hdr_lens;
>> +uint32_t hdrlen, ptype;
>> +int l4_supported = 0;
>> +
>> +/* nothing to do */
>> +if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
>> +return 0;
>> +
>> +m->ol_flags |= PKT_RX_IP_CKSUM_UNKNOWN;
>> +
>> +ptype = rte_net_get_ptype(m, _lens, RTE_PTYPE_ALL_MASK);
>> +m->packet_type = ptype;
>> +if ((ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP ||
>> +(ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_UDP ||
>> +(ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_SCTP)
>> +l4_supported = 1;
>> +
>> +if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
>> +hdrlen = hdr_lens.l2_len + hdr_lens.l3_len + hdr_lens.l4_len;
>> +if (hdr->csum_start <= hdrlen && l4_supported) {
>> +m->ol_flags |= PKT_RX_L4_CKSUM_NONE;
>> +} else {
>> +/* Unknown proto or tunnel, do sw cksum. We can assume
>> + * the cksum field is in the first segment since the
>> + * buffers we provided to the host are large enough.
>> + * In case of SCTP, this will be wrong since it's a CRC
>> + * but there's nothing we can do.
>> + */
>> +uint16_t csum, off;
>> +
>> +csum = rte_raw_cksum_mbuf(m, hdr->csum_start,
>> +rte_pktmbuf_pkt_len(m) - hdr->csum_start);
>> +if (csum != 0x)
> Why don't we do the 1-complement if 0x?

This was modified after a comment from Xiao.

In checksum arithmetic (ones' complement), there are 2 equivalent ways 
to say the checksum is 0: 0x (0-), and 0x (0+).
Some protocols like UDP use this to differentiate between 0x (packet 
checksum is 0) and 0x (packet checksum is not calculated).

Here, we want to avoid to set a checksum to 0, in case it would mean no 
checksum for UDP packets. Instead, it is set to 0x, which is also a 
valid checksum for this packet.

Regards,
Olivier

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Maxime Coquelin]

On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Signed-off-by: Olivier Matz 
> ---
>  drivers/net/virtio/virtio_ethdev.c | 14 
>  drivers/net/virtio/virtio_ethdev.h |  2 +-
>  drivers/net/virtio/virtio_rxtx.c   | 69 
> ++
>  drivers/net/virtio/virtqueue.h |  1 +
>  4 files changed, 78 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c 
> b/drivers/net/virtio/virtio_ethdev.c
> index fa56032..43cb096 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1262,7 +1262,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>   eth_dev->data->dev_flags = dev_flags;
>
>   /* reset device and negotiate default features */
> - ret = virtio_init_device(eth_dev, VIRTIO_PMD_GUEST_FEATURES);
> + ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
>   if (ret < 0)
>   return ret;
>
> @@ -1351,13 +1351,10 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>   int ret;
>
>   PMD_INIT_LOG(DEBUG, "configure");
> + req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
> + if (rxmode->hw_ip_checksum)
> + req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
>
> - if (rxmode->hw_ip_checksum) {
> - PMD_DRV_LOG(ERR, "HW IP checksum not supported");
> - return -EINVAL;
> - }
> -
> - req_features = VIRTIO_PMD_GUEST_FEATURES;
>   /* if request features changed, reinit the device */
>   if (req_features != hw->req_guest_features) {
>   ret = virtio_init_device(dev, req_features);
> @@ -1578,6 +1575,9 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct 
> rte_eth_dev_info *dev_info)
>   dev_info->default_txconf = (struct rte_eth_txconf) {
>   .txq_flags = ETH_TXQ_FLAGS_NOOFFLOADS
>   };
> + dev_info->rx_offload_capa =
> + DEV_RX_OFFLOAD_TCP_CKSUM |
> + DEV_RX_OFFLOAD_UDP_CKSUM;
>  }
>
>  /*
> diff --git a/drivers/net/virtio/virtio_ethdev.h 
> b/drivers/net/virtio/virtio_ethdev.h
> index 5d5e788..2fc9218 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -54,7 +54,7 @@
>  #define VIRTIO_MAX_RX_PKTLEN  9728
>
>  /* Features desired/implemented by this driver. */
> -#define VIRTIO_PMD_GUEST_FEATURES\
> +#define VIRTIO_PMD_DEFAULT_GUEST_FEATURES\
>   (1u << VIRTIO_NET_F_MAC   | \
>1u << VIRTIO_NET_F_STATUS| \
>1u << VIRTIO_NET_F_MQ| \
> diff --git a/drivers/net/virtio/virtio_rxtx.c 
> b/drivers/net/virtio/virtio_rxtx.c
> index 724517e..eda678a 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -50,6 +50,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "virtio_logs.h"
>  #include "virtio_ethdev.h"
> @@ -627,6 +628,56 @@ virtio_update_packet_stats(struct virtnet_stats *stats, 
> struct rte_mbuf *mbuf)
>   }
>  }
>
> +/* Optionally fill offload information in structure */
> +static int
> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
> +{
> + struct rte_net_hdr_lens hdr_lens;
> + uint32_t hdrlen, ptype;
> + int l4_supported = 0;
> +
> + /* nothing to do */
> + if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
> + return 0;
> +
> + m->ol_flags |= PKT_RX_IP_CKSUM_UNKNOWN;
> +
> + ptype = rte_net_get_ptype(m, _lens, RTE_PTYPE_ALL_MASK);
> + m->packet_type = ptype;
> + if ((ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP ||
> + (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_UDP ||
> + (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_SCTP)
> + l4_supported = 1;
> +
> + if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> + hdrlen = hdr_lens.l2_len + hdr_lens.l3_len + hdr_lens.l4_len;
> + if (hdr->csum_start <= hdrlen && l4_supported) {
> + m->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> + } else {
> + /* Unknown proto or tunnel, do sw cksum. We can assume
> +  * the cksum field is in the first segment since the
> +  * buffers we provided to the host are large enough.
> +  * In case of SCTP, this will be wrong since it's a CRC
> +  * but there's nothing we can do.
> +  */
> + uint16_t csum, off;
> +
> + csum = rte_raw_cksum_mbuf(m, hdr->csum_start,
> + rte_pktmbuf_pkt_len(m) - hdr->csum_start);
> + if (csum != 0x)
Why don't we do the 1-complement if 0x?
> + csum = ~csum;
> + off = hdr->csum_offset + hdr->csum_start;
> + if (rte_pktmbuf_data_len(m) >= off + 1)
> + *rte_pktmbuf_mtod_offset(m, uint16_t *,
> +  

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Olivier Matz]

On 10/05/2016 03:27 PM, Maxime Coquelin wrote:
>> @@ -903,7 +905,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
>> **rx_pkts, uint16_t nb_pkts)
>> rte_vlan_strip(rxm);
>>
>> /* Update offload features */
>> -   if (virtio_rx_offload(rxm, hdr) < 0) {
>> +   if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
> s/VIRTIO_NET_F_GUEST_CSUM/(1u << VIRTIO_NET_F_GUEST_CSUM)/

oooh good catch :)

> And don't forget to update the test for LRO patch.

yep

> Except this, it sounds good.

Thanks, I'll send a v3 soon.

Olivier

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Maxime Coquelin]

Hi Olivier,

On 10/05/2016 01:56 PM, Olivier Matz wrote:
> Hi Maxime,
>
> On 10/03/2016 02:51 PM, Maxime Coquelin wrote:
>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>> @@ -50,6 +50,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include "virtio_logs.h"
>>>  #include "virtio_ethdev.h"
>>> @@ -627,6 +628,56 @@ virtio_update_packet_stats(struct virtnet_stats
>>> *stats, struct rte_mbuf *mbuf)
>>>  }
>>>  }
>>>
>>> +/* Optionally fill offload information in structure */
>>> +static int
>>> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>>> +{
>>> +struct rte_net_hdr_lens hdr_lens;
>>> +uint32_t hdrlen, ptype;
>>> +int l4_supported = 0;
>>> +
>>> +/* nothing to do */
>>> +if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
>>> +return 0;
>> Maybe we could first check whether offload features were negotiated?
>> Doing this, we could return before accessing the header and so avoid a
>> cache miss.
>
> Yes, doing this would avoid reading the virtio header when the rx
> function is virtio_recv_pkts(). When using virtio_recv_mergeable_pkts(),
> it won't have a big impact since we already need to read hdr->num_buffers.
Right, it matters only for the non-mergeable buffers case.

>
>
> I plan to do something like this in both recv functions:
>
> @@ -854,6 +854,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts, uint16_t nb_pkts)
> int error;
> uint32_t i, nb_enqueued;
> uint32_t hdr_size;
> +   uint64_t features;
> struct virtio_net_hdr *hdr;
>
> nb_used = VIRTQUEUE_NUSED(vq);
> @@ -872,6 +873,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts, uint16_t nb_pkts)
> nb_rx = 0;
> nb_enqueued = 0;
> hdr_size = hw->vtnet_hdr_size;
> +   features = hw->guest_features;
>
> for (i = 0; i < num ; i++) {
> rxm = rcv_pkts[i];
> @@ -903,7 +905,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts, uint16_t nb_pkts)
> rte_vlan_strip(rxm);
>
> /* Update offload features */
> -   if (virtio_rx_offload(rxm, hdr) < 0) {
> +   if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
s/VIRTIO_NET_F_GUEST_CSUM/(1u << VIRTIO_NET_F_GUEST_CSUM)/
And don't forget to update the test for LRO patch.
Except this, it sounds good.

Thanks,
Maxime
> +   virtio_rx_offload(rxm, hdr) < 0) {
> virtio_discard_rxbuf(vq, rxm);
> rxvq->stats.errors++;
> continue;
>
> Thank you for the feedback.
> Olivier
>

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Olivier Matz]

Hi Maxime,

On 10/03/2016 02:51 PM, Maxime Coquelin wrote:
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -50,6 +50,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "virtio_logs.h"
>>  #include "virtio_ethdev.h"
>> @@ -627,6 +628,56 @@ virtio_update_packet_stats(struct virtnet_stats
>> *stats, struct rte_mbuf *mbuf)
>>  }
>>  }
>>
>> +/* Optionally fill offload information in structure */
>> +static int
>> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>> +{
>> +struct rte_net_hdr_lens hdr_lens;
>> +uint32_t hdrlen, ptype;
>> +int l4_supported = 0;
>> +
>> +/* nothing to do */
>> +if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
>> +return 0;
> Maybe we could first check whether offload features were negotiated?
> Doing this, we could return before accessing the header and so avoid a
> cache miss.

Yes, doing this would avoid reading the virtio header when the rx
function is virtio_recv_pkts(). When using virtio_recv_mergeable_pkts(),
it won't have a big impact since we already need to read hdr->num_buffers.


I plan to do something like this in both recv functions:

@@ -854,6 +854,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
**rx_pkts, uint16_t nb_pkts)
int error;
uint32_t i, nb_enqueued;
uint32_t hdr_size;
+   uint64_t features;
struct virtio_net_hdr *hdr;

nb_used = VIRTQUEUE_NUSED(vq);
@@ -872,6 +873,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
**rx_pkts, uint16_t nb_pkts)
nb_rx = 0;
nb_enqueued = 0;
hdr_size = hw->vtnet_hdr_size;
+   features = hw->guest_features;

for (i = 0; i < num ; i++) {
rxm = rcv_pkts[i];
@@ -903,7 +905,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf
**rx_pkts, uint16_t nb_pkts)
rte_vlan_strip(rxm);

/* Update offload features */
-   if (virtio_rx_offload(rxm, hdr) < 0) {
+   if ((features & VIRTIO_NET_F_GUEST_CSUM) &&
+   virtio_rx_offload(rxm, hdr) < 0) {
virtio_discard_rxbuf(vq, rxm);
rxvq->stats.errors++;
continue;


Thank you for the feedback.
Olivier

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Maxime Coquelin]

Hi Olivier,


On 10/03/2016 11:00 AM, Olivier Matz wrote:
> Signed-off-by: Olivier Matz 
> ---
>  drivers/net/virtio/virtio_ethdev.c | 14 
>  drivers/net/virtio/virtio_ethdev.h |  2 +-
>  drivers/net/virtio/virtio_rxtx.c   | 69 
> ++
>  drivers/net/virtio/virtqueue.h |  1 +
>  4 files changed, 78 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c 
> b/drivers/net/virtio/virtio_ethdev.c
> index fa56032..43cb096 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1262,7 +1262,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>   eth_dev->data->dev_flags = dev_flags;
>
>   /* reset device and negotiate default features */
> - ret = virtio_init_device(eth_dev, VIRTIO_PMD_GUEST_FEATURES);
> + ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
>   if (ret < 0)
>   return ret;
>
> @@ -1351,13 +1351,10 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>   int ret;
>
>   PMD_INIT_LOG(DEBUG, "configure");
> + req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
> + if (rxmode->hw_ip_checksum)
> + req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);
>
> - if (rxmode->hw_ip_checksum) {
> - PMD_DRV_LOG(ERR, "HW IP checksum not supported");
> - return -EINVAL;
> - }
> -
> - req_features = VIRTIO_PMD_GUEST_FEATURES;
>   /* if request features changed, reinit the device */
>   if (req_features != hw->req_guest_features) {
>   ret = virtio_init_device(dev, req_features);
> @@ -1578,6 +1575,9 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct 
> rte_eth_dev_info *dev_info)
>   dev_info->default_txconf = (struct rte_eth_txconf) {
>   .txq_flags = ETH_TXQ_FLAGS_NOOFFLOADS
>   };
> + dev_info->rx_offload_capa =
> + DEV_RX_OFFLOAD_TCP_CKSUM |
> + DEV_RX_OFFLOAD_UDP_CKSUM;
>  }
>
>  /*
> diff --git a/drivers/net/virtio/virtio_ethdev.h 
> b/drivers/net/virtio/virtio_ethdev.h
> index 5d5e788..2fc9218 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -54,7 +54,7 @@
>  #define VIRTIO_MAX_RX_PKTLEN  9728
>
>  /* Features desired/implemented by this driver. */
> -#define VIRTIO_PMD_GUEST_FEATURES\
> +#define VIRTIO_PMD_DEFAULT_GUEST_FEATURES\
>   (1u << VIRTIO_NET_F_MAC   | \
>1u << VIRTIO_NET_F_STATUS| \
>1u << VIRTIO_NET_F_MQ| \
> diff --git a/drivers/net/virtio/virtio_rxtx.c 
> b/drivers/net/virtio/virtio_rxtx.c
> index 724517e..eda678a 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -50,6 +50,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "virtio_logs.h"
>  #include "virtio_ethdev.h"
> @@ -627,6 +628,56 @@ virtio_update_packet_stats(struct virtnet_stats *stats, 
> struct rte_mbuf *mbuf)
>   }
>  }
>
> +/* Optionally fill offload information in structure */
> +static int
> +virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
> +{
> + struct rte_net_hdr_lens hdr_lens;
> + uint32_t hdrlen, ptype;
> + int l4_supported = 0;
> +
> + /* nothing to do */
> + if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
> + return 0;
Maybe we could first check whether offload features were negotiated?
Doing this, we could return before accessing the header and so avoid a
cache miss.

Maxime

[dpdk-dev] [PATCH v2 09/12] virtio: add Rx checksum offload support

[Olivier Matz]

Signed-off-by: Olivier Matz 
---
 drivers/net/virtio/virtio_ethdev.c | 14 
 drivers/net/virtio/virtio_ethdev.h |  2 +-
 drivers/net/virtio/virtio_rxtx.c   | 69 ++
 drivers/net/virtio/virtqueue.h |  1 +
 4 files changed, 78 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c 
b/drivers/net/virtio/virtio_ethdev.c
index fa56032..43cb096 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1262,7 +1262,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
eth_dev->data->dev_flags = dev_flags;

/* reset device and negotiate default features */
-   ret = virtio_init_device(eth_dev, VIRTIO_PMD_GUEST_FEATURES);
+   ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
if (ret < 0)
return ret;

@@ -1351,13 +1351,10 @@ virtio_dev_configure(struct rte_eth_dev *dev)
int ret;

PMD_INIT_LOG(DEBUG, "configure");
+   req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
+   if (rxmode->hw_ip_checksum)
+   req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM);

-   if (rxmode->hw_ip_checksum) {
-   PMD_DRV_LOG(ERR, "HW IP checksum not supported");
-   return -EINVAL;
-   }
-
-   req_features = VIRTIO_PMD_GUEST_FEATURES;
/* if request features changed, reinit the device */
if (req_features != hw->req_guest_features) {
ret = virtio_init_device(dev, req_features);
@@ -1578,6 +1575,9 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct 
rte_eth_dev_info *dev_info)
dev_info->default_txconf = (struct rte_eth_txconf) {
.txq_flags = ETH_TXQ_FLAGS_NOOFFLOADS
};
+   dev_info->rx_offload_capa =
+   DEV_RX_OFFLOAD_TCP_CKSUM |
+   DEV_RX_OFFLOAD_UDP_CKSUM;
 }

 /*
diff --git a/drivers/net/virtio/virtio_ethdev.h 
b/drivers/net/virtio/virtio_ethdev.h
index 5d5e788..2fc9218 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -54,7 +54,7 @@
 #define VIRTIO_MAX_RX_PKTLEN  9728

 /* Features desired/implemented by this driver. */
-#define VIRTIO_PMD_GUEST_FEATURES  \
+#define VIRTIO_PMD_DEFAULT_GUEST_FEATURES  \
(1u << VIRTIO_NET_F_MAC   | \
 1u << VIRTIO_NET_F_STATUS| \
 1u << VIRTIO_NET_F_MQ| \
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 724517e..eda678a 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -50,6 +50,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "virtio_logs.h"
 #include "virtio_ethdev.h"
@@ -627,6 +628,56 @@ virtio_update_packet_stats(struct virtnet_stats *stats, 
struct rte_mbuf *mbuf)
}
 }

+/* Optionally fill offload information in structure */
+static int
+virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
+{
+   struct rte_net_hdr_lens hdr_lens;
+   uint32_t hdrlen, ptype;
+   int l4_supported = 0;
+
+   /* nothing to do */
+   if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
+   return 0;
+
+   m->ol_flags |= PKT_RX_IP_CKSUM_UNKNOWN;
+
+   ptype = rte_net_get_ptype(m, _lens, RTE_PTYPE_ALL_MASK);
+   m->packet_type = ptype;
+   if ((ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP ||
+   (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_UDP ||
+   (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_SCTP)
+   l4_supported = 1;
+
+   if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
+   hdrlen = hdr_lens.l2_len + hdr_lens.l3_len + hdr_lens.l4_len;
+   if (hdr->csum_start <= hdrlen && l4_supported) {
+   m->ol_flags |= PKT_RX_L4_CKSUM_NONE;
+   } else {
+   /* Unknown proto or tunnel, do sw cksum. We can assume
+* the cksum field is in the first segment since the
+* buffers we provided to the host are large enough.
+* In case of SCTP, this will be wrong since it's a CRC
+* but there's nothing we can do.
+*/
+   uint16_t csum, off;
+
+   csum = rte_raw_cksum_mbuf(m, hdr->csum_start,
+   rte_pktmbuf_pkt_len(m) - hdr->csum_start);
+   if (csum != 0x)
+   csum = ~csum;
+   off = hdr->csum_offset + hdr->csum_start;
+   if (rte_pktmbuf_data_len(m) >= off + 1)
+   *rte_pktmbuf_mtod_offset(m, uint16_t *,
+   off) = csum;
+   }
+   } else if (hdr->flags & VIRTIO_NET_HDR_F_DATA_VALID && l4_supported) {
+   m->ol_flags |= PKT_RX_L4_CKSUM_GOOD;
+   }