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

[Vlad Yasevich <vyasevic@redhat.com>]

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