| Message ID | 20221117165554.1773409-4-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtl8139: honor large send MSS value | expand |
Hi Stefan, Your subject says "for-7.2" - that's a while off yet! I guess you need to update it to "for-6.2" for the next posting of this series. Thanks.
On Thu, 17 Nov 2022 at 12:53, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > Your subject says "for-7.2" - that's a while off yet! I guess you need > to update it to "for-6.2" for the next posting of this series. Sorry if this looked like a Linux patch series. This is a QEMU series and the next release is 7.2. Stefan
On 17/11/22 17:55, Stefan Hajnoczi wrote: > The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a > Large-Send MSS value where the driver specifies the MSS. See the > datasheet here: > http://realtek.info/pdf/rtl8139cp.pdf > > The code ignores this value and uses a hardcoded MSS of 1500 bytes > instead. When the MTU is less than 1500 bytes the hardcoded value > results in IP fragmentation and poor performance. > > Use the Large-Send MSS value to correctly size Large-Send packets. > > Jason Wang <jasowang@redhat.com> noticed that the Large-Send MSS value > mask was incorrect so it is adjusted to match the datasheet and Linux > 8139cp driver. > > This issue was discussed in the past here: > https://lore.kernel.org/all/20161114162505.GD26664@stefanha-x1.localdomain/ > > Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk> > Reported-by: Tobias Fiebig <tobias+git@fiebig.nl> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1312 > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/net/rtl8139.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > /* IP checksum offload flag */ > #define CP_TX_IPCS (1<<18) > @@ -2152,10 +2152,11 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) > goto skip_offload; > } > > - int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK; > + int large_send_mss = (txdw0 >> CP_TC_LGSEN_MSS_SHIFT) & > + CP_TC_LGSEN_MSS_MASK; Nitpicking/matter of style, the '&' is harder to miss if moved on the next line just before the mask. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Fri, Nov 18, 2022 at 12:56 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a > Large-Send MSS value where the driver specifies the MSS. See the > datasheet here: > http://realtek.info/pdf/rtl8139cp.pdf > > The code ignores this value and uses a hardcoded MSS of 1500 bytes > instead. When the MTU is less than 1500 bytes the hardcoded value > results in IP fragmentation and poor performance. > > Use the Large-Send MSS value to correctly size Large-Send packets. > > Jason Wang <jasowang@redhat.com> noticed that the Large-Send MSS value > mask was incorrect so it is adjusted to match the datasheet and Linux > 8139cp driver. > > This issue was discussed in the past here: > https://lore.kernel.org/all/20161114162505.GD26664@stefanha-x1.localdomain/ > > Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk> > Reported-by: Tobias Fiebig <tobias+git@fiebig.nl> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1312 > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Acked-by: Jason Wang <jasowang@redhat.com> Thanks > --- > hw/net/rtl8139.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c > index 6dd7a8e6e0..700b1b66b6 100644 > --- a/hw/net/rtl8139.c > +++ b/hw/net/rtl8139.c > @@ -77,7 +77,6 @@ > ( ( input ) & ( size - 1 ) ) > > #define ETHER_TYPE_LEN 2 > -#define ETH_MTU 1500 > > #define VLAN_TCI_LEN 2 > #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN) > @@ -1934,8 +1933,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) > #define CP_TX_LS (1<<28) > /* large send packet flag */ > #define CP_TX_LGSEN (1<<27) > -/* large send MSS mask, bits 16...25 */ > -#define CP_TC_LGSEN_MSS_MASK ((1 << 12) - 1) > +/* large send MSS mask, bits 16...26 */ > +#define CP_TC_LGSEN_MSS_SHIFT 16 > +#define CP_TC_LGSEN_MSS_MASK ((1 << 11) - 1) > > /* IP checksum offload flag */ > #define CP_TX_IPCS (1<<18) > @@ -2152,10 +2152,11 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) > goto skip_offload; > } > > - int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK; > + int large_send_mss = (txdw0 >> CP_TC_LGSEN_MSS_SHIFT) & > + CP_TC_LGSEN_MSS_MASK; > > - DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d " > - "frame data %d specified MSS=%d\n", ETH_MTU, > + DPRINTF("+++ C+ mode offloaded task TSO IP data %d " > + "frame data %d specified MSS=%d\n", > ip_data_len, saved_size - ETH_HLEN, large_send_mss); > > int tcp_send_offset = 0; > @@ -2180,25 +2181,22 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) > goto skip_offload; > } > > - /* ETH_MTU = ip header len + tcp header len + payload */ > int tcp_data_len = ip_data_len - tcp_hlen; > - int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen; > > DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP " > - "data len %d TCP chunk size %d\n", ip_data_len, > - tcp_hlen, tcp_data_len, tcp_chunk_size); > + "data len %d\n", ip_data_len, tcp_hlen, tcp_data_len); > > /* note the cycle below overwrites IP header data, > but restores it from saved_ip_header before sending packet */ > > int is_last_frame = 0; > > - for (tcp_send_offset = 0; tcp_send_offset < tcp_data_len; tcp_send_offset += tcp_chunk_size) > + for (tcp_send_offset = 0; tcp_send_offset < tcp_data_len; tcp_send_offset += large_send_mss) > { > - uint16_t chunk_size = tcp_chunk_size; > + uint16_t chunk_size = large_send_mss; > > /* check if this is the last frame */ > - if (tcp_send_offset + tcp_chunk_size >= tcp_data_len) > + if (tcp_send_offset + large_send_mss >= tcp_data_len) > { > is_last_frame = 1; > chunk_size = tcp_data_len - tcp_send_offset; > @@ -2247,7 +2245,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) > ip->ip_len = cpu_to_be16(hlen + tcp_hlen + chunk_size); > > /* increment IP id for subsequent frames */ > - ip->ip_id = cpu_to_be16(tcp_send_offset/tcp_chunk_size + be16_to_cpu(ip->ip_id)); > + ip->ip_id = cpu_to_be16(tcp_send_offset/large_send_mss + be16_to_cpu(ip->ip_id)); > > ip->ip_sum = 0; > ip->ip_sum = ip_checksum(eth_payload_data, hlen); > -- > 2.38.1 >
On Thu, 17 Nov 2022 at 16:58, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a > Large-Send MSS value where the driver specifies the MSS. See the > datasheet here: > http://realtek.info/pdf/rtl8139cp.pdf > > The code ignores this value and uses a hardcoded MSS of 1500 bytes > instead. When the MTU is less than 1500 bytes the hardcoded value > results in IP fragmentation and poor performance. > > Use the Large-Send MSS value to correctly size Large-Send packets. > > Jason Wang <jasowang@redhat.com> noticed that the Large-Send MSS value > mask was incorrect so it is adjusted to match the datasheet and Linux > 8139cp driver. Hi Stefan -- in v2 of this patch https://lore.kernel.org/qemu-devel/20221116154122.1705399-1-stefanha@redhat.com/ there was a check for "is the specified large_send_mss value too small?": + /* MSS too small? */ + if (tcp_hlen + hlen >= large_send_mss) { + goto skip_offload; + } but it isn't present in this final version of the patch which went into git. Was that deliberately dropped? I ask because the fuzzers have discovered that if you feed this device a descriptor where the large_send_mss value is 0, then we will now do a division by zero and crash: https://gitlab.com/qemu-project/qemu/-/issues/1582 (The datasheet, naturally, says nothing at all about what happens if the descriptor contains a bogus MSS value.) thanks -- PMM
On Thu, Apr 13, 2023 at 04:38:52PM +0100, Peter Maydell wrote: > On Thu, 17 Nov 2022 at 16:58, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a > > Large-Send MSS value where the driver specifies the MSS. See the > > datasheet here: > > http://realtek.info/pdf/rtl8139cp.pdf > > > > The code ignores this value and uses a hardcoded MSS of 1500 bytes > > instead. When the MTU is less than 1500 bytes the hardcoded value > > results in IP fragmentation and poor performance. > > > > Use the Large-Send MSS value to correctly size Large-Send packets. > > > > Jason Wang <jasowang@redhat.com> noticed that the Large-Send MSS value > > mask was incorrect so it is adjusted to match the datasheet and Linux > > 8139cp driver. > > Hi Stefan -- in v2 of this patch > > https://lore.kernel.org/qemu-devel/20221116154122.1705399-1-stefanha@redhat.com/ > > there was a check for "is the specified large_send_mss value > too small?": > > + /* MSS too small? */ > + if (tcp_hlen + hlen >= large_send_mss) { > + goto skip_offload; > + } > > but it isn't present in this final version of the patch which > went into git. Was that deliberately dropped? > > I ask because the fuzzers have discovered that if you feed this > device a descriptor where the large_send_mss value is 0, then > we will now do a division by zero and crash: > https://gitlab.com/qemu-project/qemu/-/issues/1582 > > (The datasheet, naturally, says nothing at all about what > happens if the descriptor contains a bogus MSS value.) Yes, I dropped that deliberately and forgot to add a large_send_mss == 0 check. Sorry! I have sent a patch: https://patchew.org/QEMU/20230413171946.2865726-1-stefanha@redhat.com/ Thanks, Stefan
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 6dd7a8e6e0..700b1b66b6 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -77,7 +77,6 @@ ( ( input ) & ( size - 1 ) ) #define ETHER_TYPE_LEN 2 -#define ETH_MTU 1500 #define VLAN_TCI_LEN 2 #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN) @@ -1934,8 +1933,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) #define CP_TX_LS (1<<28) /* large send packet flag */ #define CP_TX_LGSEN (1<<27) -/* large send MSS mask, bits 16...25 */ -#define CP_TC_LGSEN_MSS_MASK ((1 << 12) - 1) +/* large send MSS mask, bits 16...26 */ +#define CP_TC_LGSEN_MSS_SHIFT 16 +#define CP_TC_LGSEN_MSS_MASK ((1 << 11) - 1) /* IP checksum offload flag */ #define CP_TX_IPCS (1<<18) @@ -2152,10 +2152,11 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) goto skip_offload; } - int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK; + int large_send_mss = (txdw0 >> CP_TC_LGSEN_MSS_SHIFT) & + CP_TC_LGSEN_MSS_MASK; - DPRINTF("+++ C+ mode offloaded task TSO MTU=%d IP data %d " - "frame data %d specified MSS=%d\n", ETH_MTU, + DPRINTF("+++ C+ mode offloaded task TSO IP data %d " + "frame data %d specified MSS=%d\n", ip_data_len, saved_size - ETH_HLEN, large_send_mss); int tcp_send_offset = 0; @@ -2180,25 +2181,22 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) goto skip_offload; } - /* ETH_MTU = ip header len + tcp header len + payload */ int tcp_data_len = ip_data_len - tcp_hlen; - int tcp_chunk_size = ETH_MTU - hlen - tcp_hlen; DPRINTF("+++ C+ mode TSO IP data len %d TCP hlen %d TCP " - "data len %d TCP chunk size %d\n", ip_data_len, - tcp_hlen, tcp_data_len, tcp_chunk_size); + "data len %d\n", ip_data_len, tcp_hlen, tcp_data_len); /* note the cycle below overwrites IP header data, but restores it from saved_ip_header before sending packet */ int is_last_frame = 0; - for (tcp_send_offset = 0; tcp_send_offset < tcp_data_len; tcp_send_offset += tcp_chunk_size) + for (tcp_send_offset = 0; tcp_send_offset < tcp_data_len; tcp_send_offset += large_send_mss) { - uint16_t chunk_size = tcp_chunk_size; + uint16_t chunk_size = large_send_mss; /* check if this is the last frame */ - if (tcp_send_offset + tcp_chunk_size >= tcp_data_len) + if (tcp_send_offset + large_send_mss >= tcp_data_len) { is_last_frame = 1; chunk_size = tcp_data_len - tcp_send_offset; @@ -2247,7 +2245,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s) ip->ip_len = cpu_to_be16(hlen + tcp_hlen + chunk_size); /* increment IP id for subsequent frames */ - ip->ip_id = cpu_to_be16(tcp_send_offset/tcp_chunk_size + be16_to_cpu(ip->ip_id)); + ip->ip_id = cpu_to_be16(tcp_send_offset/large_send_mss + be16_to_cpu(ip->ip_id)); ip->ip_sum = 0; ip->ip_sum = ip_checksum(eth_payload_data, hlen);
The Large-Send Task Offload Tx Descriptor (9.2.1 Transmit) has a Large-Send MSS value where the driver specifies the MSS. See the datasheet here: http://realtek.info/pdf/rtl8139cp.pdf The code ignores this value and uses a hardcoded MSS of 1500 bytes instead. When the MTU is less than 1500 bytes the hardcoded value results in IP fragmentation and poor performance. Use the Large-Send MSS value to correctly size Large-Send packets. Jason Wang <jasowang@redhat.com> noticed that the Large-Send MSS value mask was incorrect so it is adjusted to match the datasheet and Linux 8139cp driver. This issue was discussed in the past here: https://lore.kernel.org/all/20161114162505.GD26664@stefanha-x1.localdomain/ Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk> Reported-by: Tobias Fiebig <tobias+git@fiebig.nl> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1312 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/net/rtl8139.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)