[Qemu-devel] [PULL 0/8] Net patches

[Qemu-devel] [PULL 0/8] Net patches

[Jason Wang]

The following changes since commit 4ffa88c99c54d2a30f79e3dbecec50b023eff1c8:

  Merge remote-tracking branch 'remotes/berrange/tags/pull-qcrypto-2017-11-08-1' into staging (2017-11-10 16:01:35 +0000)

are available in the git repository at:

  https://github.com/jasowang/qemu.git tags/net-pull-request

for you to fetch changes up to bb160b571fe469b03228d4502c75a18045978a74:

  net/socket: fix coverity issue (2017-11-13 18:05:12 +0800)

----------------------------------------------------------------

----------------------------------------------------------------
Jens Freimann (2):
      net: fix check for number of parameters to -netdev socket
      net/socket: fix coverity issue

Mao Zhongyi (4):
      colo-compare: Insert packet into the suitable position of packet queue directly
      colo-compare: compare the packet in a specified Connection
      colo-compare: Fix comments
      colo: Consolidate the duplicate code chunk into a routine

Mike Nawrocki (2):
      Fix eepro100 simple transmission mode
      Add new PCI ID for i82559a

 hw/net/eepro100.c    | 31 +++++++++++++-------------
 include/hw/compat.h  |  4 ++++
 include/hw/pci/pci.h |  1 +
 net/colo-compare.c   | 61 ++++++++++++++++++++++++++++++----------------------
 net/colo.c           | 18 +++++++++-------
 net/colo.h           |  1 +
 net/socket.c         |  6 +++---
 qemu-options.hx      |  2 +-
 8 files changed, 71 insertions(+), 53 deletions(-)

[Qemu-devel] [PULL 1/8] net: fix check for number of parameters to -netdev socket

[Jason Wang]

From: Jens Freimann <jfreimann@redhat.com>

Since commit 0f8c289ad "net: fix -netdev socket,fd= for UDP sockets"
we allow more than one parameter for -netdev socket. But now
we run into an assert when no parameter at all is specified

> qemu-system-x86_64 -netdev socket
socket.c:729: net_init_socket: Assertion `sock->has_udp' failed.

Fix this by reverting the change of the if condition done in 0f8c289ad.

Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-stable@nongnu.org
Fixes: 0f8c289ad539feb5135c545bea947b310a893f4b
Reported-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index e6b471c..83a2a31 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -695,8 +695,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
     assert(netdev->type == NET_CLIENT_DRIVER_SOCKET);
     sock = &netdev->u.socket;
 
-    if (sock->has_listen + sock->has_connect + sock->has_mcast +
-        sock->has_udp > 1) {
+    if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
+        sock->has_udp != 1) {
         error_setg(errp, "exactly one of listen=, connect=, mcast= or udp="
                    " is required");
         return -1;
-- 
2.7.4

[Qemu-devel] [PULL 2/8] colo-compare: Insert packet into the suitable position of packet queue directly

[Jason Wang]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Currently, a packet from pri_dev or sec_dev is fristly pushed at the
tail of the primary or secondary packet queue then sorted by the tcp
sequence number.

Now, this patch use g_queue_insert_sorted to insert the packet directly
into the suitable position to avoid ordering all packets each time when
a new packet is comming, thereby increasing efficiency.

In addition, consolidate the code that add a packet to the list of
Connection (primary or secondary) into a separate routine colo_insert_packet()
since the same chunk of code is called from two place.

Cc: Zhang Chen <zhangckid@gmail.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Zhang Chen <zhangckid@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/colo-compare.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index b3f35d7..54b6347 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -113,6 +113,26 @@ static gint seq_sorter(Packet *a, Packet *b, gpointer data)
 }
 
 /*
+ * Return 1 on success, if return 0 means the
+ * packet will be dropped
+ */
+static int colo_insert_packet(GQueue *queue, Packet *pkt)
+{
+    if (g_queue_get_length(queue) <= MAX_QUEUE_SIZE) {
+        if (pkt->ip->ip_p == IPPROTO_TCP) {
+            g_queue_insert_sorted(queue,
+                                  pkt,
+                                  (GCompareDataFunc)seq_sorter,
+                                  NULL);
+        } else {
+            g_queue_push_tail(queue, pkt);
+        }
+        return 1;
+    }
+    return 0;
+}
+
+/*
  * Return 0 on success, if return -1 means the pkt
  * is unsupported(arp and ipv6) and will be sent later
  */
@@ -149,28 +169,12 @@ static int packet_enqueue(CompareState *s, int mode)
     }
 
     if (mode == PRIMARY_IN) {
-        if (g_queue_get_length(&conn->primary_list) <=
-                               MAX_QUEUE_SIZE) {
-            g_queue_push_tail(&conn->primary_list, pkt);
-            if (conn->ip_proto == IPPROTO_TCP) {
-                g_queue_sort(&conn->primary_list,
-                             (GCompareDataFunc)seq_sorter,
-                             NULL);
-            }
-        } else {
+        if (!colo_insert_packet(&conn->primary_list, pkt)) {
             error_report("colo compare primary queue size too big,"
                          "drop packet");
         }
     } else {
-        if (g_queue_get_length(&conn->secondary_list) <=
-                               MAX_QUEUE_SIZE) {
-            g_queue_push_tail(&conn->secondary_list, pkt);
-            if (conn->ip_proto == IPPROTO_TCP) {
-                g_queue_sort(&conn->secondary_list,
-                             (GCompareDataFunc)seq_sorter,
-                             NULL);
-            }
-        } else {
+        if (!colo_insert_packet(&conn->secondary_list, pkt)) {
             error_report("colo compare secondary queue size too big,"
                          "drop packet");
         }
-- 
2.7.4

[Qemu-devel] [PULL 3/8] colo-compare: compare the packet in a specified Connection

[Jason Wang]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

A package from pri_indev or sec_indev only belongs to a particular
Connection, so we only need to compare the package in the specified
Connection's primary_list and secondary_list, rather than for each
the whole Connection list to compare. This is time-consuming and
unnecessary.

Less checkpoint more efficiency.

Cc: Zhang Chen <zhangckid@gmail.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/colo-compare.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 54b6347..5d2429b 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -136,7 +136,7 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt)
  * Return 0 on success, if return -1 means the pkt
  * is unsupported(arp and ipv6) and will be sent later
  */
-static int packet_enqueue(CompareState *s, int mode)
+static int packet_enqueue(CompareState *s, int mode, Connection **con)
 {
     ConnectionKey key;
     Packet *pkt = NULL;
@@ -179,6 +179,7 @@ static int packet_enqueue(CompareState *s, int mode)
                          "drop packet");
         }
     }
+    con = &conn;
 
     return 0;
 }
@@ -728,8 +729,9 @@ static void compare_set_vnet_hdr(Object *obj,
 static void compare_pri_rs_finalize(SocketReadState *pri_rs)
 {
     CompareState *s = container_of(pri_rs, CompareState, pri_rs);
+    Connection *conn = NULL;
 
-    if (packet_enqueue(s, PRIMARY_IN)) {
+    if (packet_enqueue(s, PRIMARY_IN, &conn)) {
         trace_colo_compare_main("primary: unsupported packet in");
         compare_chr_send(s,
                          pri_rs->buf,
@@ -737,19 +739,20 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
                          pri_rs->vnet_hdr_len);
     } else {
         /* compare connection */
-        g_queue_foreach(&s->conn_list, colo_compare_connection, s);
+        colo_compare_connection(conn, s);
     }
 }
 
 static void compare_sec_rs_finalize(SocketReadState *sec_rs)
 {
     CompareState *s = container_of(sec_rs, CompareState, sec_rs);
+    Connection *conn = NULL;
 
-    if (packet_enqueue(s, SECONDARY_IN)) {
+    if (packet_enqueue(s, SECONDARY_IN, &conn)) {
         trace_colo_compare_main("secondary: unsupported packet in");
     } else {
         /* compare connection */
-        g_queue_foreach(&s->conn_list, colo_compare_connection, s);
+        colo_compare_connection(conn, s);
     }
 }
 
-- 
2.7.4

[Qemu-devel] [PULL 4/8] colo-compare: Fix comments

[Jason Wang]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Cc: Zhang Chen <zhangckid@gmail.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Zhang Chen <zhangckid@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/colo-compare.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 5d2429b..ccdcba2 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -480,7 +480,9 @@ static void colo_old_packet_check(void *opaque)
 
 /*
  * Called from the compare thread on the primary
- * for compare connection
+ * for compare packet with secondary list of the
+ * specified connection when a new packet was
+ * queued to it.
  */
 static void colo_compare_connection(void *opaque, void *user_data)
 {
@@ -738,7 +740,7 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
                          pri_rs->packet_len,
                          pri_rs->vnet_hdr_len);
     } else {
-        /* compare connection */
+        /* compare packet in the specified connection */
         colo_compare_connection(conn, s);
     }
 }
@@ -751,7 +753,7 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs)
     if (packet_enqueue(s, SECONDARY_IN, &conn)) {
         trace_colo_compare_main("secondary: unsupported packet in");
     } else {
-        /* compare connection */
+        /* compare packet in the specified connection */
         colo_compare_connection(conn, s);
     }
 }
-- 
2.7.4

[Qemu-devel] [PULL 5/8] colo: Consolidate the duplicate code chunk into a routine

[Jason Wang]

From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Consolidate the code that extract the ip address(src,dst) and
port number(src,dst) of the packet into a separate routine
extract_ip_and_port() since the same chunk of code is called
from two place.

Cc: Zhang Chen <zhangckid@gmail.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/colo.c | 18 ++++++++++--------
 net/colo.h |  1 +
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/colo.c b/net/colo.c
index 28ce7c8..a39d600 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -82,6 +82,14 @@ int parse_packet_early(Packet *pkt)
     return 0;
 }
 
+void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key, Packet *pkt)
+{
+        key->src = pkt->ip->ip_src;
+        key->dst = pkt->ip->ip_dst;
+        key->src_port = ntohs(tmp_ports >> 16);
+        key->dst_port = ntohs(tmp_ports & 0xffff);
+}
+
 void fill_connection_key(Packet *pkt, ConnectionKey *key)
 {
     uint32_t tmp_ports;
@@ -97,17 +105,11 @@ void fill_connection_key(Packet *pkt, ConnectionKey *key)
     case IPPROTO_SCTP:
     case IPPROTO_UDPLITE:
         tmp_ports = *(uint32_t *)(pkt->transport_header);
-        key->src = pkt->ip->ip_src;
-        key->dst = pkt->ip->ip_dst;
-        key->src_port = ntohs(tmp_ports & 0xffff);
-        key->dst_port = ntohs(tmp_ports >> 16);
+        extract_ip_and_port(tmp_ports, key, pkt);
         break;
     case IPPROTO_AH:
         tmp_ports = *(uint32_t *)(pkt->transport_header + 4);
-        key->src = pkt->ip->ip_src;
-        key->dst = pkt->ip->ip_dst;
-        key->src_port = ntohs(tmp_ports & 0xffff);
-        key->dst_port = ntohs(tmp_ports >> 16);
+        extract_ip_and_port(tmp_ports, key, pkt);
         break;
     default:
         break;
diff --git a/net/colo.h b/net/colo.h
index caedb0d..0658e86 100644
--- a/net/colo.h
+++ b/net/colo.h
@@ -76,6 +76,7 @@ typedef struct Connection {
 uint32_t connection_key_hash(const void *opaque);
 int connection_key_equal(const void *opaque1, const void *opaque2);
 int parse_packet_early(Packet *pkt);
+void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key, Packet *pkt);
 void fill_connection_key(Packet *pkt, ConnectionKey *key);
 void reverse_connection_key(ConnectionKey *key);
 Connection *connection_new(ConnectionKey *key);
-- 
2.7.4

[Qemu-devel] [PULL 6/8] Fix eepro100 simple transmission mode

[Jason Wang]

From: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>

The simple transmission mode was treating the area immediately after the
transmit command block (TCB) as if it were a transmit buffer descriptor,
when in reality it is simply the packet data. This change simply copies
the data following the TCB into the packet buffer.

Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/eepro100.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 80b8f47..91dd058 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -774,23 +774,11 @@ static void tx_command(EEPRO100State *s)
     }
     assert(tcb_bytes <= sizeof(buf));
     while (size < tcb_bytes) {
-        uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev, tbd_address);
-        uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev, tbd_address + 4);
-#if 0
-        uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
-#endif
-        if (tx_buffer_size == 0) {
-            /* Prevent an endless loop. */
-            logout("loop in %s:%u\n", __FILE__, __LINE__);
-            break;
-        }
-        tbd_address += 8;
         TRACE(RXTX, logout
             ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
-             tx_buffer_address, tx_buffer_size));
-        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-        pci_dma_read(&s->dev, tx_buffer_address, &buf[size], tx_buffer_size);
-        size += tx_buffer_size;
+             tbd_address, tcb_bytes));
+        pci_dma_read(&s->dev, tbd_address, &buf[size], tcb_bytes);
+        size += tcb_bytes;
     }
     if (tbd_array == 0xffffffff) {
         /* Simplified mode. Was already handled by code above. */
-- 
2.7.4

[Qemu-devel] [PULL 7/8] Add new PCI ID for i82559a

[Jason Wang]

From: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>

Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. The
"x-use-alt-device-id" property controls whether this new ID is to be
used, and is true by default, and set to false in a compat entry.

Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/eepro100.c    | 13 +++++++++++++
 include/hw/compat.h  |  4 ++++
 include/hw/pci/pci.h |  1 +
 qemu-options.hx      |  2 +-
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 91dd058..a63ed2c 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -132,6 +132,7 @@ typedef struct {
     const char *name;
     const char *desc;
     uint16_t device_id;
+    uint16_t alt_device_id;
     uint8_t revision;
     uint16_t subsystem_vendor_id;
     uint16_t subsystem_id;
@@ -276,6 +277,7 @@ typedef struct {
     /* Quasi static device properties (no need to save them). */
     uint16_t stats_size;
     bool has_extended_tcb_support;
+    bool use_alt_device_id;
 } EEPRO100State;
 
 /* Word indices in EEPROM. */
@@ -1855,6 +1857,14 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
 
     TRACE(OTHER, logout("\n"));
 
+    /* By default, the i82559a adapter uses the legacy PCI ID (for the
+     * i82557). This allows the PCI ID to be changed to the alternate
+     * i82559 ID if needed.
+     */
+    if (s->use_alt_device_id && strcmp(info->name, "i82559a") == 0) {
+        pci_config_set_device_id(s->dev.config, info->alt_device_id);
+    }
+
     s->device = info->device;
 
     e100_pci_reset(s, &local_err);
@@ -1974,6 +1984,7 @@ static E100PCIDeviceInfo e100_devices[] = {
         .desc = "Intel i82559A Ethernet",
         .device = i82559A,
         .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .alt_device_id = PCI_DEVICE_ID_INTEL_82559,
         .revision = 0x06,
         .stats_size = 80,
         .has_extended_tcb_support = true,
@@ -2067,6 +2078,8 @@ static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s)
 
 static Property e100_properties[] = {
     DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
+    DEFINE_PROP_BOOL("x-use-alt-device-id", EEPRO100State, use_alt_device_id,
+                     true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index cf389b4..f96212c 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -10,6 +10,10 @@
         .driver   = "virtio-tablet-device",\
         .property = "wheel-axis",\
         .value    = "false",\
+    },{\
+        .driver   = "i82559a",\
+        .property = "x-use-alt-device-id",\
+        .value    = "false",\
     },
 
 #define HW_COMPAT_2_9 \
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8d02a0a..f30e2cf 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -70,6 +70,7 @@ extern bool pci_available;
 /* Intel (0x8086) */
 #define PCI_DEVICE_ID_INTEL_82551IT      0x1209
 #define PCI_DEVICE_ID_INTEL_82557        0x1229
+#define PCI_DEVICE_ID_INTEL_82559        0x1030
 #define PCI_DEVICE_ID_INTEL_82801IR      0x2922
 
 /* Red Hat / Qumranet (for QEMU) -- see pci-ids.txt */
diff --git a/qemu-options.hx b/qemu-options.hx
index 3728e9b..a39c7e4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2047,7 +2047,7 @@ that the card should have; this option currently only affects virtio cards; set
 @var{v} = 0 to disable MSI-X. If no @option{-net} option is specified, a single
 NIC is created.  QEMU can emulate several different models of network card.
 Valid values for @var{type} are
-@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559er},
+@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559a}, @code{i82559er},
 @code{ne2k_pci}, @code{ne2k_isa}, @code{pcnet}, @code{rtl8139},
 @code{e1000}, @code{smc91c111}, @code{lance} and @code{mcf_fec}.
 Not all devices are supported on all targets.  Use @code{-net nic,model=help}
-- 
2.7.4

[Qemu-devel] [PULL 8/8] net/socket: fix coverity issue

[Jason Wang]

From: Jens Freimann <jfreimann@redhat.com>

This fixes coverity issue CID1005339.

Make sure that saddr is not used uninitialized if the
mcast parameter is NULL.

Cc: qemu-stable@nongnu.org
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/socket.c b/net/socket.c
index 83a2a31..6917fbc 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -373,7 +373,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
     net_socket_read_poll(s, true);
 
     /* mcast: save bound address as dst */
-    if (is_connected) {
+    if (is_connected && mcast != NULL) {
         s->dgram_dst = saddr;
         snprintf(nc->info_str, sizeof(nc->info_str),
                  "socket: fd=%d (cloned mcast=%s:%d)",
-- 
2.7.4

Re: [Qemu-devel] [PULL 0/8] Net patches

[Peter Maydell]

On 14 November 2017 at 02:11, Jason Wang <jasowang@redhat.com> wrote:
> The following changes since commit 4ffa88c99c54d2a30f79e3dbecec50b023eff1c8:
>
>   Merge remote-tracking branch 'remotes/berrange/tags/pull-qcrypto-2017-11-08-1' into staging (2017-11-10 16:01:35 +0000)
>
> are available in the git repository at:
>
>   https://github.com/jasowang/qemu.git tags/net-pull-request
>
> for you to fetch changes up to bb160b571fe469b03228d4502c75a18045978a74:
>
>   net/socket: fix coverity issue (2017-11-13 18:05:12 +0800)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
> Jens Freimann (2):
>       net: fix check for number of parameters to -netdev socket
>       net/socket: fix coverity issue
>
> Mao Zhongyi (4):
>       colo-compare: Insert packet into the suitable position of packet queue directly
>       colo-compare: compare the packet in a specified Connection
>       colo-compare: Fix comments
>       colo: Consolidate the duplicate code chunk into a routine
>
> Mike Nawrocki (2):
>       Fix eepro100 simple transmission mode
>       Add new PCI ID for i82559a

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 7/8] Add new PCI ID for i82559a

[Stefan Weil]

Hi,

I currently think that this patch is wrong and should be reverted.

It fixes a certain use case by hacking the PCI device id, but does
not model the way how that device id is set on the real hardware
correctly.

As far as I know, all i82559 have a default PCI device id of 0x1229.
It can be changed by the EEPROM configuration, but not all network
cards do have an EEPROM.

See for example this URL for more information:
http://zoo.cs.yale.edu/classes/cs422/2010/ref/82559_eeprom.pdf

The correct solution is modeling the EEPROM and allowing QEMU
users to provide an EEPROM‌ file.

Cheers
Stefan

Am 14.11.2017 um 03:11 schrieb Jason Wang:
> From: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> 
> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. The
> "x-use-alt-device-id" property controls whether this new ID is to be
> used, and is true by default, and set to false in a compat entry.
> 
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/eepro100.c    | 13 +++++++++++++
>  include/hw/compat.h  |  4 ++++
>  include/hw/pci/pci.h |  1 +
>  qemu-options.hx      |  2 +-
>  4 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 91dd058..a63ed2c 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -132,6 +132,7 @@ typedef struct {
>      const char *name;
>      const char *desc;
>      uint16_t device_id;
> +    uint16_t alt_device_id;
>      uint8_t revision;
>      uint16_t subsystem_vendor_id;
>      uint16_t subsystem_id;
> @@ -276,6 +277,7 @@ typedef struct {
>      /* Quasi static device properties (no need to save them). */
>      uint16_t stats_size;
>      bool has_extended_tcb_support;
> +    bool use_alt_device_id;
>  } EEPRO100State;
>  
>  /* Word indices in EEPROM. */
> @@ -1855,6 +1857,14 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
>  
>      TRACE(OTHER, logout("\n"));
>  
> +    /* By default, the i82559a adapter uses the legacy PCI ID (for the
> +     * i82557). This allows the PCI ID to be changed to the alternate
> +     * i82559 ID if needed.
> +     */
> +    if (s->use_alt_device_id && strcmp(info->name, "i82559a") == 0) {
> +        pci_config_set_device_id(s->dev.config, info->alt_device_id);
> +    }
> +
>      s->device = info->device;
>  
>      e100_pci_reset(s, &local_err);
> @@ -1974,6 +1984,7 @@ static E100PCIDeviceInfo e100_devices[] = {
>          .desc = "Intel i82559A Ethernet",
>          .device = i82559A,
>          .device_id = PCI_DEVICE_ID_INTEL_82557,
> +        .alt_device_id = PCI_DEVICE_ID_INTEL_82559,
>          .revision = 0x06,
>          .stats_size = 80,
>          .has_extended_tcb_support = true,
> @@ -2067,6 +2078,8 @@ static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s)
>  
>  static Property e100_properties[] = {
>      DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
> +    DEFINE_PROP_BOOL("x-use-alt-device-id", EEPRO100State, use_alt_device_id,
> +                     true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index cf389b4..f96212c 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -10,6 +10,10 @@
>          .driver   = "virtio-tablet-device",\
>          .property = "wheel-axis",\
>          .value    = "false",\
> +    },{\
> +        .driver   = "i82559a",\
> +        .property = "x-use-alt-device-id",\
> +        .value    = "false",\
>      },
>  
>  #define HW_COMPAT_2_9 \
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 8d02a0a..f30e2cf 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -70,6 +70,7 @@ extern bool pci_available;
>  /* Intel (0x8086) */
>  #define PCI_DEVICE_ID_INTEL_82551IT      0x1209
>  #define PCI_DEVICE_ID_INTEL_82557        0x1229
> +#define PCI_DEVICE_ID_INTEL_82559        0x1030
>  #define PCI_DEVICE_ID_INTEL_82801IR      0x2922
>  
>  /* Red Hat / Qumranet (for QEMU) -- see pci-ids.txt */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3728e9b..a39c7e4 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2047,7 +2047,7 @@ that the card should have; this option currently only affects virtio cards; set
>  @var{v} = 0 to disable MSI-X. If no @option{-net} option is specified, a single
>  NIC is created.  QEMU can emulate several different models of network card.
>  Valid values for @var{type} are
> -@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559er},
> +@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559a}, @code{i82559er},
>  @code{ne2k_pci}, @code{ne2k_isa}, @code{pcnet}, @code{rtl8139},
>  @code{e1000}, @code{smc91c111}, @code{lance} and @code{mcf_fec}.
>  Not all devices are supported on all targets.  Use @code{-net nic,model=help}
> 

Re: [Qemu-devel] [PULL 7/8] Add new PCI ID for i82559a

[Jason Wang]

On 2017年11月15日 14:43, Stefan Weil wrote:
> Hi,
>
> I currently think that this patch is wrong and should be reverted.
>
> It fixes a certain use case by hacking the PCI device id, but does
> not model the way how that device id is set on the real hardware
> correctly.
>
> As far as I know, all i82559 have a default PCI device id of 0x1229.
> It can be changed by the EEPROM configuration, but not all network
> cards do have an EEPROM.
>
> See for example this URL for more information:
> http://zoo.cs.yale.edu/classes/cs422/2010/ref/82559_eeprom.pdf
>
> The correct solution is modeling the EEPROM and allowing QEMU
> users to provide an EEPROM‌ file.

Yes and unless there's new version of sepc that has different ID, I tend 
to revert this.

Thanks

>
> Cheers
> Stefan

Re: [Qemu-devel] [PULL 3/8] colo-compare: compare the packet in a specified Connection

[Peter Maydell]

On 14 November 2017 at 02:11, Jason Wang <jasowang@redhat.com> wrote:
> From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>
> A package from pri_indev or sec_indev only belongs to a particular
> Connection, so we only need to compare the package in the specified
> Connection's primary_list and secondary_list, rather than for each
> the whole Connection list to compare. This is time-consuming and
> unnecessary.
>
> Less checkpoint more efficiency.
>
> Cc: Zhang Chen <zhangckid@gmail.com>
> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/colo-compare.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 54b6347..5d2429b 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -136,7 +136,7 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt)
>   * Return 0 on success, if return -1 means the pkt
>   * is unsupported(arp and ipv6) and will be sent later
>   */
> -static int packet_enqueue(CompareState *s, int mode)
> +static int packet_enqueue(CompareState *s, int mode, Connection **con)
>  {
>      ConnectionKey key;
>      Packet *pkt = NULL;
> @@ -179,6 +179,7 @@ static int packet_enqueue(CompareState *s, int mode)
>                           "drop packet");
>          }
>      }
> +    con = &conn;
>
>      return 0;
>  }

Coverity points out that this looks a bit fishy --
presumably you meant
  *con = conn;

? The statement you have now doesn't do anything, since
'con' is unused after you change it.

(CID 1382804.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 3/8] colo-compare: compare the packet in a specified Connection

[Mao Zhongyi]

On 11/16/2017 02:57 AM, Peter Maydell wrote:
> On 14 November 2017 at 02:11, Jason Wang <jasowang@redhat.com> wrote:
>> From: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>
>> A package from pri_indev or sec_indev only belongs to a particular
>> Connection, so we only need to compare the package in the specified
>> Connection's primary_list and secondary_list, rather than for each
>> the whole Connection list to compare. This is time-consuming and
>> unnecessary.
>>
>> Less checkpoint more efficiency.
>>
>> Cc: Zhang Chen <zhangckid@gmail.com>
>> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  net/colo-compare.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index 54b6347..5d2429b 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -136,7 +136,7 @@ static int colo_insert_packet(GQueue *queue, Packet *pkt)
>>   * Return 0 on success, if return -1 means the pkt
>>   * is unsupported(arp and ipv6) and will be sent later
>>   */
>> -static int packet_enqueue(CompareState *s, int mode)
>> +static int packet_enqueue(CompareState *s, int mode, Connection **con)
>>  {
>>      ConnectionKey key;
>>      Packet *pkt = NULL;
>> @@ -179,6 +179,7 @@ static int packet_enqueue(CompareState *s, int mode)
>>                           "drop packet");
>>          }
>>      }
>> +    con = &conn;
>>
>>      return 0;
>>  }

Hi, Peter

> Coverity points out that this looks a bit fishy --
> presumably you meant
>   *con = conn;
>

Well，I will fix it right away.

> ? The statement you have now doesn't do anything, since
> 'con' is unused after you change it.

It will be used in colo_compare_connection.

Thanks,
Mao