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

[Stefan Hajnoczi]

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

> 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?