From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48817 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P2N2q-0004n0-5P for qemu-devel@nongnu.org; Sun, 03 Oct 2010 07:47:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P2N2l-0003QT-2t for qemu-devel@nongnu.org; Sun, 03 Oct 2010 07:47:55 -0400 Received: from moutng.kundenserver.de ([212.227.17.8]:58395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P2N2k-0003QD-Lx for qemu-devel@nongnu.org; Sun, 03 Oct 2010 07:47:51 -0400 Message-ID: <4CA86D61.3080200@mail.berlios.de> Date: Sun, 03 Oct 2010 13:47:45 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] eepro100: Add support for multiple individual addresses (multiple IA) References: <4CA38D83.9040106@mail.berlios.de> <1285790395-25460-1-git-send-email-weil@mail.berlios.de> <20100929203010.GA7472@laped.lan> <20101003095653.GA16313@redhat.com> <4CA856DB.2070805@mail.berlios.de> <20101003111635.GC17133@redhat.com> In-Reply-To: <20101003111635.GC17133@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: "Edgar E. Iglesias" , QEMU Developers 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 >>>>> Signed-off-by: Stefan Weil >>>>> --- >>>>> 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).