From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33127) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYOd7-0008P6-0i for qemu-devel@nongnu.org; Tue, 23 Feb 2016 20:52:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYOd3-0002PH-Pe for qemu-devel@nongnu.org; Tue, 23 Feb 2016 20:52:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42323) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYOd3-0002P2-JS for qemu-devel@nongnu.org; Tue, 23 Feb 2016 20:52:37 -0500 References: <1454423392-7732-1-git-send-email-ppandit@redhat.com> <56B465B7.8000201@redhat.com> <56CBD1AF.6090802@redhat.com> From: Jason Wang Message-ID: <56CD0CDA.9050709@redhat.com> Date: Wed, 24 Feb 2016 09:52:26 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: ne2000: check ring buffer control registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Yang Hongke , QEMU Developers On 02/23/2016 04:28 PM, P J P wrote: > Hello Jason, > > +-- On Tue, 23 Feb 2016, Jason Wang wrote --+ > | I mean with your patch, driver will only be allowed to set EN0_STOPPG > | before EN0_STARTPG. So if a driver want to set STARTPG first, the check > | > | + if (v < NE2000_PMEM_END && v < s->stop) { > | > | will prevent the driver from working correctly since s->stop is zero here. > > Before drivers could start using NIC, it'll be initialised from its ROM, > right? Which would set the PSTART & PSTOP registers to the default values. > With '-net nic,model=ne2k_pci,vlan=0' I see, > > s->start = 19456, s->stop = 32768 So in this case, if a driver want to do the following things: 1) set s->stop to 16384 2) set s->start to 8192 Then it won't work. > > | > I think any attempts to define the ring buffer limits should reset > | > 'boundary' and 'curpag' registers to s->start(STARTPG). I wonder if a > | > driver should be allowed to fiddle with the ring buffers location inside > | > contorller's memory. It does not seem right. > | > | Well, I think we could not assume the behavior of a driver, especially > | consider it may be malicious. > > Yes; That's why it'll help to keep drivers from fiddling with the ring > buffer dimensions. Right, but since setting STARTPG,STOPPG,BOUNDARY and CURPAG is not atomic. Try to limit it during value setting is hard to be correct. > IIUC, there is an upper limit to where PSTOP could > point[1], > > "In 8 bit mode the PSTOP register should not exceed to 0x60, > in 16 bit mode the PSTOP register should not exceed to 0x80" > > [1] http://www.ethernut.de/pdf/8019asds.pdf > > Kernel drivers too seem to have it fixed > -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne.c#n398 > -> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/8390/ne2k-pci.c#n342 > > | > Check if (s->start == s->stop) at each receive call? > | Or in ne2000_buffer_full()? > > ne2000_buffer_full() too assumes that 's->stop > s->start' > > ... > avail = (s->stop - s->start) - (index - boundary); Then let's return true when s->stop <= s->start? > Is there a case wherein drivers need to adjust ring buffer pointers? If not, I > think it's better to convert EN0_STARTPG:, EN0_STOPPG:, EN0_BOUNDARY: and > EN1_CURPAG: cases into no-ops. It's really hard to say there's no such driver. Which means if there's such a driver and it works on real hardware, we need make it work for qemu. > > -- > - P J P > 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F >