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 <m...@redhat.com>
> > Signed-off-by: Stefan Weil <w...@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
