* [PATCH] Fixed IPv6 payload lenght without jumbo option
@ 2020-04-05 19:19 andrew
2020-04-09 3:15 ` Jason Wang
2020-04-09 13:45 ` Eric Blake
0 siblings, 2 replies; 7+ messages in thread
From: andrew @ 2020-04-05 19:19 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, dmitry.fleytman
From: Andrew Melnychenko <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>
---
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);
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))
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixed IPv6 payload lenght without jumbo option
2020-04-05 19:19 [PATCH] Fixed IPv6 payload lenght without jumbo option andrew
@ 2020-04-09 3:15 ` Jason Wang
2020-04-09 13:35 ` Andrew Melnichenko
2020-04-09 13:45 ` Eric Blake
1 sibling, 1 reply; 7+ messages in thread
From: Jason Wang @ 2020-04-09 3:15 UTC (permalink / raw)
To: andrew, qemu-devel; +Cc: dmitry.fleytman
On 2020/4/6 上午3:19, andrew@daynix.com wrote:
> From: Andrew Melnychenko <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>
> ---
> 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))
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixed IPv6 payload lenght without jumbo option
2020-04-09 3:15 ` Jason Wang
@ 2020-04-09 13:35 ` Andrew Melnichenko
2020-04-10 2:28 ` Jason Wang
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Melnichenko @ 2020-04-09 13:35 UTC (permalink / raw)
To: Jason Wang; +Cc: dmitry.fleytman, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4460 bytes --]
> 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.
On Thu, Apr 9, 2020 at 6:15 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On 2020/4/6 上午3:19, andrew@daynix.com wrote:
> > From: Andrew Melnychenko <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>
> > ---
> > 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))
>
>
[-- Attachment #2: Type: text/html, Size: 5889 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixed IPv6 payload lenght without jumbo option
2020-04-05 19:19 [PATCH] Fixed IPv6 payload lenght without jumbo option andrew
2020-04-09 3:15 ` Jason Wang
@ 2020-04-09 13:45 ` Eric Blake
1 sibling, 0 replies; 7+ messages in thread
From: Eric Blake @ 2020-04-09 13:45 UTC (permalink / raw)
To: andrew, qemu-devel; +Cc: jasowang, dmitry.fleytman
On 4/5/20 2:19 PM, andrew@daynix.com wrote:
> From: Andrew Melnychenko <andrew@daynix.com>
In the subject: s/lenght/length/
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1708065
> e1000e driver doesn't sets 'plen' field for IPv6 for big packets
s/sets/set the/
> 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>
> ---
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixed IPv6 payload lenght without jumbo option
2020-04-09 13:35 ` Andrew Melnichenko
@ 2020-04-10 2:28 ` Jason Wang
2020-04-10 9:38 ` Andrew Melnichenko
0 siblings, 1 reply; 7+ messages in thread
From: Jason Wang @ 2020-04-10 2:28 UTC (permalink / raw)
To: Andrew Melnichenko; +Cc: dmitry.fleytman, qemu-devel
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))
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fixed IPv6 payload lenght without jumbo option
2020-04-10 2:28 ` Jason Wang
@ 2020-04-10 9:38 ` Andrew Melnichenko
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Melnichenko @ 2020-04-10 9:38 UTC (permalink / raw)
To: Jason Wang; +Cc: dmitry.fleytman, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6977 bytes --]
Ok, later - I'll prepare a new patch with length fix, segmentation and
checks.
For now, the current patch can be discarded.
On Fri, Apr 10, 2020 at 5:28 AM Jason Wang <jasowang@redhat.com> wrote:
>
> 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))
> >
>
>
[-- Attachment #2: Type: text/html, Size: 9771 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Fixed IPv6 payload lenght without jumbo option
@ 2020-04-05 19:18 andrew
0 siblings, 0 replies; 7+ messages in thread
From: andrew @ 2020-04-05 19:18 UTC (permalink / raw)
To: qemu-devel; +Cc: jasowang, dmitry.fleytman
From: Andrew Melnychenko <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>
---
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);
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))
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-10 9:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-05 19:19 [PATCH] Fixed IPv6 payload lenght without jumbo option andrew
2020-04-09 3:15 ` Jason Wang
2020-04-09 13:35 ` Andrew Melnichenko
2020-04-10 2:28 ` Jason Wang
2020-04-10 9:38 ` Andrew Melnichenko
2020-04-09 13:45 ` Eric Blake
-- strict thread matches above, loose matches on Subject: below --
2020-04-05 19:18 andrew
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.