[Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Aurelien Jarno]

The lan9118 emulation tries to compute the multicast index by calling
directly the crc32() function from zlib, but fails to get the correct
result.

Use the common compute_mcast_idx() function instead, which gives the
correct result. This fixes IPv6 support.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/lan9118.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/lan9118.c b/hw/lan9118.c
index ff0a50b..ceaf96f 100644
--- a/hw/lan9118.c
+++ b/hw/lan9118.c
@@ -500,7 +500,7 @@ static int lan9118_filter(lan9118_state *s, const uint8_t *addr)
         }
     } else {
         /* Hash matching  */
-        hash = (crc32(~0, addr, 6) >> 26);
+        hash = compute_mcast_idx(addr);
         if (hash & 0x20) {
             return (s->mac_hashh >> (hash & 0x1f)) & 1;
         } else {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Stefan Hajnoczi]

On Thu, Aug 23, 2012 at 4:39 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> The lan9118 emulation tries to compute the multicast index by calling
> directly the crc32() function from zlib, but fails to get the correct
> result.
>
> Use the common compute_mcast_idx() function instead, which gives the
> correct result. This fixes IPv6 support.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/lan9118.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

There is another crc32() call in hw/lan9118.c:lan9118_receive().  Can
that be replaced too and then #include <zlib.h> can be dropped?

Stefan

Re: [Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Aurelien Jarno]

On Fri, Aug 24, 2012 at 10:47:47AM +0100, Stefan Hajnoczi wrote:
> On Thu, Aug 23, 2012 at 4:39 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > The lan9118 emulation tries to compute the multicast index by calling
> > directly the crc32() function from zlib, but fails to get the correct
> > result.
> >
> > Use the common compute_mcast_idx() function instead, which gives the
> > correct result. This fixes IPv6 support.
> >
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  hw/lan9118.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> There is another crc32() call in hw/lan9118.c:lan9118_receive().  Can
> that be replaced too and then #include <zlib.h> can be dropped?
> 

I don't think so, at least not easily. This is a different call (the
length is variable), and most emulated NICs have a call to crc32(), but
in slightly different ways.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Stefan Hajnoczi]

On Fri, Aug 24, 2012 at 11:08 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Fri, Aug 24, 2012 at 10:47:47AM +0100, Stefan Hajnoczi wrote:
>> On Thu, Aug 23, 2012 at 4:39 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > The lan9118 emulation tries to compute the multicast index by calling
>> > directly the crc32() function from zlib, but fails to get the correct
>> > result.
>> >
>> > Use the common compute_mcast_idx() function instead, which gives the
>> > correct result. This fixes IPv6 support.
>> >
>> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> > ---
>> >  hw/lan9118.c |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> There is another crc32() call in hw/lan9118.c:lan9118_receive().  Can
>> that be replaced too and then #include <zlib.h> can be dropped?
>>
>
> I don't think so, at least not easily. This is a different call (the
> length is variable), and most emulated NICs have a call to crc32(), but
> in slightly different ways.

Okay.  I haven't looked at the datasheet for this NIC, so I have no
more input to this patch except that it looks fine.

Stefan

Re: [Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Aurelien Jarno]

On Thu, Aug 23, 2012 at 05:39:39PM +0200, Aurelien Jarno wrote:
> The lan9118 emulation tries to compute the multicast index by calling
> directly the crc32() function from zlib, but fails to get the correct
> result.
> 
> Use the common compute_mcast_idx() function instead, which gives the
> correct result. This fixes IPv6 support.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/lan9118.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/lan9118.c b/hw/lan9118.c
> index ff0a50b..ceaf96f 100644
> --- a/hw/lan9118.c
> +++ b/hw/lan9118.c
> @@ -500,7 +500,7 @@ static int lan9118_filter(lan9118_state *s, const uint8_t *addr)
>          }
>      } else {
>          /* Hash matching  */
> -        hash = (crc32(~0, addr, 6) >> 26);
> +        hash = compute_mcast_idx(addr);
>          if (hash & 0x20) {
>              return (s->mac_hashh >> (hash & 0x1f)) & 1;
>          } else {

Ping?

For the record the Linux kernel uses the ether_crc() function for
smsc911x.c, but also for 8139cp.c, 8139too.c and ethoc.c, which use
compute_mcast_idx() on the QEMU side.

To test it, just run this machine with a Linux kernel with IPv6 support
on an IPv6-enabled network with router advertisement, it should get an
IPv6 address automatically. It doesn't without this patch.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Peter Maydell]

On 7 September 2012 15:56, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Thu, Aug 23, 2012 at 05:39:39PM +0200, Aurelien Jarno wrote:
>> The lan9118 emulation tries to compute the multicast index by calling
>> directly the crc32() function from zlib, but fails to get the correct
>> result.
>>
>> Use the common compute_mcast_idx() function instead, which gives the
>> correct result. This fixes IPv6 support.
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>>  hw/lan9118.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/lan9118.c b/hw/lan9118.c
>> index ff0a50b..ceaf96f 100644
>> --- a/hw/lan9118.c
>> +++ b/hw/lan9118.c
>> @@ -500,7 +500,7 @@ static int lan9118_filter(lan9118_state *s, const uint8_t *addr)
>>          }
>>      } else {
>>          /* Hash matching  */
>> -        hash = (crc32(~0, addr, 6) >> 26);
>> +        hash = compute_mcast_idx(addr);
>>          if (hash & 0x20) {
>>              return (s->mac_hashh >> (hash & 0x1f)) & 1;
>>          } else {
>
> Ping?
>
> For the record the Linux kernel uses the ether_crc() function for
> smsc911x.c, but also for 8139cp.c, 8139too.c and ethoc.c, which use
> compute_mcast_idx() on the QEMU side.

Looks ok to me. I did check the data sheet, which helpfully doesn't
say exactly what the CRC function is, and also the zlib docs (which
suggest we should use something that isn't what we were doing here).
So I guess

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Happy for you to commit directly or I can put it in arm-devs.next
if you prefer.

-- PMM

Re: [Qemu-devel] [PATCH ] lan9118: fix multicast filtering

[Aurelien Jarno]

On Fri, Sep 07, 2012 at 04:04:16PM +0100, Peter Maydell wrote:
> On 7 September 2012 15:56, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Thu, Aug 23, 2012 at 05:39:39PM +0200, Aurelien Jarno wrote:
> >> The lan9118 emulation tries to compute the multicast index by calling
> >> directly the crc32() function from zlib, but fails to get the correct
> >> result.
> >>
> >> Use the common compute_mcast_idx() function instead, which gives the
> >> correct result. This fixes IPv6 support.
> >>
> >> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >> ---
> >>  hw/lan9118.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/lan9118.c b/hw/lan9118.c
> >> index ff0a50b..ceaf96f 100644
> >> --- a/hw/lan9118.c
> >> +++ b/hw/lan9118.c
> >> @@ -500,7 +500,7 @@ static int lan9118_filter(lan9118_state *s, const uint8_t *addr)
> >>          }
> >>      } else {
> >>          /* Hash matching  */
> >> -        hash = (crc32(~0, addr, 6) >> 26);
> >> +        hash = compute_mcast_idx(addr);
> >>          if (hash & 0x20) {
> >>              return (s->mac_hashh >> (hash & 0x1f)) & 1;
> >>          } else {
> >
> > Ping?
> >
> > For the record the Linux kernel uses the ether_crc() function for
> > smsc911x.c, but also for 8139cp.c, 8139too.c and ethoc.c, which use
> > compute_mcast_idx() on the QEMU side.
> 
> Looks ok to me. I did check the data sheet, which helpfully doesn't
> say exactly what the CRC function is, and also the zlib docs (which
> suggest we should use something that isn't what we were doing here).
> So I guess
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Happy for you to commit directly or I can put it in arm-devs.next
> if you prefer.
> 

Thanks for the review, I have applied it.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net