[Qemu-devel] eepro100 multicast

[Qemu-devel] eepro100 multicast

[Dunc]

Hi all,

I've been trying to set up some olives running inside qemu to practice
for Juniper exam. I came across the multicast issue when enabling VRRP

After lots of googling and reading forums, where people have being doing
hacky patches, I found a page that suggests this has been fixed now,
just not in a formal release

(here for reference)
http://www.internetworkpro.org/wiki/Using_QEMU_with_Olive_to_emulate_Juniper_Routers#Getting_and_Installing_QEMU

I had a look through gitweb and can see there has indeed been a commit
to fix multicast in eepro100 and so I've built a qemu based on latest
git (as of yesterday). Unfortunately it doesn't seem to fix things for
me, however I do have a different error now....

eepro100: feature is missing in this emulation: Multiple IA bit


The setup of this link is quite simple, I have 2 qemus running using tap
mode for network, and I have bridged them together on my host (Linux)
When I configure the network I can successfully ping across the link
with no problems, but as soon as I enable VRRP on either end, I start to
get the above error at regular intervals (I assume it's each time VRRP
sends a multicast hello), and also my link stops pinging altogether.
When I remove VRRP config, the link pings again.

Does anyone know what's going on, or if I'm doing anything wrong?

Thanks in advance,

Dunc

Re: [Qemu-devel] eepro100 multicast

[Stefan Weil]

Am 25.09.2010 21:11, schrieb Dunc:
> Hi all,
>
> I've been trying to set up some olives running inside qemu to practice
> for Juniper exam. I came across the multicast issue when enabling VRRP
>
> After lots of googling and reading forums, where people have being doing
> hacky patches, I found a page that suggests this has been fixed now,
> just not in a formal release
>
> (here for reference)
> http://www.internetworkpro.org/wiki/Using_QEMU_with_Olive_to_emulate_Juniper_Routers#Getting_and_Installing_QEMU
>
> I had a look through gitweb and can see there has indeed been a commit
> to fix multicast in eepro100 and so I've built a qemu based on latest
> git (as of yesterday). Unfortunately it doesn't seem to fix things for
> me, however I do have a different error now....
>
> eepro100: feature is missing in this emulation: Multiple IA bit
>
>
> The setup of this link is quite simple, I have 2 qemus running using tap
> mode for network, and I have bridged them together on my host (Linux)
> When I configure the network I can successfully ping across the link
> with no problems, but as soon as I enable VRRP on either end, I start to
> get the above error at regular intervals (I assume it's each time VRRP
> sends a multicast hello), and also my link stops pinging altogether.
> When I remove VRRP config, the link pings again.
>
> Does anyone know what's going on, or if I'm doing anything wrong?
>
> Thanks in advance,
>
> Dunc


The OS which was used here is based on FreeBSD.

I reviewed the latest sources of Linux, FreeBSD and NetBSD.
They all reset the multiple IA bit (multi_ia in BSD) to zero,
but I did not find code which sets this bit to one
(like it is done by Juniper routers).

Running Windows guests also does not set this bit.

Intel's Open Source Software Developer Manual does not
give much information on the semantics related to this bit,
so I had to guess how it works.

My patch could help Dunc, but if anybody has more information
or driver code which makes use of this bit, that would be
really helpful.

Regards
Stefan

[Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Stefan Weil]

I reviewed the latest sources of Linux, FreeBSD and NetBSD.
They all reset the multiple IA bit (multi_ia in BSD) to zero,
but I did not find code which sets this bit to one
(like it is done by some routers).

Running Windows guests also did not set this bit.

Intel's Open Source Software Developer Manual does not
give much information on the semantics related to this bit,
so I had to guess how it works. The guess was good enough
to make the router emulation work.

Related changes in this patch:
* Update naming and documentation of the internal hash register.
  It is not limited to multicast, but also used for multiple IA.
* Dump complete configuration register when debug traces are enabled.
* Debug output when multiple IA bit is set during CmdConfigure.
* Debug output when frames are received because multiple IA bit is set,
  or when they are ignored although it is set.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 2b75c8f..5f6dcb6 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -219,7 +219,8 @@ typedef enum {
 
 typedef struct {
     PCIDevice dev;
-    uint8_t mult[8];            /* multicast mask array */
+    /* Hash register (multicast mask array, multiple individual addresses). */
+    uint8_t mult[8];
     int mmio_index;
     NICState *nic;
     NICConf conf;
@@ -599,7 +600,7 @@ static void nic_reset(void *opaque)
 {
     EEPRO100State *s = opaque;
     TRACE(OTHER, logout("%p\n", s));
-    /* TODO: Clearing of multicast table for selective reset, too? */
+    /* TODO: Clearing of hash register for selective reset, too? */
     memset(&s->mult[0], 0, sizeof(s->mult));
     nic_selective_reset(s);
 }
@@ -851,7 +852,14 @@ static void action_command(EEPRO100State *s)
         case CmdConfigure:
             cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
                                      sizeof(s->configuration));
-            TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16)));
+            TRACE(OTHER, logout("configuration: %s\n",
+                                nic_dump(&s->configuration[0], 16)));
+            TRACE(OTHER, logout("configuration: %s\n",
+                                nic_dump(&s->configuration[16],
+                                ARRAY_SIZE(s->configuration) - 16)));
+            if (s->configuration[20] & BIT(6)) {
+                TRACE(OTHER, logout("Multiple IA bit\n"));
+            }
             break;
         case CmdMulticastList:
             set_multicast_list(s);
@@ -1647,12 +1655,6 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
     static const uint8_t broadcast_macaddr[6] =
         { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 
-    /* TODO: check multiple IA bit. */
-    if (s->configuration[20] & BIT(6)) {
-        missing("Multiple IA bit");
-        return -1;
-    }
-
     if (s->configuration[8] & 0x80) {
         /* CSMA is disabled. */
         logout("%p received while CSMA is disabled\n", s);
@@ -1702,6 +1704,16 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
         /* Promiscuous: receive all. */
         TRACE(RXTX, logout("%p received frame in promiscuous mode, len=%zu\n", s, size));
         rfd_status |= 0x0004;
+    } else if (s->configuration[20] & BIT(6)) {
+        /* Multiple IA bit set. */
+        unsigned mcast_idx = compute_mcast_idx(buf);
+        assert(mcast_idx < 64);
+        if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
+            TRACE(RXTX, logout("%p accepted, multiple IA bit set\n", s));
+        } else {
+            TRACE(RXTX, logout("%p frame ignored, multiple IA bit set\n", s));
+            return -1;
+        }
     } else {
         TRACE(RXTX, logout("%p received frame, ignored, len=%zu,%s\n", s, size,
               nic_dump(buf, size)));
-- 
1.7.1

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Edgar E. Iglesias]

On Wed, Sep 29, 2010 at 09:59:55PM +0200, Stefan Weil wrote:
> I reviewed the latest sources of Linux, FreeBSD and NetBSD.
> They all reset the multiple IA bit (multi_ia in BSD) to zero,
> but I did not find code which sets this bit to one
> (like it is done by some routers).
> 
> Running Windows guests also did not set this bit.
> 
> Intel's Open Source Software Developer Manual does not
> give much information on the semantics related to this bit,
> so I had to guess how it works. The guess was good enough
> to make the router emulation work.
>
> 
> Related changes in this patch:
> * Update naming and documentation of the internal hash register.
>   It is not limited to multicast, but also used for multiple IA.
> * Dump complete configuration register when debug traces are enabled.
> * Debug output when multiple IA bit is set during CmdConfigure.
> * Debug output when frames are received because multiple IA bit is set,
>   or when they are ignored although it is set.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/eepro100.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 2b75c8f..5f6dcb6 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -219,7 +219,8 @@ typedef enum {
>  
>  typedef struct {
>      PCIDevice dev;
> -    uint8_t mult[8];            /* multicast mask array */
> +    /* Hash register (multicast mask array, multiple individual addresses). */
> +    uint8_t mult[8];


Nitpick:
It seems to me like if mult is only used internally. If so, why not
represent the hash filter with an uint64_t?

Then you can replace the current memsets with ->mult = 0 and simplify
the match logic.


>      int mmio_index;
>      NICState *nic;
>      NICConf conf;
> @@ -599,7 +600,7 @@ static void nic_reset(void *opaque)
>  {
>      EEPRO100State *s = opaque;
>      TRACE(OTHER, logout("%p\n", s));
> -    /* TODO: Clearing of multicast table for selective reset, too? */
> +    /* TODO: Clearing of hash register for selective reset, too? */
>      memset(&s->mult[0], 0, sizeof(s->mult));

e.g:

s->mult = 0;

>      nic_selective_reset(s);
>  }
> @@ -851,7 +852,14 @@ static void action_command(EEPRO100State *s)
>          case CmdConfigure:
>              cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
>                                       sizeof(s->configuration));
> -            TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16)));
> +            TRACE(OTHER, logout("configuration: %s\n",
> +                                nic_dump(&s->configuration[0], 16)));
> +            TRACE(OTHER, logout("configuration: %s\n",
> +                                nic_dump(&s->configuration[16],
> +                                ARRAY_SIZE(s->configuration) - 16)));
> +            if (s->configuration[20] & BIT(6)) {
> +                TRACE(OTHER, logout("Multiple IA bit\n"));
> +            }
>              break;
>          case CmdMulticastList:
>              set_multicast_list(s);
> @@ -1647,12 +1655,6 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
>      static const uint8_t broadcast_macaddr[6] =
>          { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>  
> -    /* TODO: check multiple IA bit. */
> -    if (s->configuration[20] & BIT(6)) {
> -        missing("Multiple IA bit");
> -        return -1;
> -    }
> -
>      if (s->configuration[8] & 0x80) {
>          /* CSMA is disabled. */
>          logout("%p received while CSMA is disabled\n", s);
> @@ -1702,6 +1704,16 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
>          /* Promiscuous: receive all. */
>          TRACE(RXTX, logout("%p received frame in promiscuous mode, len=%zu\n", s, size));
>          rfd_status |= 0x0004;
> +    } else if (s->configuration[20] & BIT(6)) {
> +        /* Multiple IA bit set. */
> +        unsigned mcast_idx = compute_mcast_idx(buf);
> +        assert(mcast_idx < 64);
> +        if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {

e.g:
           if (s->mult & (1 << (mcast_idx & 63))) {

Cheers

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Stefan Weil]

Am 29.09.2010 22:30, schrieb Edgar E. Iglesias:
> On Wed, Sep 29, 2010 at 09:59:55PM +0200, Stefan Weil wrote:
>    
>> I reviewed the latest sources of Linux, FreeBSD and NetBSD.
>> They all reset the multiple IA bit (multi_ia in BSD) to zero,
>> but I did not find code which sets this bit to one
>> (like it is done by some routers).
>>
>> Running Windows guests also did not set this bit.
>>
>> Intel's Open Source Software Developer Manual does not
>> give much information on the semantics related to this bit,
>> so I had to guess how it works. The guess was good enough
>> to make the router emulation work.
>>
>>
>> Related changes in this patch:
>> * Update naming and documentation of the internal hash register.
>>    It is not limited to multicast, but also used for multiple IA.
>> * Dump complete configuration register when debug traces are enabled.
>> * Debug output when multiple IA bit is set during CmdConfigure.
>> * Debug output when frames are received because multiple IA bit is set,
>>    or when they are ignored although it is set.
>>
>> Cc: Michael S. Tsirkin<mst@redhat.com>
>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>> ---
>>   hw/eepro100.c |   30 +++++++++++++++++++++---------
>>   1 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 2b75c8f..5f6dcb6 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -219,7 +219,8 @@ typedef enum {
>>
>>   typedef struct {
>>       PCIDevice dev;
>> -    uint8_t mult[8];            /* multicast mask array */
>> +    /* Hash register (multicast mask array, multiple individual addresses). */
>> +    uint8_t mult[8];
>>      
>
> Nitpick:
> It seems to me like if mult is only used internally. If so, why not
> represent the hash filter with an uint64_t?
>
> Then you can replace the current memsets with ->mult = 0 and simplify
> the match logic.
>
>
>    
[snip]

That's correct. Is it possible to change mult in the VMStateDescription
from VMSTATE_BUFFER to VMSTATE_UINT64 without loosing
compatibility?

I doubt that there will be a large gain in runtime speed by this change.

See also hw/ne2000.c and hw/rtl8139.c - they use similar code.
So any change should be applied to those nics, too.

Regards,
Stefan

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Edgar E. Iglesias]

On Thu, Sep 30, 2010 at 06:45:18PM +0200, Stefan Weil wrote:
> Am 29.09.2010 22:30, schrieb Edgar E. Iglesias:
> > On Wed, Sep 29, 2010 at 09:59:55PM +0200, Stefan Weil wrote:
> >    
> >> I reviewed the latest sources of Linux, FreeBSD and NetBSD.
> >> They all reset the multiple IA bit (multi_ia in BSD) to zero,
> >> but I did not find code which sets this bit to one
> >> (like it is done by some routers).
> >>
> >> Running Windows guests also did not set this bit.
> >>
> >> Intel's Open Source Software Developer Manual does not
> >> give much information on the semantics related to this bit,
> >> so I had to guess how it works. The guess was good enough
> >> to make the router emulation work.
> >>
> >>
> >> Related changes in this patch:
> >> * Update naming and documentation of the internal hash register.
> >>    It is not limited to multicast, but also used for multiple IA.
> >> * Dump complete configuration register when debug traces are enabled.
> >> * Debug output when multiple IA bit is set during CmdConfigure.
> >> * Debug output when frames are received because multiple IA bit is set,
> >>    or when they are ignored although it is set.
> >>
> >> Cc: Michael S. Tsirkin<mst@redhat.com>
> >> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
> >> ---
> >>   hw/eepro100.c |   30 +++++++++++++++++++++---------
> >>   1 files changed, 21 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/eepro100.c b/hw/eepro100.c
> >> index 2b75c8f..5f6dcb6 100644
> >> --- a/hw/eepro100.c
> >> +++ b/hw/eepro100.c
> >> @@ -219,7 +219,8 @@ typedef enum {
> >>
> >>   typedef struct {
> >>       PCIDevice dev;
> >> -    uint8_t mult[8];            /* multicast mask array */
> >> +    /* Hash register (multicast mask array, multiple individual addresses). */
> >> +    uint8_t mult[8];
> >>      
> >
> > Nitpick:
> > It seems to me like if mult is only used internally. If so, why not
> > represent the hash filter with an uint64_t?
> >
> > Then you can replace the current memsets with ->mult = 0 and simplify
> > the match logic.
> >
> >
> >    
> [snip]
> 
> That's correct. Is it possible to change mult in the VMStateDescription
> from VMSTATE_BUFFER to VMSTATE_UINT64 without loosing
> compatibility?

Ah, probably not. My guess is that you'd run into
endianess issues. The current code is always little-endian but the
savevm/loadvm code would expect big endian.

Better to save the impovement for another time when the vmstate
description needs to be updated for other reasons..

Cheers

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Michael S. Tsirkin]

On Wed, Sep 29, 2010 at 10:30:10PM +0200, Edgar E. Iglesias wrote:
> On Wed, Sep 29, 2010 at 09:59:55PM +0200, Stefan Weil wrote:
> > I reviewed the latest sources of Linux, FreeBSD and NetBSD.
> > They all reset the multiple IA bit (multi_ia in BSD) to zero,
> > but I did not find code which sets this bit to one
> > (like it is done by some routers).
> > 
> > Running Windows guests also did not set this bit.
> > 
> > Intel's Open Source Software Developer Manual does not
> > give much information on the semantics related to this bit,
> > so I had to guess how it works. The guess was good enough
> > to make the router emulation work.
> >
> > 
> > Related changes in this patch:
> > * Update naming and documentation of the internal hash register.
> >   It is not limited to multicast, but also used for multiple IA.
> > * Dump complete configuration register when debug traces are enabled.
> > * Debug output when multiple IA bit is set during CmdConfigure.
> > * Debug output when frames are received because multiple IA bit is set,
> >   or when they are ignored although it is set.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> > ---
> >  hw/eepro100.c |   30 +++++++++++++++++++++---------
> >  1 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/eepro100.c b/hw/eepro100.c
> > index 2b75c8f..5f6dcb6 100644
> > --- a/hw/eepro100.c
> > +++ b/hw/eepro100.c
> > @@ -219,7 +219,8 @@ typedef enum {
> >  
> >  typedef struct {
> >      PCIDevice dev;
> > -    uint8_t mult[8];            /* multicast mask array */
> > +    /* Hash register (multicast mask array, multiple individual addresses). */
> > +    uint8_t mult[8];
> 
> 
> Nitpick:
> It seems to me like if mult is only used internally. If so, why not
> represent the hash filter with an uint64_t?
> 
> Then you can replace the current memsets with ->mult = 0 and simplify
> the match logic.
> 
> 
> >      int mmio_index;
> >      NICState *nic;
> >      NICConf conf;
> > @@ -599,7 +600,7 @@ static void nic_reset(void *opaque)
> >  {
> >      EEPRO100State *s = opaque;
> >      TRACE(OTHER, logout("%p\n", s));
> > -    /* TODO: Clearing of multicast table for selective reset, too? */
> > +    /* TODO: Clearing of hash register for selective reset, too? */
> >      memset(&s->mult[0], 0, sizeof(s->mult));
> 
> e.g:
> 
> s->mult = 0;
> 
> >      nic_selective_reset(s);
> >  }
> > @@ -851,7 +852,14 @@ static void action_command(EEPRO100State *s)
> >          case CmdConfigure:
> >              cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
> >                                       sizeof(s->configuration));
> > -            TRACE(OTHER, logout("configuration: %s\n", nic_dump(&s->configuration[0], 16)));
> > +            TRACE(OTHER, logout("configuration: %s\n",
> > +                                nic_dump(&s->configuration[0], 16)));
> > +            TRACE(OTHER, logout("configuration: %s\n",
> > +                                nic_dump(&s->configuration[16],
> > +                                ARRAY_SIZE(s->configuration) - 16)));
> > +            if (s->configuration[20] & BIT(6)) {
> > +                TRACE(OTHER, logout("Multiple IA bit\n"));
> > +            }
> >              break;
> >          case CmdMulticastList:
> >              set_multicast_list(s);
> > @@ -1647,12 +1655,6 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
> >      static const uint8_t broadcast_macaddr[6] =
> >          { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> >  
> > -    /* TODO: check multiple IA bit. */
> > -    if (s->configuration[20] & BIT(6)) {
> > -        missing("Multiple IA bit");
> > -        return -1;
> > -    }
> > -
> >      if (s->configuration[8] & 0x80) {
> >          /* CSMA is disabled. */
> >          logout("%p received while CSMA is disabled\n", s);
> > @@ -1702,6 +1704,16 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
> >          /* Promiscuous: receive all. */
> >          TRACE(RXTX, logout("%p received frame in promiscuous mode, len=%zu\n", s, size));
> >          rfd_status |= 0x0004;
> > +    } else if (s->configuration[20] & BIT(6)) {
> > +        /* Multiple IA bit set. */
> > +        unsigned mcast_idx = compute_mcast_idx(buf);
> > +        assert(mcast_idx < 64);
> > +        if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
> 
> e.g:
>            if (s->mult & (1 << (mcast_idx & 63))) {
> 
> Cheers

This 1 must really be 1ULL, otherwise we get an int shifted by a value > 31,
which triggers undefined behaviour.

-- 
MST

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Stefan Weil]

Am 03.10.2010 11:56, schrieb Michael S. Tsirkin:
> On Wed, Sep 29, 2010 at 10:30:10PM +0200, Edgar E. Iglesias wrote:
>> On Wed, Sep 29, 2010 at 09:59:55PM +0200, Stefan Weil wrote:
>>> I reviewed the latest sources of Linux, FreeBSD and NetBSD.
>>> They all reset the multiple IA bit (multi_ia in BSD) to zero,
>>> but I did not find code which sets this bit to one
>>> (like it is done by some routers).
>>>
>>> Running Windows guests also did not set this bit.
>>>
>>> Intel's Open Source Software Developer Manual does not
>>> give much information on the semantics related to this bit,
>>> so I had to guess how it works. The guess was good enough
>>> to make the router emulation work.
>>>
>>>
>>> Related changes in this patch:
>>> * Update naming and documentation of the internal hash register.
>>> It is not limited to multicast, but also used for multiple IA.
>>> * Dump complete configuration register when debug traces are enabled.
>>> * Debug output when multiple IA bit is set during CmdConfigure.
>>> * Debug output when frames are received because multiple IA bit is set,
>>> or when they are ignored although it is set.
>>>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>> ---
>>> hw/eepro100.c | 30 +++++++++++++++++++++---------
>>> 1 files changed, 21 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>> index 2b75c8f..5f6dcb6 100644
>>> --- a/hw/eepro100.c
>>> +++ b/hw/eepro100.c
>>> @@ -219,7 +219,8 @@ typedef enum {
>>>
>>> typedef struct {
>>> PCIDevice dev;
>>> - uint8_t mult[8]; /* multicast mask array */
>>> + /* Hash register (multicast mask array, multiple individual 
>>> addresses). */
>>> + uint8_t mult[8];
>>
>>
>> Nitpick:
>> It seems to me like if mult is only used internally. If so, why not
>> represent the hash filter with an uint64_t?
>>
>> Then you can replace the current memsets with ->mult = 0 and simplify
>> the match logic.
>>
>>
>>> int mmio_index;
>>> NICState *nic;
>>> NICConf conf;
>>> @@ -599,7 +600,7 @@ static void nic_reset(void *opaque)
>>> {
>>> EEPRO100State *s = opaque;
>>> TRACE(OTHER, logout("%p\n", s));
>>> - /* TODO: Clearing of multicast table for selective reset, too? */
>>> + /* TODO: Clearing of hash register for selective reset, too? */
>>> memset(&s->mult[0], 0, sizeof(s->mult));
>>
>> e.g:
>>
>> s->mult = 0;
>>
>>> nic_selective_reset(s);
>>> }
>>> @@ -851,7 +852,14 @@ static void action_command(EEPRO100State *s)
>>> case CmdConfigure:
>>> cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
>>> sizeof(s->configuration));
>>> - TRACE(OTHER, logout("configuration: %s\n", 
>>> nic_dump(&s->configuration[0], 16)));
>>> + TRACE(OTHER, logout("configuration: %s\n",
>>> + nic_dump(&s->configuration[0], 16)));
>>> + TRACE(OTHER, logout("configuration: %s\n",
>>> + nic_dump(&s->configuration[16],
>>> + ARRAY_SIZE(s->configuration) - 16)));
>>> + if (s->configuration[20] & BIT(6)) {
>>> + TRACE(OTHER, logout("Multiple IA bit\n"));
>>> + }
>>> break;
>>> case CmdMulticastList:
>>> set_multicast_list(s);
>>> @@ -1647,12 +1655,6 @@ static ssize_t nic_receive(VLANClientState 
>>> *nc, const uint8_t * buf, size_t size
>>> static const uint8_t broadcast_macaddr[6] =
>>> { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>>>
>>> - /* TODO: check multiple IA bit. */
>>> - if (s->configuration[20] & BIT(6)) {
>>> - missing("Multiple IA bit");
>>> - return -1;
>>> - }
>>> -
>>> if (s->configuration[8] & 0x80) {
>>> /* CSMA is disabled. */
>>> logout("%p received while CSMA is disabled\n", s);
>>> @@ -1702,6 +1704,16 @@ static ssize_t nic_receive(VLANClientState 
>>> *nc, const uint8_t * buf, size_t size
>>> /* Promiscuous: receive all. */
>>> TRACE(RXTX, logout("%p received frame in promiscuous mode, 
>>> len=%zu\n", s, size));
>>> rfd_status |= 0x0004;
>>> + } else if (s->configuration[20] & BIT(6)) {
>>> + /* Multiple IA bit set. */
>>> + unsigned mcast_idx = compute_mcast_idx(buf);
>>> + assert(mcast_idx < 64);
>>> + if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
>>
>> e.g:
>> if (s->mult & (1 << (mcast_idx & 63))) {
>>
>> Cheers
>
> This 1 must really be 1ULL, otherwise we get an int shifted by a value 
> > 31,
> which triggers undefined behaviour.


Hello Michael,

that's correct. But there is no urgent need to switch to Edgar's code,
and the current code is ok.

Could you please add my patch to your pci patch queue?
It might be applied to the stable branches, too.

Thanks,
Stefan

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Michael S. Tsirkin]

On Sun, Oct 03, 2010 at 12:11:39PM +0200, Stefan Weil wrote:
> Am 03.10.2010 11:56, schrieb Michael S. Tsirkin:
> >On Wed, Sep 29, 2010 at 10:30:10PM +0200, Edgar E. Iglesias wrote:
> >>On Wed, Sep 29, 2010 at 09:59:55PM +0200, Stefan Weil wrote:
> >>>I reviewed the latest sources of Linux, FreeBSD and NetBSD.
> >>>They all reset the multiple IA bit (multi_ia in BSD) to zero,
> >>>but I did not find code which sets this bit to one
> >>>(like it is done by some routers).
> >>>
> >>>Running Windows guests also did not set this bit.
> >>>
> >>>Intel's Open Source Software Developer Manual does not
> >>>give much information on the semantics related to this bit,
> >>>so I had to guess how it works. The guess was good enough
> >>>to make the router emulation work.

BTW, I think we should add a link to the manual:
http://www.intel.com/design/network/manuals/8255x_opensdm.htm


> >>>
> >>>Related changes in this patch:
> >>>* Update naming and documentation of the internal hash register.
> >>>It is not limited to multicast, but also used for multiple IA.
> >>>* Dump complete configuration register when debug traces are enabled.
> >>>* Debug output when multiple IA bit is set during CmdConfigure.
> >>>* Debug output when frames are received because multiple IA bit is set,
> >>>or when they are ignored although it is set.
> >>>
> >>>Cc: Michael S. Tsirkin <mst@redhat.com>
> >>>Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> >>>---
> >>>hw/eepro100.c | 30 +++++++++++++++++++++---------
> >>>1 files changed, 21 insertions(+), 9 deletions(-)
> >>>
> >>>diff --git a/hw/eepro100.c b/hw/eepro100.c
> >>>index 2b75c8f..5f6dcb6 100644
> >>>--- a/hw/eepro100.c
> >>>+++ b/hw/eepro100.c
> >>>@@ -219,7 +219,8 @@ typedef enum {
> >>>
> >>>typedef struct {
> >>>PCIDevice dev;
> >>>- uint8_t mult[8]; /* multicast mask array */
> >>>+ /* Hash register (multicast mask array, multiple individual
> >>>addresses). */
> >>>+ uint8_t mult[8];
> >>
> >>
> >>Nitpick:
> >>It seems to me like if mult is only used internally. If so, why not
> >>represent the hash filter with an uint64_t?
> >>
> >>Then you can replace the current memsets with ->mult = 0 and simplify
> >>the match logic.
> >>
> >>
> >>>int mmio_index;
> >>>NICState *nic;
> >>>NICConf conf;
> >>>@@ -599,7 +600,7 @@ static void nic_reset(void *opaque)
> >>>{
> >>>EEPRO100State *s = opaque;
> >>>TRACE(OTHER, logout("%p\n", s));
> >>>- /* TODO: Clearing of multicast table for selective reset, too? */
> >>>+ /* TODO: Clearing of hash register for selective reset, too? */
> >>>memset(&s->mult[0], 0, sizeof(s->mult));
> >>
> >>e.g:
> >>
> >>s->mult = 0;
> >>
> >>>nic_selective_reset(s);
> >>>}
> >>>@@ -851,7 +852,14 @@ static void action_command(EEPRO100State *s)
> >>>case CmdConfigure:
> >>>cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
> >>>sizeof(s->configuration));
> >>>- TRACE(OTHER, logout("configuration: %s\n",
> >>>nic_dump(&s->configuration[0], 16)));
> >>>+ TRACE(OTHER, logout("configuration: %s\n",
> >>>+ nic_dump(&s->configuration[0], 16)));
> >>>+ TRACE(OTHER, logout("configuration: %s\n",
> >>>+ nic_dump(&s->configuration[16],
> >>>+ ARRAY_SIZE(s->configuration) - 16)));
> >>>+ if (s->configuration[20] & BIT(6)) {
> >>>+ TRACE(OTHER, logout("Multiple IA bit\n"));
> >>>+ }
> >>>break;
> >>>case CmdMulticastList:
> >>>set_multicast_list(s);
> >>>@@ -1647,12 +1655,6 @@ static ssize_t
> >>>nic_receive(VLANClientState *nc, const uint8_t * buf, size_t
> >>>size
> >>>static const uint8_t broadcast_macaddr[6] =
> >>>{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> >>>
> >>>- /* TODO: check multiple IA bit. */
> >>>- if (s->configuration[20] & BIT(6)) {
> >>>- missing("Multiple IA bit");
> >>>- return -1;
> >>>- }
> >>>-
> >>>if (s->configuration[8] & 0x80) {
> >>>/* CSMA is disabled. */
> >>>logout("%p received while CSMA is disabled\n", s);
> >>>@@ -1702,6 +1704,16 @@ static ssize_t
> >>>nic_receive(VLANClientState *nc, const uint8_t * buf, size_t
> >>>size
> >>>/* Promiscuous: receive all. */
> >>>TRACE(RXTX, logout("%p received frame in promiscuous mode,
> >>>len=%zu\n", s, size));
> >>>rfd_status |= 0x0004;
> >>>+ } else if (s->configuration[20] & BIT(6)) {
> >>>+ /* Multiple IA bit set. */
> >>>+ unsigned mcast_idx = compute_mcast_idx(buf);
> >>>+ assert(mcast_idx < 64);
> >>>+ if (s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7))) {
> >>
> >>e.g:
> >>if (s->mult & (1 << (mcast_idx & 63))) {
> >>
> >>Cheers
> >
> >This 1 must really be 1ULL, otherwise we get an int shifted by a
> >value > 31,
> >which triggers undefined behaviour.
> 
> 
> Hello Michael,
> 
> that's correct. But there is no urgent need to switch to Edgar's code,
> and the current code is ok.
> 
> Could you please add my patch to your pci patch queue?

I've been on holidays so I only got to reviewing patches today.
Let me get rid of the backlog and I'll get to look at your patch.

> It might be applied to the stable branches, too.
> 
> Thanks,
> Stefan

To me, this doesn't look like a stable branch material: this adds is a
new feature, not a bugfix.  Which guests benefit and how does
one use the routing emulation?

-- 
MST

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Stefan Weil]

Am 03.10.2010 13:16, schrieb Michael S. Tsirkin:
> On Sun, Oct 03, 2010 at 12:11:39PM +0200, Stefan Weil wrote:
>> Am 03.10.2010 11:56, schrieb Michael S. Tsirkin:
>>> On Wed, Sep 29, 2010 at 10:30:10PM +0200, Edgar E. Iglesias wrote:
>>>> On Wed, Sep 29, 2010 at 09:59:55PM +0200, Stefan Weil wrote:
>>>>> I reviewed the latest sources of Linux, FreeBSD and NetBSD.
>>>>> They all reset the multiple IA bit (multi_ia in BSD) to zero,
>>>>> but I did not find code which sets this bit to one
>>>>> (like it is done by some routers).
>>>>>
>>>>> Running Windows guests also did not set this bit.
>>>>>
>>>>> Intel's Open Source Software Developer Manual does not
>>>>> give much information on the semantics related to this bit,
>>>>> so I had to guess how it works. The guess was good enough
>>>>> to make the router emulation work.
>
> BTW, I think we should add a link to the manual:
> http://www.intel.com/design/network/manuals/8255x_opensdm.htm

http://wiki.qemu.org/Documentation/HardwareManuals
already contains a link to the manual.
I don't think it should be added to the code.

The code has a reference to the manual, so getting it is very easy.
That's better than a link which might be invalid tomorrow.

>
>
>>>>>
>>>>> Related changes in this patch:
>>>>> * Update naming and documentation of the internal hash register.
>>>>> It is not limited to multicast, but also used for multiple IA.
>>>>> * Dump complete configuration register when debug traces are enabled.
>>>>> * Debug output when multiple IA bit is set during CmdConfigure.
>>>>> * Debug output when frames are received because multiple IA bit is 
>>>>> set,
>>>>> or when they are ignored although it is set.
>>>>>
>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>>>> ---
>>>>> hw/eepro100.c | 30 +++++++++++++++++++++---------
>>>>> 1 files changed, 21 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>>> index 2b75c8f..5f6dcb6 100644
>>>>> --- a/hw/eepro100.c
>>>>> +++ b/hw/eepro100.c
>>>>> @@ -219,7 +219,8 @@ typedef enum {
>>>>>
>>>>> typedef struct {
>>>>> PCIDevice dev;
>>>>> - uint8_t mult[8]; /* multicast mask array */
>>>>> + /* Hash register (multicast mask array, multiple individual
>>>>> addresses). */
>>>>> + uint8_t mult[8];
>>>>
>>>>
>>>> Nitpick:
>>>> It seems to me like if mult is only used internally. If so, why not
>>>> represent the hash filter with an uint64_t?
>>>>
>>>> Then you can replace the current memsets with ->mult = 0 and simplify
>>>> the match logic.
[snip]
>>>>
>>>> e.g:
>>>> if (s->mult & (1 << (mcast_idx & 63))) {
>>>>
>>>> Cheers
>>>
>>> This 1 must really be 1ULL, otherwise we get an int shifted by a
>>> value > 31,
>>> which triggers undefined behaviour.
>>
>>
>> Hello Michael,
>>
>> that's correct. But there is no urgent need to switch to Edgar's code,
>> and the current code is ok.
>>
>> Could you please add my patch to your pci patch queue?
>
> I've been on holidays so I only got to reviewing patches today.
> Let me get rid of the backlog and I'll get to look at your patch.
>
>> It might be applied to the stable branches, too.
>>
>> Thanks,
>> Stefan
>
> To me, this doesn't look like a stable branch material: this adds is a
> new feature, not a bugfix. Which guests benefit and how does
> one use the routing emulation?


The first mail in this thread should answer your question.

It depends on your point of view whether better emulation
adds a new feature or fixes a bug:

I'm a developer, so I know that most emulations are not
complete. From my point of view, support for the multiple IA bit
is a new feature added to the emulation.

Dunc (the user who reported this problem) might call it a bug.
Users expect that the emulation simply works as good as the
original hardware and don't think much about limitations.

As far as I know the only guests which benefits use some
proprietary router software normally used on routers
of a well known router manufacturer.

A lot of people use qemu with eepro100 to emulate these
routers simply to learn the handling or to test certain
network configurations. See this URL for more information:
http://www.internetworkpro.org/wiki/Using_QEMU_with_Olive_to_emulate_Juniper_Routers

According to feedback which I got from Dunc, the patch
fixed his problem, so he can do more with his router
emulation.

There is no routing emulation. It's a nic emulation, and
the nic is used by a lot of different hardware (PCs,
routers and other embedded devices).

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Michael S. Tsirkin]

On Sun, Oct 03, 2010 at 01:47:45PM +0200, Stefan Weil wrote:
> >>It might be applied to the stable branches, too.
> >>
> >>Thanks,
> >>Stefan
> >
> >To me, this doesn't look like a stable branch material: this adds is a
> >new feature, not a bugfix. Which guests benefit and how does
> >one use the routing emulation?
> 
> 
> The first mail in this thread should answer your question.
> 
> It depends on your point of view whether better emulation
> adds a new feature or fixes a bug:

When in doubt, excercise caution. But in the end, I think it's your call:
do you want this in 0.13 stable, too?

-- 
MST

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Stefan Weil]

Am 03.10.2010 15:20, schrieb Michael S. Tsirkin:
> On Sun, Oct 03, 2010 at 01:47:45PM +0200, Stefan Weil wrote:
>>>> It might be applied to the stable branches, too.
>>>>
>>>> Thanks,
>>>> Stefan
>>>
>>> To me, this doesn't look like a stable branch material: this adds is a
>>> new feature, not a bugfix. Which guests benefit and how does
>>> one use the routing emulation?
>>
>>
>> The first mail in this thread should answer your question.
>>
>> It depends on your point of view whether better emulation
>> adds a new feature or fixes a bug:
>
> When in doubt, excercise caution. But in the end, I think it's your call:
> do you want this in 0.13 stable, too?


Yes. The patch is strictly limited to eepro100.c, and even there
its risk is limited to a highly specialized functionality.

On the other side we get a stable version 0.13 with increased
usability for a lot of people who use qemu with eepro100 to
emulate their routers.

Therefore it would be good to have this patch in 0.13 as well.

Regards,
Stefan

Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA)

[Michael S. Tsirkin]

On Mon, Oct 04, 2010 at 02:57:31PM +0200, Stefan Weil wrote:
> Am 03.10.2010 15:20, schrieb Michael S. Tsirkin:
> >On Sun, Oct 03, 2010 at 01:47:45PM +0200, Stefan Weil wrote:
> >>>>It might be applied to the stable branches, too.
> >>>>
> >>>>Thanks,
> >>>>Stefan
> >>>
> >>>To me, this doesn't look like a stable branch material: this adds is a
> >>>new feature, not a bugfix. Which guests benefit and how does
> >>>one use the routing emulation?
> >>
> >>
> >>The first mail in this thread should answer your question.
> >>
> >>It depends on your point of view whether better emulation
> >>adds a new feature or fixes a bug:
> >
> >When in doubt, excercise caution. But in the end, I think it's your call:
> >do you want this in 0.13 stable, too?
> 
> 
> Yes. The patch is strictly limited to eepro100.c, and even there
> its risk is limited to a highly specialized functionality.
> 
> On the other side we get a stable version 0.13 with increased
> usability for a lot of people who use qemu with eepro100 to
> emulate their routers.
> 
> Therefore it would be good to have this patch in 0.13 as well.
> 
> Regards,
> Stefan

OK then.
I've applied the patch in my tree, and I have no objections
against it going into stable.

-- 
MST