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