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

[Vlad Yasevich <vyasevic@redhat.com>]

On 11/13/2013 03:00 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 13, 2013 at 11:21:18AM -0500, Vlad Yasevich wrote:
>> 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?
>>>
>>> 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).
>>>
>>
>> Hi Michael
>>
>> I started looking at migrating this thing and I now starting to question
>> the whole approach.
>>
>> The only reason to migrate this is if we can migrate between writes to
>> the mac address registers.
>
> Absolutely. For some reason below you only discuss cross version
> migration but it needs to be migrated for same version migration too.
>
>>   We can fairly easily solve the issue of
>> migrating from net to old versions.
>
> I'm not sure it's easier, but it needs to happen anymore.
>
>>   The more interesting question
>> is migrating from old to new versions.
>>
>> If someone is migrating from an older version (without this feature)
>> to a newer version and doing so between writes, the bitmap state will
>> have no idea that a partial write has already happened.  The completing
>> write will just set one of the bits and notifications that we are
>> looking for do not happen.
>>
>> -vlad
>
> Yep, that's a problem too.
>
>
> For 1.8 just send the bitmap across.

Right.  That's simple enough.

>
> For cross version migration I would say we should just detect -M 1.7
> and older and just emulate old behaviour, disregard the bitmap
> completely.
> Don't do special tricks just for migration.
>

The compat flags allow for simple migration to 1.7.  However, it's the
migration from 1.7 to 1.8 that's the issue.

There is a version number on the E1000 state and I am thinking of maybe
bumping that so that we can catch older state that doesn't support mac
sate change.  Thoughts?

-vlad