Re: [Qemu-devel] [PATCH v2 2/2] e1000: decrement RDT if equal to RDH

[Richard Tollerton]

"Michael S. Tsirkin" <address@hidden> writes:

> Richard, can you respond please?
> I'd like to see this clarified in code comment or
> commit message before applying this patchset.

Apologies, and thanks for reminding me.

On Thu, Dec 18, 2014 at 12:01:48AM -0500, Jason Wang wrote:

> > Some drivers set RDT=RDH. Oddly, this works on real hardware. To work
> > around this, autodecrement RDT when this happens.
> 
> Please describe more details on the issue. The spec 3.2.6 said: "When
> the head pointer is equal to the tail pointer, the ring is empty." So
> RDT=RDH in fact empty the ring. No?

That is incorrect; the spec explicitly states that RDT=RDH means the
ring is full. The linux e1000 driver more or less implies the same thing.

You forgot to include the sentence after that in section 3.2.6 :)

"When the head pointer is equal to the tail pointer, the ring is empty.
Hardware stops storing packets in system memory until software advances
the tail pointer, making more receive buffers available."

Yeah, this seems really poorly worded to me too. :( You appear to be
interpreting "ring is empty" in the usual sense, i.e. "all N elements of
the ring buffer are available for use by hardware". In fact, the correct
interpretation [1] is the exact opposite, "none of the elements are
available for use by hardware". The last sentence in the quote makes
this explicit. See also linux e1000 driver sources at [2] [3] [4].

See also [5] which implies that hardware DMA is kicked off by setting
tail != head at initialization. I'm *guessing* (?) that the DMA engine
isn't correspondingly stopped when software sets RDT=RDH, so that once
packets start getting received, the hardware can more or less ignore it.
In this context, my patch makes sense.

(Yes, this is totally an ex-post-facto justification for the patch; it
arrived to me secondhand, and I had not been familiar with the driver
source before now.)

[1] http://sourceforge.net/p/e1000/mailman/message/29280078/
[2] 
http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/e1000/e1000_main.c#L398
[3] 
http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/e1000/e1000.h#L215
[4] 
http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/e1000/e1000_main.c#L4302
[5] http://sourceforge.net/p/e1000/mailman/message/29969887/

>> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> > index 44ae3a8..b8cbfc1 100644
>> > --- a/hw/net/e1000.c
>> > +++ b/hw/net/e1000.c
>> > @@ -1152,6 +1152,12 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>> >  static void
>> >  set_rdt(E1000State *s, int index, uint32_t val)
>> >  {
>> > +    if (val == s->mac_reg[RDH]) {     /* Decrement RDT if it's too big */
>> > +        if (val == 0) {
>> > +            val = s->mac_reg[RDLEN] / sizeof(struct e1000_rx_desc);
>> > +        }
>> > +        val--;
>> > +    }
>> >      s->mac_reg[index] = val & 0xffff;
>> >      if (e1000_has_rxbufs(s, 1)) {
>> >          qemu_flush_queued_packets(qemu_get_queue(s->nic));
>> > --
>> > 2.1.3
>> > 
>> > 
>> > 

-- 
Richard Tollerton <address@hidden>