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

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


>      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