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
>
