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 <address@hidden>
>
> 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.