| Message ID | 1345736379-16101-1-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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
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.
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 {
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(-)