[Qemu-devel] [PATCHv2 0/5] net: introduce common net_crc32() and net_crc32_le() functions

[Qemu-devel] [PATCHv2 0/5] 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.

The first two patches introduce the relevant functions while the last 3 patches
switch the pcnet, eepro100 and sunhme drivers over to use them, allowing us to remove
their private implementations.

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

v2:
- Add sumhme net_crc32_le() conversion


Mark Cave-Ayland (5):
  net: move CRC32 calculation from compute_mcast_idx() into its own
    net_crc32() function
  net: introduce net_crc32_le() function
  pcnet: switch lnc_mchash() over to use net_crc32_le()
  eepro100: switch e100_compute_mcast_idx() over to use net_crc32()
  sunhme: switch sunhme_receive() over to use net_crc32_le()

 hw/net/eepro100.c | 19 +------------------
 hw/net/pcnet.c    | 16 +---------------
 hw/net/sunhme.c   | 25 +------------------------
 include/net/net.h |  5 ++++-
 net/net.c         | 38 +++++++++++++++++++++++++++++++++-----
 5 files changed, 40 insertions(+), 63 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCHv2 1/5] 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.

Then remove the existing implementation from compute_mcast_idx() and call the
new function in its place.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/net/net.h |  3 ++-
 net/net.c         | 16 +++++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

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..02a567c18f 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, 6) >> 26;
 }
 
 QemuOptsList qemu_netdev_opts = {
-- 
2.11.0

Re: [Qemu-devel] [PATCHv2 1/5] net: move CRC32 calculation from compute_mcast_idx() into its own net_crc32() function

[Philippe Mathieu-Daudé]

On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
> 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.
> 
> Then remove the existing implementation from compute_mcast_idx() and call the
> new function in its place.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

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

> ---
>  include/net/net.h |  3 ++-
>  net/net.c         | 16 +++++++++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> 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..02a567c18f 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, 6) >> 26;
>  }
>  
>  QemuOptsList qemu_netdev_opts = {
> 

[Qemu-devel] [PATCHv2 2/5] 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>
---
 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 02a567c18f..38008fe8e7 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 = crc ^ POLYNOMIAL_LE;
+            }
+        }
+    }
+
+    return crc;
+}
+
 unsigned compute_mcast_idx(const uint8_t *ep)
 {
     return net_crc32(ep, 6) >> 26;
-- 
2.11.0

Re: [Qemu-devel] [PATCHv2 2/5] net: introduce net_crc32_le() function

[Eric Blake]

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

On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
> This provides a standard ethernet CRC32 little-endian implementation.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  include/net/net.h |  2 ++
>  net/net.c         | 22 ++++++++++++++++++++++
>  2 files changed, 24 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> +            if (carry) {
> +                crc = crc ^ POLYNOMIAL_LE;
> +            }

Worth writing as 'crc ^= POLYNOMIAL_LE;'?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCHv2 2/5] net: introduce net_crc32_le() function

[Mark Cave-Ayland]

On 05/12/17 14:31, Eric Blake wrote:

> On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
>> This provides a standard ethernet CRC32 little-endian implementation.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   include/net/net.h |  2 ++
>>   net/net.c         | 22 ++++++++++++++++++++++
>>   2 files changed, 24 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>> +            if (carry) {
>> +                crc = crc ^ POLYNOMIAL_LE;
>> +            }
> 
> Worth writing as 'crc ^= POLYNOMIAL_LE;'?

Yes, works for me.


ATB,

Mark.

Re: [Qemu-devel] [PATCHv2 2/5] net: introduce net_crc32_le() function

[Philippe Mathieu-Daudé]

On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
> This provides a standard ethernet CRC32 little-endian implementation.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

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 02a567c18f..38008fe8e7 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 = crc ^ POLYNOMIAL_LE;
> +            }
> +        }
> +    }
> +
> +    return crc;
> +}
> +
>  unsigned compute_mcast_idx(const uint8_t *ep)
>  {
>      return net_crc32(ep, 6) >> 26;
> 

[Qemu-devel] [PATCHv2 3/5] pcnet: switch lnc_mchash() over to use net_crc32_le()

[Mark Cave-Ayland]

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

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 654455355f..c050993aa9 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -522,23 +522,9 @@ 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
+    return net_crc32_le(ether_addr, 6);
 }
 
 #define CRC(crc, ch)	 (crc = (crc >> 8) ^ crctab[(crc ^ (ch)) & 0xff])
-- 
2.11.0

Re: [Qemu-devel] [PATCHv2 3/5] pcnet: switch lnc_mchash() over to use net_crc32_le()

[Eric Blake]

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

On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/pcnet.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCHv2 3/5] pcnet: switch lnc_mchash() over to use net_crc32_le()

[Philippe Mathieu-Daudé]

On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

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

> ---
>  hw/net/pcnet.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index 654455355f..c050993aa9 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -522,23 +522,9 @@ 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
> +    return net_crc32_le(ether_addr, 6);
>  }
>  
>  #define CRC(crc, ch)	 (crc = (crc >> 8) ^ crctab[(crc ^ (ch)) & 0xff])
> 

[Qemu-devel] [PATCHv2 4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

[Mark Cave-Ayland]

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

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 1c0def555b..4fe94b7471 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -327,26 +327,9 @@ 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) | carry);
-            }
-        }
-    }
-    return (crc & BITS(7, 2)) >> 2;
+    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
 }
 
 /* Read a 16 bit control/status (CSR) register. */
-- 
2.11.0

Re: [Qemu-devel] [PATCHv2 4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

[Eric Blake]

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

On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/eepro100.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 

> -            if (carry) {
> -                crc = ((crc ^ POLYNOMIAL) | carry);

How does this compile after 1/5 renames POLYNOMIAL to POLYNOMIAL_BE in
net.h?

/me looks

Oh, you have a redundant definition in the .c file, which is now a dead
define.  Patch 1 should be updated to remove the duplicate definitions,
and fix code to uniformly use POLYNOMIAL_BE.

But overall, I like what the series is doing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCHv2 4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

[Mark Cave-Ayland]

On 05/12/17 14:28, Eric Blake wrote:

> On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/net/eepro100.c | 19 +------------------
>>   1 file changed, 1 insertion(+), 18 deletions(-)
>>
> 
>> -            if (carry) {
>> -                crc = ((crc ^ POLYNOMIAL) | carry);
> 
> How does this compile after 1/5 renames POLYNOMIAL to POLYNOMIAL_BE in
> net.h?
> 
> /me looks
> 
> Oh, you have a redundant definition in the .c file, which is now a dead
> define.  Patch 1 should be updated to remove the duplicate definitions,
> and fix code to uniformly use POLYNOMIAL_BE.

Ah yes, I can fix that up on a v3.

> But overall, I like what the series is doing.

Great stuff, in that case I'll fix it up based upon all the comments and 
continue. It has been lying around in a local branch for months now...

BTW one thing I did notice is that sungem.c calls zlib's crc32 function 
directly which doesn't seem right, so I'll probably add that into the 
next version too. Once this has been done, switching the new 
net_crc32()/net_crc32_le() functions over to use a LUT or zlib or 
something else as the underlying implementation should be trivial.


ATB,

Mark.

Re: [Qemu-devel] [PATCHv2 4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

[Stefan Weil]

Am 05.12.2017 um 09:17 schrieb Mark Cave-Ayland:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/eepro100.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 1c0def555b..4fe94b7471 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -327,26 +327,9 @@ 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) | carry);
> -            }
> -        }
> -    }
> -    return (crc & BITS(7, 2)) >> 2;
> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>  }
>  
>  /* Read a 16 bit control/status (CSR) register. */


What about eliminating the intermediate function e100_compute_mcast_idx (and function lnc_mchash, too)?
You did that for lnc_mchash, and I think that is cleaner and saves some lines of code.

With or without that minor change:

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Regards
Stefan

Re: [Qemu-devel] [PATCHv2 4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

[Stefan Weil]

Am 05.12.2017 um 16:13 schrieb Stefan Weil:
> Am 05.12.2017 um 09:17 schrieb Mark Cave-Ayland:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/net/eepro100.c | 19 +------------------
>>  1 file changed, 1 insertion(+), 18 deletions(-)
>>
>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>> index 1c0def555b..4fe94b7471 100644
>> --- a/hw/net/eepro100.c
>> +++ b/hw/net/eepro100.c
>> @@ -327,26 +327,9 @@ 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) | carry);
>> -            }
>> -        }
>> -    }
>> -    return (crc & BITS(7, 2)) >> 2;
>> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>>  }
>>  
>>  /* Read a 16 bit control/status (CSR) register. */
> 
> 
> What about eliminating the intermediate function e100_compute_mcast_idx (and function lnc_mchash, too)?
> You did that for lnc_mchash, and I think that is cleaner and saves some lines of code.

This should be "You did that for sunhme_crc32_le" ...

Stefan

Re: [Qemu-devel] [PATCHv2 4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

[Mark Cave-Ayland]

On 05/12/17 15:13, Stefan Weil wrote:

> Am 05.12.2017 um 09:17 schrieb Mark Cave-Ayland:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/net/eepro100.c | 19 +------------------
>>   1 file changed, 1 insertion(+), 18 deletions(-)
>>
>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>> index 1c0def555b..4fe94b7471 100644
>> --- a/hw/net/eepro100.c
>> +++ b/hw/net/eepro100.c
>> @@ -327,26 +327,9 @@ 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) | carry);
>> -            }
>> -        }
>> -    }
>> -    return (crc & BITS(7, 2)) >> 2;
>> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>>   }
>>   
>>   /* Read a 16 bit control/status (CSR) register. */
> 
> 
> What about eliminating the intermediate function e100_compute_mcast_idx (and function lnc_mchash, too)?
> You did that for lnc_mchash, and I think that is cleaner and saves some lines of code.

Yes, I can do if you like. The reason I've left these as they are for 
the moment is that I don't have something readily available to test 
multicast for eepro100 post-conversion (my SPARC/PPC images cover 
pcnet/sunhme) but if you are happy the functionality is the same during 
review then I can go ahead and do it.

I don't really mind exactly how we do the conversion as long as we aim 
for consistency.

> With or without that minor change:
> 
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> 
> Regards
> Stefan

ATB,

Mark.

Re: [Qemu-devel] [PATCHv2 4/5] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()

[Philippe Mathieu-Daudé]

On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

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

> ---
>  hw/net/eepro100.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 1c0def555b..4fe94b7471 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -327,26 +327,9 @@ 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) | carry);
> -            }
> -        }
> -    }
> -    return (crc & BITS(7, 2)) >> 2;
> +    return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
>  }
>  
>  /* Read a 16 bit control/status (CSR) register. */
> 

[Qemu-devel] [PATCHv2 5/5] sunhme: switch sunhme_receive() over to use net_crc32_le()

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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..df66e2630c 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, 6) >> 26;
             if (!(s->macregs[(HME_MACI_HASHTAB0 >> 2) - (mcast_idx >> 4)] &
                     (1 << (mcast_idx & 0xf)))) {
                 /* Didn't match hash filter */
-- 
2.11.0

Re: [Qemu-devel] [PATCHv2 5/5] sunhme: switch sunhme_receive() over to use net_crc32_le()

[Eric Blake]

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

On 12/05/2017 02:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/net/sunhme.c | 25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCHv2 5/5] sunhme: switch sunhme_receive() over to use net_crc32_le()

[Philippe Mathieu-Daudé]

Hi Mark,

On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  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..df66e2630c 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;

This could be:

               int mcast_idx = compute_mcast_idx_le(buf);

With:

unsigned compute_mcast_idx_le(const uint8_t *ep)
{
    return net_crc32(ep, ETH_ALEN) >> 26;
}

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

> +            int mcast_idx = net_crc32_le(buf, 6) >> 26;
>              if (!(s->macregs[(HME_MACI_HASHTAB0 >> 2) - (mcast_idx >> 4)] &
>                      (1 << (mcast_idx & 0xf)))) {
>                  /* Didn't match hash filter */
> 

Re: [Qemu-devel] [PATCHv2 5/5] sunhme: switch sunhme_receive() over to use net_crc32_le()

[Philippe Mathieu-Daudé]

On Wed, Dec 6, 2017 at 12:34 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Mark,
>
> On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  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..df66e2630c 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;
>
> This could be:
>
>                int mcast_idx = compute_mcast_idx_le(buf);
>
> With:
>
> unsigned compute_mcast_idx_le(const uint8_t *ep)
> {
>     return net_crc32(ep, ETH_ALEN) >> 26;

Oops not ETH_ALEN but MAC_ALEN ;) from sizeof(MACAddr)

> }
>
> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> +            int mcast_idx = net_crc32_le(buf, 6) >> 26;
>>              if (!(s->macregs[(HME_MACI_HASHTAB0 >> 2) - (mcast_idx >> 4)] &
>>                      (1 << (mcast_idx & 0xf)))) {
>>                  /* Didn't match hash filter */
>>

Re: [Qemu-devel] [PATCHv2 5/5] sunhme: switch sunhme_receive() over to use net_crc32_le()

[Mark Cave-Ayland]

On 06/12/17 03:34, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 12/05/2017 05:17 AM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   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..df66e2630c 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;
> 
> This could be:
> 
>                 int mcast_idx = compute_mcast_idx_le(buf);
> 
> With:
> 
> unsigned compute_mcast_idx_le(const uint8_t *ep)
> {
>      return net_crc32(ep, ETH_ALEN) >> 26;
> }

It looks like Stefan is suggesting in his comments that the inline 
approach is better, so I'll try to keep that style for the moment.

I do like the use of ETH_ALEN though so I'll add that in to the next 
version.

> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>> +            int mcast_idx = net_crc32_le(buf, 6) >> 26;
>>               if (!(s->macregs[(HME_MACI_HASHTAB0 >> 2) - (mcast_idx >> 4)] &
>>                       (1 << (mcast_idx & 0xf)))) {
>>                   /* Didn't match hash filter */
>>

ATB,

Mark.