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

[Edgar E. Iglesias]

On Thu, Sep 30, 2010 at 06:45:18PM +0200, Stefan Weil wrote:
> Am 29.09.2010 22:30, schrieb Edgar E. Iglesias:
> > 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<address@hidden>
> >> Signed-off-by: Stefan Weil<address@hidden>
> >> ---
> >>   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]
> 
> That's correct. Is it possible to change mult in the VMStateDescription
> from VMSTATE_BUFFER to VMSTATE_UINT64 without loosing
> compatibility?

Ah, probably not. My guess is that you'd run into
endianess issues. The current code is always little-endian but the
savevm/loadvm code would expect big endian.

Better to save the impovement for another time when the vmstate
description needs to be updated for other reasons..

Cheers