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 <jasow...@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; >>