qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/2] Ignore RX tail kicks when RX disabled.


From: Dmitry Fleytman
Subject: Re: [Qemu-devel] [PATCH 1/2] Ignore RX tail kicks when RX disabled.
Date: Thu, 18 Oct 2012 10:34:36 +0200

Stefan,

The real purpose of check_rxov it a bit confusing indeed, mainly
because of unclear name (rename?),
however it works as following:

There are 2 possible when RDT == RDH for RX ring:
    1. Device used all the buffers from ring, no empty buffers available
    2. Driver fully refilled the ring and all buffers are empty and ready to use

check_rxov is used to distinguish these 2 cases:
    1. It must be 1 initially (init, reset, etc.)
    2. It must be set to one when device uses buffer
    3. It must be set to 0 when driver adds buffer to the ring
check_rxov == 1 - ring is empty
check_rxov == 0 - ring is full

Indeed, RX init sequence doesn't look logical, however this is the way
all Intel driver behave from e1000 and up to ixgbe.
Also see some explanation here:
http://permalink.gmane.org/gmane.linux.kernel/1375917

If we drop check_rxov and always treat RDH == RDT as empty ring we'll
probably get correct behavior for current Linux driver's code (needs
testing of course),
however we have no idea how Windows drivers work.

Also drivers tend to change...

Dmitry.

On Thu, Oct 18, 2012 at 10:09 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Wed, Oct 17, 2012 at 08:31:46PM +0200, Dmitry Fleytman wrote:
>> Device RX initization from driver's side consists of following steps:
>>   1. Initialize head and tail of RX ring to 0
>>   2. Enable Rx (set bit in RCTL register)
>>   3. Allocate buffers, fill descriptors
>>   4. Write ring tail
>>
>> Forth operation signals hardware that RX buffers available
>> and it may start packets indication.
>>
>> Current implementation treats first operation (write 0 to ring tail)
>> as signal of buffers availability and starts data transfers as soon
>> as RX enable indicaton arrives.
>>
>> This is not correct because there is a chance that ring is still
>> empty (third action not performed yet) and then memory corruption
>> occures.
>
> Any idea what the point of hw/e1000.c check_rxov is?  I see nothing in
> the datasheet that requires these semantics.
>
> The Linux e1000 driver never enables the RXO (rx fifo overflow)
> interrupt, only RXDMT0 (receive descriptor minimum threshold).  This
> means hw/e1000.c will not upset the Linux e1000 driver when
> e1000_receive() gets called with check_rxov == 1 and RDH == RDT == 0.
>
> BTW the Linux e1000 driver does not follow the sequence recommended in
> the datasheet 14.4 Receive Initialization, which would avoid the weird
> window of time where RDH == RDT == 0.
>
> If we get rid of check_rxov and always check rxbuf space then we have
> the correct behavior.  I'm a little nervous of simply dropping it
> because its purpose is unclear to me :(.
>
> Stefan



-- 
Dmitry Fleytman
Technology Expert and Consultant,

Daynix Computing Ltd.

Cell: +972-54-2819481

Skype: dmitry.fleytman



reply via email to

[Prev in Thread] Current Thread [Next in Thread]