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

[Vlad Yasevich <vyasevic@redhat.com>]

On 08/26/2015 08:36 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2015 at 02:59:25PM -0700, 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 catches the "receive buffer full" condition
>> using an unused C+ register.  This is done to simplify
>> migration and not require a new machine type.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  hw/net/rtl8139.c | 36 ++++++++++++++++++++++++++++++++++--
>>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> The rtl8139 code duplicates the following expression in several places:
> 
>   MOD2(s->RxBufferSize + s->RxBufAddr - s->RxBufPtr, s->RxBufferSize);
> 
> It may be cleaner to keep a rx_unread_bytes counter so that all these
> users can simply look at that variable.
> 
> That cleanup also eliminates the rx full vs empty problem because then
> we'll know whether rx_unread_bytes == 0 or rx_unread_bytes ==
> s->RxBufferSize.
> 
> The same trick of stashing the value in s->currCPlusRxDesc could be
> used.
> 

Good idea.  I'll give it a try.


>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>> index 359e001..3d572ab 100644
>> --- a/hw/net/rtl8139.c
>> +++ b/hw/net/rtl8139.c
>> @@ -816,6 +816,23 @@ static int rtl8139_can_receive(NetClientState *nc)
>>      }
>>  }
>>  
>> +static void rtl8139_set_rxbuf_full(RTL8139State *s, bool full)
>> +{
>> +    /* In standard mode, C+ RxDesc isn't used.  Reuse it
>> +     * to store the rx_buf_full status.
>> +     */
> 
> assert(!s->cplus_enabled)?
> 
>> +    s->currCPlusRxDesc = full;
>> +    DPRINTF("received: rx buffer full\n");
>> +}
>> +
>> +static bool rtl8139_rxbuf_full(RTL8139State *s)
>> +{
>> +    /* In standard mode, C+ RxDesc isn't used.  Reuse it
>> +     * to store the rx_buf_full status.
>> +     */
> 
> assert(!s->cplus_enabled)?
> 
>> @@ -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, clear full buffer state */
>> +    rtl8139_set_rxbuf_full(s, false);
>> +
>>      /* more buffer space may be available so try to receive */
>>      qemu_flush_queued_packets(qemu_get_queue(s->nic));
> 
> What if the guest writes this register while we're in C+ mode?
> 

In C+ mode the guest uses a descriptor ring instead of liner buffer so a well behaved
C+ guest wouldn't write this value.  Validated this with a linux guest.
I guess a malicious guest could do this, but then it could do a lot of other things too.

-vlad