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

["Michael S. Tsirkin" <mst@redhat.com>]

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