Re: [Qemu-devel] [PATCH] net: ne2000: check ring buffer control registers

[Jason Wang <jasowang@redhat.com>]

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
>