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

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

On Thu, Nov 07, 2013 at 03:32:29PM +0800, Amos Kong wrote:
> 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.

Yes but again, the window can just be too small to matter
on real hardware.

Our emulation is not perfect, fixing this to be just like real
hardware just might expose other bugs we can't fix
that easily.

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