[Qemu-devel] [PATCH for-2.4 00/12] hw/net: Fix .can_receive() for NICs

[Qemu-devel] [PATCH for-2.4 00/12] hw/net: Fix .can_receive() for NICs

[Fam Zheng]

Since a90a742 "tap: Drop tap_can_send", all nics that returns false from
.can_receive() are required to explicitly flush the incoming queue when the
status of it is changing back to true, otherwise the backend will sop
processing more rx packets.

The purpose of this callback is to tell the peer backend (tap, socket, etc)
"hold on until guest consumes old data because my buffer is not ready". More
often than not NICs also do this when driver deactivated the card or disabled
rx, causing the packets being unnessarily queued, where they should actualy be
dropped.

This series adds such missing qemu_flush_queued_packets calls for all NICs, and
drops such unnecessary conditions in .can_receive(), so that NICs now:

  - returns false from .can_receive when guest buffers are busy.
  - calls qemu_flush_queued_packets when buffers are available again.
  - returns -1 from .receive when rx is not enabled.

e1000, ne2000, rocker and vmxnet3 are not included because they're fixed by
other patches on the list and applied to Stefan's tree.

virtio-net is covered by another thread:

https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07377.html

All other NICs are okay, as they already do the flush on the state transition
points.

Please review.

Fam Zheng (12):
  xgmac: Drop packets with eth_can_rx is false.
  pcnet: Drop pcnet_can_receive
  eepro100: Drop nic_can_receive
  usbnet: Drop usbnet_can_receive
  etsec: Move etsec_can_receive into etsec_receive
  etsec: Flush queue when rx buffer is consumed
  mcf_fec: Drop mcf_fec_can_receive
  milkymist-minimac2: Flush queued packets when link comes up
  mipsnet: Flush queued packets when receiving is enabled
  stellaris_enet: Flush queued packets when read done
  dp8393x: Flush packets when link comes up
  axienet: Flush queued packets when rx is done

 hw/net/dp8393x.c            |  8 ++++++++
 hw/net/eepro100.c           | 11 -----------
 hw/net/fsl_etsec/etsec.c    | 20 ++++++++++----------
 hw/net/fsl_etsec/etsec.h    |  4 +++-
 hw/net/fsl_etsec/rings.c    | 15 +++++++++------
 hw/net/lance.c              |  1 -
 hw/net/mcf_fec.c            |  9 +--------
 hw/net/milkymist-minimac2.c | 12 ++++++------
 hw/net/mipsnet.c            |  9 +++++++--
 hw/net/pcnet-pci.c          |  1 -
 hw/net/pcnet.c              |  9 ---------
 hw/net/pcnet.h              |  1 -
 hw/net/stellaris_enet.c     | 14 +++++---------
 hw/net/xgmac.c              |  8 ++++----
 hw/net/xilinx_axienet.c     | 22 ++++++++++++++++++++++
 hw/usb/dev-network.c        | 20 ++++----------------
 16 files changed, 79 insertions(+), 85 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 01/12] xgmac: Drop packets with eth_can_rx is false.

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/xgmac.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/xgmac.c b/hw/net/xgmac.c
index b068f3a..15fb681 100644
--- a/hw/net/xgmac.c
+++ b/hw/net/xgmac.c
@@ -312,10 +312,8 @@ static const MemoryRegionOps enet_mem_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static int eth_can_rx(NetClientState *nc)
+static int eth_can_rx(XgmacState *s)
 {
-    XgmacState *s = qemu_get_nic_opaque(nc);
-
     /* RX enabled?  */
     return s->regs[DMA_CONTROL] & DMA_CONTROL_SR;
 }
@@ -329,6 +327,9 @@ static ssize_t eth_rx(NetClientState *nc, const uint8_t *buf, size_t size)
     struct desc bd;
     ssize_t ret;
 
+    if (!eth_can_rx(s)) {
+        return -1;
+    }
     unicast = ~buf[0] & 0x1;
     broadcast = memcmp(buf, sa_bcast, 6) == 0;
     multicast = !unicast && !broadcast;
@@ -371,7 +372,6 @@ out:
 static NetClientInfo net_xgmac_enet_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = eth_can_rx,
     .receive = eth_rx,
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 02/12] pcnet: Drop pcnet_can_receive

[Fam Zheng]

pcnet_receive already checks the conditions and drop packets if false.
Due to the new semantics since 6e99c63 ("net/socket: Drop
net_socket_can_send"), having .can_receive returning 0 requires us to
explicitly flush the queued packets when the conditions are becoming
true, but queuing the packets when guest driver is not ready doesn't
make much sense.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/lance.c     | 1 -
 hw/net/pcnet-pci.c | 1 -
 hw/net/pcnet.c     | 9 ---------
 hw/net/pcnet.h     | 1 -
 4 files changed, 12 deletions(-)

diff --git a/hw/net/lance.c b/hw/net/lance.c
index 4baa016..780b39d 100644
--- a/hw/net/lance.c
+++ b/hw/net/lance.c
@@ -94,7 +94,6 @@ static const MemoryRegionOps lance_mem_ops = {
 static NetClientInfo net_lance_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = pcnet_can_receive,
     .receive = pcnet_receive,
     .link_status_changed = pcnet_set_link_status,
 };
diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 8305d1b..b4d60b8 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -273,7 +273,6 @@ static void pci_pcnet_uninit(PCIDevice *dev)
 static NetClientInfo net_pci_pcnet_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = pcnet_can_receive,
     .receive = pcnet_receive,
     .link_status_changed = pcnet_set_link_status,
 };
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 68b9981..3437376 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -995,15 +995,6 @@ static int pcnet_tdte_poll(PCNetState *s)
     return !!(CSR_CXST(s) & 0x8000);
 }
 
-int pcnet_can_receive(NetClientState *nc)
-{
-    PCNetState *s = qemu_get_nic_opaque(nc);
-    if (CSR_STOP(s) || CSR_SPND(s))
-        return 0;
-
-    return sizeof(s->buffer)-16;
-}
-
 #define MIN_BUF_SIZE 60
 
 ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
diff --git a/hw/net/pcnet.h b/hw/net/pcnet.h
index 79c4c84..dec8de8 100644
--- a/hw/net/pcnet.h
+++ b/hw/net/pcnet.h
@@ -60,7 +60,6 @@ uint32_t pcnet_ioport_readw(void *opaque, uint32_t addr);
 void pcnet_ioport_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr);
 uint32_t pcnet_bcr_readw(PCNetState *s, uint32_t rap);
-int pcnet_can_receive(NetClientState *nc);
 ssize_t pcnet_receive(NetClientState *nc, const uint8_t *buf, size_t size_);
 void pcnet_set_link_status(NetClientState *nc);
 void pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info);
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 03/12] eepro100: Drop nic_can_receive

[Fam Zheng]

nic_receive already checks the conditions and drop packets if false.
Due to the new semantics since 6e99c63 ("net/socket: Drop
net_socket_can_send"), having .can_receive returning 0 requires us to
explicitly flush the queued packets when the conditions are becoming
true, but queuing the packets when guest driver is not ready doesn't
make much sense.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/eepro100.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index c374c1a..60333b7 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1617,16 +1617,6 @@ static const MemoryRegionOps eepro100_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static int nic_can_receive(NetClientState *nc)
-{
-    EEPRO100State *s = qemu_get_nic_opaque(nc);
-    TRACE(RXTX, logout("%p\n", s));
-    return get_ru_state(s) == ru_ready;
-#if 0
-    return !eepro100_buffer_full(s);
-#endif
-}
-
 static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf, size_t size)
 {
     /* TODO:
@@ -1844,7 +1834,6 @@ static void pci_nic_uninit(PCIDevice *pci_dev)
 static NetClientInfo net_eepro100_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = nic_can_receive,
     .receive = nic_receive,
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 04/12] usbnet: Drop usbnet_can_receive

[Fam Zheng]

usbnet_receive already drops packet if rndis_state is not
RNDIS_DATA_INITIALIZED, and queues packet if in buffer is not available.
The only difference is s->dev.config but that is similar to rndis_state.

Drop usbnet_can_receive and move these checks to usbnet_receive, so that
we don't need to explicitly flush the queue when s->dev.config changes
value.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/usb/dev-network.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 5eeb4c6..7800cee 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1268,6 +1268,10 @@ static ssize_t usbnet_receive(NetClientState *nc, const uint8_t *buf, size_t siz
     uint8_t *in_buf = s->in_buf;
     size_t total_size = size;
 
+    if (!s->dev.config) {
+        return -1;
+    }
+
     if (is_rndis(s)) {
         if (s->rndis_state != RNDIS_DATA_INITIALIZED) {
             return -1;
@@ -1309,21 +1313,6 @@ static ssize_t usbnet_receive(NetClientState *nc, const uint8_t *buf, size_t siz
     return size;
 }
 
-static int usbnet_can_receive(NetClientState *nc)
-{
-    USBNetState *s = qemu_get_nic_opaque(nc);
-
-    if (!s->dev.config) {
-        return 0;
-    }
-
-    if (is_rndis(s) && s->rndis_state != RNDIS_DATA_INITIALIZED) {
-        return 1;
-    }
-
-    return !s->in_len;
-}
-
 static void usbnet_cleanup(NetClientState *nc)
 {
     USBNetState *s = qemu_get_nic_opaque(nc);
@@ -1343,7 +1332,6 @@ static void usb_net_handle_destroy(USBDevice *dev)
 static NetClientInfo net_usbnet_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = usbnet_can_receive,
     .receive = usbnet_receive,
     .cleanup = usbnet_cleanup,
 };
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 05/12] etsec: Move etsec_can_receive into etsec_receive

[Fam Zheng]

When etsec_reset returns 0, peer would queue the packet as if
.can_receive returns false. Drop etsec_can_receive and let etsec_receive
carry the semantics.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/fsl_etsec/etsec.c | 11 +----------
 hw/net/fsl_etsec/etsec.h |  2 +-
 hw/net/fsl_etsec/rings.c | 14 ++++++++------
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index c57365f..f5170ae 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -338,13 +338,6 @@ static void etsec_reset(DeviceState *d)
         MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
 }
 
-static int etsec_can_receive(NetClientState *nc)
-{
-    eTSEC *etsec = qemu_get_nic_opaque(nc);
-
-    return etsec->rx_buffer_len == 0;
-}
-
 static ssize_t etsec_receive(NetClientState *nc,
                              const uint8_t  *buf,
                              size_t          size)
@@ -355,8 +348,7 @@ static ssize_t etsec_receive(NetClientState *nc,
     fprintf(stderr, "%s receive size:%d\n", etsec->nic->nc.name, size);
     qemu_hexdump(buf, stderr, "", size);
 #endif
-    etsec_rx_ring_write(etsec, buf, size);
-    return size;
+    return etsec_rx_ring_write(etsec, buf, size);
 }
 
 
@@ -370,7 +362,6 @@ static void etsec_set_link_status(NetClientState *nc)
 static NetClientInfo net_etsec_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = etsec_can_receive,
     .receive = etsec_receive,
     .link_status_changed = etsec_set_link_status,
 };
diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index 78d2c57..fc41773 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -162,7 +162,7 @@ DeviceState *etsec_create(hwaddr        base,
 
 void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
 void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
-void etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
+ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
 
 void etsec_write_miim(eTSEC          *etsec,
                       eTSEC_Register *reg,
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index d4a494f..a11280b 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -481,40 +481,42 @@ static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, size_t size)
                etsec->rx_buffer_len, etsec->rx_padding);
 }
 
-void etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
+ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
 {
     int ring_nbr = 0;           /* Always use ring0 (no filer) */
 
     if (etsec->rx_buffer_len != 0) {
         RING_DEBUG("%s: We can't receive now,"
                    " a buffer is already in the pipe\n", __func__);
-        return;
+        return 0;
     }
 
     if (etsec->regs[RSTAT].value & 1 << (23 - ring_nbr)) {
         RING_DEBUG("%s: The ring is halted\n", __func__);
-        return;
+        return -1;
     }
 
     if (etsec->regs[DMACTRL].value & DMACTRL_GRS) {
         RING_DEBUG("%s: Graceful receive stop\n", __func__);
-        return;
+        return -1;
     }
 
     if (!(etsec->regs[MACCFG1].value & MACCFG1_RX_EN)) {
         RING_DEBUG("%s: MAC Receive not enabled\n", __func__);
-        return;
+        return -1;
     }
 
     if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
         /* CRC is not in the packet yet, so short frame is below 60 bytes */
         RING_DEBUG("%s: Drop short frame\n", __func__);
-        return;
+        return -1;
     }
 
     rx_init_frame(etsec, buf, size);
 
     etsec_walk_rx_ring(etsec, ring_nbr);
+
+    return size;
 }
 
 void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.4 05/12] etsec: Move etsec_can_receive into etsec_receive

[Jason Wang]

On 07/14/2015 03:53 PM, Fam Zheng wrote:
> When etsec_reset returns 0, peer would queue the packet as if
> .can_receive returns false. Drop etsec_can_receive and let etsec_receive
> carry the semantics.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/net/fsl_etsec/etsec.c | 11 +----------
>  hw/net/fsl_etsec/etsec.h |  2 +-
>  hw/net/fsl_etsec/rings.c | 14 ++++++++------
>  3 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index c57365f..f5170ae 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -338,13 +338,6 @@ static void etsec_reset(DeviceState *d)
>          MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
>  }
>  
> -static int etsec_can_receive(NetClientState *nc)
> -{
> -    eTSEC *etsec = qemu_get_nic_opaque(nc);
> -
> -    return etsec->rx_buffer_len == 0;
> -}
> -
>  static ssize_t etsec_receive(NetClientState *nc,
>                               const uint8_t  *buf,
>                               size_t          size)
> @@ -355,8 +348,7 @@ static ssize_t etsec_receive(NetClientState *nc,
>      fprintf(stderr, "%s receive size:%d\n", etsec->nic->nc.name, size);
>      qemu_hexdump(buf, stderr, "", size);
>  #endif
> -    etsec_rx_ring_write(etsec, buf, size);
> -    return size;
> +    return etsec_rx_ring_write(etsec, buf, size);
>  }
>  
>  
> @@ -370,7 +362,6 @@ static void etsec_set_link_status(NetClientState *nc)
>  static NetClientInfo net_etsec_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_NIC,
>      .size = sizeof(NICState),
> -    .can_receive = etsec_can_receive,
>      .receive = etsec_receive,
>      .link_status_changed = etsec_set_link_status,
>  };
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index 78d2c57..fc41773 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -162,7 +162,7 @@ DeviceState *etsec_create(hwaddr        base,
>  
>  void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
>  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
> -void etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> +ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
>  
>  void etsec_write_miim(eTSEC          *etsec,
>                        eTSEC_Register *reg,
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d4a494f..a11280b 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -481,40 +481,42 @@ static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, size_t size)
>                 etsec->rx_buffer_len, etsec->rx_padding);
>  }
>  
> -void etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
> +ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
>  {
>      int ring_nbr = 0;           /* Always use ring0 (no filer) */
>  
>      if (etsec->rx_buffer_len != 0) {
>          RING_DEBUG("%s: We can't receive now,"
>                     " a buffer is already in the pipe\n", __func__);
> -        return;
> +        return 0;

Is queue be flushed when rx buffer is available again?

>      }
>  
>      if (etsec->regs[RSTAT].value & 1 << (23 - ring_nbr)) {
>          RING_DEBUG("%s: The ring is halted\n", __func__);
> -        return;
> +        return -1;
>      }
>  
>      if (etsec->regs[DMACTRL].value & DMACTRL_GRS) {
>          RING_DEBUG("%s: Graceful receive stop\n", __func__);
> -        return;
> +        return -1;
>      }
>  
>      if (!(etsec->regs[MACCFG1].value & MACCFG1_RX_EN)) {
>          RING_DEBUG("%s: MAC Receive not enabled\n", __func__);
> -        return;
> +        return -1;
>      }
>  
>      if ((etsec->regs[RCTRL].value & RCTRL_RSF) && (size < 60)) {
>          /* CRC is not in the packet yet, so short frame is below 60 bytes */
>          RING_DEBUG("%s: Drop short frame\n", __func__);
> -        return;
> +        return -1;
>      }
>  
>      rx_init_frame(etsec, buf, size);
>  
>      etsec_walk_rx_ring(etsec, ring_nbr);
> +
> +    return size;
>  }
>  
>  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)

Re: [Qemu-devel] [PATCH for-2.4 05/12] etsec: Move etsec_can_receive into etsec_receive

[Fam Zheng]

On Tue, 07/14 17:30, Jason Wang wrote:
> 
> 
> On 07/14/2015 03:53 PM, Fam Zheng wrote:
> > When etsec_reset returns 0, peer would queue the packet as if
> > .can_receive returns false. Drop etsec_can_receive and let etsec_receive
> > carry the semantics.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/net/fsl_etsec/etsec.c | 11 +----------
> >  hw/net/fsl_etsec/etsec.h |  2 +-
> >  hw/net/fsl_etsec/rings.c | 14 ++++++++------
> >  3 files changed, 10 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> > index c57365f..f5170ae 100644
> > --- a/hw/net/fsl_etsec/etsec.c
> > +++ b/hw/net/fsl_etsec/etsec.c
> > @@ -338,13 +338,6 @@ static void etsec_reset(DeviceState *d)
> >          MII_SR_100X_FD_CAPS     | MII_SR_100T4_CAPS;
> >  }
> >  
> > -static int etsec_can_receive(NetClientState *nc)
> > -{
> > -    eTSEC *etsec = qemu_get_nic_opaque(nc);
> > -
> > -    return etsec->rx_buffer_len == 0;
> > -}
> > -
> >  static ssize_t etsec_receive(NetClientState *nc,
> >                               const uint8_t  *buf,
> >                               size_t          size)
> > @@ -355,8 +348,7 @@ static ssize_t etsec_receive(NetClientState *nc,
> >      fprintf(stderr, "%s receive size:%d\n", etsec->nic->nc.name, size);
> >      qemu_hexdump(buf, stderr, "", size);
> >  #endif
> > -    etsec_rx_ring_write(etsec, buf, size);
> > -    return size;
> > +    return etsec_rx_ring_write(etsec, buf, size);
> >  }
> >  
> >  
> > @@ -370,7 +362,6 @@ static void etsec_set_link_status(NetClientState *nc)
> >  static NetClientInfo net_etsec_info = {
> >      .type = NET_CLIENT_OPTIONS_KIND_NIC,
> >      .size = sizeof(NICState),
> > -    .can_receive = etsec_can_receive,
> >      .receive = etsec_receive,
> >      .link_status_changed = etsec_set_link_status,
> >  };
> > diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> > index 78d2c57..fc41773 100644
> > --- a/hw/net/fsl_etsec/etsec.h
> > +++ b/hw/net/fsl_etsec/etsec.h
> > @@ -162,7 +162,7 @@ DeviceState *etsec_create(hwaddr        base,
> >  
> >  void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr);
> >  void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr);
> > -void etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> > +ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size);
> >  
> >  void etsec_write_miim(eTSEC          *etsec,
> >                        eTSEC_Register *reg,
> > diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> > index d4a494f..a11280b 100644
> > --- a/hw/net/fsl_etsec/rings.c
> > +++ b/hw/net/fsl_etsec/rings.c
> > @@ -481,40 +481,42 @@ static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, size_t size)
> >                 etsec->rx_buffer_len, etsec->rx_padding);
> >  }
> >  
> > -void etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
> > +ssize_t etsec_rx_ring_write(eTSEC *etsec, const uint8_t *buf, size_t size)
> >  {
> >      int ring_nbr = 0;           /* Always use ring0 (no filer) */
> >  
> >      if (etsec->rx_buffer_len != 0) {
> >          RING_DEBUG("%s: We can't receive now,"
> >                     " a buffer is already in the pipe\n", __func__);
> > -        return;
> > +        return 0;
> 
> Is queue be flushed when rx buffer is available again?

Previously not, and it's fixed in patch 6.

Fam

[Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Fam Zheng]

The BH will be scheduled when etsec->rx_buffer_len is becoming 0, which
is the condition of queuing.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/fsl_etsec/etsec.c | 9 +++++++++
 hw/net/fsl_etsec/etsec.h | 2 ++
 hw/net/fsl_etsec/rings.c | 1 +
 3 files changed, 12 insertions(+)

diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index f5170ae..fcde9b4 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -366,6 +366,13 @@ static NetClientInfo net_etsec_info = {
     .link_status_changed = etsec_set_link_status,
 };
 
+static void etsec_flush_rx_queue(void *opaque)
+{
+    eTSEC *etsec = opaque;
+
+    qemu_flush_queued_packets(qemu_get_queue(etsec->nic));
+}
+
 static void etsec_realize(DeviceState *dev, Error **errp)
 {
     eTSEC        *etsec = ETSEC_COMMON(dev);
@@ -378,6 +385,8 @@ static void etsec_realize(DeviceState *dev, Error **errp)
     etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
     etsec->ptimer = ptimer_init(etsec->bh);
     ptimer_set_freq(etsec->ptimer, 100);
+
+    etsec->flush_bh = qemu_bh_new(etsec_flush_rx_queue, etsec);
 }
 
 static void etsec_instance_init(Object *obj)
diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index fc41773..05bb7f8 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -144,6 +144,8 @@ typedef struct eTSEC {
     QEMUBH *bh;
     struct ptimer_state *ptimer;
 
+    QEMUBH *flush_bh;
+
 } eTSEC;
 
 #define TYPE_ETSEC_COMMON "eTSEC"
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index a11280b..7aae93e 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -646,6 +646,7 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
     } else {
         etsec->rx_buffer_len = 0;
         etsec->rx_buffer     = NULL;
+        qemu_bh_schedule(etsec->flush_bh);
     }
 
     RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data);
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Jason Wang]

On 07/14/2015 03:53 PM, Fam Zheng wrote:
> The BH will be scheduled when etsec->rx_buffer_len is becoming 0, which
> is the condition of queuing.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>

I suggest to squash this into patch 05/12 to unbreak bisection.

And can we do this without a bh? Otherwise, we may need to stop and
restart the bh during vm stop and start?

> ---
>  hw/net/fsl_etsec/etsec.c | 9 +++++++++
>  hw/net/fsl_etsec/etsec.h | 2 ++
>  hw/net/fsl_etsec/rings.c | 1 +
>  3 files changed, 12 insertions(+)
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index f5170ae..fcde9b4 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -366,6 +366,13 @@ static NetClientInfo net_etsec_info = {
>      .link_status_changed = etsec_set_link_status,
>  };
>  
> +static void etsec_flush_rx_queue(void *opaque)
> +{
> +    eTSEC *etsec = opaque;
> +
> +    qemu_flush_queued_packets(qemu_get_queue(etsec->nic));
> +}
> +
>  static void etsec_realize(DeviceState *dev, Error **errp)
>  {
>      eTSEC        *etsec = ETSEC_COMMON(dev);
> @@ -378,6 +385,8 @@ static void etsec_realize(DeviceState *dev, Error **errp)
>      etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
>      etsec->ptimer = ptimer_init(etsec->bh);
>      ptimer_set_freq(etsec->ptimer, 100);
> +
> +    etsec->flush_bh = qemu_bh_new(etsec_flush_rx_queue, etsec);
>  }
>  
>  static void etsec_instance_init(Object *obj)
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index fc41773..05bb7f8 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -144,6 +144,8 @@ typedef struct eTSEC {
>      QEMUBH *bh;
>      struct ptimer_state *ptimer;
>  
> +    QEMUBH *flush_bh;
> +
>  } eTSEC;
>  
>  #define TYPE_ETSEC_COMMON "eTSEC"
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index a11280b..7aae93e 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -646,6 +646,7 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
>      } else {
>          etsec->rx_buffer_len = 0;
>          etsec->rx_buffer     = NULL;
> +        qemu_bh_schedule(etsec->flush_bh);
>      }
>  
>      RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data);

Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Fam Zheng]

On Tue, 07/14 17:33, Jason Wang wrote:
> 
> 
> On 07/14/2015 03:53 PM, Fam Zheng wrote:
> > The BH will be scheduled when etsec->rx_buffer_len is becoming 0, which
> > is the condition of queuing.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> I suggest to squash this into patch 05/12 to unbreak bisection.

05/12 didn't change the logic, it just moved the semantics from returning 0 in
.can_receive() to .receive(). So I think it's OK to split to make reviewing
easier.

> 
> And can we do this without a bh? Otherwise, we may need to stop and
> restart the bh during vm stop and start?

A bh doesn't hurt when vm stop and restart (we get superfluous flush),
otherwise the call stack could go very deap with a long queue, something like:

    etsec_receive
      etsec_rx_ring_write
        etsec_walk_rx_ring
          etsec_flush_rx_queue
            qemu_flush_queued_packets
              ...
                etsec_receive
                  etsec_rx_ring_write
                    etsec_walk_rx_ring
                      etsec_flush_rx_queue
                        qemu_flush_queued_packets
                          /* loop */
                            ...

> 
> > ---
> >  hw/net/fsl_etsec/etsec.c | 9 +++++++++
> >  hw/net/fsl_etsec/etsec.h | 2 ++
> >  hw/net/fsl_etsec/rings.c | 1 +
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> > index f5170ae..fcde9b4 100644
> > --- a/hw/net/fsl_etsec/etsec.c
> > +++ b/hw/net/fsl_etsec/etsec.c
> > @@ -366,6 +366,13 @@ static NetClientInfo net_etsec_info = {
> >      .link_status_changed = etsec_set_link_status,
> >  };
> >  
> > +static void etsec_flush_rx_queue(void *opaque)
> > +{
> > +    eTSEC *etsec = opaque;
> > +
> > +    qemu_flush_queued_packets(qemu_get_queue(etsec->nic));
> > +}
> > +
> >  static void etsec_realize(DeviceState *dev, Error **errp)
> >  {
> >      eTSEC        *etsec = ETSEC_COMMON(dev);
> > @@ -378,6 +385,8 @@ static void etsec_realize(DeviceState *dev, Error **errp)
> >      etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
> >      etsec->ptimer = ptimer_init(etsec->bh);
> >      ptimer_set_freq(etsec->ptimer, 100);
> > +
> > +    etsec->flush_bh = qemu_bh_new(etsec_flush_rx_queue, etsec);
> >  }
> >  
> >  static void etsec_instance_init(Object *obj)
> > diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> > index fc41773..05bb7f8 100644
> > --- a/hw/net/fsl_etsec/etsec.h
> > +++ b/hw/net/fsl_etsec/etsec.h
> > @@ -144,6 +144,8 @@ typedef struct eTSEC {
> >      QEMUBH *bh;
> >      struct ptimer_state *ptimer;
> >  
> > +    QEMUBH *flush_bh;
> > +
> >  } eTSEC;
> >  
> >  #define TYPE_ETSEC_COMMON "eTSEC"
> > diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> > index a11280b..7aae93e 100644
> > --- a/hw/net/fsl_etsec/rings.c
> > +++ b/hw/net/fsl_etsec/rings.c
> > @@ -646,6 +646,7 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
> >      } else {
> >          etsec->rx_buffer_len = 0;
> >          etsec->rx_buffer     = NULL;
> > +        qemu_bh_schedule(etsec->flush_bh);
> >      }
> >  
> >      RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data);
> 

Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Jason Wang]

On 07/14/2015 05:48 PM, Fam Zheng wrote:
> On Tue, 07/14 17:33, Jason Wang wrote:
>>
>> On 07/14/2015 03:53 PM, Fam Zheng wrote:
>>> The BH will be scheduled when etsec->rx_buffer_len is becoming 0, which
>>> is the condition of queuing.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> I suggest to squash this into patch 05/12 to unbreak bisection.
> 05/12 didn't change the logic, it just moved the semantics from returning 0 in
> .can_receive() to .receive(). So I think it's OK to split to make reviewing
> easier.

Ok.

>
>> And can we do this without a bh? Otherwise, we may need to stop and
>> restart the bh during vm stop and start?
> A bh doesn't hurt when vm stop and restart (we get superfluous flush),

The problem is qemu_flush_queued_packets() does not check runstate. So
it may call .receive() which may modify guest state (DMA or registers).

> otherwise the call stack could go very deap with a long queue, something like:
>
>     etsec_receive
>       etsec_rx_ring_write
>         etsec_walk_rx_ring
>           etsec_flush_rx_queue
>             qemu_flush_queued_packets
>               ...
>                 etsec_receive
>                   etsec_rx_ring_write
>                     etsec_walk_rx_ring
>                       etsec_flush_rx_queue
>                         qemu_flush_queued_packets
>                           /* loop */
>                             ...

I get the point, thanks.

>>> ---
>>>  hw/net/fsl_etsec/etsec.c | 9 +++++++++
>>>  hw/net/fsl_etsec/etsec.h | 2 ++
>>>  hw/net/fsl_etsec/rings.c | 1 +
>>>  3 files changed, 12 insertions(+)
>>>
>>> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
>>> index f5170ae..fcde9b4 100644
>>> --- a/hw/net/fsl_etsec/etsec.c
>>> +++ b/hw/net/fsl_etsec/etsec.c
>>> @@ -366,6 +366,13 @@ static NetClientInfo net_etsec_info = {
>>>      .link_status_changed = etsec_set_link_status,
>>>  };
>>>  
>>> +static void etsec_flush_rx_queue(void *opaque)
>>> +{
>>> +    eTSEC *etsec = opaque;
>>> +
>>> +    qemu_flush_queued_packets(qemu_get_queue(etsec->nic));
>>> +}
>>> +
>>>  static void etsec_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      eTSEC        *etsec = ETSEC_COMMON(dev);
>>> @@ -378,6 +385,8 @@ static void etsec_realize(DeviceState *dev, Error **errp)
>>>      etsec->bh     = qemu_bh_new(etsec_timer_hit, etsec);
>>>      etsec->ptimer = ptimer_init(etsec->bh);
>>>      ptimer_set_freq(etsec->ptimer, 100);
>>> +
>>> +    etsec->flush_bh = qemu_bh_new(etsec_flush_rx_queue, etsec);
>>>  }
>>>  
>>>  static void etsec_instance_init(Object *obj)
>>> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
>>> index fc41773..05bb7f8 100644
>>> --- a/hw/net/fsl_etsec/etsec.h
>>> +++ b/hw/net/fsl_etsec/etsec.h
>>> @@ -144,6 +144,8 @@ typedef struct eTSEC {
>>>      QEMUBH *bh;
>>>      struct ptimer_state *ptimer;
>>>  
>>> +    QEMUBH *flush_bh;
>>> +
>>>  } eTSEC;
>>>  
>>>  #define TYPE_ETSEC_COMMON "eTSEC"
>>> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
>>> index a11280b..7aae93e 100644
>>> --- a/hw/net/fsl_etsec/rings.c
>>> +++ b/hw/net/fsl_etsec/rings.c
>>> @@ -646,6 +646,7 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
>>>      } else {
>>>          etsec->rx_buffer_len = 0;
>>>          etsec->rx_buffer     = NULL;
>>> +        qemu_bh_schedule(etsec->flush_bh);
>>>      }
>>>  
>>>      RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data);

Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Fam Zheng]

On Wed, 07/15 13:10, Jason Wang wrote:
> >> And can we do this without a bh? Otherwise, we may need to stop and
> >> restart the bh during vm stop and start?
> > A bh doesn't hurt when vm stop and restart (we get superfluous flush),
> 
> The problem is qemu_flush_queued_packets() does not check runstate. So
> it may call .receive() which may modify guest state (DMA or registers).

You're right, .can_receive will be called incorrectly if the following sequence
of events is processed by main loop right after we schedule it:

 1) QMP 'stop' command:
    Runstate is changed to STOP.

 2) tap read:
    A new packet is read in, but since qemu_can_send_packet is false, it will
    be queued.

 3) aio_dispatch:
    This BH is called too late here, and the queue is flushed, which calls
    .receive().

An ideal fix would be stopping tap with a vmstate handler, but for this patch,
does the following work?

diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index f5170ae..0f5cf44 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -342,13 +342,22 @@ static ssize_t etsec_receive(NetClientState *nc,
                              const uint8_t  *buf,
                              size_t          size)
 {
+    ssize_t ret;
     eTSEC *etsec = qemu_get_nic_opaque(nc);
 
 #if defined(HEX_DUMP)
     fprintf(stderr, "%s receive size:%d\n", etsec->nic->nc.name, size);
     qemu_hexdump(buf, stderr, "", size);
 #endif
-    return etsec_rx_ring_write(etsec, buf, size);
+    /* Flush is unnecessary as are already in receiving path */
+    etsec->need_flush = false;
+    ret = etsec_rx_ring_write(etsec, buf, size);
+    if (ret == 0) {
+        /* The packet will be queued, let's flush it when buffer is avilable
+         * again. */
+        etsec->need_flush = true;
+    }
+    return ret;
 }
 
 
diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
index fc41773..e7dc0a4 100644
--- a/hw/net/fsl_etsec/etsec.h
+++ b/hw/net/fsl_etsec/etsec.h
@@ -144,6 +144,8 @@ typedef struct eTSEC {
     QEMUBH *bh;
     struct ptimer_state *ptimer;
 
+    /* Whether we should flush the rx queue when buffer becomes available. */
+    bool need_flush;
 } eTSEC;
 
 #define TYPE_ETSEC_COMMON "eTSEC"
diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index a11280b..68e7b6d 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -646,6 +646,9 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
     } else {
         etsec->rx_buffer_len = 0;
         etsec->rx_buffer     = NULL;
+        if (etsec->need_flush) {
+            qemu_flush_queued_packets(qemu_get_queue(etsec->nic));
+        }
     }
 
     RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data);

Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Jason Wang]

On 07/15/2015 02:01 PM, Fam Zheng wrote:
> On Wed, 07/15 13:10, Jason Wang wrote:
>>>> And can we do this without a bh? Otherwise, we may need to stop and
>>>> restart the bh during vm stop and start?
>>> A bh doesn't hurt when vm stop and restart (we get superfluous flush),
>> The problem is qemu_flush_queued_packets() does not check runstate. So
>> it may call .receive() which may modify guest state (DMA or registers).
> You're right, .can_receive will be called incorrectly if the following sequence
> of events is processed by main loop right after we schedule it:
>
>  1) QMP 'stop' command:
>     Runstate is changed to STOP.
>
>  2) tap read:
>     A new packet is read in, but since qemu_can_send_packet is false, it will
>     be queued.
>
>  3) aio_dispatch:
>     This BH is called too late here, and the queue is flushed, which calls
>     .receive().
>
> An ideal fix would be stopping tap with a vmstate handler, but for this patch,
> does the following work?

Looks good for me. How about axienet then since in your series it also
uses a bh?

>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index f5170ae..0f5cf44 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -342,13 +342,22 @@ static ssize_t etsec_receive(NetClientState *nc,
>                               const uint8_t  *buf,
>                               size_t          size)
>  {
> +    ssize_t ret;
>      eTSEC *etsec = qemu_get_nic_opaque(nc);
>  
>  #if defined(HEX_DUMP)
>      fprintf(stderr, "%s receive size:%d\n", etsec->nic->nc.name, size);
>      qemu_hexdump(buf, stderr, "", size);
>  #endif
> -    return etsec_rx_ring_write(etsec, buf, size);
> +    /* Flush is unnecessary as are already in receiving path */
> +    etsec->need_flush = false;
> +    ret = etsec_rx_ring_write(etsec, buf, size);
> +    if (ret == 0) {
> +        /* The packet will be queued, let's flush it when buffer is avilable
> +         * again. */
> +        etsec->need_flush = true;
> +    }
> +    return ret;
>  }
>  
>  
> diff --git a/hw/net/fsl_etsec/etsec.h b/hw/net/fsl_etsec/etsec.h
> index fc41773..e7dc0a4 100644
> --- a/hw/net/fsl_etsec/etsec.h
> +++ b/hw/net/fsl_etsec/etsec.h
> @@ -144,6 +144,8 @@ typedef struct eTSEC {
>      QEMUBH *bh;
>      struct ptimer_state *ptimer;
>  
> +    /* Whether we should flush the rx queue when buffer becomes available. */
> +    bool need_flush;
>  } eTSEC;
>  
>  #define TYPE_ETSEC_COMMON "eTSEC"
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index a11280b..68e7b6d 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -646,6 +646,9 @@ void etsec_walk_rx_ring(eTSEC *etsec, int ring_nbr)
>      } else {
>          etsec->rx_buffer_len = 0;
>          etsec->rx_buffer     = NULL;
> +        if (etsec->need_flush) {
> +            qemu_flush_queued_packets(qemu_get_queue(etsec->nic));
> +        }
>      }
>  
>      RING_DEBUG("eTSEC End of ring_write: remaining_data:%zu\n", remaining_data);
>
>

Re: [Qemu-devel] [PATCH for-2.4 06/12] etsec: Flush queue when rx buffer is consumed

[Fam Zheng]

On Wed, 07/15 15:38, Jason Wang wrote:
> 
> 
> On 07/15/2015 02:01 PM, Fam Zheng wrote:
> > On Wed, 07/15 13:10, Jason Wang wrote:
> >>>> And can we do this without a bh? Otherwise, we may need to stop and
> >>>> restart the bh during vm stop and start?
> >>> A bh doesn't hurt when vm stop and restart (we get superfluous flush),
> >> The problem is qemu_flush_queued_packets() does not check runstate. So
> >> it may call .receive() which may modify guest state (DMA or registers).
> > You're right, .can_receive will be called incorrectly if the following sequence
> > of events is processed by main loop right after we schedule it:
> >
> >  1) QMP 'stop' command:
> >     Runstate is changed to STOP.
> >
> >  2) tap read:
> >     A new packet is read in, but since qemu_can_send_packet is false, it will
> >     be queued.
> >
> >  3) aio_dispatch:
> >     This BH is called too late here, and the queue is flushed, which calls
> >     .receive().
> >
> > An ideal fix would be stopping tap with a vmstate handler, but for this patch,
> > does the following work?
> 
> Looks good for me. How about axienet then since in your series it also
> uses a bh?

Good point, the same applies, I'll fix that too.

Fam

[Qemu-devel] [PATCH for-2.4 07/12] mcf_fec: Drop mcf_fec_can_receive

[Fam Zheng]

The semantics of .can_receive requires us to flush the queue explicitly
when s->rx_enabled becomes true after it returns 0, but the packet being
queued is not meaningful since the guest hasn't activated the card.
Let's just drop the packet in this case.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/mcf_fec.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/net/mcf_fec.c b/hw/net/mcf_fec.c
index 0255612..0b17c1f 100644
--- a/hw/net/mcf_fec.c
+++ b/hw/net/mcf_fec.c
@@ -351,12 +351,6 @@ static void mcf_fec_write(void *opaque, hwaddr addr,
     mcf_fec_update(s);
 }
 
-static int mcf_fec_can_receive(NetClientState *nc)
-{
-    mcf_fec_state *s = qemu_get_nic_opaque(nc);
-    return s->rx_enabled;
-}
-
 static ssize_t mcf_fec_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
     mcf_fec_state *s = qemu_get_nic_opaque(nc);
@@ -370,7 +364,7 @@ static ssize_t mcf_fec_receive(NetClientState *nc, const uint8_t *buf, size_t si
 
     DPRINTF("do_rx len %d\n", size);
     if (!s->rx_enabled) {
-        fprintf(stderr, "mcf_fec_receive: Unexpected packet\n");
+        return -1;
     }
     /* 4 bytes for the CRC.  */
     size += 4;
@@ -442,7 +436,6 @@ static const MemoryRegionOps mcf_fec_ops = {
 static NetClientInfo net_mcf_fec_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = mcf_fec_can_receive,
     .receive = mcf_fec_receive,
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 08/12] milkymist-minimac2: Flush queued packets when link comes up

[Fam Zheng]

Drop .can_receive and move the semantics into minimac2_rx, by returning
0.

That is once minimac2_rx returns 0, incoming packets will be queued
until the queue is explicitly flushed. We do this when s->regs[R_STATE0]
or s->regs[R_STATE1] is changed in minimac2_write.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/milkymist-minimac2.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/net/milkymist-minimac2.c b/hw/net/milkymist-minimac2.c
index f06afaa..cd38a06 100644
--- a/hw/net/milkymist-minimac2.c
+++ b/hw/net/milkymist-minimac2.c
@@ -303,8 +303,7 @@ static ssize_t minimac2_rx(NetClientState *nc, const uint8_t *buf, size_t size)
         r_state = R_STATE1;
         rx_buf = s->rx1_buf;
     } else {
-        trace_milkymist_minimac2_drop_rx_frame(buf);
-        return size;
+        return 0;
     }
 
     /* assemble frame */
@@ -354,6 +353,7 @@ minimac2_read(void *opaque, hwaddr addr, unsigned size)
     return r;
 }
 
+static int minimac2_can_rx(MilkymistMinimac2State *s);
 static void
 minimac2_write(void *opaque, hwaddr addr, uint64_t value,
                unsigned size)
@@ -387,6 +387,9 @@ minimac2_write(void *opaque, hwaddr addr, uint64_t value,
     case R_STATE1:
         s->regs[addr] = value;
         update_rx_interrupt(s);
+        if (minimac2_can_rx(s)) {
+            qemu_flush_queued_packets(qemu_get_queue(s->nic));
+        }
         break;
     case R_SETUP:
     case R_COUNT0:
@@ -411,10 +414,8 @@ static const MemoryRegionOps minimac2_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static int minimac2_can_rx(NetClientState *nc)
+static int minimac2_can_rx(MilkymistMinimac2State *s)
 {
-    MilkymistMinimac2State *s = qemu_get_nic_opaque(nc);
-
     if (s->regs[R_STATE0] == STATE_LOADED) {
         return 1;
     }
@@ -445,7 +446,6 @@ static void milkymist_minimac2_reset(DeviceState *d)
 static NetClientInfo net_milkymist_minimac2_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = minimac2_can_rx,
     .receive = minimac2_rx,
 };
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.4 08/12] milkymist-minimac2: Flush queued packets when link comes up

[Michael Walle]

Am 2015-07-14 09:53, schrieb Fam Zheng:
> Drop .can_receive and move the semantics into minimac2_rx, by returning
> 0.
> 
> That is once minimac2_rx returns 0, incoming packets will be queued
> until the queue is explicitly flushed. We do this when 
> s->regs[R_STATE0]
> or s->regs[R_STATE1] is changed in minimac2_write.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/net/milkymist-minimac2.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/milkymist-minimac2.c b/hw/net/milkymist-minimac2.c
> index f06afaa..cd38a06 100644
> --- a/hw/net/milkymist-minimac2.c
> +++ b/hw/net/milkymist-minimac2.c
> @@ -303,8 +303,7 @@ static ssize_t minimac2_rx(NetClientState *nc,
> const uint8_t *buf, size_t size)
>          r_state = R_STATE1;
>          rx_buf = s->rx1_buf;
>      } else {
> -        trace_milkymist_minimac2_drop_rx_frame(buf);
> -        return size;
> +        return 0;

trace removed?

>      }
> 
>      /* assemble frame */
> @@ -354,6 +353,7 @@ minimac2_read(void *opaque, hwaddr addr, unsigned 
> size)
>      return r;
>  }
> 
> +static int minimac2_can_rx(MilkymistMinimac2State *s);
>  static void
>  minimac2_write(void *opaque, hwaddr addr, uint64_t value,
>                 unsigned size)
> @@ -387,6 +387,9 @@ minimac2_write(void *opaque, hwaddr addr, uint64_t 
> value,
>      case R_STATE1:
>          s->regs[addr] = value;
>          update_rx_interrupt(s);
> +        if (minimac2_can_rx(s)) {
> +            qemu_flush_queued_packets(qemu_get_queue(s->nic));
> +        }
>          break;
>      case R_SETUP:
>      case R_COUNT0:
> @@ -411,10 +414,8 @@ static const MemoryRegionOps minimac2_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
> 
> -static int minimac2_can_rx(NetClientState *nc)
> +static int minimac2_can_rx(MilkymistMinimac2State *s)
>  {
> -    MilkymistMinimac2State *s = qemu_get_nic_opaque(nc);
> -
>      if (s->regs[R_STATE0] == STATE_LOADED) {
>          return 1;
>      }

please move this above the minimac2_write call, so the forward 
declaration isn't necessary anymore.



-michael

Re: [Qemu-devel] [PATCH for-2.4 08/12] milkymist-minimac2: Flush queued packets when link comes up

[Fam Zheng]

On Tue, 07/14 13:02, Michael Walle wrote:
> Am 2015-07-14 09:53, schrieb Fam Zheng:
> >Drop .can_receive and move the semantics into minimac2_rx, by returning
> >0.
> >
> >That is once minimac2_rx returns 0, incoming packets will be queued
> >until the queue is explicitly flushed. We do this when s->regs[R_STATE0]
> >or s->regs[R_STATE1] is changed in minimac2_write.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> > hw/net/milkymist-minimac2.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> >diff --git a/hw/net/milkymist-minimac2.c b/hw/net/milkymist-minimac2.c
> >index f06afaa..cd38a06 100644
> >--- a/hw/net/milkymist-minimac2.c
> >+++ b/hw/net/milkymist-minimac2.c
> >@@ -303,8 +303,7 @@ static ssize_t minimac2_rx(NetClientState *nc,
> >const uint8_t *buf, size_t size)
> >         r_state = R_STATE1;
> >         rx_buf = s->rx1_buf;
> >     } else {
> >-        trace_milkymist_minimac2_drop_rx_frame(buf);
> >-        return size;
> >+        return 0;
> 
> trace removed?

Because the frame is no longer dropped as we return 0 - it is queued and will
be sent again when we do qemu_flush_queued_packets later. Should I rename the
trace point?

Fam

> 
> >     }
> >
> >     /* assemble frame */
> >@@ -354,6 +353,7 @@ minimac2_read(void *opaque, hwaddr addr, unsigned
> >size)
> >     return r;
> > }
> >
> >+static int minimac2_can_rx(MilkymistMinimac2State *s);
> > static void
> > minimac2_write(void *opaque, hwaddr addr, uint64_t value,
> >                unsigned size)
> >@@ -387,6 +387,9 @@ minimac2_write(void *opaque, hwaddr addr, uint64_t
> >value,
> >     case R_STATE1:
> >         s->regs[addr] = value;
> >         update_rx_interrupt(s);
> >+        if (minimac2_can_rx(s)) {
> >+            qemu_flush_queued_packets(qemu_get_queue(s->nic));
> >+        }
> >         break;
> >     case R_SETUP:
> >     case R_COUNT0:
> >@@ -411,10 +414,8 @@ static const MemoryRegionOps minimac2_ops = {
> >     .endianness = DEVICE_NATIVE_ENDIAN,
> > };
> >
> >-static int minimac2_can_rx(NetClientState *nc)
> >+static int minimac2_can_rx(MilkymistMinimac2State *s)
> > {
> >-    MilkymistMinimac2State *s = qemu_get_nic_opaque(nc);
> >-
> >     if (s->regs[R_STATE0] == STATE_LOADED) {
> >         return 1;
> >     }
> 
> please move this above the minimac2_write call, so the forward declaration
> isn't necessary anymore.
> 
> 
> 
> -michael
> 

Re: [Qemu-devel] [PATCH for-2.4 08/12] milkymist-minimac2: Flush queued packets when link comes up

[Michael Walle]

Am 2015-07-14 13:07, schrieb Fam Zheng:
> On Tue, 07/14 13:02, Michael Walle wrote:
>> Am 2015-07-14 09:53, schrieb Fam Zheng:
>> >Drop .can_receive and move the semantics into minimac2_rx, by returning
>> >0.
>> >
>> >That is once minimac2_rx returns 0, incoming packets will be queued
>> >until the queue is explicitly flushed. We do this when s->regs[R_STATE0]
>> >or s->regs[R_STATE1] is changed in minimac2_write.
>> >
>> >Signed-off-by: Fam Zheng <famz@redhat.com>
>> >---
>> > hw/net/milkymist-minimac2.c | 12 ++++++------
>> > 1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/hw/net/milkymist-minimac2.c b/hw/net/milkymist-minimac2.c
>> >index f06afaa..cd38a06 100644
>> >--- a/hw/net/milkymist-minimac2.c
>> >+++ b/hw/net/milkymist-minimac2.c
>> >@@ -303,8 +303,7 @@ static ssize_t minimac2_rx(NetClientState *nc,
>> >const uint8_t *buf, size_t size)
>> >         r_state = R_STATE1;
>> >         rx_buf = s->rx1_buf;
>> >     } else {
>> >-        trace_milkymist_minimac2_drop_rx_frame(buf);
>> >-        return size;
>> >+        return 0;
>> 
>> trace removed?
> 
> Because the frame is no longer dropped as we return 0 - it is queued 
> and will
> be sent again when we do qemu_flush_queued_packets later. Should I 
> rename the
> trace point?

ah, ok. no i guess we won't need a tracepoint for that event. but please 
remove the tracepoint from trace-events, too.

-michael

[Qemu-devel] [PATCH for-2.4 09/12] mipsnet: Flush queued packets when receiving is enabled

[Fam Zheng]

Drop .can_receive and move the semantics to mipsnet_receive, by
returning 0.

After 0 is returned, we must flush the queue explicitly to restart it:
Call qemu_flush_queued_packets when s->busy or s->rx_count is being
updated.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/mipsnet.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
index c813e0c..f261011 100644
--- a/hw/net/mipsnet.c
+++ b/hw/net/mipsnet.c
@@ -80,7 +80,7 @@ static ssize_t mipsnet_receive(NetClientState *nc, const uint8_t *buf, size_t si
 
     trace_mipsnet_receive(size);
     if (!mipsnet_can_receive(nc))
-        return -1;
+        return 0;
 
     s->busy = 1;
 
@@ -134,6 +134,9 @@ static uint64_t mipsnet_ioport_read(void *opaque, hwaddr addr,
         if (s->rx_count) {
             s->rx_count--;
             ret = s->rx_buffer[s->rx_read++];
+            if (mipsnet_can_receive(s->nic->ncs)) {
+                qemu_flush_queued_packets(qemu_get_queue(s->nic));
+            }
         }
         break;
     /* Reads as zero. */
@@ -170,6 +173,9 @@ static void mipsnet_ioport_write(void *opaque, hwaddr addr,
         }
         s->busy = !!s->intctl;
         mipsnet_update_irq(s);
+        if (mipsnet_can_receive(s->nic->ncs)) {
+            qemu_flush_queued_packets(qemu_get_queue(s->nic));
+        }
         break;
     case MIPSNET_TX_DATA_BUFFER:
         s->tx_buffer[s->tx_written++] = val;
@@ -214,7 +220,6 @@ static const VMStateDescription vmstate_mipsnet = {
 static NetClientInfo net_mipsnet_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = mipsnet_can_receive,
     .receive = mipsnet_receive,
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 10/12] stellaris_enet: Flush queued packets when read done

[Fam Zheng]

If s->np reaches 31, the queue will be disabled by peer when it sees
stellaris_enet_can_receive() returns false, until we explicitly flushes
it which notifies the peer. Do this when guest is done reading all
existing data.

Move the semantics to stellaris_enet_receive, by returning 0 when the
buffer is full, so that new packets will be queued.  In
stellaris_enet_read, flush and restart the queue when guest has done
reading.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/stellaris_enet.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 278a654..21a4773 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -228,8 +228,7 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
     if ((s->rctl & SE_RCTL_RXEN) == 0)
         return -1;
     if (s->np >= 31) {
-        DPRINTF("Packet dropped\n");
-        return -1;
+        return 0;
     }
 
     DPRINTF("Received packet len=%zu\n", size);
@@ -260,13 +259,8 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
     return size;
 }
 
-static int stellaris_enet_can_receive(NetClientState *nc)
+static int stellaris_enet_can_receive(stellaris_enet_state *s)
 {
-    stellaris_enet_state *s = qemu_get_nic_opaque(nc);
-
-    if ((s->rctl & SE_RCTL_RXEN) == 0)
-        return 1;
-
     return (s->np < 31);
 }
 
@@ -307,6 +301,9 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
                 s->next_packet = 0;
             s->np--;
             DPRINTF("RX done np=%d\n", s->np);
+            if (!s->np && stellaris_enet_can_receive(s)) {
+                qemu_flush_queued_packets(qemu_get_queue(s->nic));
+            }
         }
         return val;
     }
@@ -454,7 +451,6 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
 static NetClientInfo net_stellaris_enet_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = stellaris_enet_can_receive,
     .receive = stellaris_enet_receive,
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 11/12] dp8393x: Flush packets when link comes up

[Fam Zheng]

.can_receive callback changes semantics that once return 0, backend will
try sending again until explicitly flushed, change the device to meet
that.

dp8393x_can_receive checks SONIC_CR_RXEN bit in SONIC_CR register and
SONIC_ISR_RBE bit in SONIC_ISR register, try flushing the queue when
either bit is being updated.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/dp8393x.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cd889bc..451ff72 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -327,9 +327,14 @@ static void dp8393x_do_stop_timer(dp8393xState *s)
     dp8393x_update_wt_regs(s);
 }
 
+static int dp8393x_can_receive(NetClientState *nc);
+
 static void dp8393x_do_receiver_enable(dp8393xState *s)
 {
     s->regs[SONIC_CR] &= ~SONIC_CR_RXDIS;
+    if (dp8393x_can_receive(s->nic->ncs)) {
+        qemu_flush_queued_packets(qemu_get_queue(s->nic));
+    }
 }
 
 static void dp8393x_do_receiver_disable(dp8393xState *s)
@@ -569,6 +574,9 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
                 dp8393x_do_read_rra(s);
             }
             dp8393x_update_irq(s);
+            if (dp8393x_can_receive(s->nic->ncs)) {
+                qemu_flush_queued_packets(qemu_get_queue(s->nic));
+            }
             break;
         /* Ignore least significant bit */
         case SONIC_RSA:
-- 
2.4.3

[Qemu-devel] [PATCH for-2.4 12/12] axienet: Flush queued packets when rx is done

[Fam Zheng]

eth_can_rx checks s->rxsize and returns false if it is non-zero. Because
of the .can_receive semantics change, this will make the incoming queue
disabled by peer, until it is explicitly flushed. So we should flush it
when s->rxsize is becoming zero.

Do it by adding a BH that calls qemu_flush_queued_packets after
decrementing s->rxsize in axienet_eth_rx_notify. BH is necessary to
avoid too deep recursive call stack.

The other conditions, "!axienet_rx_resetting(s) &&
axienet_rx_enabled(s)" are OK because enet_write already calls
qemu_flush_queued_packets when the register bits are changed.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/net/xilinx_axienet.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 9205770..bbc0ea8 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -28,6 +28,7 @@
 #include "net/checksum.h"
 
 #include "hw/stream.h"
+#include "qemu/main-loop.h"
 
 #define DPHY(x)
 
@@ -401,6 +402,8 @@ struct XilinxAXIEnet {
 
     uint8_t rxapp[CONTROL_PAYLOAD_SIZE];
     uint32_t rxappsize;
+
+    QEMUBH *flush_bh;
 };
 
 static void axienet_rx_reset(XilinxAXIEnet *s)
@@ -681,8 +684,15 @@ static int enet_match_addr(const uint8_t *buf, uint32_t f0, uint32_t f1)
     return match;
 }
 
+static void xilinx_flush_cb(void *opaque)
+{
+    XilinxAXIEnet *s = opaque;
+    qemu_flush_queued_packets(qemu_get_queue(s->nic));
+}
+
 static void axienet_eth_rx_notify(void *opaque)
 {
+    bool flush = false;
     XilinxAXIEnet *s = XILINX_AXI_ENET(opaque);
 
     while (s->rxappsize && stream_can_push(s->tx_control_dev,
@@ -701,9 +711,13 @@ static void axienet_eth_rx_notify(void *opaque)
         s->rxpos += ret;
         if (!s->rxsize) {
             s->regs[R_IS] |= IS_RX_COMPLETE;
+            flush = true;
         }
     }
     enet_update_irq(s);
+    if (flush) {
+        qemu_bh_schedule(s->flush_bh);
+    }
 }
 
 static ssize_t eth_rx(NetClientState *nc, const uint8_t *buf, size_t size)
@@ -967,6 +981,7 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
     s->TEMAC.parent = s;
 
     s->rxmem = g_malloc(s->c_rxmem);
+    s->flush_bh = qemu_bh_new(xilinx_flush_cb, s);
     return;
 
 xilinx_enet_realize_fail:
@@ -975,6 +990,12 @@ xilinx_enet_realize_fail:
     }
 }
 
+static void xilinx_enet_unrealize(DeviceState *dev, Error **errp)
+{
+    XilinxAXIEnet *s = XILINX_AXI_ENET(dev);
+    qemu_bh_delete(s->flush_bh);
+}
+
 static void xilinx_enet_init(Object *obj)
 {
     XilinxAXIEnet *s = XILINX_AXI_ENET(obj);
@@ -1020,6 +1041,7 @@ static void xilinx_enet_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = xilinx_enet_realize;
+    dc->unrealize = xilinx_enet_unrealize;
     dc->props = xilinx_enet_properties;
     dc->reset = xilinx_axienet_reset;
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.4 00/12] hw/net: Fix .can_receive() for NICs

[Wen Congyang]

On 07/14/2015 03:53 PM, Fam Zheng wrote:
> Since a90a742 "tap: Drop tap_can_send", all nics that returns false from
> .can_receive() are required to explicitly flush the incoming queue when the
> status of it is changing back to true, otherwise the backend will sop
> processing more rx packets.
> 
> The purpose of this callback is to tell the peer backend (tap, socket, etc)
> "hold on until guest consumes old data because my buffer is not ready". More
> often than not NICs also do this when driver deactivated the card or disabled
> rx, causing the packets being unnessarily queued, where they should actualy be
> dropped.
> 
> This series adds such missing qemu_flush_queued_packets calls for all NICs, and
> drops such unnecessary conditions in .can_receive(), so that NICs now:
> 
>   - returns false from .can_receive when guest buffers are busy.
>   - calls qemu_flush_queued_packets when buffers are available again.
>   - returns -1 from .receive when rx is not enabled.
> 
> e1000, ne2000, rocker and vmxnet3 are not included because they're fixed by
> other patches on the list and applied to Stefan's tree.
> 
> virtio-net is covered by another thread:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07377.html

When will the virtio-net patch be accepted? Without this patch, virtio-net + tap
can not work.

Thanks
Wen Congyang

> 
> All other NICs are okay, as they already do the flush on the state transition
> points.
> 
> Please review.
> 
> Fam Zheng (12):
>   xgmac: Drop packets with eth_can_rx is false.
>   pcnet: Drop pcnet_can_receive
>   eepro100: Drop nic_can_receive
>   usbnet: Drop usbnet_can_receive
>   etsec: Move etsec_can_receive into etsec_receive
>   etsec: Flush queue when rx buffer is consumed
>   mcf_fec: Drop mcf_fec_can_receive
>   milkymist-minimac2: Flush queued packets when link comes up
>   mipsnet: Flush queued packets when receiving is enabled
>   stellaris_enet: Flush queued packets when read done
>   dp8393x: Flush packets when link comes up
>   axienet: Flush queued packets when rx is done
> 
>  hw/net/dp8393x.c            |  8 ++++++++
>  hw/net/eepro100.c           | 11 -----------
>  hw/net/fsl_etsec/etsec.c    | 20 ++++++++++----------
>  hw/net/fsl_etsec/etsec.h    |  4 +++-
>  hw/net/fsl_etsec/rings.c    | 15 +++++++++------
>  hw/net/lance.c              |  1 -
>  hw/net/mcf_fec.c            |  9 +--------
>  hw/net/milkymist-minimac2.c | 12 ++++++------
>  hw/net/mipsnet.c            |  9 +++++++--
>  hw/net/pcnet-pci.c          |  1 -
>  hw/net/pcnet.c              |  9 ---------
>  hw/net/pcnet.h              |  1 -
>  hw/net/stellaris_enet.c     | 14 +++++---------
>  hw/net/xgmac.c              |  8 ++++----
>  hw/net/xilinx_axienet.c     | 22 ++++++++++++++++++++++
>  hw/usb/dev-network.c        | 20 ++++----------------
>  16 files changed, 79 insertions(+), 85 deletions(-)
> 

Re: [Qemu-devel] [PATCH for-2.4 00/12] hw/net: Fix .can_receive() for NICs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]

On Tue, Jul 14, 2015 at 04:34:19PM +0800, Wen Congyang wrote:
> On 07/14/2015 03:53 PM, Fam Zheng wrote:
> > Since a90a742 "tap: Drop tap_can_send", all nics that returns false from
> > .can_receive() are required to explicitly flush the incoming queue when the
> > status of it is changing back to true, otherwise the backend will sop
> > processing more rx packets.
> > 
> > The purpose of this callback is to tell the peer backend (tap, socket, etc)
> > "hold on until guest consumes old data because my buffer is not ready". More
> > often than not NICs also do this when driver deactivated the card or disabled
> > rx, causing the packets being unnessarily queued, where they should actualy be
> > dropped.
> > 
> > This series adds such missing qemu_flush_queued_packets calls for all NICs, and
> > drops such unnecessary conditions in .can_receive(), so that NICs now:
> > 
> >   - returns false from .can_receive when guest buffers are busy.
> >   - calls qemu_flush_queued_packets when buffers are available again.
> >   - returns -1 from .receive when rx is not enabled.
> > 
> > e1000, ne2000, rocker and vmxnet3 are not included because they're fixed by
> > other patches on the list and applied to Stefan's tree.
> > 
> > virtio-net is covered by another thread:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07377.html
> 
> When will the virtio-net patch be accepted? Without this patch, virtio-net + tap
> can not work.

Fam's recent net fixes are all-or-nothing.  We either have to merge all
of them or none of them, because the patches remove .can_receive().

I'm waiting until reviewers are satisfied on all patches.  The plan is
to merge them for QEMU 2.4.

If that isn't possible due to running out of time, I'll have to revert
Fam's older qemu_set_fd_handler2() refactoring patches which caused the
breakage you are seeing.

There is not much time left for QEMU 2.4 but we're close to happy on all
of Fam's patches now.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH for-2.4 00/12] hw/net: Fix .can_receive() for NICs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3037 bytes --]

On Tue, Jul 14, 2015 at 03:53:29PM +0800, Fam Zheng wrote:
> Since a90a742 "tap: Drop tap_can_send", all nics that returns false from
> .can_receive() are required to explicitly flush the incoming queue when the
> status of it is changing back to true, otherwise the backend will sop
> processing more rx packets.
> 
> The purpose of this callback is to tell the peer backend (tap, socket, etc)
> "hold on until guest consumes old data because my buffer is not ready". More
> often than not NICs also do this when driver deactivated the card or disabled
> rx, causing the packets being unnessarily queued, where they should actualy be
> dropped.
> 
> This series adds such missing qemu_flush_queued_packets calls for all NICs, and
> drops such unnecessary conditions in .can_receive(), so that NICs now:
> 
>   - returns false from .can_receive when guest buffers are busy.
>   - calls qemu_flush_queued_packets when buffers are available again.
>   - returns -1 from .receive when rx is not enabled.
> 
> e1000, ne2000, rocker and vmxnet3 are not included because they're fixed by
> other patches on the list and applied to Stefan's tree.
> 
> virtio-net is covered by another thread:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07377.html
> 
> All other NICs are okay, as they already do the flush on the state transition
> points.
> 
> Please review.
> 
> Fam Zheng (12):
>   xgmac: Drop packets with eth_can_rx is false.
>   pcnet: Drop pcnet_can_receive
>   eepro100: Drop nic_can_receive
>   usbnet: Drop usbnet_can_receive
>   etsec: Move etsec_can_receive into etsec_receive
>   etsec: Flush queue when rx buffer is consumed
>   mcf_fec: Drop mcf_fec_can_receive
>   milkymist-minimac2: Flush queued packets when link comes up
>   mipsnet: Flush queued packets when receiving is enabled
>   stellaris_enet: Flush queued packets when read done
>   dp8393x: Flush packets when link comes up
>   axienet: Flush queued packets when rx is done
> 
>  hw/net/dp8393x.c            |  8 ++++++++
>  hw/net/eepro100.c           | 11 -----------
>  hw/net/fsl_etsec/etsec.c    | 20 ++++++++++----------
>  hw/net/fsl_etsec/etsec.h    |  4 +++-
>  hw/net/fsl_etsec/rings.c    | 15 +++++++++------
>  hw/net/lance.c              |  1 -
>  hw/net/mcf_fec.c            |  9 +--------
>  hw/net/milkymist-minimac2.c | 12 ++++++------
>  hw/net/mipsnet.c            |  9 +++++++--
>  hw/net/pcnet-pci.c          |  1 -
>  hw/net/pcnet.c              |  9 ---------
>  hw/net/pcnet.h              |  1 -
>  hw/net/stellaris_enet.c     | 14 +++++---------
>  hw/net/xgmac.c              |  8 ++++----
>  hw/net/xilinx_axienet.c     | 22 ++++++++++++++++++++++
>  hw/usb/dev-network.c        | 20 ++++----------------
>  16 files changed, 79 insertions(+), 85 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

I'm waiting to see if Michael is happy with the virtio-net changes, then
we can apply these + hub + virtio-net + other outstanding fixes.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]