* [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.
2015-08-21 21:59 [Qemu-devel] [PATCH " Vladislav Yasevich
@ 2015-08-21 21:59 ` Vladislav Yasevich
2015-08-26 12:18 ` Stefan Hajnoczi
0 siblings, 1 reply; 7+ messages in thread
From: Vladislav Yasevich @ 2015-08-21 21:59 UTC (permalink / raw
To: qemu-devel; +Cc: Vladislav Yasevich, jasowang, stefanha
When operation in standard mode, we currently return the size
of packet during buffer overflow. This consumes the overflow
packet. Return 0 instead so we can re-process the overflow packet
when we have room.
This fixes issues with lost/dropped fragments of large messages.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
hw/net/rtl8139.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index edbb61c..359e001 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
s->IntrStatus |= RxOverflow;
++s->RxMissed;
rtl8139_update_irq(s);
- return size_;
+ return 0;
}
packet_header |= RxStatusOK;
--
1.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.
2015-08-21 21:59 ` [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during " Vladislav Yasevich
@ 2015-08-26 12:18 ` Stefan Hajnoczi
2015-08-26 13:02 ` Vlad Yasevich
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2015-08-26 12:18 UTC (permalink / raw
To: Vladislav Yasevich; +Cc: jasowang, qemu-devel
On Fri, Aug 21, 2015 at 02:59:24PM -0700, Vladislav Yasevich wrote:
> When operation in standard mode, we currently return the size
> of packet during buffer overflow. This consumes the overflow
> packet. Return 0 instead so we can re-process the overflow packet
> when we have room.
>
> This fixes issues with lost/dropped fragments of large messages.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
> hw/net/rtl8139.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index edbb61c..359e001 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
> s->IntrStatus |= RxOverflow;
> ++s->RxMissed;
> rtl8139_update_irq(s);
> - return size_;
> + return 0;
Every .receive() return 0 must be paired with a
qemu_flush_queued_packets() call.
Is rtl8139_RxBufPtr_write() guaranteed to be called when the guest
refills rx buffers?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.
2015-08-26 12:18 ` Stefan Hajnoczi
@ 2015-08-26 13:02 ` Vlad Yasevich
0 siblings, 0 replies; 7+ messages in thread
From: Vlad Yasevich @ 2015-08-26 13:02 UTC (permalink / raw
To: Stefan Hajnoczi; +Cc: jasowang, qemu-devel
On 08/26/2015 08:18 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2015 at 02:59:24PM -0700, Vladislav Yasevich wrote:
>> When operation in standard mode, we currently return the size
>> of packet during buffer overflow. This consumes the overflow
>> packet. Return 0 instead so we can re-process the overflow packet
>> when we have room.
>>
>> This fixes issues with lost/dropped fragments of large messages.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>> hw/net/rtl8139.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>> index edbb61c..359e001 100644
>> --- a/hw/net/rtl8139.c
>> +++ b/hw/net/rtl8139.c
>> @@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
>> s->IntrStatus |= RxOverflow;
>> ++s->RxMissed;
>> rtl8139_update_irq(s);
>> - return size_;
>> + return 0;
>
> Every .receive() return 0 must be paired with a
> qemu_flush_queued_packets() call.
Isn't that already there. A few drivers already return 0 to trigger a requeue. What's
missing?
>
> Is rtl8139_RxBufPtr_write() guaranteed to be called when the guest
> refills rx buffers?
>
It calls it on read, once it's consumed a packet, to advance to the next packet in the
buffer.
-vlad
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow in standard mode
@ 2015-08-26 19:51 Vladislav Yasevich
2015-08-26 19:51 ` [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during " Vladislav Yasevich
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Vladislav Yasevich @ 2015-08-26 19:51 UTC (permalink / raw
To: qemu-devel; +Cc: Vladislav Yasevich, jasowang, stefanha
When rtl8139 card is running in standard mode, it is very easy
to overlflow and the receive buffer and get into a siutation
where all packets are dropped. Simply reproduction case is
to ping the guest from the host with 6500 byte packets.
There are actually 2 problems here.
1) When the rtl8129 buffer is overflow, the card emulation
returns the size of the packet back to queue transmission.
This signals successful reception even though the packet
has been dropped. The proper solution is to return 0, so
that the packet is re-queued and will be resubmitted later.
2) When packets are sized such that the fragments end up completely
filling the receive buffer without overflow, the device thinks
that the buffer is actually empty (instead of full). This causes
next packet to over-write the existing packets. With the above
ping reproducer, ever ICMP packet fills the buffer and thus keeps
overwriting the previous packet and never waking up the guest.
The solution here is track the number of unread bytes separately
so we would know if we have anything in buffer to read or not.
V2: instead of tracking buffer_full condition, changed the code, as
suggested by Stefan Hajnoczi, to track the number of unread bytes
instead. We initialize it to 0 at the start, adjust it on every
receive from the network and read from the guest and can set
the number of unread of bytes to full buffer size when the buffer
full.
Vladislav Yasevich (2):
rtl8139: Do not consume the packet during overflow in standard mode.
rtl8139: correctly track full receive buffer in standard mode
hw/net/rtl8139.c | 44 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 39 insertions(+), 5 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during overflow in standard mode.
2015-08-26 19:51 [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow in standard mode Vladislav Yasevich
@ 2015-08-26 19:51 ` Vladislav Yasevich
2015-08-26 19:51 ` [Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer " Vladislav Yasevich
2015-08-27 1:19 ` [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow " Vlad Yasevich
2 siblings, 0 replies; 7+ messages in thread
From: Vladislav Yasevich @ 2015-08-26 19:51 UTC (permalink / raw
To: qemu-devel; +Cc: Vladislav Yasevich, jasowang, stefanha
When operation in standard mode, we currently return the size
of packet during buffer overflow. This consumes the overflow
packet. Return 0 instead so we can re-process the overflow packet
when we have room.
This fixes issues with lost/dropped fragments of large messages.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
hw/net/rtl8139.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index edbb61c..359e001 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1157,7 +1157,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
s->IntrStatus |= RxOverflow;
++s->RxMissed;
rtl8139_update_irq(s);
- return size_;
+ return 0;
}
packet_header |= RxStatusOK;
--
1.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer in standard mode
2015-08-26 19:51 [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow in standard mode Vladislav Yasevich
2015-08-26 19:51 ` [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during " Vladislav Yasevich
@ 2015-08-26 19:51 ` Vladislav Yasevich
2015-08-27 1:19 ` [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow " Vlad Yasevich
2 siblings, 0 replies; 7+ messages in thread
From: Vladislav Yasevich @ 2015-08-26 19:51 UTC (permalink / raw
To: qemu-devel; +Cc: Vladislav Yasevich, jasowang, stefanha
In standard operation mode, when the receive ring buffer
is full, the buffer actually appears empty to the driver since
the RxBufAddr (the location we wirte new data to) and RxBufPtr
(the location guest would stat reading from) are the same.
As a result, the call to rtl8139_RxBufferEmpty ends up
returning true indicating that the receive buffer is empty.
This would result in the next packet overwriting the recevie buffer
again and stalling receive operations.
This patch tracks the number of unread bytes in the rxbuffer
using an unused C+ register. On every read and write, the
number is adjsted and the special case of a full buffer is also
trapped.
The C+ register trick is used to simplify migration and not require
a new machine type. This register is not used in regular mode
and C+ mode doesn't have the same issue.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
hw/net/rtl8139.c | 40 ++++++++++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 359e001..381dedb 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -794,6 +794,27 @@ static bool rtl8139_cp_rx_valid(RTL8139State *s)
return !(s->RxRingAddrLO == 0 && s->RxRingAddrHI == 0);
}
+static uint32_t rtl8139_unread_bytes(RTL8139State *s)
+{
+ return MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr, s->RxBufferSize);
+}
+
+static void rtl8139_set_unread_bytes(RTL8139State *s, uint32_t unread)
+{
+ /* In standard mode, C+ RxDesc isn't used. Reuse it
+ * to store the number of unread bytes.
+ */
+ s->currCPlusRxDesc = unread;
+}
+
+static uint32_t rtl8139_get_unread_bytes(RTL8139State *s)
+{
+ /* In standard mode, C+ RxDesc isn't used. Reuse it
+ * to store the number of unread bytes in linear buffer.
+ */
+ return s->currCPlusRxDesc;
+}
+
static int rtl8139_can_receive(NetClientState *nc)
{
RTL8139State *s = qemu_get_nic_opaque(nc);
@@ -810,8 +831,7 @@ static int rtl8139_can_receive(NetClientState *nc)
This is a hack to work around slirp deficiencies anyway. */
return 1;
} else {
- avail = MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr,
- s->RxBufferSize);
+ avail = rtl8139_get_unread_bytes(s);
return (avail == 0 || avail >= 1514 || (s->IntrMask & RxOverflow));
}
}
@@ -1144,7 +1164,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
DPRINTF("in ring Rx mode ================\n");
/* begin ring receiver mode */
- int avail = MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr, s->RxBufferSize);
+ int avail = rtl8139_get_unread_bytes(s);
/* if receiver buffer is empty then avail == 0 */
@@ -1178,6 +1198,14 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
/* correct buffer write pointer */
s->RxBufAddr = MOD2((s->RxBufAddr + 3) & ~0x3, s->RxBufferSize);
+ /* Catch buffer full condition. Without this, we end up
+ * assuming that buffer is empty.
+ */
+ rtl8139_set_unread_bytes(s,
+ (s->RxBufAddr == s->RxBufPtr) ?
+ s->RxBufferSize :
+ rtl8139_unread_bytes(s));
+
/* now we can signal we have received something */
DPRINTF("received: rx buffer length %d head 0x%04x read 0x%04x\n",
@@ -1204,6 +1232,7 @@ static void rtl8139_reset_rxring(RTL8139State *s, uint32_t bufferSize)
s->RxBufferSize = bufferSize;
s->RxBufPtr = 0;
s->RxBufAddr = 0;
+ rtl8139_set_unread_bytes(s, 0);
}
static void rtl8139_reset(DeviceState *d)
@@ -1412,7 +1441,7 @@ static void rtl8139_ChipCmd_write(RTL8139State *s, uint32_t val)
static int rtl8139_RxBufferEmpty(RTL8139State *s)
{
- int unread = MOD2(s->RxBufferSize + s->RxBufAddr - s->RxBufPtr, s->RxBufferSize);
+ int unread = rtl8139_get_unread_bytes(s);
if (unread != 0)
{
@@ -2601,6 +2630,9 @@ static void rtl8139_RxBufPtr_write(RTL8139State *s, uint32_t val)
/* this value is off by 16 */
s->RxBufPtr = MOD2(val + 0x10, s->RxBufferSize);
+ /* We just read data, update unread bytes */
+ rtl8139_set_unread_bytes(s, rtl8139_unread_bytes(s));
+
/* more buffer space may be available so try to receive */
qemu_flush_queued_packets(qemu_get_queue(s->nic));
--
1.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow in standard mode
2015-08-26 19:51 [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow in standard mode Vladislav Yasevich
2015-08-26 19:51 ` [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during " Vladislav Yasevich
2015-08-26 19:51 ` [Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer " Vladislav Yasevich
@ 2015-08-27 1:19 ` Vlad Yasevich
2 siblings, 0 replies; 7+ messages in thread
From: Vlad Yasevich @ 2015-08-27 1:19 UTC (permalink / raw
To: qemu-devel; +Cc: jasowang, stefanha
On 08/26/2015 03:51 PM, Vladislav Yasevich wrote:
> When rtl8139 card is running in standard mode, it is very easy
> to overlflow and the receive buffer and get into a siutation
> where all packets are dropped. Simply reproduction case is
> to ping the guest from the host with 6500 byte packets.
>
> There are actually 2 problems here.
> 1) When the rtl8129 buffer is overflow, the card emulation
> returns the size of the packet back to queue transmission.
> This signals successful reception even though the packet
> has been dropped. The proper solution is to return 0, so
> that the packet is re-queued and will be resubmitted later.
>
> 2) When packets are sized such that the fragments end up completely
> filling the receive buffer without overflow, the device thinks
> that the buffer is actually empty (instead of full). This causes
> next packet to over-write the existing packets. With the above
> ping reproducer, ever ICMP packet fills the buffer and thus keeps
> overwriting the previous packet and never waking up the guest.
> The solution here is track the number of unread bytes separately
> so we would know if we have anything in buffer to read or not.
>
> V2: instead of tracking buffer_full condition, changed the code, as
> suggested by Stefan Hajnoczi, to track the number of unread bytes
> instead. We initialize it to 0 at the start, adjust it on every
> receive from the network and read from the guest and can set
> the number of unread of bytes to full buffer size when the buffer
> full.
>
> Vladislav Yasevich (2):
> rtl8139: Do not consume the packet during overflow in standard mode.
> rtl8139: correctly track full receive buffer in standard mode
>
Self nack. The second patch is wrong. Will resubmit when fixed.
-vlad
> hw/net/rtl8139.c | 44 +++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 39 insertions(+), 5 deletions(-)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-08-27 1:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-26 19:51 [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow in standard mode Vladislav Yasevich
2015-08-26 19:51 ` [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during " Vladislav Yasevich
2015-08-26 19:51 ` [Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer " Vladislav Yasevich
2015-08-27 1:19 ` [Qemu-devel] [PATCH v2 0/2] rtl8139: Fix buffer overflow " Vlad Yasevich
-- strict thread matches above, loose matches on Subject: below --
2015-08-21 21:59 [Qemu-devel] [PATCH " Vladislav Yasevich
2015-08-21 21:59 ` [Qemu-devel] [PATCH 1/2] rtl8139: Do not consume the packet during " Vladislav Yasevich
2015-08-26 12:18 ` Stefan Hajnoczi
2015-08-26 13:02 ` Vlad Yasevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).