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 <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. > > > 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
