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

[Stefan Weil <weil@mail.berlios.de>]

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