diff mbox

[v3,2/2] rtl8139: correctly track full receive buffer in standard mode

Message ID 1440770817-20555-3-git-send-email-vyasevic@redhat.com
State New
Headers show

Commit Message

Vlad Yasevich Aug. 28, 2015, 2:06 p.m. UTC
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 | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

Comments

Jason Wang Aug. 31, 2015, 9:59 a.m. UTC | #1
On 08/28/2015 10:06 PM, Vladislav Yasevich wrote:
> 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 41 insertions(+), 4 deletions(-)

I'm not sure this can happen. For example, looks like the following
check in rtl8139_do_receive():

        if (avail != 0 && size + 8 >= avail)
        {

can guarantee there's no overwriting?
Vlad Yasevich Aug. 31, 2015, 2:24 p.m. UTC | #2
On 08/31/2015 05:59 AM, Jason Wang wrote:
> 
> 
> On 08/28/2015 10:06 PM, Vladislav Yasevich wrote:
>> 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 41 insertions(+), 4 deletions(-)
> 
> I'm not sure this can happen. For example, looks like the following
> check in rtl8139_do_receive():
> 
>         if (avail != 0 && size + 8 >= avail)
>         {
> 
> can guarantee there's no overwriting?
> 

The problem is the calculation of avail.  When the buffer is full,
avail will be the the size of the receive buffer.  So the test
above will be false because the driver thinks there is actually
enough room.

With his patch, 'avail' will be calculated to 0.

-vlad
Jason Wang Sept. 1, 2015, 3:15 a.m. UTC | #3
On 08/31/2015 10:24 PM, Vlad Yasevich wrote:
> On 08/31/2015 05:59 AM, Jason Wang wrote:
>>
>> On 08/28/2015 10:06 PM, Vladislav Yasevich wrote:
>>> 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 41 insertions(+), 4 deletions(-)
>> I'm not sure this can happen. For example, looks like the following
>> check in rtl8139_do_receive():
>>
>>         if (avail != 0 && size + 8 >= avail)
>>         {
>>
>> can guarantee there's no overwriting?
>>
> The problem is the calculation of avail.  When the buffer is full,
> avail will be the the size of the receive buffer.  So the test
> above will be false because the driver thinks there is actually
> enough room.
>
> With his patch, 'avail' will be calculated to 0.
>
> -vlad
>

If believe the condition size + 8 >= avail can guarantee that the buffer
won't be full (if we allow size + 8 == avail, buffer will be full)? So
avail == 0 means the buffer is empty. Or is there anything I miss?
Vlad Yasevich Sept. 1, 2015, 11:29 a.m. UTC | #4
On 08/31/2015 11:15 PM, Jason Wang wrote:
> 
> 
> On 08/31/2015 10:24 PM, Vlad Yasevich wrote:
>> On 08/31/2015 05:59 AM, Jason Wang wrote:
>>>
>>> On 08/28/2015 10:06 PM, Vladislav Yasevich wrote:
>>>> 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>>>>  1 file changed, 41 insertions(+), 4 deletions(-)
>>> I'm not sure this can happen. For example, looks like the following
>>> check in rtl8139_do_receive():
>>>
>>>         if (avail != 0 && size + 8 >= avail)
>>>         {
>>>
>>> can guarantee there's no overwriting?
>>>
>> The problem is the calculation of avail.  When the buffer is full,
>> avail will be the the size of the receive buffer.  So the test
>> above will be false because the driver thinks there is actually
>> enough room.
>>
>> With his patch, 'avail' will be calculated to 0.
>>
>> -vlad
>>
> 
> If believe the condition size + 8 >= avail can guarantee that the buffer
> won't be full (if we allow size + 8 == avail, buffer will be full)? So
> avail == 0 means the buffer is empty. Or is there anything I miss?
> 

So the issue is that the RxBufAddr is 4 byte aligned, but when we do
availability check above, we don't 4 byte align the size+8 calculation.
That causes the check above to fail when it should succeed and we never
catch the overflow condition.

I'll resubmit with a simple alignment patch that makes this work.

-vlad
diff mbox

Patch

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 359e001..0e13242 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -794,6 +794,32 @@  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->RxBufAddr - s->RxBufPtr, 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 uint32_t rtl8139_get_avail_bytes(RTL8139State *s)
+{
+    return s->RxBufferSize - rtl8139_get_unread_bytes(s);
+}
+
 static int rtl8139_can_receive(NetClientState *nc)
 {
     RTL8139State *s = qemu_get_nic_opaque(nc);
@@ -810,8 +836,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_avail_bytes(s);
         return (avail == 0 || avail >= 1514 || (s->IntrMask & RxOverflow));
     }
 }
@@ -1144,7 +1169,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_avail_bytes(s);
 
         /* if receiver buffer is empty then avail == 0 */
 
@@ -1178,6 +1203,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 +1237,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 +1446,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 +2635,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));