[PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value

[PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

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

[PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits

[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

[PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate

[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

[PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

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

Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value

[Russell King (Oracle)]

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!

Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

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

Re: [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits

[Philippe Mathieu-Daudé]

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>

Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value

[Philippe Mathieu-Daudé]

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>

Re: [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate

[Philippe Mathieu-Daudé]

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>

Re: [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value

[Philippe Mathieu-Daudé]

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>

Re: [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits

[Jason Wang]

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
>

Re: [PATCH for-7.2 v3 2/3] rtl8139: keep Tx command mode 0 and 1 separate

[Jason Wang]

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
>

Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value

[Jason Wang]

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
>

Re: [PATCH for-7.2 v3 1/3] rtl8139: avoid clobbering tx descriptor bits

[Stefan Hajnoczi]

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
> >
>
>

Re: [PATCH for-7.2 v3 0/3] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value

[Peter Maydell]

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

Re: [PATCH for-7.2 v3 3/3] rtl8139: honor large send MSS value

[Stefan Hajnoczi]

[-- 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 --]