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

[Michael S. Tsirkin]

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.

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