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 <vyase...@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
