Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written

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

On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
> What about this approach?  This only updates the monitory when all the
> bits have been written to.
> 
> -vlad


Thanks!
Some comments below.

> -- >8 --
> Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
> 
> We currently just update the HMP NIC info when the last bit of macaddr
> is written. This assumes that guest driver will write all the macaddr
> from bit 0 to bit 5 when it changes the macaddr, this is the current
> behavior of linux driver (e1000/rtl8139cp), but we can't do this
> assumption.

I would rather say "it seems better not to make this assumption".
This does look somewhat safer than what Amos proposed.

> 
> The macaddr that is used for rx-filter will be updated when every bit
> is changed. This patch updates the e1000/rtl8139 nic to update HMP NIC
> info when every bit has been changed. It will be same as virtio-net.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  hw/net/e1000.c   | 17 ++++++++++++++++-
>  hw/net/rtl8139.c | 14 +++++++++-----
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 8387443..a5967ed 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -149,6 +149,10 @@ typedef struct E1000State_st {
>  #define E1000_FLAG_AUTONEG (1 << E1000_FLAG_AUTONEG_BIT)
>  #define E1000_FLAG_MIT (1 << E1000_FLAG_MIT_BIT)
>      uint32_t compat_flags;
> +    uint32_t mac_changed;

Hmm why uint32_t? uint8_t is enough here isn't?

This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version (see compat_flags for one way to
detect this compatibility mode).

> +#define E1000_RA0_CHANGED 0
> +#define E1000_RA1_CHANGED 1
> +#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)

I don't get it. So E1000_RA_ALL_CHANGED is 0 | 1 == 1.
it looks like the trigger is when E1000_RA1_CHANGED
so that's more or less equivalent.

>  } E1000State;
>  
>  #define TYPE_E1000 "e1000"
> @@ -402,6 +406,7 @@ static void e1000_reset(void *opaque)
>          d->mac_reg[RA + 1] |= (i < 2) ? macaddr[i + 4] << (8 * i) : 0;
>      }
>      qemu_format_nic_info_str(qemu_get_queue(d->nic), macaddr);
> +    d->mac_changed = 0;
>  }
>  
>  static void
> @@ -1106,10 +1111,20 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>  
>      s->mac_reg[index] = val;
>  
> -    if (index == RA + 1) {
> +    switch (index) {
> +    case RA:
> +        s->mac_changed |= E1000_RA0_CHANGED;
> +        break;
> +    case (RA + 1):
> +        s->mac_changed |= E1000_RA1_CHANGED;
> +        break;
> +    }
> +
> +    if (s->mac_changed ==  E1000_RA_ALL_CHANGED) {

Some whitespace damage here.

>          macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
>          macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
>          qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);
> +	s->mac_changed = 0;

Need to use spaces for indent in qemu.

>      }
>  }
>  
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c

Best to split out in a separate commit.

> index 5329f44..6dac10c 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -476,6 +476,8 @@ typedef struct RTL8139State {
>  
>      uint16_t CpCmd;
>      uint8_t  TxThresh;
> +    uint8_t  mac_changed;

This new state has to be migrated then, and
we need to fallback to old behaviour if migrating to/from
an old version.

> +#define RTL8139_MAC_CHANGED_ALL 0x3F
>  
>      NICState *nic;
>      NICConf conf;
> @@ -1215,6 +1217,7 @@ static void rtl8139_reset(DeviceState *d)
>      /* restore MAC address */
>      memcpy(s->phys, s->conf.macaddr.a, 6);
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> +    s->mac_changed = 0;
>  
>      /* reset interrupt mask */
>      s->IntrStatus = 0;
> @@ -2741,12 +2744,13 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val)
>  
>      switch (addr)
>      {
> -        case MAC0 ... MAC0+4:
> -            s->phys[addr - MAC0] = val;
> -            break;
> -        case MAC0+5:
> +        case MAC0 ... MAC0+5:
>              s->phys[addr - MAC0] = val;
> -            qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> +            s->mac_changed |= (1 << (addr - MAC0));

Better drop the external () here otherwise it starts looking like lisp :)

> +            if (s->mac_changed == RTL8139_MAC_CHANGED_ALL) {
> +                qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> +                s->mac_changed = 0;
> +            }
>              break;
>          case MAC0+6 ... MAC0+7:
>              /* reserved */
> -- 
> 1.8.4.2