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

[Dmitry Fleytman]

> On 18 Oct 2016, at 17:27 PM, Kevin Wolf <address@hidden> wrote:
> 
> Am 18.10.2016 um 16:10 hat Dmitry Fleytman geschrieben:
>>    On 17 Oct 2016, at 01:35 AM, Kevin Wolf <address@hidden> 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 <address@hidden>
>> 
>> Thanks for catching this!
> 
> Thanks, Dmitry!
> 
> I assume that your R-b implies that you don't send a pull request

Right.

> 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?

Jason usually picks up patches like these.

> 
> 
> 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.

Right, this according to the spec this value should be 0 by default and 
throttling should be disabled.

Current device implementation does not allow specification of throttling 
interval less than 500
and treats interval 0 as throttling enabled with interval 500.

This is done by intention because according to the spec (10.2.4.2) device 
cannot produce more than
7813 interrupts per second even when throttling is disabled. Therefore, even in 
case of interrupt storm
(continuous interrupt re-injection by device), number of interrupts produced by 
device is limited
and CPU (driver) has enough time to do its job and handle problematic interrupt 
state.

Opposed to this, virtual device is able to raise interrupts with rate limited 
by CPU speed only
therefore driver has no chance to fix interrupt storm condition. 

Windows e1000e drivers rely on upper limit for number of interrupts per second 
in some cases
and absence of this limit leads to infinite interrupt storms.

To summarise, while usage of throttling mechanisms is a little bit different 
from what specification says,
effective emulated device behavior is totally compliant to the real device.

> 
> 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.

This part that looks suspicious.

Even when throttling is enabled, timers should be idle
unless there are packets on corresponding path.

In case you see times firing when device is idle - something
might be wrong in throttling mechanisms.

~Dmitry

> 
> Kevin