Re: [Qemu-devel] [PATCH v4 7/8] Add ENET/Gbps Ethernet support to FEC device

[Jason Wang <jasowang@redhat.com>]

On 2016年05月21日 05:23, Jean-Christophe DUBOIS wrote:
> Le 20/05/2016 04:34, Jason Wang a écrit :
>>
>>
>> On 2016年05月20日 02:14, Jean-Christophe DUBOIS wrote:
>>> Le 19/05/2016 05:48, Jason Wang a écrit :
>>>>
>>>>
>>>> On 2016年05月19日 06:23, Jean-Christophe Dubois wrote:
>>>>> The ENET device (present in i.MX6) is "derived" from FEC and backward
>>>>> compatible with it.
>>>>>
>>>>> This patch adds the necessary support of the added feature in the 
>>>>> ENET
>>>>> device to allow Linux to use it (on supported processors).
>>>>>
>>>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>>>> ---
>>>>>
>>>>> Changes since v1:
>>>>>   * Not present on v1
>>>>>     Changes since v2:
>>>>>   * Not present on v2
>>>>>   Changes since v3:
>>>>>   * Separate and fix the 2 supported interrupts
>>>>>
>>>>>   hw/arm/fsl-imx25.c       |   3 +
>>>>>   hw/net/imx_fec.c         | 713 
>>>>> ++++++++++++++++++++++++++++++++++++++---------
>>>>>   include/hw/net/imx_fec.h | 195 ++++++++++---
>>>>>   3 files changed, 745 insertions(+), 166 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>>   -static Property imx_fec_properties[] = {
>>>>> +static Property imx_eth_properties[] = {
>>>>>       DEFINE_NIC_PROPERTIES(IMXFECState, conf),
>>>>> +    DEFINE_PROP_BOOL("is-fec", IMXFECState, is_fec, false),
>>>>>       DEFINE_PROP_END_OF_LIST(),
>>>>>   };
>>>>
>>>> It's ok to decide with "is-fec", but is it better to use a new type 
>>>> for that?
>>>
>>> Well, there is a lot of common code between FEC and ENET because 
>>> ENET is basically backward compatible with FEC. So most/all of the 
>>> FEC code needs to go in the ENET device.
>>>
>>> I thought this way of doing thing was the best way to avoid 
>>> duplicating things.
>>
>> You can still share almost all the codes. E.g you can have a look at 
>> e1000.c which have 3 types of rather similar devices.
>
> OK, I'll change it in next patch to match the pl110 model as proposed 
> by Peter. No more additional property...
>
>>
>> Another question not relate to this patch, I saw version were bumped 
>> for vmstate version. Is it better to keep the vmstate for FEC and 
>> using a new vmstate for ENET (register array).
>
> Well, as I moved the FEC to register array, the VMState structure has 
> changed and therefore I believe the vmstate version needs to be bumped.
>
> Am I wrong? 

You're right. But migration to old version does not work in this way, 
even if there aren't any changes in FEC.

I'm not sure the policy for arm, but we used to do lots of works in the 
past to make migration to older version works.

You can achieve this by using two different vmstates, and keep the FEC's 
state (by just use parts of array).

Perter, any thoughts on this? Should we try to keep the migration 
compatibility here?

Thanks