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

[Qemu-devel] [PATCH 0/4] 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 two patches
switch the pcnet and eepro100 drivers over to use them, allowing us to remove
their private implementations.

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

Mark Cave-Ayland (4):
  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()

 hw/net/eepro100.c |   19 +------------------
 hw/net/pcnet.c    |   16 +---------------
 include/net/net.h |    5 ++++-
 net/net.c         |   38 +++++++++++++++++++++++++++++++++-----
 4 files changed, 39 insertions(+), 39 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4] 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 1c55a93..586098c 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 0e28099..a856907 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1568,25 +1568,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 = {
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/4] 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 586098c..4afac1a 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 a856907..c99e705 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1590,6 +1590,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;
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/4] 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 6544553..c050993 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])
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/4] 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 5a4774a..4226572 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. */
-- 
1.7.10.4

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

[Philippe Mathieu-Daudé]

On 08/18/2017 11:15 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>
> ---
>   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 1c55a93..586098c 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 0e28099..a856907 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1568,25 +1568,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)

imho there is no point in using signed length.

>   {
>       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;

while here let's use ETH_ALEN

>   }
>   
>   QemuOptsList qemu_netdev_opts = {
> 
with "uint32_t net_crc32(const uint8_t *p, size_t len);":
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH 2/4] net: introduce net_crc32_le() function

[Philippe Mathieu-Daudé]

On 08/18/2017 11:15 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(+)
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index 586098c..4afac1a 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);

same here, with size_t len:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>   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 a856907..c99e705 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1590,6 +1590,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;
> 

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

[Philippe Mathieu-Daudé]

On 08/18/2017 11:15 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(-)
> 
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index 6544553..c050993 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);

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

>   }
>   
>   #define CRC(crc, ch)	 (crc = (crc >> 8) ^ crctab[(crc ^ (ch)) & 0xff])

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

[Philippe Mathieu-Daudé]

On 08/18/2017 01:51 PM, Philippe Mathieu-Daudé wrote:
> On 08/18/2017 11:15 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>
>> ---
>>   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 1c55a93..586098c 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 0e28099..a856907 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -1568,25 +1568,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)
> 
> imho there is no point in using signed length.
> 
>>   {
>>       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;
> 
> while here let's use ETH_ALEN
>

reading the next patches, what do you think about:

static inline uint32_t mac_crc32_be(const uint8_t *p)
{
     return net_crc32_be(ep, ETH_ALEN);
}

unsigned compute_mcast_idx(const uint8_t *ep)
{
     return mac_crc32_be(p) >> 26;
}

>>   }
>>   QemuOptsList qemu_netdev_opts = {
>>
> with "uint32_t net_crc32(const uint8_t *p, size_t len);":
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

[Philippe Mathieu-Daudé]

On 08/18/2017 01:59 PM, Philippe Mathieu-Daudé wrote:
> On 08/18/2017 11:15 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(-)
>>
>> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
>> index 6544553..c050993 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);

with previous suggestion from patch 1/4:

        return mac_crc32_le(ether_addr);

> 
> using ETH_ALEN:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>>   }
>>   #define CRC(crc, ch)     (crc = (crc >> 8) ^ crctab[(crc ^ (ch)) & 
>> 0xff])
> 

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

[Philippe Mathieu-Daudé]

On 08/18/2017 11:15 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(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 5a4774a..4226572 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;

with previous suggestion from patch 1/4:

        return (mac_crc32_be(ep) & BITS(7, 2)) >> 2;

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

>   }
>   
>   /* Read a 16 bit control/status (CSR) register. */
> 

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

[Mark Cave-Ayland]

On 18/08/17 18:06, Philippe Mathieu-Daudé wrote:

> On 08/18/2017 01:51 PM, Philippe Mathieu-Daudé wrote:
>> On 08/18/2017 11:15 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>
>>> ---
>>>   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 1c55a93..586098c 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 0e28099..a856907 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -1568,25 +1568,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)
>>
>> imho there is no point in using signed length.
>>
>>>   {
>>>       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;
>>
>> while here let's use ETH_ALEN
>>
> 
> reading the next patches, what do you think about:
> 
> static inline uint32_t mac_crc32_be(const uint8_t *p)
> {
>     return net_crc32_be(ep, ETH_ALEN);
> }
> 
> unsigned compute_mcast_idx(const uint8_t *ep)
> {
>     return mac_crc32_be(p) >> 26;
> }

Yes, using ETH_ALEN looks like a good idea - I can do that for the next
revision.

As for the naming of the functions, I've followed the kernel convention
that by default the CRC32 functions are big-endian unless they have a
_le suffix. I don't really feel strongly either way, so I'm happy to go
with whatever naming scheme Jason prefers.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/4] net: introduce common net_crc32() and net_crc32_le() functions

[Mark Cave-Ayland]

On 18/08/17 15:15, 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.
> 
> The first two patches introduce the relevant functions while the last two patches
> switch the pcnet and eepro100 drivers over to use them, allowing us to remove
> their private implementations.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Mark Cave-Ayland (4):
>   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()
> 
>  hw/net/eepro100.c |   19 +------------------
>  hw/net/pcnet.c    |   16 +---------------
>  include/net/net.h |    5 ++++-
>  net/net.c         |   38 +++++++++++++++++++++++++++++++++-----
>  4 files changed, 39 insertions(+), 39 deletions(-)

Hi Jason,

Any thoughts on this patchset? I now have another virtual NIC
implementation for upstream that makes use of the new net_crc32_le()
function created by this patchset and I would really like to use this
instead of copy/pasting yet another different CRC32 implementation :)


ATB,

Mark.