From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59413) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUaQY-0005JA-2D for qemu-devel@nongnu.org; Wed, 26 Aug 2015 09:07:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUaQT-0003Dn-1U for qemu-devel@nongnu.org; Wed, 26 Aug 2015 09:07:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57529) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUaQS-0003DZ-QR for qemu-devel@nongnu.org; Wed, 26 Aug 2015 09:07:36 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 6D3D65BA33 for ; Wed, 26 Aug 2015 13:07:36 +0000 (UTC) Message-ID: <55DDBA16.10301@redhat.com> Date: Wed, 26 Aug 2015 09:07:34 -0400 From: Vlad Yasevich MIME-Version: 1.0 References: <1440194365-27610-1-git-send-email-vyasevic@redhat.com> <1440194365-27610-3-git-send-email-vyasevic@redhat.com> <20150826123617.GF30715@stefanha-thinkpad.redhat.com> In-Reply-To: <20150826123617.GF30715@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] rtl8139: correctly track full receive buffer in standard mode Reply-To: vyasevic@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: jasowang@redhat.com, qemu-devel@nongnu.org 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 >> --- >> 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