From baba731bc6e7920ec2dca7d449b6bf7d42e4901f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 14 Feb 2020 01:47:53 +0100 Subject: [PATCH 01/14] hw/net/i82596: Correct command bitmask (CID 1419392) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The command is 32-bit, but we are loading the 16 upper bits with the 'get_uint16(s->scb + 2)' call. Once shifted by 16, the command bits match the status bits: - Command Bit 31 ACK-CX Acknowledges that the CU completed an Action Command. Bit 30 ACK-FR Acknowledges that the RU received a frame. Bit 29 ACK-CNA Acknowledges that the Command Unit became not active. Bit 28 ACK-RNR Acknowledges that the Receive Unit became not ready. - Status Bit 15 CX The CU finished executing a command with its I(interrupt) bit set. Bit 14 FR The RU finished receiving a frame. Bit 13 CNA The Command Unit left the Active state. Bit 12 RNR The Receive Unit left the Ready state. Add the SCB_COMMAND_ACK_MASK definition to simplify the code. This fixes Coverity 1419392 (CONSTANT_EXPRESSION_RESULT): /hw/net/i82596.c: 352 in examine_scb() 346 cuc = (command >> 8) & 0x7; 347 ruc = (command >> 4) & 0x7; 348 DBG(printf("MAIN COMMAND %04x cuc %02x ruc %02x\n", command, cuc, ruc)); 349 /* and clear the scb command word */ 350 set_uint16(s->scb + 2, 0); 351 >>> CID 1419392: (CONSTANT_EXPRESSION_RESULT) >>> "command & (2147483648UL /* 1UL << 31 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if". 352 if (command & BIT(31)) /* ACK-CX */ 353 s->scb_status &= ~SCB_STATUS_CX; >>> CID 1419392: (CONSTANT_EXPRESSION_RESULT) >>> "command & (1073741824UL /* 1UL << 30 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if". 354 if (command & BIT(30)) /*ACK-FR */ 355 s->scb_status &= ~SCB_STATUS_FR; >>> CID 1419392: (CONSTANT_EXPRESSION_RESULT) >>> "command & (536870912UL /* 1UL << 29 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if". 356 if (command & BIT(29)) /*ACK-CNA */ 357 s->scb_status &= ~SCB_STATUS_CNA; >>> CID 1419392: (CONSTANT_EXPRESSION_RESULT) >>> "command & (268435456UL /* 1UL << 28 */)" is always 0 regardless of the values of its operands. This occurs as the logical operand of "if". 358 if (command & BIT(28)) /*ACK-RNR */ 359 s->scb_status &= ~SCB_STATUS_RNR; Fixes: Covertiy CID 1419392 (commit 376b851909) Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/i82596.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/hw/net/i82596.c b/hw/net/i82596.c index fe9f2390a9..ecdb9aa4f8 100644 --- a/hw/net/i82596.c +++ b/hw/net/i82596.c @@ -43,6 +43,9 @@ #define SCB_STATUS_CNA 0x2000 /* CU left active state */ #define SCB_STATUS_RNR 0x1000 /* RU left active state */ +#define SCB_COMMAND_ACK_MASK \ + (SCB_STATUS_CX | SCB_STATUS_FR | SCB_STATUS_CNA | SCB_STATUS_RNR) + #define CU_IDLE 0 #define CU_SUSPENDED 1 #define CU_ACTIVE 2 @@ -348,14 +351,7 @@ static void examine_scb(I82596State *s) /* and clear the scb command word */ set_uint16(s->scb + 2, 0); - if (command & BIT(31)) /* ACK-CX */ - s->scb_status &= ~SCB_STATUS_CX; - if (command & BIT(30)) /*ACK-FR */ - s->scb_status &= ~SCB_STATUS_FR; - if (command & BIT(29)) /*ACK-CNA */ - s->scb_status &= ~SCB_STATUS_CNA; - if (command & BIT(28)) /*ACK-RNR */ - s->scb_status &= ~SCB_STATUS_RNR; + s->scb_status &= ~(command & SCB_COMMAND_ACK_MASK); switch (cuc) { case 0: /* no change */ From a43790f2f6155597e120409f467a3d41de3cfb53 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Thu, 12 Mar 2020 20:16:38 +0000 Subject: [PATCH 02/14] hw/net/i82596.c: Avoid reading off end of buffer in i82596_receive() The i82596_receive() function attempts to pass the guest a buffer which is effectively the concatenation of the data it is passed and a 4 byte CRC value. However, rather than implementing this as "write the data; then write the CRC" it instead bumps the length value of the data by 4, and writes 4 extra bytes from beyond the end of the buffer, which it then overwrites with the CRC. It also assumed that we could always fit all four bytes of the CRC into the final receive buffer, which might not be true if the CRC needs to be split over two receive buffers. Calculate separately how many bytes we need to transfer into the guest's receive buffer from the source buffer, and how many we need to transfer from the CRC work. We add a count 'bufsz' of the number of bytes left in the source buffer, which we use purely to assert() that we don't overrun. Spotted by Coverity (CID 1419396) for the specific case when we end up using a local array as the source buffer. Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/i82596.c | 44 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/hw/net/i82596.c b/hw/net/i82596.c index ecdb9aa4f8..3b0efd0b57 100644 --- a/hw/net/i82596.c +++ b/hw/net/i82596.c @@ -497,7 +497,8 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz) uint32_t rfd_p; uint32_t rbd; uint16_t is_broadcast = 0; - size_t len = sz; + size_t len = sz; /* length of data for guest (including CRC) */ + size_t bufsz = sz; /* length of data in buf */ uint32_t crc; uint8_t *crc_ptr; uint8_t buf1[MIN_BUF_SIZE + VLAN_HLEN]; @@ -591,6 +592,7 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz) if (len < MIN_BUF_SIZE) { len = MIN_BUF_SIZE; } + bufsz = len; } /* Calculate the ethernet checksum (4 bytes) */ @@ -623,6 +625,7 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz) while (len) { uint16_t buffer_size, num; uint32_t rba; + size_t bufcount, crccount; /* printf("Receive: rbd is %08x\n", rbd); */ buffer_size = get_uint16(rbd + 12); @@ -635,14 +638,37 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz) } rba = get_uint32(rbd + 8); /* printf("rba is 0x%x\n", rba); */ - address_space_write(&address_space_memory, rba, - MEMTXATTRS_UNSPECIFIED, buf, num); - rba += num; - buf += num; - len -= num; - if (len == 0) { /* copy crc */ - address_space_write(&address_space_memory, rba - 4, - MEMTXATTRS_UNSPECIFIED, crc_ptr, 4); + /* + * Calculate how many bytes we want from buf[] and how many + * from the CRC. + */ + if ((len - num) >= 4) { + /* The whole guest buffer, we haven't hit the CRC yet */ + bufcount = num; + } else { + /* All that's left of buf[] */ + bufcount = len - 4; + } + crccount = num - bufcount; + + if (bufcount > 0) { + /* Still some of the actual data buffer to transfer */ + assert(bufsz >= bufcount); + bufsz -= bufcount; + address_space_write(&address_space_memory, rba, + MEMTXATTRS_UNSPECIFIED, buf, bufcount); + rba += bufcount; + buf += bufcount; + len -= bufcount; + } + + /* Write as much of the CRC as fits */ + if (crccount > 0) { + address_space_write(&address_space_memory, rba, + MEMTXATTRS_UNSPECIFIED, crc_ptr, crccount); + rba += crccount; + crc_ptr += crccount; + len -= crccount; } num |= 0x4000; /* set F BIT */ From f22a57ac09abdd5afd8a974b52c19eda9347cffd Mon Sep 17 00:00:00 2001 From: Andrew Melnychenko Date: Wed, 4 Mar 2020 16:20:58 +0200 Subject: [PATCH 03/14] Fixed integer overflow in e1000e MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1737400 Fixed setting max_queue_num if there are no peers in NICConf. qemu_new_nic() creates NICState with 1 NetClientState(index 0) without peers, set max_queue_num to 0 - It prevents undefined behavior and possible crashes, especially during pcie hotplug. Fixes: 6f3fbe4ed06 ("net: Introduce e1000e device emulation") Signed-off-by: Andrew Melnychenko Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Dmitry Fleytman Signed-off-by: Jason Wang --- hw/net/e1000e.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index a91dbdca3c..f2cc1552c5 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -328,7 +328,7 @@ e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, uint8_t *macaddr) s->nic = qemu_new_nic(&net_e1000e_info, &s->conf, object_get_typename(OBJECT(s)), dev->id, s); - s->core.max_queue_num = s->conf.peers.queues - 1; + s->core.max_queue_num = s->conf.peers.queues ? s->conf.peers.queues - 1 : 0; trace_e1000e_mac_set_permanent(MAC_ARG(macaddr)); memcpy(s->core.permanent_mac, macaddr, sizeof(s->core.permanent_mac)); From 205ce5670f8ec38693ab945493e4fcc155b25cec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 5 Mar 2020 18:56:45 +0100 Subject: [PATCH 04/14] hw/net/e1000e_core: Let e1000e_can_receive() return a boolean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The e1000e_can_receive() function simply returns a boolean value. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Paolo Bonzini Signed-off-by: Jason Wang --- hw/net/e1000e_core.c | 2 +- hw/net/e1000e_core.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index df957e0c1a..d5676871fa 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -967,7 +967,7 @@ e1000e_start_recv(E1000ECore *core) } } -int +bool e1000e_can_receive(E1000ECore *core) { int i; diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h index 49abb136dd..aee32f7e48 100644 --- a/hw/net/e1000e_core.h +++ b/hw/net/e1000e_core.h @@ -143,7 +143,7 @@ e1000e_core_set_link_status(E1000ECore *core); void e1000e_core_pci_uninit(E1000ECore *core); -int +bool e1000e_can_receive(E1000ECore *core); ssize_t From 0002c3a696b2def7eda13d5fb722e45679959d22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 5 Mar 2020 18:56:46 +0100 Subject: [PATCH 05/14] hw/net/smc91c111: Let smc91c111_can_receive() return a boolean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The smc91c111_can_receive() function simply returns a boolean value. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Cédric Le Goater Signed-off-by: Jason Wang --- hw/net/smc91c111.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c index e9eb6f6c05..02be60c955 100644 --- a/hw/net/smc91c111.c +++ b/hw/net/smc91c111.c @@ -130,16 +130,16 @@ static void smc91c111_update(smc91c111_state *s) qemu_set_irq(s->irq, level); } -static int smc91c111_can_receive(smc91c111_state *s) +static bool smc91c111_can_receive(smc91c111_state *s) { if ((s->rcr & RCR_RXEN) == 0 || (s->rcr & RCR_SOFT_RST)) { - return 1; + return true; } if (s->allocated == (1 << NUM_PACKETS) - 1 || s->rx_fifo_len == NUM_PACKETS) { - return 0; + return false; } - return 1; + return true; } static inline void smc91c111_flush_queued_packets(smc91c111_state *s) From 2fa3d2d401b95036294361da52e4f3cd0f98155e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 5 Mar 2020 18:56:47 +0100 Subject: [PATCH 06/14] hw/net/rtl8139: Simplify if/else statement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite: if (E) { return A; } else { return B; } /* EOF */ } as: if (E) { return A; } return B; } Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Cédric Le Goater Signed-off-by: Jason Wang --- hw/net/rtl8139.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index ae4739bc09..ef3211537f 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -808,11 +808,11 @@ static int rtl8139_can_receive(NetClientState *nc) /* ??? Flow control not implemented in c+ mode. This is a hack to work around slirp deficiencies anyway. */ return 1; - } else { - avail = MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr, - s->RxBufferSize); - return (avail == 0 || avail >= 1514 || (s->IntrMask & RxOverflow)); } + + avail = MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr, + s->RxBufferSize); + return avail == 0 || avail >= 1514 || (s->IntrMask & RxOverflow); } static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t size_, int do_interrupt) From 3317db743972f665e2753c75703538d51241350a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 5 Mar 2020 18:56:48 +0100 Subject: [PATCH 07/14] hw/net/rtl8139: Update coding style to make checkpatch.pl happy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will modify this code in the next commit. Clean it up first to avoid checkpatch.pl errors. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Cédric Le Goater Signed-off-by: Jason Wang --- hw/net/rtl8139.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index ef3211537f..be9a0af629 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -799,10 +799,12 @@ static int rtl8139_can_receive(NetClientState *nc) int avail; /* Receive (drop) packets if card is disabled. */ - if (!s->clock_enabled) - return 1; - if (!rtl8139_receiver_enabled(s)) - return 1; + if (!s->clock_enabled) { + return 1; + } + if (!rtl8139_receiver_enabled(s)) { + return 1; + } if (rtl8139_cp_receiver_enabled(s) && rtl8139_cp_rx_valid(s)) { /* ??? Flow control not implemented in c+ mode. From b8c4b67e3ec3918a40234e5c56221f780c7856fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 5 Mar 2020 18:56:49 +0100 Subject: [PATCH 08/14] hw/net: Make NetCanReceive() return a boolean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The NetCanReceive handler return whether the device can or can not receive new packets. Make it obvious by returning a boolean type. Signed-off-by: Philippe Mathieu-Daudé Acked-by: David Gibson Reviewed-by: Alistair Francis Reviewed-by: Cédric Le Goater Signed-off-by: Jason Wang --- hw/net/allwinner_emac.c | 2 +- hw/net/cadence_gem.c | 8 ++++---- hw/net/dp8393x.c | 8 +++----- hw/net/e1000.c | 2 +- hw/net/e1000e.c | 2 +- hw/net/ftgmac100.c | 6 +++--- hw/net/i82596.c | 10 +++++----- hw/net/i82596.h | 2 +- hw/net/imx_fec.c | 2 +- hw/net/opencores_eth.c | 5 ++--- hw/net/rtl8139.c | 8 ++++---- hw/net/smc91c111.c | 2 +- hw/net/spapr_llan.c | 4 ++-- hw/net/sungem.c | 6 +++--- hw/net/sunhme.c | 4 ++-- hw/net/virtio-net.c | 10 +++++----- hw/net/xilinx_ethlite.c | 2 +- include/net/net.h | 2 +- net/filter-buffer.c | 2 +- net/hub.c | 6 +++--- 20 files changed, 45 insertions(+), 48 deletions(-) diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c index e9bbff8710..ddddf35c45 100644 --- a/hw/net/allwinner_emac.c +++ b/hw/net/allwinner_emac.c @@ -178,7 +178,7 @@ static uint32_t fifo8_pop_word(Fifo8 *fifo) return ret; } -static int aw_emac_can_receive(NetClientState *nc) +static bool aw_emac_can_receive(NetClientState *nc) { AwEmacState *s = qemu_get_nic_opaque(nc); diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index 6340c1eaf8..51ec5a072d 100644 --- a/hw/net/cadence_gem.c +++ b/hw/net/cadence_gem.c @@ -505,7 +505,7 @@ static void phy_update_link(CadenceGEMState *s) } } -static int gem_can_receive(NetClientState *nc) +static bool gem_can_receive(NetClientState *nc) { CadenceGEMState *s; int i; @@ -518,7 +518,7 @@ static int gem_can_receive(NetClientState *nc) s->can_rx_state = 1; DB_PRINT("can't receive - no enable\n"); } - return 0; + return false; } for (i = 0; i < s->num_priority_queues; i++) { @@ -532,14 +532,14 @@ static int gem_can_receive(NetClientState *nc) s->can_rx_state = 2; DB_PRINT("can't receive - all the buffer descriptors are busy\n"); } - return 0; + return false; } if (s->can_rx_state != 0) { s->can_rx_state = 0; DB_PRINT("can receive\n"); } - return 1; + return true; } /* diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 1563c11b9e..c54db0d62d 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -414,7 +414,7 @@ static void dp8393x_do_stop_timer(dp8393xState *s) dp8393x_update_wt_regs(s); } -static int dp8393x_can_receive(NetClientState *nc); +static bool dp8393x_can_receive(NetClientState *nc); static void dp8393x_do_receiver_enable(dp8393xState *s) { @@ -718,13 +718,11 @@ static void dp8393x_watchdog(void *opaque) dp8393x_update_irq(s); } -static int dp8393x_can_receive(NetClientState *nc) +static bool dp8393x_can_receive(NetClientState *nc) { dp8393xState *s = qemu_get_nic_opaque(nc); - if (!(s->regs[SONIC_CR] & SONIC_CR_RXEN)) - return 0; - return 1; + return !!(s->regs[SONIC_CR] & SONIC_CR_RXEN); } static int dp8393x_receive_filter(dp8393xState *s, const uint8_t * buf, diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 9233248c9a..2a69eee63f 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -845,7 +845,7 @@ static bool e1000_has_rxbufs(E1000State *s, size_t total_size) return total_size <= bufs * s->rxbuf_size; } -static int +static bool e1000_can_receive(NetClientState *nc) { E1000State *s = qemu_get_nic_opaque(nc); diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index f2cc1552c5..79ba158d41 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -199,7 +199,7 @@ static const MemoryRegionOps io_ops = { }, }; -static int +static bool e1000e_nc_can_receive(NetClientState *nc) { E1000EState *s = qemu_get_nic_opaque(nc); diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c index 2f92b65d4e..041ed21017 100644 --- a/hw/net/ftgmac100.c +++ b/hw/net/ftgmac100.c @@ -562,18 +562,18 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring, ftgmac100_update_irq(s); } -static int ftgmac100_can_receive(NetClientState *nc) +static bool ftgmac100_can_receive(NetClientState *nc) { FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc)); FTGMAC100Desc bd; if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN)) != (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN)) { - return 0; + return false; } if (ftgmac100_read_bd(&bd, s->rx_descriptor)) { - return 0; + return false; } return !(bd.des0 & FTGMAC100_RXDES0_RXPKT_RDY); } diff --git a/hw/net/i82596.c b/hw/net/i82596.c index 3b0efd0b57..055c3a1470 100644 --- a/hw/net/i82596.c +++ b/hw/net/i82596.c @@ -470,23 +470,23 @@ void i82596_h_reset(void *opaque) i82596_s_reset(s); } -int i82596_can_receive(NetClientState *nc) +bool i82596_can_receive(NetClientState *nc) { I82596State *s = qemu_get_nic_opaque(nc); if (s->rx_status == RX_SUSPENDED) { - return 0; + return false; } if (!s->lnkst) { - return 0; + return false; } if (USE_TIMER && !timer_pending(s->flush_queue_timer)) { - return 1; + return true; } - return 1; + return true; } #define MIN_BUF_SIZE 60 diff --git a/hw/net/i82596.h b/hw/net/i82596.h index 1238ac11f8..f0bbe810eb 100644 --- a/hw/net/i82596.h +++ b/hw/net/i82596.h @@ -48,7 +48,7 @@ void i82596_ioport_writel(void *opaque, uint32_t addr, uint32_t val); uint32_t i82596_ioport_readl(void *opaque, uint32_t addr); uint32_t i82596_bcr_readw(I82596State *s, uint32_t rap); ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t size_); -int i82596_can_receive(NetClientState *nc); +bool i82596_can_receive(NetClientState *nc); void i82596_set_link_status(NetClientState *nc); void i82596_common_init(DeviceState *dev, I82596State *s, NetClientInfo *info); extern const VMStateDescription vmstate_i82596; diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index 5c145a8197..a35c33683e 100644 --- a/hw/net/imx_fec.c +++ b/hw/net/imx_fec.c @@ -1049,7 +1049,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value, imx_eth_update(s); } -static int imx_eth_can_receive(NetClientState *nc) +static bool imx_eth_can_receive(NetClientState *nc) { IMXFECState *s = IMX_FEC(qemu_get_nic_opaque(nc)); diff --git a/hw/net/opencores_eth.c b/hw/net/opencores_eth.c index 6b338c2f29..2ba0dc8c2f 100644 --- a/hw/net/opencores_eth.c +++ b/hw/net/opencores_eth.c @@ -349,12 +349,11 @@ static void open_eth_reset(void *opaque) open_eth_set_link_status(qemu_get_queue(s->nic)); } -static int open_eth_can_receive(NetClientState *nc) +static bool open_eth_can_receive(NetClientState *nc) { OpenEthState *s = qemu_get_nic_opaque(nc); - return GET_REGBIT(s, MODER, RXEN) && - (s->regs[TX_BD_NUM] < 0x80); + return GET_REGBIT(s, MODER, RXEN) && (s->regs[TX_BD_NUM] < 0x80); } static ssize_t open_eth_receive(NetClientState *nc, diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index be9a0af629..70aca7ec26 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -793,23 +793,23 @@ static bool rtl8139_cp_rx_valid(RTL8139State *s) return !(s->RxRingAddrLO == 0 && s->RxRingAddrHI == 0); } -static int rtl8139_can_receive(NetClientState *nc) +static bool rtl8139_can_receive(NetClientState *nc) { RTL8139State *s = qemu_get_nic_opaque(nc); int avail; /* Receive (drop) packets if card is disabled. */ if (!s->clock_enabled) { - return 1; + return true; } if (!rtl8139_receiver_enabled(s)) { - return 1; + return true; } if (rtl8139_cp_receiver_enabled(s) && rtl8139_cp_rx_valid(s)) { /* ??? Flow control not implemented in c+ mode. This is a hack to work around slirp deficiencies anyway. */ - return 1; + return true; } avail = MOD2(s->RxBufferSize + s->RxBufPtr - s->RxBufAddr, diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c index 02be60c955..b3240b9335 100644 --- a/hw/net/smc91c111.c +++ b/hw/net/smc91c111.c @@ -667,7 +667,7 @@ static void smc91c111_writefn(void *opaque, hwaddr addr, } } -static int smc91c111_can_receive_nc(NetClientState *nc) +static bool smc91c111_can_receive_nc(NetClientState *nc) { smc91c111_state *s = qemu_get_nic_opaque(nc); diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c index 80f5a1dd37..a2377025a7 100644 --- a/hw/net/spapr_llan.c +++ b/hw/net/spapr_llan.c @@ -110,11 +110,11 @@ typedef struct SpaprVioVlan { RxBufPool *rx_pool[RX_MAX_POOLS]; /* Receive buffer descriptor pools */ } SpaprVioVlan; -static int spapr_vlan_can_receive(NetClientState *nc) +static bool spapr_vlan_can_receive(NetClientState *nc) { SpaprVioVlan *dev = qemu_get_nic_opaque(nc); - return (dev->isopen && dev->rx_bufs > 0); + return dev->isopen && dev->rx_bufs > 0; } /** diff --git a/hw/net/sungem.c b/hw/net/sungem.c index 89da51f7f6..b01197d952 100644 --- a/hw/net/sungem.c +++ b/hw/net/sungem.c @@ -433,7 +433,7 @@ static bool sungem_rx_full(SunGEMState *s, uint32_t kick, uint32_t done) return kick == ((done + 1) & s->rx_mask); } -static int sungem_can_receive(NetClientState *nc) +static bool sungem_can_receive(NetClientState *nc) { SunGEMState *s = qemu_get_nic_opaque(nc); uint32_t kick, done, rxdma_cfg, rxmac_cfg; @@ -445,11 +445,11 @@ static int sungem_can_receive(NetClientState *nc) /* If MAC disabled, can't receive */ if ((rxmac_cfg & MAC_RXCFG_ENAB) == 0) { trace_sungem_rx_mac_disabled(); - return 0; + return false; } if ((rxdma_cfg & RXDMA_CFG_ENABLE) == 0) { trace_sungem_rx_txdma_disabled(); - return 0; + return false; } /* Check RX availability */ diff --git a/hw/net/sunhme.c b/hw/net/sunhme.c index 8863601f6c..9c38583180 100644 --- a/hw/net/sunhme.c +++ b/hw/net/sunhme.c @@ -657,11 +657,11 @@ static void sunhme_transmit(SunHMEState *s) sunhme_update_irq(s); } -static int sunhme_can_receive(NetClientState *nc) +static bool sunhme_can_receive(NetClientState *nc) { SunHMEState *s = qemu_get_nic_opaque(nc); - return s->macregs[HME_MACI_RXCFG >> 2] & HME_MAC_RXCFG_ENABLE; + return !!(s->macregs[HME_MACI_RXCFG >> 2] & HME_MAC_RXCFG_ENABLE); } static void sunhme_link_status_changed(NetClientState *nc) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 3627bb1717..a46e3b37a7 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1234,26 +1234,26 @@ static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq) qemu_flush_queued_packets(qemu_get_subqueue(n->nic, queue_index)); } -static int virtio_net_can_receive(NetClientState *nc) +static bool virtio_net_can_receive(NetClientState *nc) { VirtIONet *n = qemu_get_nic_opaque(nc); VirtIODevice *vdev = VIRTIO_DEVICE(n); VirtIONetQueue *q = virtio_net_get_subqueue(nc); if (!vdev->vm_running) { - return 0; + return false; } if (nc->queue_index >= n->curr_queues) { - return 0; + return false; } if (!virtio_queue_ready(q->rx_vq) || !(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { - return 0; + return false; } - return 1; + return true; } static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize) diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c index cf07e698b3..71d16fef3d 100644 --- a/hw/net/xilinx_ethlite.c +++ b/hw/net/xilinx_ethlite.c @@ -175,7 +175,7 @@ static const MemoryRegionOps eth_ops = { } }; -static int eth_can_rx(NetClientState *nc) +static bool eth_can_rx(NetClientState *nc) { struct xlx_ethlite *s = qemu_get_nic_opaque(nc); unsigned int rxbase = s->rxbuf * (0x800 / 4); diff --git a/include/net/net.h b/include/net/net.h index 094e966af9..39085d9444 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -42,7 +42,7 @@ typedef struct NICConf { /* Net clients */ typedef void (NetPoll)(NetClientState *, bool enable); -typedef int (NetCanReceive)(NetClientState *); +typedef bool (NetCanReceive)(NetClientState *); typedef ssize_t (NetReceive)(NetClientState *, const uint8_t *, size_t); typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int); typedef void (NetCleanup) (NetClientState *); diff --git a/net/filter-buffer.c b/net/filter-buffer.c index 88da78f821..12e0254287 100644 --- a/net/filter-buffer.c +++ b/net/filter-buffer.c @@ -74,7 +74,7 @@ static ssize_t filter_buffer_receive_iov(NetFilterState *nf, * the filter can still accept packets until its internal queue is full. * For example: * For some reason, receiver could not receive more packets - * (.can_receive() returns zero). Without a filter, at most one packet + * (.can_receive() returns false). Without a filter, at most one packet * will be queued in incoming queue and sender's poll will be disabled * unit its sent_cb() was called. With a filter, it will keep receiving * the packets without caring about the receiver. This is suboptimal. diff --git a/net/hub.c b/net/hub.c index 88cfb876f3..1375738bf1 100644 --- a/net/hub.c +++ b/net/hub.c @@ -90,7 +90,7 @@ static NetHub *net_hub_new(int id) return hub; } -static int net_hub_port_can_receive(NetClientState *nc) +static bool net_hub_port_can_receive(NetClientState *nc) { NetHubPort *port; NetHubPort *src_port = DO_UPCAST(NetHubPort, nc, nc); @@ -102,11 +102,11 @@ static int net_hub_port_can_receive(NetClientState *nc) } if (qemu_can_send_packet(&port->nc)) { - return 1; + return true; } } - return 0; + return false; } static ssize_t net_hub_port_receive(NetClientState *nc, From 767cc9a9c1daf9b6c8efbfef656e5ca12c853a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Thu, 5 Mar 2020 18:56:50 +0100 Subject: [PATCH 09/14] hw/net/can: Make CanBusClientInfo::can_receive() return a boolean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CanBusClientInfo::can_receive handler return whether the device can or can not receive new frames. Make it obvious by returning a boolean type. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Reviewed-by: Cédric Le Goater Signed-off-by: Jason Wang --- hw/net/allwinner-sun8i-emac.c | 2 +- hw/net/can/can_sja1000.c | 8 ++++---- hw/net/can/can_sja1000.h | 2 +- include/net/can_emu.h | 2 +- net/can/can_socketcan.c | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c index 3fc5e34640..033eaa85bf 100644 --- a/hw/net/allwinner-sun8i-emac.c +++ b/hw/net/allwinner-sun8i-emac.c @@ -395,7 +395,7 @@ static void allwinner_sun8i_emac_flush_desc(FrameDescriptor *desc, cpu_physical_memory_write(phys_addr, desc, sizeof(*desc)); } -static int allwinner_sun8i_emac_can_receive(NetClientState *nc) +static bool allwinner_sun8i_emac_can_receive(NetClientState *nc) { AwSun8iEmacState *s = qemu_get_nic_opaque(nc); FrameDescriptor desc; diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c index 39c78faf9b..ea915a023a 100644 --- a/hw/net/can/can_sja1000.c +++ b/hw/net/can/can_sja1000.c @@ -733,21 +733,21 @@ uint64_t can_sja_mem_read(CanSJA1000State *s, hwaddr addr, unsigned size) return temp; } -int can_sja_can_receive(CanBusClientState *client) +bool can_sja_can_receive(CanBusClientState *client) { CanSJA1000State *s = container_of(client, CanSJA1000State, bus_client); if (s->clock & 0x80) { /* PeliCAN Mode */ if (s->mode & 0x01) { /* reset mode. */ - return 0; + return false; } } else { /* BasicCAN mode */ if (s->control & 0x01) { - return 0; + return false; } } - return 1; /* always return 1, when operation mode */ + return true; /* always return true, when operation mode */ } ssize_t can_sja_receive(CanBusClientState *client, const qemu_can_frame *frames, diff --git a/hw/net/can/can_sja1000.h b/hw/net/can/can_sja1000.h index 220a622087..7ca9cd681e 100644 --- a/hw/net/can/can_sja1000.h +++ b/hw/net/can/can_sja1000.h @@ -137,7 +137,7 @@ void can_sja_disconnect(CanSJA1000State *s); int can_sja_init(CanSJA1000State *s, qemu_irq irq); -int can_sja_can_receive(CanBusClientState *client); +bool can_sja_can_receive(CanBusClientState *client); ssize_t can_sja_receive(CanBusClientState *client, const qemu_can_frame *frames, size_t frames_cnt); diff --git a/include/net/can_emu.h b/include/net/can_emu.h index d4fc51b57d..fce9770928 100644 --- a/include/net/can_emu.h +++ b/include/net/can_emu.h @@ -83,7 +83,7 @@ typedef struct CanBusClientState CanBusClientState; typedef struct CanBusState CanBusState; typedef struct CanBusClientInfo { - int (*can_receive)(CanBusClientState *); + bool (*can_receive)(CanBusClientState *); ssize_t (*receive)(CanBusClientState *, const struct qemu_can_frame *frames, size_t frames_cnt); } CanBusClientInfo; diff --git a/net/can/can_socketcan.c b/net/can/can_socketcan.c index 29bfacd4f8..807f31fcde 100644 --- a/net/can/can_socketcan.c +++ b/net/can/can_socketcan.c @@ -110,9 +110,9 @@ static void can_host_socketcan_read(void *opaque) } } -static int can_host_socketcan_can_receive(CanBusClientState *client) +static bool can_host_socketcan_can_receive(CanBusClientState *client) { - return 1; + return true; } static ssize_t can_host_socketcan_receive(CanBusClientState *client, From 9cc43c94b31321cdb2ef1d83f99d7a5b8a8b1d9e Mon Sep 17 00:00:00 2001 From: Zhang Chen Date: Wed, 18 Mar 2020 16:23:19 +0800 Subject: [PATCH 10/14] net/colo-compare.c: Expose "compare_timeout" to users The "compare_timeout" determines the maximum time to hold the primary net packet. This patch expose the "compare_timeout", make user have ability to adjest the value according to application scenarios. QMP command demo: { "execute": "qom-get", "arguments": { "path": "/objects/comp0", "property": "compare_timeout" } } { "execute": "qom-set", "arguments": { "path": "/objects/comp0", "property": "compare_timeout", "value": 5000} } Signed-off-by: Zhang Chen Signed-off-by: Jason Wang --- net/colo-compare.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-- qemu-options.hx | 8 +++++--- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/net/colo-compare.c b/net/colo-compare.c index 7ee17f2cf8..ec09b2a524 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -50,6 +50,7 @@ static NotifierList colo_compare_notifiers = /* TODO: Should be configurable */ #define REGULAR_PACKET_CHECK_MS 3000 +#define DEFAULT_TIME_OUT_MS 3000 static QemuMutex event_mtx; static QemuCond event_complete_cond; @@ -92,6 +93,7 @@ typedef struct CompareState { SocketReadState sec_rs; SocketReadState notify_rs; bool vnet_hdr; + uint32_t compare_timeout; /* * Record the connection that through the NIC @@ -607,10 +609,9 @@ static int colo_old_packet_check_one_conn(Connection *conn, CompareState *s) { GList *result = NULL; - int64_t check_time = REGULAR_PACKET_CHECK_MS; result = g_queue_find_custom(&conn->primary_list, - &check_time, + &s->compare_timeout, (GCompareFunc)colo_old_packet_check_one); if (result) { @@ -984,6 +985,39 @@ static void compare_set_notify_dev(Object *obj, const char *value, Error **errp) s->notify_dev = g_strdup(value); } +static void compare_get_timeout(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + CompareState *s = COLO_COMPARE(obj); + uint32_t value = s->compare_timeout; + + visit_type_uint32(v, name, &value, errp); +} + +static void compare_set_timeout(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + CompareState *s = COLO_COMPARE(obj); + Error *local_err = NULL; + uint32_t value; + + visit_type_uint32(v, name, &value, &local_err); + if (local_err) { + goto out; + } + if (!value) { + error_setg(&local_err, "Property '%s.%s' requires a positive value", + object_get_typename(obj), name); + goto out; + } + s->compare_timeout = value; + +out: + error_propagate(errp, local_err); +} + static void compare_pri_rs_finalize(SocketReadState *pri_rs) { CompareState *s = container_of(pri_rs, CompareState, pri_rs); @@ -1090,6 +1124,11 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) return; } + if (!s->compare_timeout) { + /* Set default value to 3000 MS */ + s->compare_timeout = DEFAULT_TIME_OUT_MS; + } + if (find_and_check_chardev(&chr, s->pri_indev, errp) || !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) { return; @@ -1185,6 +1224,10 @@ static void colo_compare_init(Object *obj) compare_get_notify_dev, compare_set_notify_dev, NULL); + object_property_add(obj, "compare_timeout", "uint32", + compare_get_timeout, + compare_set_timeout, NULL, NULL, NULL); + s->vnet_hdr = false; object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr, compare_set_vnet_hdr, NULL); diff --git a/qemu-options.hx b/qemu-options.hx index 962a5ebaa6..9e48e13b26 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4615,7 +4615,7 @@ SRST stored. The file format is libpcap, so it can be analyzed with tools such as tcpdump or Wireshark. - ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id]`` + ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}]`` Colo-compare gets packet from primary\_inchardevid and secondary\_inchardevid, than compare primary packet with secondary packet. If the packets are same, we will output @@ -4624,8 +4624,10 @@ SRST outdevchardevid. In order to improve efficiency, we need to put the task of comparison in another thread. If it has the vnet\_hdr\_support flag, colo compare will send/recv packet with - vnet\_hdr\_len. If you want to use Xen COLO, will need the - notify\_dev to notify Xen colo-frame to do checkpoint. + vnet\_hdr\_len. Then compare\_timeout=@var{ms} determines the + maximum delay colo-compare wait for the packet. + If you want to use Xen COLO, will need the notify\_dev to + notify Xen colo-frame to do checkpoint. we must use it with the help of filter-mirror and filter-redirector. From cca35ac4d16c0633a7d3bd28448e4ef52f0d46b0 Mon Sep 17 00:00:00 2001 From: Zhang Chen Date: Wed, 18 Mar 2020 16:23:20 +0800 Subject: [PATCH 11/14] net/colo-compare.c: Expose "expired_scan_cycle" to users The "expired_scan_cycle" determines period of scanning expired primary node net packets. Signed-off-by: Zhang Chen Signed-off-by: Jason Wang --- net/colo-compare.c | 48 +++++++++++++++++++++++++++++++++++++++++++--- qemu-options.hx | 4 +++- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/net/colo-compare.c b/net/colo-compare.c index ec09b2a524..10c0239f9d 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -48,7 +48,6 @@ static NotifierList colo_compare_notifiers = #define COLO_COMPARE_FREE_PRIMARY 0x01 #define COLO_COMPARE_FREE_SECONDARY 0x02 -/* TODO: Should be configurable */ #define REGULAR_PACKET_CHECK_MS 3000 #define DEFAULT_TIME_OUT_MS 3000 @@ -94,6 +93,7 @@ typedef struct CompareState { SocketReadState notify_rs; bool vnet_hdr; uint32_t compare_timeout; + uint32_t expired_scan_cycle; /* * Record the connection that through the NIC @@ -823,7 +823,7 @@ static void check_old_packet_regular(void *opaque) /* if have old packet we will notify checkpoint */ colo_old_packet_check(s); timer_mod(s->packet_check_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + - REGULAR_PACKET_CHECK_MS); + s->expired_scan_cycle); } /* Public API, Used for COLO frame to notify compare event */ @@ -853,7 +853,7 @@ static void colo_compare_timer_init(CompareState *s) SCALE_MS, check_old_packet_regular, s); timer_mod(s->packet_check_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + - REGULAR_PACKET_CHECK_MS); + s->expired_scan_cycle); } static void colo_compare_timer_del(CompareState *s) @@ -1018,6 +1018,39 @@ out: error_propagate(errp, local_err); } +static void compare_get_expired_scan_cycle(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + CompareState *s = COLO_COMPARE(obj); + uint32_t value = s->expired_scan_cycle; + + visit_type_uint32(v, name, &value, errp); +} + +static void compare_set_expired_scan_cycle(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + CompareState *s = COLO_COMPARE(obj); + Error *local_err = NULL; + uint32_t value; + + visit_type_uint32(v, name, &value, &local_err); + if (local_err) { + goto out; + } + if (!value) { + error_setg(&local_err, "Property '%s.%s' requires a positive value", + object_get_typename(obj), name); + goto out; + } + s->expired_scan_cycle = value; + +out: + error_propagate(errp, local_err); +} + static void compare_pri_rs_finalize(SocketReadState *pri_rs) { CompareState *s = container_of(pri_rs, CompareState, pri_rs); @@ -1129,6 +1162,11 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) s->compare_timeout = DEFAULT_TIME_OUT_MS; } + if (!s->expired_scan_cycle) { + /* Set default value to 3000 MS */ + s->expired_scan_cycle = REGULAR_PACKET_CHECK_MS; + } + if (find_and_check_chardev(&chr, s->pri_indev, errp) || !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) { return; @@ -1228,6 +1266,10 @@ static void colo_compare_init(Object *obj) compare_get_timeout, compare_set_timeout, NULL, NULL, NULL); + object_property_add(obj, "expired_scan_cycle", "uint32", + compare_get_expired_scan_cycle, + compare_set_expired_scan_cycle, NULL, NULL, NULL); + s->vnet_hdr = false; object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr, compare_set_vnet_hdr, NULL); diff --git a/qemu-options.hx b/qemu-options.hx index 9e48e13b26..16debd03cb 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4615,7 +4615,7 @@ SRST stored. The file format is libpcap, so it can be analyzed with tools such as tcpdump or Wireshark. - ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}]`` + ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}`` Colo-compare gets packet from primary\_inchardevid and secondary\_inchardevid, than compare primary packet with secondary packet. If the packets are same, we will output @@ -4626,6 +4626,8 @@ SRST vnet\_hdr\_support flag, colo compare will send/recv packet with vnet\_hdr\_len. Then compare\_timeout=@var{ms} determines the maximum delay colo-compare wait for the packet. + The expired\_scan\_cycle=@var{ms} to set the period of scanning + expired primary node network packets. If you want to use Xen COLO, will need the notify\_dev to notify Xen colo-frame to do checkpoint. From 8ffb7265af64ec81748335ec8f20e7ab542c3850 Mon Sep 17 00:00:00 2001 From: Prasad J Pandit Date: Tue, 24 Mar 2020 22:57:22 +0530 Subject: [PATCH 12/14] net: tulip: check frame size and r/w data length Tulip network driver while copying tx/rx buffers does not check frame size against r/w data length. This may lead to OOB buffer access. Add check to avoid it. Limit iterations over descriptors to avoid potential infinite loop issue in tulip_xmit_list_update. Reported-by: Li Qiang Reported-by: Ziming Zhang Reported-by: Jason Wang Tested-by: Li Qiang Reviewed-by: Li Qiang Signed-off-by: Prasad J Pandit Signed-off-by: Jason Wang --- hw/net/tulip.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/hw/net/tulip.c b/hw/net/tulip.c index cfac2719d3..1295f51d07 100644 --- a/hw/net/tulip.c +++ b/hw/net/tulip.c @@ -170,6 +170,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor *desc) } else { len = s->rx_frame_len; } + + if (s->rx_frame_len + len > sizeof(s->rx_frame)) { + return; + } pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame + (s->rx_frame_size - s->rx_frame_len), len); s->rx_frame_len -= len; @@ -181,6 +185,10 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor *desc) } else { len = s->rx_frame_len; } + + if (s->rx_frame_len + len > sizeof(s->rx_frame)) { + return; + } pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame + (s->rx_frame_size - s->rx_frame_len), len); s->rx_frame_len -= len; @@ -227,7 +235,8 @@ static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf, size_t size) trace_tulip_receive(buf, size); - if (size < 14 || size > 2048 || s->rx_frame_len || tulip_rx_stopped(s)) { + if (size < 14 || size > sizeof(s->rx_frame) - 4 + || s->rx_frame_len || tulip_rx_stopped(s)) { return 0; } @@ -275,7 +284,6 @@ static ssize_t tulip_receive_nc(NetClientState *nc, return tulip_receive(qemu_get_nic_opaque(nc), buf, size); } - static NetClientInfo net_tulip_info = { .type = NET_CLIENT_DRIVER_NIC, .size = sizeof(NICState), @@ -558,7 +566,7 @@ static void tulip_tx(TULIPState *s, struct tulip_descriptor *desc) if ((s->csr[6] >> CSR6_OM_SHIFT) & CSR6_OM_MASK) { /* Internal or external Loopback */ tulip_receive(s, s->tx_frame, s->tx_frame_len); - } else { + } else if (s->tx_frame_len <= sizeof(s->tx_frame)) { qemu_send_packet(qemu_get_queue(s->nic), s->tx_frame, s->tx_frame_len); } @@ -570,23 +578,31 @@ static void tulip_tx(TULIPState *s, struct tulip_descriptor *desc) } } -static void tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor *desc) +static int tulip_copy_tx_buffers(TULIPState *s, struct tulip_descriptor *desc) { int len1 = (desc->control >> TDES1_BUF1_SIZE_SHIFT) & TDES1_BUF1_SIZE_MASK; int len2 = (desc->control >> TDES1_BUF2_SIZE_SHIFT) & TDES1_BUF2_SIZE_MASK; + if (s->tx_frame_len + len1 > sizeof(s->tx_frame)) { + return -1; + } if (len1) { pci_dma_read(&s->dev, desc->buf_addr1, s->tx_frame + s->tx_frame_len, len1); s->tx_frame_len += len1; } + if (s->tx_frame_len + len2 > sizeof(s->tx_frame)) { + return -1; + } if (len2) { pci_dma_read(&s->dev, desc->buf_addr2, s->tx_frame + s->tx_frame_len, len2); s->tx_frame_len += len2; } desc->status = (len1 + len2) ? 0 : 0x7fffffff; + + return 0; } static void tulip_setup_filter_addr(TULIPState *s, uint8_t *buf, int n) @@ -651,13 +667,15 @@ static uint32_t tulip_ts(TULIPState *s) static void tulip_xmit_list_update(TULIPState *s) { +#define TULIP_DESC_MAX 128 + uint8_t i = 0; struct tulip_descriptor desc; if (tulip_ts(s) != CSR5_TS_SUSPENDED) { return; } - for (;;) { + for (i = 0; i < TULIP_DESC_MAX; i++) { tulip_desc_read(s, s->current_tx_desc, &desc); tulip_dump_tx_descriptor(s, &desc); @@ -675,10 +693,10 @@ static void tulip_xmit_list_update(TULIPState *s) s->tx_frame_len = 0; } - tulip_copy_tx_buffers(s, &desc); - - if (desc.control & TDES1_LS) { - tulip_tx(s, &desc); + if (!tulip_copy_tx_buffers(s, &desc)) { + if (desc.control & TDES1_LS) { + tulip_tx(s, &desc); + } } } tulip_desc_write(s, s->current_tx_desc, &desc); From b88fb1247b8f82ff49a82e69965adca6d329a3b3 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 24 Mar 2020 21:21:03 +0000 Subject: [PATCH 13/14] hw/net/allwinner-sun8i-emac.c: Fix REG_ADDR_HIGH/LOW reads Coverity points out (CID 1421926) that the read code for REG_ADDR_HIGH reads off the end of the buffer, because it does a 32-bit read from byte 4 of a 6-byte buffer. The code also has an endianness issue for both REG_ADDR_HIGH and REG_ADDR_LOW, because it will do the wrong thing on a big-endian host. Rewrite the read code to use ldl_le_p() and lduw_le_p() to fix this; the write code is not incorrect, but for consistency we make it use stl_le_p() and stw_le_p(). Reviewed-by: Richard Henderson Tested-by: Niek Linnenbank Reviewed-by: Niek Linnenbank Signed-off-by: Peter Maydell Signed-off-by: Jason Wang --- hw/net/allwinner-sun8i-emac.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/hw/net/allwinner-sun8i-emac.c b/hw/net/allwinner-sun8i-emac.c index 033eaa85bf..28637ff4c1 100644 --- a/hw/net/allwinner-sun8i-emac.c +++ b/hw/net/allwinner-sun8i-emac.c @@ -611,10 +611,10 @@ static uint64_t allwinner_sun8i_emac_read(void *opaque, hwaddr offset, value = s->mii_data; break; case REG_ADDR_HIGH: /* MAC Address High */ - value = *(((uint32_t *) (s->conf.macaddr.a)) + 1); + value = lduw_le_p(s->conf.macaddr.a + 4); break; case REG_ADDR_LOW: /* MAC Address Low */ - value = *(uint32_t *) (s->conf.macaddr.a); + value = ldl_le_p(s->conf.macaddr.a); break; case REG_TX_DMA_STA: /* Transmit DMA Status */ break; @@ -728,14 +728,10 @@ static void allwinner_sun8i_emac_write(void *opaque, hwaddr offset, s->mii_data = value; break; case REG_ADDR_HIGH: /* MAC Address High */ - s->conf.macaddr.a[4] = (value & 0xff); - s->conf.macaddr.a[5] = (value & 0xff00) >> 8; + stw_le_p(s->conf.macaddr.a + 4, value); break; case REG_ADDR_LOW: /* MAC Address Low */ - s->conf.macaddr.a[0] = (value & 0xff); - s->conf.macaddr.a[1] = (value & 0xff00) >> 8; - s->conf.macaddr.a[2] = (value & 0xff0000) >> 16; - s->conf.macaddr.a[3] = (value & 0xff000000) >> 24; + stl_le_p(s->conf.macaddr.a, value); break; case REG_TX_DMA_STA: /* Transmit DMA Status */ case REG_TX_CUR_DESC: /* Transmit Current Descriptor */ From 1153cf9f5b67fad41ca6f8571e9a26e2c7c70759 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Mon, 30 Mar 2020 07:52:01 -0700 Subject: [PATCH 14/14] qtest: add tulip test case The tulip networking card emulation has an OOB issue in 'tulip_copy_tx_buffers' when the guest provide malformed descriptor. This test will trigger a ASAN heap overflow crash. To trigger this issue we can construct the data as following: 1. construct a 'tulip_descriptor'. Its control is set to '0x7ff | 0x7ff << 11', this will make the 'tulip_copy_tx_buffers's 'len1' and 'len2' to 0x7ff(2047). So 'len1+len2' will overflow 'TULIPState's 'tx_frame' field. This descriptor's 'buf_addr1' and 'buf_addr2' should set to a guest address. 2. write this descriptor to tulip device's CSR4 register. This will set the 'TULIPState's 'current_tx_desc' field. 3. write 'CSR6_ST' to tulip device's CSR6 register. This will trigger 'tulip_xmit_list_update' and finally calls 'tulip_copy_tx_buffers'. Following shows the backtrack of crash: ==31781==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x628000007cd0 at pc 0x7fe03c5a077a bp 0x7fff05b46770 sp 0x7fff05b45f18 WRITE of size 2047 at 0x628000007cd0 thread T0 #0 0x7fe03c5a0779 (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x79779) #1 0x5575fb6daa6a in flatview_read_continue /home/test/qemu/exec.c:3194 #2 0x5575fb6daccb in flatview_read /home/test/qemu/exec.c:3227 #3 0x5575fb6dae66 in address_space_read_full /home/test/qemu/exec.c:3240 #4 0x5575fb6db0cb in address_space_rw /home/test/qemu/exec.c:3268 #5 0x5575fbdfd460 in dma_memory_rw_relaxed /home/test/qemu/include/sysemu/dma.h:87 #6 0x5575fbdfd4b5 in dma_memory_rw /home/test/qemu/include/sysemu/dma.h:110 #7 0x5575fbdfd866 in pci_dma_rw /home/test/qemu/include/hw/pci/pci.h:787 #8 0x5575fbdfd8a3 in pci_dma_read /home/test/qemu/include/hw/pci/pci.h:794 #9 0x5575fbe02761 in tulip_copy_tx_buffers hw/net/tulip.c:585 #10 0x5575fbe0366b in tulip_xmit_list_update hw/net/tulip.c:678 #11 0x5575fbe04073 in tulip_write hw/net/tulip.c:783 Signed-off-by: Li Qiang Signed-off-by: Jason Wang --- tests/qtest/Makefile.include | 1 + tests/qtest/tulip-test.c | 91 ++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 tests/qtest/tulip-test.c diff --git a/tests/qtest/Makefile.include b/tests/qtest/Makefile.include index 10a28de8a3..9e5a51d033 100644 --- a/tests/qtest/Makefile.include +++ b/tests/qtest/Makefile.include @@ -217,6 +217,7 @@ qos-test-obj-y += tests/qtest/es1370-test.o qos-test-obj-y += tests/qtest/ipoctal232-test.o qos-test-obj-y += tests/qtest/megasas-test.o qos-test-obj-y += tests/qtest/ne2000-test.o +qos-test-obj-y += tests/qtest/tulip-test.o qos-test-obj-y += tests/qtest/nvme-test.o qos-test-obj-y += tests/qtest/pca9552-test.o qos-test-obj-y += tests/qtest/pci-test.o diff --git a/tests/qtest/tulip-test.c b/tests/qtest/tulip-test.c new file mode 100644 index 0000000000..2fb6c4d5a7 --- /dev/null +++ b/tests/qtest/tulip-test.c @@ -0,0 +1,91 @@ +/* + * QTest testcase for DEC/Intel Tulip 21143 + * + * Copyright (c) 2020 Li Qiang + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "libqtest.h" +#include "qemu/module.h" +#include "libqos/qgraph.h" +#include "libqos/pci.h" +#include "qemu/bitops.h" +#include "hw/net/tulip.h" + +typedef struct QTulip_pci QTulip_pci; + +struct QTulip_pci { + QOSGraphObject obj; + QPCIDevice dev; +}; + +static void *tulip_pci_get_driver(void *obj, const char *interface) +{ + QTulip_pci *tulip_pci = obj; + + if (!g_strcmp0(interface, "pci-device")) { + return &tulip_pci->dev; + } + + fprintf(stderr, "%s not present in tulip_pci\n", interface); + g_assert_not_reached(); +} + +static void *tulip_pci_create(void *pci_bus, QGuestAllocator *alloc, void *addr) +{ + QTulip_pci *tulip_pci = g_new0(QTulip_pci, 1); + QPCIBus *bus = pci_bus; + + qpci_device_init(&tulip_pci->dev, bus, addr); + tulip_pci->obj.get_driver = tulip_pci_get_driver; + + return &tulip_pci->obj; +} + +static void tulip_large_tx(void *obj, void *data, QGuestAllocator *alloc) +{ + QTulip_pci *tulip_pci = obj; + QPCIDevice *dev = &tulip_pci->dev; + QPCIBar bar; + struct tulip_descriptor context; + char guest_data[4096]; + uint64_t context_pa; + uint64_t guest_pa; + + qpci_device_enable(dev); + bar = qpci_iomap(dev, 0, NULL); + context_pa = guest_alloc(alloc, sizeof(context)); + guest_pa = guest_alloc(alloc, 4096); + memset(guest_data, 'A', sizeof(guest_data)); + context.status = TDES0_OWN; + context.control = TDES1_BUF2_SIZE_MASK << TDES1_BUF2_SIZE_SHIFT | + TDES1_BUF1_SIZE_MASK << TDES1_BUF1_SIZE_SHIFT; + context.buf_addr2 = guest_pa; + context.buf_addr1 = guest_pa; + + qtest_memwrite(dev->bus->qts, context_pa, &context, sizeof(context)); + qtest_memwrite(dev->bus->qts, guest_pa, guest_data, sizeof(guest_data)); + qpci_io_writel(dev, bar, 0x20, context_pa); + qpci_io_writel(dev, bar, 0x30, CSR6_ST); + guest_free(alloc, context_pa); + guest_free(alloc, guest_pa); +} + +static void tulip_register_nodes(void) +{ + QOSGraphEdgeOptions opts = { + .extra_device_opts = "addr=04.0", + }; + add_qpci_address(&opts, &(QPCIAddress) { .devfn = QPCI_DEVFN(4, 0) }); + + qos_node_create_driver("tulip", tulip_pci_create); + qos_node_consumes("tulip", "pci-bus", &opts); + qos_node_produces("tulip", "pci-device"); + + qos_add_test("tulip_large_tx", "tulip", tulip_large_tx, NULL); +} + +libqos_init(tulip_register_nodes);