qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vlad Yasevich
Subject: Re: [Qemu-devel] [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
Date: Thu, 07 Nov 2013 10:49:00 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0

On 11/07/2013 09:33 AM, 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.

Yes, but the issue is exercebated in the non-atomic case.  RTL8139
is the worst since it has byte oriented writes so that for ever mac
change, we have the worst case potential to generate 6 notificates
(assuming libvirt is so fast as to pick up ever change).

Additionally, once libvirt is updated, this would cause rather serious
churn on the host as for ever update, libvirt is going to push the
address down to the physical host nic and the fewer of these updates
we do the better.


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,


Reporting half-complete state is not the right thing, IMO.  Right now,
it's doesn't have much impact, but if we start writing these addresses
to the host nic, then this will have a much bigger impact as I said above.

-vlad

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









reply via email to

[Prev in Thread] Current Thread [Next in Thread]