[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [V2 PATCH] rtl8139: check the buffer availiability
From: |
Jason Wang |
Subject: |
Re: [Qemu-devel] [V2 PATCH] rtl8139: check the buffer availiability |
Date: |
Sat, 22 Oct 2011 12:30:25 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20110929 Thunderbird/7.0.1 |
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 <address@hidden>
>> ---
>> 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;
>>