Re: [PATCH] Fixed IPv6 payload lenght without jumbo option

[Jason Wang <jasowang@redhat.com>]

On 2020/4/9 下午9:35, Andrew Melnichenko wrote:
> > Do we support ipv6 segmentation then?
> No, but - if the backend supports big frames there is an issue that 
> IPv6 plen doesn't have valid value.
> Actually, It's a good idea to add IPv6 fragmentation - I would do it 
> later.


Right, another question.

E.g for virtio-net, we will do the following check:

     if (!peer_has_vnet_hdr(n)) {
         virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
         virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
         virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO6);
         virtio_clear_feature(&features, VIRTIO_NET_F_HOST_ECN);

         virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_CSUM);
         virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO4);
         virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
         virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
     }

I think we should do something similar in e1000e. But I can only see 
disable_vnet parameter but not a checking of the ability of its peer.

1) when peer has vnet hdr, it supports receiving GSO packets, we don't 
need software segmentation.
2) when peer does not have vnet hdr, we need to use software path 
segmentation.

This means we need:

1) check peer_has_vnet_hdr() and disable_vnet in net_pkt_send() before 
calling net_tx_pkt_sendv() and fallback to software segmentation
2) fix the ipv6 payload len
3) add the ipv6 software segmentation support

It would be better if we can fix all of these issue in one series.

Thanks


>
>
> On Thu, Apr 9, 2020 at 6:15 AM Jason Wang <jasowang@redhat.com 
> <mailto:jasowang@redhat.com>> wrote:
>
>
>     On 2020/4/6 上午3:19, andrew@daynix.com <mailto:andrew@daynix.com>
>     wrote:
>     > From: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     >
>     > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1708065
>     > e1000e driver doesn't sets 'plen' field for IPv6 for big packets
>     > if TSO is enabled. Jumbo option isn't added yet, until
>     > qemu supports packets greater than 64K.
>     >
>     > Signed-off-by: Andrew Melnychenko <andrew@daynix.com
>     <mailto:andrew@daynix.com>>
>     > ---
>     >   hw/net/e1000e_core.c |  1 +
>     >   hw/net/net_tx_pkt.c  | 31 +++++++++++++++++++++++++++++++
>     >   hw/net/net_tx_pkt.h  |  7 +++++++
>     >   include/net/eth.h    |  1 +
>     >   4 files changed, 40 insertions(+)
>     >
>     > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>     > index d5676871fa..a1ec55598b 100644
>     > --- a/hw/net/e1000e_core.c
>     > +++ b/hw/net/e1000e_core.c
>     > @@ -656,6 +656,7 @@ e1000e_tx_pkt_send(E1000ECore *core, struct
>     e1000e_tx *tx, int queue_index)
>     >       NetClientState *queue = qemu_get_subqueue(core->owner_nic,
>     target_queue);
>     >
>     >       e1000e_setup_tx_offloads(core, tx);
>     > +    net_tx_pkt_fix_ip6_payload_len(tx->tx_pkt);
>
>
>     A question here:
>
>     I don't see any code that set ip6_plen during
>     net_tx_pkt_do_sw_fragmentation(). This is described in 7.3.6.2.1 and
>     7.3.6.2.2 in the datasheet.
>
>     And:
>
>     1) eth_setup_ip4_fragmentation() only deal with ipv4
>
>     2) eth_fix_ip4_checksum() assumes a ipv4 header
>
>     Do we support ipv6 segmentation then?
>
>     Thanks
>
>
>     >
>     >       net_tx_pkt_dump(tx->tx_pkt);
>     >
>     > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
>     > index 162f802dd7..b05d554ac3 100644
>     > --- a/hw/net/net_tx_pkt.c
>     > +++ b/hw/net/net_tx_pkt.c
>     > @@ -635,3 +635,34 @@ bool net_tx_pkt_send_loopback(struct
>     NetTxPkt *pkt, NetClientState *nc)
>     >
>     >       return res;
>     >   }
>     > +
>     > +void net_tx_pkt_fix_ip6_payload_len(struct NetTxPkt *pkt)
>     > +{
>     > +    /*
>     > +     * If ipv6 payload length field is 0 - then there should be
>     Hop-by-Hop
>     > +     * option for packets greater than 65,535.
>     > +     * For packets with payload less than 65,535: fix 'plen' field.
>     > +     * For now, qemu drops every packet with size greater 64K
>     > +     * (see net_tx_pkt_send()) so, there is no reason to add
>     jumbo option to ip6
>     > +     * hop-by-hop extension if it's missed
>     > +     */
>     > +
>     > +    struct iovec *l2 = &pkt->vec[NET_TX_PKT_L2HDR_FRAG];
>     > +    if (eth_get_l3_proto(l2, 1, l2->iov_len) == ETH_P_IPV6) {
>     > +        struct ip6_header *ip6 = (struct ip6_header *) pkt->l3_hdr;
>     > +        /*
>     > +         * TODO: if qemu would support >64K packets - add jumbo
>     option check
>     > +         * something like that:
>     > +         * 'if (ip6->ip6_plen == 0 && !has_jumbo_option(ip6)) {'
>     > +         */
>     > +        if (ip6->ip6_plen == 0) {
>     > +            if (pkt->payload_len <= ETH_MAX_IP_DGRAM_LEN) {
>     > +                ip6->ip6_plen = htons(pkt->payload_len);
>     > +            }
>     > +            /*
>     > +             * TODO: if qemu would support >64K packets
>     > +             * add jumbo option for packets greater then 65,535
>     bytes
>     > +             */
>     > +        }
>     > +    }
>     > +}
>     > diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
>     > index 212ecc62fc..912d56ef13 100644
>     > --- a/hw/net/net_tx_pkt.h
>     > +++ b/hw/net/net_tx_pkt.h
>     > @@ -187,4 +187,11 @@ bool net_tx_pkt_parse(struct NetTxPkt *pkt);
>     >   */
>     >   bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt);
>     >
>     > +/**
>     > + * Fix IPv6 'plen' field.
>     > + *
>     > + * @pkt            packet
>     > + */
>     > +void net_tx_pkt_fix_ip6_payload_len(struct NetTxPkt *pkt);
>     > +
>     >   #endif
>     > diff --git a/include/net/eth.h b/include/net/eth.h
>     > index 7f45c678e7..0671be6916 100644
>     > --- a/include/net/eth.h
>     > +++ b/include/net/eth.h
>     > @@ -186,6 +186,7 @@ struct tcp_hdr {
>     >
>     >   #define ip6_nxt      ip6_ctlun.ip6_un1.ip6_un1_nxt
>     >   #define ip6_ecn_acc  ip6_ctlun.ip6_un3.ip6_un3_ecn
>     > +#define ip6_plen     ip6_ctlun.ip6_un1.ip6_un1_plen
>     >
>     >   #define PKT_GET_ETH_HDR(p)        \
>     >       ((struct eth_header *)(p))
>