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

[Vlad Yasevich <vyasevic@redhat.com>]

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 <vyasevic@redhat.com>
>> ---
>>   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