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

[Vlad Yasevich]

On 11/10/2013 07:11 AM, Michael S. Tsirkin wrote:
> On Fri, Nov 08, 2013 at 02:42:27PM -0500, Vlad Yasevich wrote:
> > What about this approach?  This only updates the monitory when all the
> > bits have been written to.
> >
> > -vlad
>
>
> Thanks!
> Some comments below.
>
> > -- >8 --
> > Subject: [PATCH] e1000/rtl8139: update HMP NIC when every bit is written
> >
> > 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.
>
> I would rather say "it seems better not to make this assumption".
> This does look somewhat safer than what Amos proposed.
>
> > 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 has been changed. It will be same as virtio-net.
> >
> > Signed-off-by: Vlad Yasevich <address@hidden>
> > ---
> >   hw/net/e1000.c   | 17 ++++++++++++++++-
> >   hw/net/rtl8139.c | 14 +++++++++-----
> >   2 files changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 8387443..a5967ed 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_changed;
>
> Hmm why uint32_t? uint8_t is enough here isn't?

Yes, that should be fine.  I'll fix and find a better spot to
put it. (may be a hole in the struct).

> This new state has to be migrated then, and
> we need to fallback to old behaviour if migrating to/from
> an old version (see compat_flags for one way to
> detect this compatibility mode).

Good point.  Didn't think of that...

> > +#define E1000_RA0_CHANGED 0
> > +#define E1000_RA1_CHANGED 1
> > +#define E1000_RA_ALL_CHANGED (E1000_RA0_CHANGED|E1000_RA1_CHANGED)
>
> I don't get it. So E1000_RA_ALL_CHANGED is 0 | 1 == 1.
> it looks like the trigger is when E1000_RA1_CHANGED
> so that's more or less equivalent.

Goofed on the bits.  That should have been (1<<0) and (1<<1).

> >   } 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_changed = 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_changed |= E1000_RA0_CHANGED;
> > +        break;
> > +    case (RA + 1):
> > +        s->mac_changed |= E1000_RA1_CHANGED;
> > +        break;
> > +    }
> > +
> > +    if (s->mac_changed ==  E1000_RA_ALL_CHANGED) {
>
> Some whitespace damage here.
>
> >           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_changed = 0;
>
> Need to use spaces for indent in qemu.
>
> >       }
> >   }
> >
> > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>
> Best to split out in a separate commit.

OK

> > index 5329f44..6dac10c 100644
> > --- a/hw/net/rtl8139.c
> > +++ b/hw/net/rtl8139.c
> > @@ -476,6 +476,8 @@ typedef struct RTL8139State {
> >
> >       uint16_t CpCmd;
> >       uint8_t  TxThresh;
> > +    uint8_t  mac_changed;
>
> This new state has to be migrated then, and
> we need to fallback to old behaviour if migrating to/from
> an old version.
>
> > +#define RTL8139_MAC_CHANGED_ALL 0x3F
> >
> >       NICState *nic;
> >       NICConf conf;
> > @@ -1215,6 +1217,7 @@ static void rtl8139_reset(DeviceState *d)
> >       /* restore MAC address */
> >       memcpy(s->phys, s->conf.macaddr.a, 6);
> >       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> > +    s->mac_changed = 0;
> >
> >       /* reset interrupt mask */
> >       s->IntrStatus = 0;
> > @@ -2741,12 +2744,13 @@ 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);
> > +            s->mac_changed |= (1 << (addr - MAC0));
>
> Better drop the external () here otherwise it starts looking like lisp :)

Heh.  OK.

Thanks
-vlad

> > +            if (s->mac_changed == RTL8139_MAC_CHANGED_ALL) {
> > +                qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> > +                s->mac_changed = 0;
> > +            }
> >               break;
> >           case MAC0+6 ... MAC0+7:
> >               /* reserved */
> > --
> > 1.8.4.2