Re: [Qemu-devel] [PATCH v5 0/8] e1000: Various fixes and registers' implementation

[Jason Wang]

On 11/11/2015 04:06 PM, Leonid Bloch wrote:
> On Wed, Nov 11, 2015 at 5:22 AM, Jason Wang <address@hidden> wrote:
>> >
>> >
>> > On 11/10/2015 09:19 PM, Leonid Bloch wrote:
>>> >> On Tue, Nov 10, 2015 at 3:01 PM, Jason Wang <address@hidden> wrote:
>>>> >>>
>>>> >>> On 11/10/2015 07:39 PM, Leonid Bloch wrote:
>>>>> >>>> On Tue, Nov 10, 2015 at 8:21 AM, Jason Wang <address@hidden> wrote:
>>>>>> >>>>> On 11/09/2015 10:59 PM, Leonid Bloch wrote:
>>>>>>> >>>>>> This series fixes issues with packet/octet counting in e1000's 
>>>>>>> >>>>>> Statistic
>>>>>>> >>>>>> registers, fixes a bug in the packet address filtering 
>>>>>>> >>>>>> procedure, and
>>>>>>> >>>>>> implements many MAC registers that were absent before, some 
>>>>>>> >>>>>> Statistic
>>>>>>> >>>>>> counters among them.
>>>>>>> >>>>>>
>>>>>>> >>>>>> Besides this, the series introduces a parameter which, if set to 
>>>>>>> >>>>>> "on"
>>>>>>> >>>>>> (default), will cause the entire MAC registers' array to migrate 
>>>>>>> >>>>>> during
>>>>>>> >>>>>> live migration (please see patch #2 for details). The rational 
>>>>>>> >>>>>> behind
>>>>>>> >>>>>> this is the ability to implement additional MAC registers in the 
>>>>>>> >>>>>> future,
>>>>>>> >>>>>> without worrying about migration compatibility between future 
>>>>>>> >>>>>> versions.
>>>>>>> >>>>>> For compatibility with previous versions, the above mentioned 
>>>>>>> >>>>>> parameter
>>>>>>> >>>>>> can be set to "off".
>>>>>>> >>>>>>
>>>>>>> >>>>>> Also, a new array is introduced to control the access to the 
>>>>>>> >>>>>> various MAC
>>>>>>> >>>>>> registers. This takes care of situations when a MAC register 
>>>>>>> >>>>>> requires a
>>>>>>> >>>>>> certain parameter to be accessed, or is partially implemented, 
>>>>>>> >>>>>> and
>>>>>>> >>>>>> requires a debug warning to be printed on access attempts.
>>>>>>> >>>>>>
>>>>>>> >>>>>> Additionally, several cosmetic changes are made.
>>>>>>> >>>>>>
>>>>>>> >>>>>> Differences v1-2:
>>>>>>> >>>>>> --------------------
>>>>>>> >>>>>> * Wording of several commit messages corrected.
>>>>>>> >>>>>> * For trivially implemented Diagnostic registers, a debug 
>>>>>>> >>>>>> message is
>>>>>>> >>>>>>   added on read/write attempts, alerting of incomplete 
>>>>>>> >>>>>> implementation.
>>>>>>> >>>>>> * Following testing on a physical device, only the lower 16 bits 
>>>>>>> >>>>>> can now
>>>>>>> >>>>>>   be read from AIT, and only the lower 4 - from FFMT*.
>>>>>>> >>>>>> * The grow_8reg_if_not_full function is rewritten.
>>>>>>> >>>>>> * inc_tx_bcast_or_mcast_count and increase_size_stats are now 
>>>>>>> >>>>>> called
>>>>>>> >>>>>>   from within e1000_send_packet, to avoid code duplication.
>>>>>>> >>>>>>
>>>>>>> >>>>>> Differences v2-3:
>>>>>>> >>>>>> --------------------
>>>>>>> >>>>>> * Minor rewordings of some commit messages (0002, 0003).
>>>>>>> >>>>>> * Live migration capability is added to the newly implemented 
>>>>>>> >>>>>> registers.
>>>>>>> >>>>>>
>>>>>>> >>>>>> Differences v3-4:
>>>>>>> >>>>>> --------------------
>>>>>>> >>>>>> * Introduction of the "full_mac_registers" parameter (see above).
>>>>>>> >>>>>> * Reversion of the live migration handling introduced in v3.
>>>>>>> >>>>>> * Small alignment changes in patch #1 to correspond with the 
>>>>>>> >>>>>> following
>>>>>>> >>>>>>   patches.
>>>>>>> >>>>>>
>>>>>>> >>>>>> Differences v4-v5:
>>>>>>> >>>>>> --------------------
>>>>>>> >>>>>> * Introduction of an array to control the access to the MAC 
>>>>>>> >>>>>> registers.
>>>>>>> >>>>>> * Removal of the specific functions that warned of partial
>>>>>>> >>>>>>   implementation on read/write from patch 4.
>>>>>>> >>>>>> * Adequate changes to patches 4 and 8: mainly adding the 
>>>>>>> >>>>>> registers
>>>>>>> >>>>>>   introduced there to the new array.
>>>>>>> >>>>>>
>>>>>>> >>>>>> The majority of these changes result from Jason Wang's review - 
>>>>>>> >>>>>> thank
>>>>>>> >>>>>> you, Jason!
>>>>>> >>>>> Thanks a lot for the patches. Almost done with two minor concerns:
>>>>>> >>>>>
>>>>>> >>>>> 1) to unbreak bisection we'd better enable the extra_mac_registers 
>>>>>> >>>>> (and
>>>>>> >>>>> compatibility stuffs) in patch 8 or patch 9
>>>>> >>>> Do you mean by that changing patch 2, so that the compatibility would
>>>>> >>>> be "on" by default, and setting it to "off" by default only in patch
>>>>> >>>> 8, or an additional patch 9?
>>>> >>> I mean do not introduce the property "extra_mac_registers" until patch 
>>>> >>> 8
>>>> >>> and 9. In this case all function will be enabled completely at that 
>>>> >>> time
>>>> >>> instead of partially patch by patch in this series.
>>> >> But won't there be compatibility issues between the patches if done
>>> >> like that?
>> >
>> > Looks not, all new mac registers and mac array migration will be
>> > functional or used only in last patch. We don't have any compatibility
>> > issue since E1000_FLAG_MAC can only be set for the last patch since the
>> > meaning of it should be "migrate the whole mac array and enable the
>> > various mac registers". And if we just use the order like this series,
>> > we may have compatibility issue during bisection. E.g migrate between w/
>> > patch 8 and w/o patch 8.
> But the trivial MAC registers are introduced in patch 4 already (in
> order to minimize code changes in the subsequent patches). Besides, if
> migration of the full MAC registers' array will be enabled, even
> before all the new registers are introduced, what will be the the
> problem with migration? One should be able to migrate from w/ patch 8,
> to anywhere after patch 2 with no problem, and to before that - with
> compatibility flag set.

Yes, migration can complete. But the value of the counters will suddenly
drop to zero after migration from patch 8 to patch 7. This is suboptimal
and can lead unexpected results.