Re: [Qemu-devel] [PATCH v3 2/2] rtl8139: correctly track full receive buffer in standard mode

[Jason Wang]

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 <address@hidden>
>>> ---
>>>  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?