Re: [Qemu-devel] [V2 PATCH] rtl8139: check the buffer availiability

[Jason Wang <jasowang@redhat.com>]

On 10/19/2011 09:28 AM, Michael S. Tsirkin wrote:
> On Mon, Oct 17, 2011 at 10:55:57AM +0800, Jason Wang wrote:
>> Reduce spurious packet drops on RX ring empty when in c+ mode by verifying that
>> we have at least 1 buffer ahead of the time.
>>
>> Change from v1:
>> Fix style comments from Stefan.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  hw/rtl8139.c |   44 ++++++++++++++++++++++++++++++--------------
>>  1 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
>> index 3753950..bcbc5e3 100644
>> --- a/hw/rtl8139.c
>> +++ b/hw/rtl8139.c
>> @@ -84,6 +84,19 @@
>>  #define VLAN_TCI_LEN 2
>>  #define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
>>  
>> +/* w0 ownership flag */
>> +#define CP_RX_OWN (1<<31)
>> +/* w0 end of ring flag */
>> +#define CP_RX_EOR (1<<30)
>> +/* w0 bits 0...12 : buffer size */
>> +#define CP_RX_BUFFER_SIZE_MASK ((1<<13) - 1)
>> +/* w1 tag available flag */
>> +#define CP_RX_TAVA (1<<16)
>> +/* w1 bits 0...15 : VLAN tag */
>> +#define CP_RX_VLAN_TAG_MASK ((1<<16) - 1)
>> +/* w2 low  32bit of Rx buffer ptr */
>> +/* w3 high 32bit of Rx buffer ptr */
>> +
>>  #if defined (DEBUG_RTL8139)
>>  #  define DPRINTF(fmt, ...) \
>>      do { fprintf(stderr, "RTL8139: " fmt, ## __VA_ARGS__); } while (0)
>> @@ -805,6 +818,22 @@ static inline target_phys_addr_t rtl8139_addr64(uint32_t low, uint32_t high)
>>  #endif
>>  }
>>  
>> +/* Verify that we have at least one available rx buffer */
>> +static int rtl8139_cp_has_rxbuf(RTL8139State *s)
>> +{
>> +    uint32_t val, rxdw0;
>> +    target_phys_addr_t cplus_rx_ring_desc = rtl8139_addr64(s->RxRingAddrLO,
>> +                                                           s->RxRingAddrHI);
>> +    cplus_rx_ring_desc += 16 * s->currCPlusRxDesc;
>> +    cpu_physical_memory_read(cplus_rx_ring_desc, &val, 4);
> 
> Interesting. Please note that cpu_physical_memory_read is not
> done atomically. Can guest be writing the value while
> we read it? If yes we'll get a corrupted value here,
> because CP_RX_OWN is the high bit so it is read last.
> Correct?
> 

Yes, there's a race, we need use atomic memory access method.

> I realize we have the same pattern in other places in this
> device, probably a bug as well?
> 

Yes, we need use atomic method at least for ownership check during TX/RX.

> 
>> +    rxdw0 = le32_to_cpu(val);
>> +    if (rxdw0 & CP_RX_OWN) {
>> +        return 1;
>> +    } else {
>> +        return 0;
>> +    }
> 
> Do we need to check that buffer size is large enough
> to include the packet like we do for non c+ mode?
> 

Not sure here is the best place, the buffer size is checked during
receiving.

>> +}
>> +
>>  static int rtl8139_can_receive(VLANClientState *nc)
>>  {
>>      RTL8139State *s = DO_UPCAST(NICState, nc, nc)->opaque;
>> @@ -819,7 +848,7 @@ static int rtl8139_can_receive(VLANClientState *nc)
>>      if (rtl8139_cp_receiver_enabled(s)) {
>>          /* ??? Flow control not implemented in c+ mode.
>>             This is a hack to work around slirp deficiencies anyway.  */
>> -        return 1;
>> +        return rtl8139_cp_has_rxbuf(s);
>>      } else {
>>          avail = MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr,
>>                       s->RxBufferSize);
>> @@ -965,19 +994,6 @@ static ssize_t rtl8139_do_receive(VLANClientState *nc, const uint8_t *buf, size_
>>  
>>          /* begin C+ receiver mode */
>>  
>> -/* w0 ownership flag */
>> -#define CP_RX_OWN (1<<31)
>> -/* w0 end of ring flag */
>> -#define CP_RX_EOR (1<<30)
>> -/* w0 bits 0...12 : buffer size */
>> -#define CP_RX_BUFFER_SIZE_MASK ((1<<13) - 1)
>> -/* w1 tag available flag */
>> -#define CP_RX_TAVA (1<<16)
>> -/* w1 bits 0...15 : VLAN tag */
>> -#define CP_RX_VLAN_TAG_MASK ((1<<16) - 1)
>> -/* w2 low  32bit of Rx buffer ptr */
>> -/* w3 high 32bit of Rx buffer ptr */
>> -
>>          int descriptor = s->currCPlusRxDesc;
>>          target_phys_addr_t cplus_rx_ring_desc;
>>