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

[Amos Kong <akong@redhat.com>]

On Thu, Nov 07, 2013 at 08:59:22AM +0200, Michael S. Tsirkin wrote:
> 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.

This patch just effects the monitor display of macaddr.
During each writing, the macaddr is used for rx-filter is really
changed.

In the real hardware, it supports to just write part of bits,
the rx-filtering is effected by every bit writing.

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

At least, linux rtl8139cp/e1000 writes macaddr from bit 0 to bit 5.
It works to just watch the last bit.
 
Thanks, Amos

> 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