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

[Vlad Yasevich <vyasevic@redhat.com>]

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