[Qemu-devel] [PATCHv3 00/13] net: introduce common net_crc32() and net_crc32_le() functions

[Qemu-devel] [PATCHv3 00/13] net: introduce common net_crc32() and net_crc32_le() functions

[Mark Cave-Ayland]

Whilst trying to debug a CRC32 endian issue for NIC multicast hash lookups, it
struck me that it would make sense to have a common set of standard ethernet
CRC32 functions (both little and big endian variants) in net.c.

Patches 1 and 2 introduce the new net_crc32() and net_crc32_le() functions for
use by NIC multicast hash lookups.

Patches 3 to 5 switch the pcnet, eepro100 and sunhme drivers over to use the
new functions instead of having their own private implementations.

Patch 6 fixes the sungem multicast filter CRC calculation, since we can see from
the Linux sungem driver that the CRC is calculated using ether_crc32_le() and so
the direct use of the zlib crc32() function is incorrect. The solution here is to
simply use the corresponding net_crc32_le() function.

Patches 7 to 12 switch the remaining users of compute_mcast_idx() over to use
the new net_crc32() function as it becomes much easier to spot errors in the
multicast hash calculations (see below).

Finally patch 13 removes the now unused compute_mcast_idx() function.

One of the motivations for removing compute_mcast_idx() and using the CRC and
bitshift inline is because it makes it much easier to spot potential errors
when comparing with the corresponding driver source code. This led to the following
curiosities whilst developing the patchset:

1) The sungem multicast filter CRC calculation was incorrect (fixed by patch 6 as
   I was able to test locally)

2) After conversion we can see in eepro100.c line 1682 that there is one single
   remaining multicast hash calculation that doesn't apply a BITS(7, 2) mask to
   the filter index when compared with all the others. This looks incorrect, but
   I don't have an easy way to verify this.

3) In ftgmac100.c we see a comment next to the multicast hash filter calculation
   that states "TODO: this does not seem to work for ftgmac100". Upon consulting the
   Linux driver source we can see that ether_crc32_le() is used in the driver
   suggesting that changing net_crc32() to net_crc32_le() should fix the calculation.
   Again I've left this as I don't have an easy way to verify the fix.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

v3:
- Add R-B and S-o-B tags
- Fix sungem multicast hash filter
- Use ETH_ALEN to specift MAC address length as suggested by Phillipe
- Switch to using inline CRC + bitshift as suggested by Stefan (i.e. remove
  what's left of the remaining hash function)
- Convert all remaining users of compute_mcast_idx() to inline CRC + bitshift
  (and then remove it) to make it easier to validate the multicast hash index
  calculation with the corresponding driver source

v2:
- Add sumhme net_crc32_le() conversion

Mark Cave-Ayland (13):
  net: move CRC32 calculation from compute_mcast_idx() into its own
    net_crc32() function
  net: introduce net_crc32_le() function
  pcnet: switch pcnet over to use net_crc32_le()
  eepro100: switch eepro100 e100_compute_mcast_idx() over to use
    net_crc32()
  sunhme: switch sunhme over to use net_crc32_le()
  sungem: fix multicast filter CRC calculation
  eepro100: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  opencores_eth: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  lan9118: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  ftgmac100: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  ne2000: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  rtl8139: use inline net_crc32() and bitshift instead of
    compute_mcast_idx()
  net: remove unused compute_mcast_idx() function

 hw/net/eepro100.c      | 32 +++++---------------------------
 hw/net/ftgmac100.c     |  2 +-
 hw/net/lan9118.c       |  3 ++-
 hw/net/ne2000.c        |  3 ++-
 hw/net/opencores_eth.c |  3 ++-
 hw/net/pcnet.c         | 22 ++--------------------
 hw/net/rtl8139.c       |  2 +-
 hw/net/sungem.c        |  5 ++---
 hw/net/sunhme.c        | 25 +------------------------
 include/net/net.h      |  5 ++++-
 net/net.c              | 33 ++++++++++++++++++++++++++++-----
 11 files changed, 50 insertions(+), 85 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCHv3 01/13] net: move CRC32 calculation from compute_mcast_idx() into its own net_crc32() function

[Mark Cave-Ayland]

Separate out the standard ethernet CRC32 calculation into a new net_crc32()
function, renaming the constant POLYNOMIAL to POLYNOMIAL_BE to make it clear
that this is a big-endian CRC32 calculation.

As part of the constant rename, remove the duplicate definition of POLYNOMIAL
from eepro100.c and use the new POLYNOMIAL_BE constant instead.

Once this is complete remove the existing CRC32 implementation from
compute_mcast_idx() and call the new net_crc32() function in its place.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/eepro100.c |  4 +---
 include/net/net.h |  3 ++-
 net/net.c         | 16 +++++++++++-----
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 1c0def555b..71cddfece4 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -323,8 +323,6 @@ static const uint16_t eepro100_mdi_mask[] = {
     0xffff, 0xffff, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
 };
 
-#define POLYNOMIAL 0x04c11db6
-
 static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
 
 /* From FreeBSD (locally modified). */
@@ -342,7 +340,7 @@ static unsigned e100_compute_mcast_idx(const uint8_t *ep)
             crc <<= 1;
             b >>= 1;
             if (carry) {
-                crc = ((crc ^ POLYNOMIAL) | carry);
+                crc = ((crc ^ POLYNOMIAL_BE) | carry);
             }
         }
     }
diff --git a/include/net/net.h b/include/net/net.h
index 1c55a93588..586098cb94 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -227,7 +227,8 @@ NetClientState *net_hub_port_find(int hub_id);
 
 void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
 
-#define POLYNOMIAL 0x04c11db6
+#define POLYNOMIAL_BE 0x04c11db6
+uint32_t net_crc32(const uint8_t *p, int len);
 unsigned compute_mcast_idx(const uint8_t *ep);
 
 #define vmstate_offset_macaddr(_state, _field)                       \
diff --git a/net/net.c b/net/net.c
index 39ef546708..a14dc9910c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1581,25 +1581,31 @@ int net_client_parse(QemuOptsList *opts_list, const char *optarg)
 
 /* From FreeBSD */
 /* XXX: optimize */
-unsigned compute_mcast_idx(const uint8_t *ep)
+uint32_t net_crc32(const uint8_t *p, int len)
 {
     uint32_t crc;
     int carry, i, j;
     uint8_t b;
 
     crc = 0xffffffff;
-    for (i = 0; i < 6; i++) {
-        b = *ep++;
+    for (i = 0; i < len; i++) {
+        b = *p++;
         for (j = 0; j < 8; j++) {
             carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
             crc <<= 1;
             b >>= 1;
             if (carry) {
-                crc = ((crc ^ POLYNOMIAL) | carry);
+                crc = ((crc ^ POLYNOMIAL_BE) | carry);
             }
         }
     }
-    return crc >> 26;
+
+    return crc;
+}
+
+unsigned compute_mcast_idx(const uint8_t *ep)
+{
+    return net_crc32(ep, ETH_ALEN) >> 26;
 }
 
 QemuOptsList qemu_netdev_opts = {
-- 
2.11.0

[Qemu-devel] [PATCHv3 02/13] net: introduce net_crc32_le() function

[Mark Cave-Ayland]

This provides a standard ethernet CRC32 little-endian implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/net/net.h |  2 ++
 net/net.c         | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 586098cb94..4afac1a9dd 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -228,7 +228,9 @@ NetClientState *net_hub_port_find(int hub_id);
 void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
 
 #define POLYNOMIAL_BE 0x04c11db6
+#define POLYNOMIAL_LE 0xedb88320
 uint32_t net_crc32(const uint8_t *p, int len);
+uint32_t net_crc32_le(const uint8_t *p, int len);
 unsigned compute_mcast_idx(const uint8_t *ep);
 
 #define vmstate_offset_macaddr(_state, _field)                       \
diff --git a/net/net.c b/net/net.c
index a14dc9910c..4ecaf80bd1 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1603,6 +1603,28 @@ uint32_t net_crc32(const uint8_t *p, int len)
     return crc;
 }
 
+uint32_t net_crc32_le(const uint8_t *p, int len)
+{
+    uint32_t crc;
+    int carry, i, j;
+    uint8_t b;
+
+    crc = 0xffffffff;
+    for (i = 0; i < len; i++) {
+        b = *p++;
+        for (j = 0; j < 8; j++) {
+            carry = (crc & 0x1) ^ (b & 0x01);
+            crc >>= 1;
+            b >>= 1;
+            if (carry) {
+                crc ^= POLYNOMIAL_LE;
+            }
+        }
+    }
+
+    return crc;
+}
+
 unsigned compute_mcast_idx(const uint8_t *ep)
 {
     return net_crc32(ep, ETH_ALEN) >> 26;
-- 
2.11.0

[Qemu-devel] [PATCHv3 03/13] pcnet: switch pcnet over to use net_crc32_le()

[Mark Cave-Ayland]

Instead of lnc_mchash() using its own implementation, we can simply call
net_crc32_le() directly and apply the bit shift inline.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/pcnet.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 654455355f..39d5d93525 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -38,6 +38,7 @@
 #include "qemu/osdep.h"
 #include "hw/qdev.h"
 #include "net/net.h"
+#include "net/eth.h"
 #include "qemu/timer.h"
 #include "qemu/sockets.h"
 #include "sysemu/sysemu.h"
@@ -522,25 +523,6 @@ static inline void pcnet_rmd_store(PCNetState *s, struct pcnet_RMD *rmd,
            be16_to_cpu(hdr->ether_type));       \
 } while (0)
 
-#define MULTICAST_FILTER_LEN 8
-
-static inline uint32_t lnc_mchash(const uint8_t *ether_addr)
-{
-#define LNC_POLYNOMIAL          0xEDB88320UL
-    uint32_t crc = 0xFFFFFFFF;
-    int idx, bit;
-    uint8_t data;
-
-    for (idx = 0; idx < 6; idx++) {
-        for (data = *ether_addr++, bit = 0; bit < MULTICAST_FILTER_LEN; bit++) {
-            crc = (crc >> 1) ^ (((crc ^ data) & 1) ? LNC_POLYNOMIAL : 0);
-            data >>= 1;
-        }
-    }
-    return crc;
-#undef LNC_POLYNOMIAL
-}
-
 #define CRC(crc, ch)	 (crc = (crc >> 8) ^ crctab[(crc ^ (ch)) & 0xff])
 
 /* generated using the AUTODIN II polynomial
@@ -656,7 +638,7 @@ static inline int ladr_match(PCNetState *s, const uint8_t *buf, int size)
             s->csr[10] & 0xff, s->csr[10] >> 8,
             s->csr[11] & 0xff, s->csr[11] >> 8
         };
-        int index = lnc_mchash(hdr->ether_dhost) >> 26;
+        int index = net_crc32_le(hdr->ether_dhost, ETH_ALEN) >> 26;
         return !!(ladr[index >> 3] & (1 << (index & 7)));
     }
     return 0;
-- 
2.11.0

[Qemu-devel] [PATCHv3 04/13] eepro100: switch eepro100 e100_compute_mcast_idx() over to use net_crc32()

[Mark Cave-Ayland]

Instead of e100_compute_mcast_idx() using its own implementation, we can
simply call net_crc32() directly and apply the bit shift inline.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
---
 hw/net/eepro100.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 71cddfece4..e30fed830b 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -44,6 +44,7 @@
 #include "hw/hw.h"
 #include "hw/pci/pci.h"
 #include "net/net.h"
+#include "net/eth.h"
 #include "hw/nvram/eeprom93xx.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/dma.h"
@@ -325,28 +326,6 @@ static const uint16_t eepro100_mdi_mask[] = {
 
 static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
 
-/* From FreeBSD (locally modified). */
-static unsigned e100_compute_mcast_idx(const uint8_t *ep)
-{
-    uint32_t crc;
-    int carry, i, j;
-    uint8_t b;
-
-    crc = 0xffffffff;
-    for (i = 0; i < 6; i++) {
-        b = *ep++;
-        for (j = 0; j < 8; j++) {
-            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
-            crc <<= 1;
-            b >>= 1;
-            if (carry) {
-                crc = ((crc ^ POLYNOMIAL_BE) | carry);
-            }
-        }
-    }
-    return (crc & BITS(7, 2)) >> 2;
-}
-
 /* Read a 16 bit control/status (CSR) register. */
 static uint16_t e100_read_reg2(EEPRO100State *s, E100RegisterOffset addr)
 {
@@ -843,7 +822,8 @@ static void set_multicast_list(EEPRO100State *s)
         uint8_t multicast_addr[6];
         pci_dma_read(&s->dev, s->cb_address + 10 + i, multicast_addr, 6);
         TRACE(OTHER, logout("multicast entry %s\n", nic_dump(multicast_addr, 6)));
-        unsigned mcast_idx = e100_compute_mcast_idx(multicast_addr);
+        unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
+                              BITS(7, 2)) >> 2;
         assert(mcast_idx < 64);
         s->mult[mcast_idx >> 3] |= (1 << (mcast_idx & 7));
     }
@@ -1679,7 +1659,7 @@ static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf, size_t size)
         if (s->configuration[21] & BIT(3)) {
           /* Multicast all bit is set, receive all multicast frames. */
         } else {
-          unsigned mcast_idx = e100_compute_mcast_idx(buf);
+          unsigned mcast_idx = (net_crc32(buf, ETH_ALEN) & BITS(7, 2)) >> 2;
           assert(mcast_idx < 64);
           if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
             /* Multicast frame is allowed in hash table. */
-- 
2.11.0

Re: [Qemu-devel] [PATCHv3 04/13] eepro100: switch eepro100 e100_compute_mcast_idx() over to use net_crc32()

[Philippe Mathieu-Daudé]

On 12/15/2017 03:41 PM, Mark Cave-Ayland wrote:
> Instead of e100_compute_mcast_idx() using its own implementation, we can
> simply call net_crc32() directly and apply the bit shift inline.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Stefan Weil <sw@weilnetz.de>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/net/eepro100.c | 28 ++++------------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 71cddfece4..e30fed830b 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -44,6 +44,7 @@
>  #include "hw/hw.h"
>  #include "hw/pci/pci.h"
>  #include "net/net.h"
> +#include "net/eth.h"
>  #include "hw/nvram/eeprom93xx.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/dma.h"
> @@ -325,28 +326,6 @@ static const uint16_t eepro100_mdi_mask[] = {
>  
>  static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
>  
> -/* From FreeBSD (locally modified). */
> -static unsigned e100_compute_mcast_idx(const uint8_t *ep)
> -{
> -    uint32_t crc;
> -    int carry, i, j;
> -    uint8_t b;
> -
> -    crc = 0xffffffff;
> -    for (i = 0; i < 6; i++) {
> -        b = *ep++;
> -        for (j = 0; j < 8; j++) {
> -            carry = ((crc & 0x80000000L) ? 1 : 0) ^ (b & 0x01);
> -            crc <<= 1;
> -            b >>= 1;
> -            if (carry) {
> -                crc = ((crc ^ POLYNOMIAL_BE) | carry);
> -            }
> -        }
> -    }
> -    return (crc & BITS(7, 2)) >> 2;
> -}
> -
>  /* Read a 16 bit control/status (CSR) register. */
>  static uint16_t e100_read_reg2(EEPRO100State *s, E100RegisterOffset addr)
>  {
> @@ -843,7 +822,8 @@ static void set_multicast_list(EEPRO100State *s)
>          uint8_t multicast_addr[6];
>          pci_dma_read(&s->dev, s->cb_address + 10 + i, multicast_addr, 6);
>          TRACE(OTHER, logout("multicast entry %s\n", nic_dump(multicast_addr, 6)));
> -        unsigned mcast_idx = e100_compute_mcast_idx(multicast_addr);
> +        unsigned mcast_idx = (net_crc32(multicast_addr, ETH_ALEN) &
> +                              BITS(7, 2)) >> 2;
>          assert(mcast_idx < 64);
>          s->mult[mcast_idx >> 3] |= (1 << (mcast_idx & 7));
>      }
> @@ -1679,7 +1659,7 @@ static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf, size_t size)
>          if (s->configuration[21] & BIT(3)) {
>            /* Multicast all bit is set, receive all multicast frames. */
>          } else {
> -          unsigned mcast_idx = e100_compute_mcast_idx(buf);
> +          unsigned mcast_idx = (net_crc32(buf, ETH_ALEN) & BITS(7, 2)) >> 2;
>            assert(mcast_idx < 64);
>            if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
>              /* Multicast frame is allowed in hash table. */
> 

[Qemu-devel] [PATCHv3 05/13] sunhme: switch sunhme over to use net_crc32_le()

[Mark Cave-Ayland]

Instead of sunhme_crc32_le() using its own implementation, we can simply call
net_crc32_le() directly and apply the bit shift inline.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/sunhme.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/hw/net/sunhme.c b/hw/net/sunhme.c
index b1efa1b88d..7558fca8f9 100644
--- a/hw/net/sunhme.c
+++ b/hw/net/sunhme.c
@@ -698,29 +698,6 @@ static inline void sunhme_set_rx_ring_nr(SunHMEState *s, int i)
     s->erxregs[HME_ERXI_RING >> 2] = ring;
 }
 
-#define POLYNOMIAL_LE 0xedb88320
-static uint32_t sunhme_crc32_le(const uint8_t *p, int len)
-{
-    uint32_t crc;
-    int carry, i, j;
-    uint8_t b;
-
-    crc = 0xffffffff;
-    for (i = 0; i < len; i++) {
-        b = *p++;
-        for (j = 0; j < 8; j++) {
-            carry = (crc & 0x1) ^ (b & 0x01);
-            crc >>= 1;
-            b >>= 1;
-            if (carry) {
-                crc = crc ^ POLYNOMIAL_LE;
-            }
-        }
-    }
-
-    return crc;
-}
-
 #define MIN_BUF_SIZE 60
 
 static ssize_t sunhme_receive(NetClientState *nc, const uint8_t *buf,
@@ -761,7 +738,7 @@ static ssize_t sunhme_receive(NetClientState *nc, const uint8_t *buf,
             trace_sunhme_rx_filter_bcast_match();
         } else if (s->macregs[HME_MACI_RXCFG >> 2] & HME_MAC_RXCFG_HENABLE) {
             /* Didn't match local address, check hash filter */
-            int mcast_idx = sunhme_crc32_le(buf, 6) >> 26;
+            int mcast_idx = net_crc32_le(buf, ETH_ALEN) >> 26;
             if (!(s->macregs[(HME_MACI_HASHTAB0 >> 2) - (mcast_idx >> 4)] &
                     (1 << (mcast_idx & 0xf)))) {
                 /* Didn't match hash filter */
-- 
2.11.0

[Qemu-devel] [PATCHv3 06/13] sungem: fix multicast filter CRC calculation

[Mark Cave-Ayland]

>From the Linux sungem driver, we know that the multicast filter CRC is
implemented using ether_crc_le() which isn't the same as calling zlib's
crc32() function (the zlib implementation requires a complemented initial value
and also returns the complemented result).

Fix the multicast filter by simply using the new net_crc32_le() function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/sungem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/net/sungem.c b/hw/net/sungem.c
index 6aa8d1117b..60f1e479f3 100644
--- a/hw/net/sungem.c
+++ b/hw/net/sungem.c
@@ -11,12 +11,11 @@
 #include "hw/pci/pci.h"
 #include "qemu/log.h"
 #include "net/net.h"
+#include "net/eth.h"
 #include "net/checksum.h"
 #include "hw/net/mii.h"
 #include "sysemu/sysemu.h"
 #include "trace.h"
-/* For crc32 */
-#include <zlib.h>
 
 #define TYPE_SUNGEM "sungem"
 
@@ -595,7 +594,7 @@ static ssize_t sungem_receive(NetClientState *nc, const uint8_t *buf,
     }
 
     /* Get MAC crc */
-    mac_crc = crc32(~0, buf, 6);
+    mac_crc = net_crc32_le(buf, ETH_ALEN);
 
     /* Packet isn't for me ? */
     rx_cond = sungem_check_rx_mac(s, buf, mac_crc);
-- 
2.11.0

Re: [Qemu-devel] [PATCHv3 06/13] sungem: fix multicast filter CRC calculation

[Philippe Mathieu-Daudé]

On 12/15/2017 03:41 PM, Mark Cave-Ayland wrote:
> From the Linux sungem driver, we know that the multicast filter CRC is
> implemented using ether_crc_le() which isn't the same as calling zlib's
> crc32() function (the zlib implementation requires a complemented initial value
> and also returns the complemented result).
> 
> Fix the multicast filter by simply using the new net_crc32_le() function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/net/sungem.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/sungem.c b/hw/net/sungem.c
> index 6aa8d1117b..60f1e479f3 100644
> --- a/hw/net/sungem.c
> +++ b/hw/net/sungem.c
> @@ -11,12 +11,11 @@
>  #include "hw/pci/pci.h"
>  #include "qemu/log.h"
>  #include "net/net.h"
> +#include "net/eth.h"
>  #include "net/checksum.h"
>  #include "hw/net/mii.h"
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
> -/* For crc32 */
> -#include <zlib.h>
>  
>  #define TYPE_SUNGEM "sungem"
>  
> @@ -595,7 +594,7 @@ static ssize_t sungem_receive(NetClientState *nc, const uint8_t *buf,
>      }
>  
>      /* Get MAC crc */
> -    mac_crc = crc32(~0, buf, 6);
> +    mac_crc = net_crc32_le(buf, ETH_ALEN);
>  
>      /* Packet isn't for me ? */
>      rx_cond = sungem_check_rx_mac(s, buf, mac_crc);
> 

[Qemu-devel] [PATCHv3 07/13] eepro100: use inline net_crc32() and bitshift instead of compute_mcast_idx()

[Mark Cave-Ayland]

This makes it much easier to compare the multicast CRC calculation endian and
bitshift against the Linux driver implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/eepro100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index e30fed830b..a07a63247e 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1679,7 +1679,7 @@ static ssize_t nic_receive(NetClientState *nc, const uint8_t * buf, size_t size)
         rfd_status |= 0x0004;
     } else if (s->configuration[20] & BIT(6)) {
         /* Multiple IA bit set. */
-        unsigned mcast_idx = compute_mcast_idx(buf);
+        unsigned mcast_idx = net_crc32(buf, ETH_ALEN) >> 26;
         assert(mcast_idx < 64);
         if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
             TRACE(RXTX, logout("%p accepted, multiple IA bit set\n", s));
-- 
2.11.0

[Qemu-devel] [PATCHv3 08/13] opencores_eth: use inline net_crc32() and bitshift instead of compute_mcast_idx()

[Mark Cave-Ayland]

This makes it much easier to compare the multicast CRC calculation endian and
bitshift against the Linux driver implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/opencores_eth.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/opencores_eth.c b/hw/net/opencores_eth.c
index 268d6a7892..d42b79c08c 100644
--- a/hw/net/opencores_eth.c
+++ b/hw/net/opencores_eth.c
@@ -36,6 +36,7 @@
 #include "hw/net/mii.h"
 #include "hw/sysbus.h"
 #include "net/net.h"
+#include "net/eth.h"
 #include "sysemu/sysemu.h"
 #include "trace.h"
 
@@ -373,7 +374,7 @@ static ssize_t open_eth_receive(NetClientState *nc,
         if (memcmp(buf, bcast_addr, sizeof(bcast_addr)) == 0) {
             miss = GET_REGBIT(s, MODER, BRO);
         } else if ((buf[0] & 0x1) || GET_REGBIT(s, MODER, IAM)) {
-            unsigned mcast_idx = compute_mcast_idx(buf);
+            unsigned mcast_idx = net_crc32(buf, ETH_ALEN) >> 26;
             miss = !(s->regs[HASH0 + mcast_idx / 32] &
                     (1 << (mcast_idx % 32)));
             trace_open_eth_receive_mcast(
-- 
2.11.0

[Qemu-devel] [PATCHv3 09/13] lan9118: use inline net_crc32() and bitshift instead of compute_mcast_idx()

[Mark Cave-Ayland]

This makes it much easier to compare the multicast CRC calculation endian and
bitshift against the Linux driver implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/lan9118.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 3db8937cac..b9032dac59 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "net/net.h"
+#include "net/eth.h"
 #include "hw/devices.h"
 #include "sysemu/sysemu.h"
 #include "hw/ptimer.h"
@@ -504,7 +505,7 @@ static int lan9118_filter(lan9118_state *s, const uint8_t *addr)
         }
     } else {
         /* Hash matching  */
-        hash = compute_mcast_idx(addr);
+        hash = net_crc32(addr, ETH_ALEN) >> 26;
         if (hash & 0x20) {
             return (s->mac_hashh >> (hash & 0x1f)) & 1;
         } else {
-- 
2.11.0

[Qemu-devel] [PATCHv3 10/13] ftgmac100: use inline net_crc32() and bitshift instead of compute_mcast_idx()

[Mark Cave-Ayland]

This makes it much easier to compare the multicast CRC calculation endian and
bitshift against the Linux driver implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/ftgmac100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 3c36ab9cec..704f452067 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -762,7 +762,7 @@ static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
             }
 
             /* TODO: this does not seem to work for ftgmac100 */
-            mcast_idx = compute_mcast_idx(buf);
+            mcast_idx = net_crc32(buf, ETH_ALEN) >> 26;
             if (!(s->math[mcast_idx / 32] & (1 << (mcast_idx % 32)))) {
                 return 0;
             }
-- 
2.11.0

[Qemu-devel] [PATCHv3 11/13] ne2000: use inline net_crc32() and bitshift instead of compute_mcast_idx()

[Mark Cave-Ayland]

This makes it much easier to compare the multicast CRC calculation endian and
bitshift against the Linux driver implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/ne2000.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index 3938e6ddd8..7e3b77c00f 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -25,6 +25,7 @@
 #include "hw/hw.h"
 #include "hw/pci/pci.h"
 #include "net/net.h"
+#include "net/eth.h"
 #include "ne2000.h"
 #include "hw/loader.h"
 #include "sysemu/sysemu.h"
@@ -201,7 +202,7 @@ ssize_t ne2000_receive(NetClientState *nc, const uint8_t *buf, size_t size_)
             /* multicast */
             if (!(s->rxcr & 0x08))
                 return size;
-            mcast_idx = compute_mcast_idx(buf);
+            mcast_idx = net_crc32(buf, ETH_ALEN) >> 26;
             if (!(s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))))
                 return size;
         } else if (s->mem[0] == buf[0] &&
-- 
2.11.0

[Qemu-devel] [PATCHv3 12/13] rtl8139: use inline net_crc32() and bitshift instead of compute_mcast_idx()

[Mark Cave-Ayland]

This makes it much easier to compare the multicast CRC calculation endian and
bitshift against the Linux driver implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/net/rtl8139.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index a6b2a9f7a4..1cc95b8cba 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -882,7 +882,7 @@ static ssize_t rtl8139_do_receive(NetClientState *nc, const uint8_t *buf, size_t
                 return size;
             }
 
-            int mcast_idx = compute_mcast_idx(buf);
+            int mcast_idx = net_crc32(buf, ETH_ALEN) >> 26;
 
             if (!(s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))))
             {
-- 
2.11.0

[Qemu-devel] [PATCHv3 13/13] net: remove unused compute_mcast_idx() function

[Mark Cave-Ayland]

Now that all of the callers have been converted to compute the multicast index
inline using new net CRC functions, this function can now be dropped.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 net/net.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/net.c b/net/net.c
index 4ecaf80bd1..5bc0a343ae 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1625,11 +1625,6 @@ uint32_t net_crc32_le(const uint8_t *p, int len)
     return crc;
 }
 
-unsigned compute_mcast_idx(const uint8_t *ep)
-{
-    return net_crc32(ep, ETH_ALEN) >> 26;
-}
-
 QemuOptsList qemu_netdev_opts = {
     .name = "netdev",
     .implied_opt_name = "type",
-- 
2.11.0

Re: [Qemu-devel] [PATCHv3 00/13] net: introduce common net_crc32() and net_crc32_le() functions

[Jason Wang]

On 2017年12月16日 02:41, Mark Cave-Ayland wrote:
> Whilst trying to debug a CRC32 endian issue for NIC multicast hash lookups, it
> struck me that it would make sense to have a common set of standard ethernet
> CRC32 functions (both little and big endian variants) in net.c.
>
> Patches 1 and 2 introduce the new net_crc32() and net_crc32_le() functions for
> use by NIC multicast hash lookups.
>
> Patches 3 to 5 switch the pcnet, eepro100 and sunhme drivers over to use the
> new functions instead of having their own private implementations.
>
> Patch 6 fixes the sungem multicast filter CRC calculation, since we can see from
> the Linux sungem driver that the CRC is calculated using ether_crc32_le() and so
> the direct use of the zlib crc32() function is incorrect. The solution here is to
> simply use the corresponding net_crc32_le() function.
>
> Patches 7 to 12 switch the remaining users of compute_mcast_idx() over to use
> the new net_crc32() function as it becomes much easier to spot errors in the
> multicast hash calculations (see below).
>
> Finally patch 13 removes the now unused compute_mcast_idx() function.
>
> One of the motivations for removing compute_mcast_idx() and using the CRC and
> bitshift inline is because it makes it much easier to spot potential errors
> when comparing with the corresponding driver source code. This led to the following
> curiosities whilst developing the patchset:
>
> 1) The sungem multicast filter CRC calculation was incorrect (fixed by patch 6 as
>     I was able to test locally)
>
> 2) After conversion we can see in eepro100.c line 1682 that there is one single
>     remaining multicast hash calculation that doesn't apply a BITS(7, 2) mask to
>     the filter index when compared with all the others. This looks incorrect, but
>     I don't have an easy way to verify this.
>
> 3) In ftgmac100.c we see a comment next to the multicast hash filter calculation
>     that states "TODO: this does not seem to work for ftgmac100". Upon consulting the
>     Linux driver source we can see that ether_crc32_le() is used in the driver
>     suggesting that changing net_crc32() to net_crc32_le() should fix the calculation.
>     Again I've left this as I don't have an easy way to verify the fix.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Series looks good to me.

A small question is that, is this better to keep compute_mcast_idx()?

Thanks

>
> v3:
> - Add R-B and S-o-B tags
> - Fix sungem multicast hash filter
> - Use ETH_ALEN to specift MAC address length as suggested by Phillipe
> - Switch to using inline CRC + bitshift as suggested by Stefan (i.e. remove
>    what's left of the remaining hash function)
> - Convert all remaining users of compute_mcast_idx() to inline CRC + bitshift
>    (and then remove it) to make it easier to validate the multicast hash index
>    calculation with the corresponding driver source
>
> v2:
> - Add sumhme net_crc32_le() conversion
>
> Mark Cave-Ayland (13):
>    net: move CRC32 calculation from compute_mcast_idx() into its own
>      net_crc32() function
>    net: introduce net_crc32_le() function
>    pcnet: switch pcnet over to use net_crc32_le()
>    eepro100: switch eepro100 e100_compute_mcast_idx() over to use
>      net_crc32()
>    sunhme: switch sunhme over to use net_crc32_le()
>    sungem: fix multicast filter CRC calculation
>    eepro100: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    opencores_eth: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    lan9118: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    ftgmac100: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    ne2000: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    rtl8139: use inline net_crc32() and bitshift instead of
>      compute_mcast_idx()
>    net: remove unused compute_mcast_idx() function
>
>   hw/net/eepro100.c      | 32 +++++---------------------------
>   hw/net/ftgmac100.c     |  2 +-
>   hw/net/lan9118.c       |  3 ++-
>   hw/net/ne2000.c        |  3 ++-
>   hw/net/opencores_eth.c |  3 ++-
>   hw/net/pcnet.c         | 22 ++--------------------
>   hw/net/rtl8139.c       |  2 +-
>   hw/net/sungem.c        |  5 ++---
>   hw/net/sunhme.c        | 25 +------------------------
>   include/net/net.h      |  5 ++++-
>   net/net.c              | 33 ++++++++++++++++++++++++++++-----
>   11 files changed, 50 insertions(+), 85 deletions(-)
>

Re: [Qemu-devel] [PATCHv3 00/13] net: introduce common net_crc32() and net_crc32_le() functions

[Mark Cave-Ayland]

On 20/12/17 08:35, Jason Wang wrote:
> 
> 
> On 2017年12月16日 02:41, Mark Cave-Ayland wrote:
>> Whilst trying to debug a CRC32 endian issue for NIC multicast hash 
>> lookups, it
>> struck me that it would make sense to have a common set of standard 
>> ethernet
>> CRC32 functions (both little and big endian variants) in net.c.
>>
>> Patches 1 and 2 introduce the new net_crc32() and net_crc32_le() 
>> functions for
>> use by NIC multicast hash lookups.
>>
>> Patches 3 to 5 switch the pcnet, eepro100 and sunhme drivers over to 
>> use the
>> new functions instead of having their own private implementations.
>>
>> Patch 6 fixes the sungem multicast filter CRC calculation, since we 
>> can see from
>> the Linux sungem driver that the CRC is calculated using 
>> ether_crc32_le() and so
>> the direct use of the zlib crc32() function is incorrect. The solution 
>> here is to
>> simply use the corresponding net_crc32_le() function.
>>
>> Patches 7 to 12 switch the remaining users of compute_mcast_idx() over 
>> to use
>> the new net_crc32() function as it becomes much easier to spot errors 
>> in the
>> multicast hash calculations (see below).
>>
>> Finally patch 13 removes the now unused compute_mcast_idx() function.
>>
>> One of the motivations for removing compute_mcast_idx() and using the 
>> CRC and
>> bitshift inline is because it makes it much easier to spot potential 
>> errors
>> when comparing with the corresponding driver source code. This led to 
>> the following
>> curiosities whilst developing the patchset:
>>
>> 1) The sungem multicast filter CRC calculation was incorrect (fixed by 
>> patch 6 as
>>     I was able to test locally)
>>
>> 2) After conversion we can see in eepro100.c line 1682 that there is 
>> one single
>>     remaining multicast hash calculation that doesn't apply a BITS(7, 
>> 2) mask to
>>     the filter index when compared with all the others. This looks 
>> incorrect, but
>>     I don't have an easy way to verify this.
>>
>> 3) In ftgmac100.c we see a comment next to the multicast hash filter 
>> calculation
>>     that states "TODO: this does not seem to work for ftgmac100". Upon 
>> consulting the
>>     Linux driver source we can see that ether_crc32_le() is used in 
>> the driver
>>     suggesting that changing net_crc32() to net_crc32_le() should fix 
>> the calculation.
>>     Again I've left this as I don't have an easy way to verify the fix.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Series looks good to me.
> 
> A small question is that, is this better to keep compute_mcast_idx()?
> 
> Thanks

Hi Jason,

I did think about this, however at the very minimum you'd need 
big-endian and little-endian variants of compute_mcast_idx(), and then 
you see that eepro100 applies a different bitmask/shift which is yet 
another variant...

For this reason I moved them all inline to the QEMU driver and that made 
it possible to compare the hash calculation directly with the 
corresponding Linux driver which found the 3 potential bugs above. So I 
think this is a net win (pardon the pun) on all sides :)


ATB,

Mark.

Re: [Qemu-devel] [PATCHv3 00/13] net: introduce common net_crc32() and net_crc32_le() functions

[Jason Wang]

On 2017年12月20日 16:58, Mark Cave-Ayland wrote:
>> Series looks good to me.
>>
>> A small question is that, is this better to keep compute_mcast_idx()?
>>
>> Thanks
>
> Hi Jason,
>
> I did think about this, however at the very minimum you'd need 
> big-endian and little-endian variants of compute_mcast_idx(), and then 
> you see that eepro100 applies a different bitmask/shift which is yet 
> another variant...
>
> For this reason I moved them all inline to the QEMU driver and that 
> made it possible to compare the hash calculation directly with the 
> corresponding Linux driver which found the 3 potential bugs above. So 
> I think this is a net win (pardon the pun) on all sides :)
>
>
> ATB,

Ok, applied.

Thanks