[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: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [V2 PATCH] rtl8139: check the buffer availiability |
Date: |
Wed, 19 Oct 2011 03:28:13 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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?
I realize we have the same pattern in other places in this
device, probably a bug as well?
> + 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?
> +}
> +
> 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;
>