Re: [Qemu-devel] [PATCH] e1000e: Don't zero out buffer address in rx descriptor

[Kevin Wolf <kwolf@redhat.com>]

Am 18.10.2016 um 16:10 hat Dmitry Fleytman geschrieben:
>     On 17 Oct 2016, at 01:35 AM, Kevin Wolf <mail@kevin-wolf.de> wrote:
> 
>     The e1000e emulation zeroes out any used rx descriptor and then writes a
>     completely newly constructed value there. By doing this, it doesn't only
>     update the write-back area of the descriptors (as it's supposed to do),
>     but it also clears the buffer address, which real hardware doesn't do.
> 
>     The spec explicitly mentions in chapter 7.1.8 that it is valid for a
>     driver to reuse a descriptor and only update the status field while
>     doing so, i.e. reusing the old buffer address:
> 
>        If software statically allocates buffers, and uses memory read to
>        check for completed descriptors, it simply has to zero the status
>        byte in the descriptor to make it ready for reuse by hardware.
> 
>     This patch fixes the behaviour to leave the buffer address in
>     descriptors unchanged even after the descriptor has been used.
> 
> 
> Hi Kevin,
> 
> Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>
> 
> Thanks for catching this!

Thanks, Dmitry!

I assume that your R-b implies that you don't send a pull request
yourself. If so, what's the process for getting the patch merged? Is
Jason going to pick it up normally or should I send a pull request of my
own?


Another related thing that I noticed while debugging this and turning on
tracing is that the interrupt throttling timers kept firing even if
there was no activity at all. Something might be wrong, there, too.

Next thing I wondered why throttling was enabled at all because the spec
says the default is 0 (turned off). So one thing that I'm pretty sure is
just a misunderstanding is the following defintion:

#define E1000E_MIN_XITR     (500) /* No more then 7813 interrupts per
                                     second according to spec 10.2.4.2 */

As I understand it, the spec is just giving an example there and lower
values are valid as well. At the very least, 0 should be accepted as a
special case because it means "disabled" and it's specified to be the
default.

As this wasn't blocking me after I had patched the above constant
locally to 0 so that I could see the actually meaningful trace events, I
didn't dig deeper, but I suspect that my local workaround for the
trace point spam may actually be a valid fix.

Kevin