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

[Jason Wang]

On Tue, Jan 13, 2015 at 3:12 AM, Richard Tollerton 
<address@hidden> wrote:
> "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". 

Yes, I do think 'empty' means no available buffer for device to receive 
:)
> The last sentence in the quote makes
> this explicit. See also linux e1000 driver sources at [2] [3] [4].

Btw, [2],[3],[4] are all codes that deal with driver's internal 
variable, not the one that the hardware use.
> See also [5] which implies that hardware DMA is kicked off by setting
> tail != head at initialization. 

Yes, and we trigger receiving in set_rdt().
> I'm *guessing* (?) that the DMA engine
> isn't correspondingly stopped when software sets RDT=RDH, so that once
> packets start getting received, 

Do you mean in qemu? I/O are single threaded, so looks like we are safe.
> 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.)

True, we've found many undocumented behavior in the past (some even 
conflicts with spec). I don't have a 82540EM in my hand, but I think 
the best thing is to check this behavior in real hardware to prevent 
this patch from breaking many existing drivers. 
> [1] http://sourceforge.net/p/e1000/mailman/message/29280078/

This issue mentioned in the thread seems solved.

Current e1000_has_rxbufs() will return false if RDT==RDH.
> [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/

Looks like what mentioned in this thread was also solved.

We check both RCTL and e1000_has_rxbufs() in e1000_can_receive().
And flush queued packets in set_rx_control().
> > >  > 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>