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

[Vlad Yasevich]

On 11/07/2013 10:27 AM, Michael S. Tsirkin wrote:
> On Thu, Nov 07, 2013 at 07:33:57AM -0700, Alex Williamson wrote:
> > 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 <address@hidden>
> > > > >
> > > > > 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.
>
> I think modern nics make address updates atomic.
> Problem is, we are emulating an ancient one,
> so to make host configuration of a modern one
> reasonable we need to resort to tricks.
>
> > 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.
>
> I agree what we have is a hack. Maybe we need some better hacks.
> For example, maybe we should update mac at link state change
> (I think  link is always down when mac is updated?).
> Needs some thought.

I thought this too, but checking recent linux kernel, e1000 and rtl8139 
seem to allow live mac change so link is up.

So here is a stupid, untested patch for e1000 to notify only once:
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 8387443..b99eba4 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_change_flags;
+#define E1000_RA0_CHANGED 0
+#define E1000_RA1_CHANGED 1
+#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
 } 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_change_flags = 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_change_flags |= E1000_RA0_CHANGED;
+                   break;
+           case (RA + 1):
+                   s->mac_change_flags |= E1000_RA1_CHANGED;
+                   break;
+    }
+
+    if (!(s->mac_change_flags ^ E1000_RA_ALL_CHANGED)) {
         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_change_flags = 0;
     }
 }

Any thoughts?

-vlad