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

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2013-11-07 at 12:26 +0200, Michael S. Tsirkin wrote:
> 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.

If we were to implement mac change notification, then every partial
update would send a notify and the host.  Is that a problem?  It seems
no different than if the device had an atomic mac update procedure and
the guest admin changed the mac several times.

The problem with assuming that a given byte is always written last is
that unless the hardware spec identifies an order, we're just basing our
code on examples where we know what the guest driver does, either by
code inspection or access tracing.  If there are examples of guests that
write the last byte first, then the host will never know the correct mac
address.  Maybe there are no guests that use the wrong order, but that's
a pretty exhaustive search.

The patch doesn't change anything about how the NIC operates, only when
mac changes are updated.  During an update the mac is in a transitory
state and we can't go back in time to make it atomic on this hardware
design to avoid a window where the wrong mac may be seen.  I think the
patch is the right thing to do.  Thanks,

Alex

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