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

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

On Tue, Nov 05, 2013 at 07:17:18PM +0800, Amos Kong wrote:
> 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.
> 
> 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 is changed. It will be same as virtio-net.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

I'm not sure I buy this.

If we actually implement e.g. mac change notifications,
sending them on writes of random bytes will confuse
the host.

I would say let's leave e1000/rtl8139 well alone unless
we see guests that actually write mac without touching
the last byte.

Then think of ways to detect when mac change is done
for these.

> ---
>  hw/net/e1000.c   | 2 +-
>  hw/net/rtl8139.c | 5 +----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index ec8ecd7..2d60639 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1110,7 +1110,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>  
>      s->mac_reg[index] = val;
>  
> -    if (index == RA + 1) {
> +    if (index == RA || index == RA + 1) {
>          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);
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 5329f44..7f2b4db 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -2741,10 +2741,7 @@ 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);
>              break;
> -- 
> 1.8.3.1