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 <ak...@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. > 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 Yes but this info is propaged to host NIC by libvirt so it better make sense. > > > > 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 > > > >