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 <address@hidden>
> > Signed-off-by: Stefan Weil <address@hidden>
> > ---
> >  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