From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55793) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWjko-0003rv-CV for qemu-devel@nongnu.org; Tue, 01 Sep 2015 07:29:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWjkk-0000Xq-C0 for qemu-devel@nongnu.org; Tue, 01 Sep 2015 07:29:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40855) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWjkk-0000Xm-6L for qemu-devel@nongnu.org; Tue, 01 Sep 2015 07:29:26 -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 B06A53DD42 for ; Tue, 1 Sep 2015 11:29:25 +0000 (UTC) Message-ID: <55E58C13.70008@redhat.com> Date: Tue, 01 Sep 2015 07:29:23 -0400 From: Vlad Yasevich MIME-Version: 1.0 References: <1440770817-20555-1-git-send-email-vyasevic@redhat.com> <1440770817-20555-3-git-send-email-vyasevic@redhat.com> <55E4256F.9010302@redhat.com> <55E4638E.3090404@redhat.com> <55E51858.40607@redhat.com> In-Reply-To: <55E51858.40607@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 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: Jason Wang , qemu-devel@nongnu.org Cc: stefanha@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 >>>> --- >>>> 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