* [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits
2022-11-17 16:55 [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value Stefan Hajnoczi
@ 2022-11-17 16:55 ` Stefan Hajnoczi
2022-11-18 7:18 ` Philippe Mathieu-Daudé
2022-11-21 4:16 ` Jason Wang
2022-11-17 16:55 ` [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2022-11-17 16:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Tobias Fiebig, Jason Wang, qemu-stable, Stefan Hajnoczi
The device turns the Tx Descriptor into a Tx Status descriptor after
fully reading the descriptor. This involves clearing Tx Own (bit 31) to
indicate that the driver has ownership of the descriptor again as well
as several other bits.
The code keeps the first dword of the Tx Descriptor in the txdw0 local
variable. txdw0 is reused to build the first word of the Tx Status
descriptor. Later on the code uses txdw0 again, incorrectly assuming
that it still contains the first dword of the Tx Descriptor. The tx
offloading code misbehaves because it sees bogus bits in txdw0.
Use a separate local variable for Tx Status and preserve Tx Descriptor
in txdw0.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/net/rtl8139.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index e6643e3c9d..ffef3789b5 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2027,18 +2027,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
s->currCPlusTxDesc = 0;
}
+ /* Build the Tx Status Descriptor */
+ uint32_t tx_status = txdw0;
+
/* transfer ownership to target */
- txdw0 &= ~CP_TX_OWN;
+ tx_status &= ~CP_TX_OWN;
/* reset error indicator bits */
- txdw0 &= ~CP_TX_STATUS_UNF;
- txdw0 &= ~CP_TX_STATUS_TES;
- txdw0 &= ~CP_TX_STATUS_OWC;
- txdw0 &= ~CP_TX_STATUS_LNKF;
- txdw0 &= ~CP_TX_STATUS_EXC;
+ tx_status &= ~CP_TX_STATUS_UNF;
+ tx_status &= ~CP_TX_STATUS_TES;
+ tx_status &= ~CP_TX_STATUS_OWC;
+ tx_status &= ~CP_TX_STATUS_LNKF;
+ tx_status &= ~CP_TX_STATUS_EXC;
/* update ring data */
- val = cpu_to_le32(txdw0);
+ val = cpu_to_le32(tx_status);
pci_dma_write(d, cplus_tx_ring_desc, (uint8_t *)&val, 4);
/* Now decide if descriptor being processed is holding the last segment of packet */
--
2.38.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits
2022-11-17 16:55 ` [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits Stefan Hajnoczi
@ 2022-11-18 7:18 ` Philippe Mathieu-Daudé
2022-11-21 4:16 ` Jason Wang
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-18 7:18 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: Tobias Fiebig, Jason Wang, qemu-stable
On 17/11/22 17:55, Stefan Hajnoczi wrote:
> The device turns the Tx Descriptor into a Tx Status descriptor after
> fully reading the descriptor. This involves clearing Tx Own (bit 31) to
> indicate that the driver has ownership of the descriptor again as well
> as several other bits.
>
> The code keeps the first dword of the Tx Descriptor in the txdw0 local
> variable. txdw0 is reused to build the first word of the Tx Status
> descriptor. Later on the code uses txdw0 again, incorrectly assuming
> that it still contains the first dword of the Tx Descriptor. The tx
> offloading code misbehaves because it sees bogus bits in txdw0.
>
> Use a separate local variable for Tx Status and preserve Tx Descriptor
> in txdw0.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> hw/net/rtl8139.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits
2022-11-17 16:55 ` [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits Stefan Hajnoczi
2022-11-18 7:18 ` Philippe Mathieu-Daudé
@ 2022-11-21 4:16 ` Jason Wang
2022-11-21 12:31 ` Stefan Hajnoczi
1 sibling, 1 reply; 17+ messages in thread
From: Jason Wang @ 2022-11-21 4:16 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Tobias Fiebig, qemu-stable
On Fri, Nov 18, 2022 at 12:56 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The device turns the Tx Descriptor into a Tx Status descriptor after
> fully reading the descriptor. This involves clearing Tx Own (bit 31) to
> indicate that the driver has ownership of the descriptor again as well
> as several other bits.
>
> The code keeps the first dword of the Tx Descriptor in the txdw0 local
> variable. txdw0 is reused to build the first word of the Tx Status
> descriptor. Later on the code uses txdw0 again, incorrectly assuming
> that it still contains the first dword of the Tx Descriptor. The tx
> offloading code misbehaves because it sees bogus bits in txdw0.
(This is only noticeable with patch 2).
>
> Use a separate local variable for Tx Status and preserve Tx Descriptor
> in txdw0.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
> ---
> hw/net/rtl8139.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index e6643e3c9d..ffef3789b5 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -2027,18 +2027,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
> s->currCPlusTxDesc = 0;
> }
>
> + /* Build the Tx Status Descriptor */
> + uint32_t tx_status = txdw0;
> +
> /* transfer ownership to target */
> - txdw0 &= ~CP_TX_OWN;
> + tx_status &= ~CP_TX_OWN;
>
> /* reset error indicator bits */
> - txdw0 &= ~CP_TX_STATUS_UNF;
> - txdw0 &= ~CP_TX_STATUS_TES;
> - txdw0 &= ~CP_TX_STATUS_OWC;
> - txdw0 &= ~CP_TX_STATUS_LNKF;
> - txdw0 &= ~CP_TX_STATUS_EXC;
> + tx_status &= ~CP_TX_STATUS_UNF;
> + tx_status &= ~CP_TX_STATUS_TES;
> + tx_status &= ~CP_TX_STATUS_OWC;
> + tx_status &= ~CP_TX_STATUS_LNKF;
> + tx_status &= ~CP_TX_STATUS_EXC;
>
> /* update ring data */
> - val = cpu_to_le32(txdw0);
> + val = cpu_to_le32(tx_status);
> pci_dma_write(d, cplus_tx_ring_desc, (uint8_t *)&val, 4);
>
> /* Now decide if descriptor being processed is holding the last segment of packet */
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits
2022-11-21 4:16 ` Jason Wang
@ 2022-11-21 12:31 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2022-11-21 12:31 UTC (permalink / raw)
To: Jason Wang; +Cc: Stefan Hajnoczi, qemu-devel, Tobias Fiebig, qemu-stable
On Sun, 20 Nov 2022 at 23:17, Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Nov 18, 2022 at 12:56 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > The device turns the Tx Descriptor into a Tx Status descriptor after
> > fully reading the descriptor. This involves clearing Tx Own (bit 31) to
> > indicate that the driver has ownership of the descriptor again as well
> > as several other bits.
> >
> > The code keeps the first dword of the Tx Descriptor in the txdw0 local
> > variable. txdw0 is reused to build the first word of the Tx Status
> > descriptor. Later on the code uses txdw0 again, incorrectly assuming
> > that it still contains the first dword of the Tx Descriptor. The tx
> > offloading code misbehaves because it sees bogus bits in txdw0.
>
> (This is only noticeable with patch 2).
Yes, although the large_send_mss variable is already junk because some
bits have been cleared:
int large_send_mss = (txdw0 >> 16) & CP_TC_LGSEN_MSS_MASK;
Luckily it's not used yet, aside from DPRINTF().
>
> >
> > Use a separate local variable for Tx Status and preserve Tx Descriptor
> > in txdw0.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> > ---
> > hw/net/rtl8139.c | 17 ++++++++++-------
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> > index e6643e3c9d..ffef3789b5 100644
> > --- a/hw/net/rtl8139.c
> > +++ b/hw/net/rtl8139.c
> > @@ -2027,18 +2027,21 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
> > s->currCPlusTxDesc = 0;
> > }
> >
> > + /* Build the Tx Status Descriptor */
> > + uint32_t tx_status = txdw0;
> > +
> > /* transfer ownership to target */
> > - txdw0 &= ~CP_TX_OWN;
> > + tx_status &= ~CP_TX_OWN;
> >
> > /* reset error indicator bits */
> > - txdw0 &= ~CP_TX_STATUS_UNF;
> > - txdw0 &= ~CP_TX_STATUS_TES;
> > - txdw0 &= ~CP_TX_STATUS_OWC;
> > - txdw0 &= ~CP_TX_STATUS_LNKF;
> > - txdw0 &= ~CP_TX_STATUS_EXC;
> > + tx_status &= ~CP_TX_STATUS_UNF;
> > + tx_status &= ~CP_TX_STATUS_TES;
> > + tx_status &= ~CP_TX_STATUS_OWC;
> > + tx_status &= ~CP_TX_STATUS_LNKF;
> > + tx_status &= ~CP_TX_STATUS_EXC;
> >
> > /* update ring data */
> > - val = cpu_to_le32(txdw0);
> > + val = cpu_to_le32(tx_status);
> > pci_dma_write(d, cplus_tx_ring_desc, (uint8_t *)&val, 4);
> >
> > /* Now decide if descriptor being processed is holding the last segment of packet */
> > --
> > 2.38.1
> >
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate
2022-11-17 16:55 [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value Stefan Hajnoczi
2022-11-17 16:55 ` [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits Stefan Hajnoczi
@ 2022-11-17 16:55 ` Stefan Hajnoczi
2022-11-18 7:25 ` Philippe Mathieu-Daudé
2022-11-21 4:16 ` Jason Wang
2022-11-17 16:55 ` [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2022-11-17 16:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Tobias Fiebig, Jason Wang, qemu-stable, Stefan Hajnoczi
There are two Tx Descriptor formats called mode 0 and mode 1. The mode
is determined by the Large Send bit.
CP_TX_IPCS (bit 18) is defined in mode 1 but the code checks the bit
unconditionally. In mode 0 bit 18 is part of the Large Send MSS value.
Explicitly check the Large Send bit to distinguish Tx command modes.
This avoids bugs where modes are confused. Note that I didn't find any
actual bugs aside from needlessly computing the IP checksum when the
Large Send bit is enabled.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/net/rtl8139.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index ffef3789b5..6dd7a8e6e0 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2135,7 +2135,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
}
ip_data_len -= hlen;
- if (txdw0 & CP_TX_IPCS)
+ if (!(txdw0 & CP_TX_LGSEN) && (txdw0 & CP_TX_IPCS))
{
DPRINTF("+++ C+ mode need IP checksum\n");
@@ -2268,7 +2268,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
/* Stop sending this frame */
saved_size = 0;
}
- else if (txdw0 & (CP_TX_TCPCS|CP_TX_UDPCS))
+ else if (!(txdw0 & CP_TX_LGSEN) && (txdw0 & (CP_TX_TCPCS|CP_TX_UDPCS)))
{
DPRINTF("+++ C+ mode need TCP or UDP checksum\n");
--
2.38.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate
2022-11-17 16:55 ` [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate Stefan Hajnoczi
@ 2022-11-18 7:25 ` Philippe Mathieu-Daudé
2022-11-21 4:16 ` Jason Wang
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-18 7:25 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: Tobias Fiebig, Jason Wang, qemu-stable
On 17/11/22 17:55, Stefan Hajnoczi wrote:
> There are two Tx Descriptor formats called mode 0 and mode 1. The mode
> is determined by the Large Send bit.
>
> CP_TX_IPCS (bit 18) is defined in mode 1 but the code checks the bit
> unconditionally. In mode 0 bit 18 is part of the Large Send MSS value.
>
> Explicitly check the Large Send bit to distinguish Tx command modes.
> This avoids bugs where modes are confused. Note that I didn't find any
> actual bugs aside from needlessly computing the IP checksum when the
> Large Send bit is enabled.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> hw/net/rtl8139.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate
2022-11-17 16:55 ` [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate Stefan Hajnoczi
2022-11-18 7:25 ` Philippe Mathieu-Daudé
@ 2022-11-21 4:16 ` Jason Wang
1 sibling, 0 replies; 17+ messages in thread
From: Jason Wang @ 2022-11-21 4:16 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Tobias Fiebig, qemu-stable
On Fri, Nov 18, 2022 at 12:56 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> There are two Tx Descriptor formats called mode 0 and mode 1. The mode
> is determined by the Large Send bit.
>
> CP_TX_IPCS (bit 18) is defined in mode 1 but the code checks the bit
> unconditionally. In mode 0 bit 18 is part of the Large Send MSS value.
>
> Explicitly check the Large Send bit to distinguish Tx command modes.
> This avoids bugs where modes are confused. Note that I didn't find any
> actual bugs aside from needlessly computing the IP checksum when the
> Large Send bit is enabled.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
> ---
> hw/net/rtl8139.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index ffef3789b5..6dd7a8e6e0 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -2135,7 +2135,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
> }
> ip_data_len -= hlen;
>
> - if (txdw0 & CP_TX_IPCS)
> + if (!(txdw0 & CP_TX_LGSEN) && (txdw0 & CP_TX_IPCS))
> {
> DPRINTF("+++ C+ mode need IP checksum\n");
>
> @@ -2268,7 +2268,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
> /* Stop sending this frame */
> saved_size = 0;
> }
> - else if (txdw0 & (CP_TX_TCPCS|CP_TX_UDPCS))
> + else if (!(txdw0 & CP_TX_LGSEN) && (txdw0 & (CP_TX_TCPCS|CP_TX_UDPCS)))
> {
> DPRINTF("+++ C+ mode need TCP or UDP checksum\n");
>
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value
2022-11-17 16:55 [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value Stefan Hajnoczi
2022-11-17 16:55 ` [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits Stefan Hajnoczi
2022-11-17 16:55 ` [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate Stefan Hajnoczi
@ 2022-11-17 16:55 ` Stefan Hajnoczi
2022-11-17 17:51 ` Russell King (Oracle)
` (3 more replies)
2022-11-18 7:27 ` [PATCH for-7.2 v3 0/3] " Philippe Mathieu-Daudé
2022-11-21 15:55 ` Stefan Hajnoczi
4 siblings, 4 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2022-11-17 16:55 UTC (permalink / raw)
To: qemu-devel
Cc: Tobias Fiebig, Jason Wang, qemu-stable, Stefan Hajnoczi,
Russell King - ARM Linux
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(-)
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value
2022-11-17 16:55 ` [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value Stefan Hajnoczi
@ 2022-11-17 17:51 ` Russell King (Oracle)
2022-11-17 18:05 ` Stefan Hajnoczi
2022-11-18 7:24 ` Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Russell King (Oracle) @ 2022-11-17 17:51 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Tobias Fiebig, Jason Wang, qemu-stable
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value
2022-11-17 17:51 ` Russell King (Oracle)
@ 2022-11-17 18:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2022-11-17 18:05 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Stefan Hajnoczi, qemu-devel, Tobias Fiebig, Jason Wang, qemu-stable
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value
2022-11-17 16:55 ` [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value Stefan Hajnoczi
2022-11-17 17:51 ` Russell King (Oracle)
@ 2022-11-18 7:24 ` Philippe Mathieu-Daudé
2022-11-21 4:16 ` Jason Wang
2023-04-13 15:38 ` Peter Maydell
3 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-18 7:24 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Tobias Fiebig, Jason Wang, qemu-stable, Russell King - ARM Linux
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value
2022-11-17 16:55 ` [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value Stefan Hajnoczi
2022-11-17 17:51 ` Russell King (Oracle)
2022-11-18 7:24 ` Philippe Mathieu-Daudé
@ 2022-11-21 4:16 ` Jason Wang
2023-04-13 15:38 ` Peter Maydell
3 siblings, 0 replies; 17+ messages in thread
From: Jason Wang @ 2022-11-21 4:16 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Tobias Fiebig, qemu-stable, Russell King - ARM Linux
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value
2022-11-17 16:55 ` [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value Stefan Hajnoczi
` (2 preceding siblings ...)
2022-11-21 4:16 ` Jason Wang
@ 2023-04-13 15:38 ` Peter Maydell
2023-04-13 17:24 ` Stefan Hajnoczi
3 siblings, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2023-04-13 15:38 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Jason Wang
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value
2023-04-13 15:38 ` Peter Maydell
@ 2023-04-13 17:24 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2023-04-13 17:24 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, Jason Wang
[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value
2022-11-17 16:55 [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value Stefan Hajnoczi
` (2 preceding siblings ...)
2022-11-17 16:55 ` [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value Stefan Hajnoczi
@ 2022-11-18 7:27 ` Philippe Mathieu-Daudé
2022-11-21 15:55 ` Stefan Hajnoczi
4 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-18 7:27 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: Tobias Fiebig, Jason Wang, qemu-stable
On 17/11/22 17:55, Stefan Hajnoczi wrote:
> v3:
> - Add Patch 1 to avoid clobbering tx descriptor bits
> - Add Patch 2 to avoid confusing tx command modes
> - Exclude IP and TCP headers from large send MSS value
> Stefan Hajnoczi (3):
> rtl8139: avoid clobbering tx descriptor bits
> rtl8139: keep Tx command mode 0 and 1 separate
> rtl8139: honor large send MSS value
>
> hw/net/rtl8139.c | 47 ++++++++++++++++++++++++-----------------------
> 1 file changed, 24 insertions(+), 23 deletions(-)
>
Per
https://lore.kernel.org/qemu-devel/014101d8fac5$2cad8420$86088c60$@fiebig.nl/:
Tested-by: Tobias Fiebig <tobias@fiebig.nl>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value
2022-11-17 16:55 [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value Stefan Hajnoczi
` (3 preceding siblings ...)
2022-11-18 7:27 ` [PATCH for-7.2 v3 0/3] " Philippe Mathieu-Daudé
@ 2022-11-21 15:55 ` Stefan Hajnoczi
4 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2022-11-21 15:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Tobias Fiebig, Jason Wang, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]
On Thu, Nov 17, 2022 at 11:55:51AM -0500, Stefan Hajnoczi wrote:
> v3:
> - Add Patch 1 to avoid clobbering tx descriptor bits
> - Add Patch 2 to avoid confusing tx command modes
> - Exclude IP and TCP headers from large send MSS value
>
> The rtl8139 large send offload currently does not honor the MSS value provided
> by the driver. This is a problem especially when the MTU is small because IP
> fragmentation occurs and leads to poor performance.
>
> These patches make use of the large send MSS value.
>
> I have done basic testing by running the following Python code inside the
> guest:
>
> import socket
> s = socket.socket()
> s.connect(('10.0.2.2', 1234))
> buf = b'A' * 4096
> s.send(buf)
>
> The device sees a 4150 byte tx descriptor that results in the transmission of 3
> Ethernet frames with sizes of 1514, 1514, and 1230 bytes when the large send
> MSS value is 1460 bytes.
>
> Stefan Hajnoczi (3):
> rtl8139: avoid clobbering tx descriptor bits
> rtl8139: keep Tx command mode 0 and 1 separate
> rtl8139: honor large send MSS value
>
> hw/net/rtl8139.c | 47 ++++++++++++++++++++++++-----------------------
> 1 file changed, 24 insertions(+), 23 deletions(-)
>
> --
> 2.38.1
>
Thanks, applied to qemu.git/master!
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread